FreeRTOS / FreeRTOS-Cellular-Interface

FreeRTOS Cellular Interface implementation of the 3GPP TS v27.007 standard.
MIT License
85 stars 59 forks source link

Fix log formatting warnings #151

Closed guillaumeVolery closed 1 year ago

guillaumeVolery commented 1 year ago

Description

Test Steps

Checklist:

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.

moninom1 commented 1 year ago

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

chinglee-iot commented 1 year ago

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

tpecar-ltek commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

moninom1 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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