Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix log formatting warnings #151

Closed
wants to merge 3 commits into from
Closed

Fix log formatting warnings #151

wants to merge 3 commits into from

Conversation

guillaumeVolery
Copy link

@guillaumeVolery guillaumeVolery commented Oct 5, 2023

Description

Test Steps

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#150

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

source/cellular_pktio.c Outdated Show resolved Hide resolved
source/cellular_pktio.c Outdated Show resolved Hide resolved
source/cellular_pktio.c Outdated Show resolved Hide resolved
source/cellular_pktio.c Outdated Show resolved Hide resolved
source/cellular_pktio.c Outdated Show resolved Hide resolved
@moninom1 moninom1 changed the title Issue #150 Log formatting warnings are resolved. Ready to be reviewed and merged to main. Fix log formatting warnings Oct 6, 2023
@moninom1
Copy link
Member

moninom1 commented Oct 6, 2023

Can you please also add a little bit of description for the PR. Thank you

@chinglee-iot
Copy link
Member

chinglee-iot commented Oct 6, 2023

@guillaumeVolery
( Edited. Sorry I didn't check the issue first )

@tpecar-ltek
Copy link

I do not see this PR in this current state beneficial. What you've done is changed one architecture-specific format specifier for another one.

The above will probably cause issues on 64bit platforms, which we encounter when compiling FreeRTOS / FreeRTOS-Cellular on the host platform

https://github.com/FreeRTOS/FreeRTOS/tree/main/FreeRTOS-Plus/Demo/FreeRTOS_Cellular_Interface_Windows_Simulator

Consider using inttypes.h format specifiers
https://en.cppreference.com/w/c/types/integer#Format_constants_for_the_fprintf_family_of_functions

@guillaumeVolery
Copy link
Author

@moninom1

Thank you for your review. I agree your remarks and I have pushed the corrections.

@tpecar-ltek

Thank you for your remark. We are using inttypes.h in our IoT project and only declaring variables/constants based on the given types in inttypes.h. The fact is that this cellular library is causing warning in our project due to the use of wrong constant formatters. It cannot stay like this on our side as we have the rule to release a warning-free software.

Please test to compile this branch on 64 bits architecture (I assume that it should not produce any warning also).

@guillaumeVolery
Copy link
Author

Please, could you accept the pull request ? Thank you in advance.

@moninom1
Copy link
Member

moninom1 commented Oct 9, 2023

Hi @guillaumeVolery , We are reviewing and validating the PR internally, it might take some time. We will merge it as soon as it is done. Thank you

@chinglee-iot chinglee-iot mentioned this pull request Oct 12, 2023
2 tasks
@chinglee-iot
Copy link
Member

chinglee-iot commented Oct 12, 2023

@guillaumeVolery
I also compile this PR with GCC 32 bits and GCC 64 bits compiler. There will be warning with GCC 32 and GCC 64 compiler.
For example,

./FreeRTOS-Cellular-Interface/source/cellular_pktio.c:1430:17: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t’ {aka ‘unsigned int’} [-Werror=format=]
 1430 |     LogDebug( ( "PktioSendData sent %lu bytes", sentLen ) );
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~
      |                                                 |
      |                                                 uint32_t {aka unsigned int}

Therefore, I created another PR to configure the log format for different compile in cellular config.
Please help to take a look at #153 and feedback in this PR.

( Edited to update the PR link )

@chinglee-iot chinglee-iot mentioned this pull request Oct 12, 2023
2 tasks
@chinglee-iot
Copy link
Member

This PR is covered in PR #154. Thank you for creating this PR.

@guillaumeVolery guillaumeVolery deleted the Issue150 branch October 16, 2023 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants