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.

Sign up for GitHub

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

Jump to bottom

Enable COAP logs from cmake #716

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enable COAP logs from cmake #716

wants to merge 1 commit into from

Conversation

parmi93
Copy link
Contributor

@parmi93 parmi93 commented Jun 15, 2023

It is now possible to enable COAP logs via the cmake file, without having to edit the coap/er-coap-13/er-coap-13.c file.
Also usually the printf(...) function is not available with embedded platforms, so it has been replaced with lwm2m_coap_printf(...) which must be defined in the platform.c file.

@parmi93
Enable COAP logs from cmake
069b405 
It is now possible to enable COAP logs via the cmake file, without
having to edit the "coap/er-coap-13/er-coap-13.c" file.
Also usually the printf() function is not available with embedded
platforms, so it has been replaced with lwm2m_coap_printf() which must
be defined in the platform.c file.
rettichschnidi
rettichschnidi approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea to me, thanks!

If you feel like, I suggest the following to be done too:

  • Add documentation about this flag to the README.md file. There is no section yet for debugging flags, so feel free to come up with a proposal.
  • Add a build with this new flag enabled, so that CI can check it does not break.
  • Add a format attribute to the function lwm2m_coap_printf(), so that passed arguments can be checked at compile time.

Please let me know what you think and whether you have time/motivation to implement it. If not - no problem at all, I can do this after merging this PR.

include/liblwm2m.h
@@ -124,6 +124,11 @@ time_t lwm2m_gettime(void);
void lwm2m_printf(const char * format, ...);
#endif

#ifdef LWM2M_WITH_COAP_LOGS
// Same usage as C89 printf()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think C89 is too specific (will also work with C99, C11, etc.)

Suggested change
// Same usage as C89 printf()
// Same usage as printf()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied this from the comment above lwm2m_printf(...), anyway no problem, I'll fix this.

@parmi93
Copy link
Contributor Author

parmi93 commented Jun 16, 2023

  • Add a format attribute to the function lwm2m_coap_printf(), so that passed arguments can be checked at compile time.

Do you mean add the format like this (in the liblwm2m.h file)?

__attribute__ ((format (printf, 1, 2))) void lwm2m_coap_printf(const char * format, ...);

When I added this attribute the compiler gave me several warnings in the er-coap-13.c file about the format specifies type (which I fixed).
Some of the warnings that I fixed:

./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:658:3: warning: format specifies type 'unsigned long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
  COAP_SERIALIZE_BLOCK_OPTION(  COAP_OPTION_BLOCK2,         block2, "Block2")
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.h:301:43: note: expanded from macro 'COAP_SERIALIZE_BLOCK_OPTION'
      PRINTF(text" [%lu%s (%u B/blk)]\n", coap_pkt->field##_num, coap_pkt->field##_more ? "+" : "", coap_pkt->field##_size); \
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:52:39: note: expanded from macro 'PRINTF'
#define PRINTF(...) lwm2m_coap_printf(__VA_ARGS__)
                                      ^~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:659:3: warning: format specifies type 'unsigned long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
  COAP_SERIALIZE_BLOCK_OPTION(  COAP_OPTION_BLOCK1,         block1, "Block1")
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.h:301:43: note: expanded from macro 'COAP_SERIALIZE_BLOCK_OPTION'
      PRINTF(text" [%lu%s (%u B/blk)]\n", coap_pkt->field##_num, coap_pkt->field##_more ? "+" : "", coap_pkt->field##_size); \
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:52:39: note: expanded from macro 'PRINTF'
#define PRINTF(...) lwm2m_coap_printf(__VA_ARGS__)

@parmi93
Copy link
Contributor Author

parmi93 commented Jun 16, 2023 •
edited
Loading

Do you mean that I should append this in the wakaama/examples/client/CMakeLists.txt file?

# Client without DTLS support and CoAP logs enabled
add_executable(lwm2mclient ${SOURCES})
target_compile_definitions(lwm2mclient PRIVATE LWM2M_CLIENT_MODE LWM2M_BOOTSTRAP LWM2M_SUPPORT_SENML_JSON LWM2M_WITH_COAP_LOGS)
target_sources_wakaama(lwm2mclient)
target_sources_shared(lwm2mclient)

@parmi93
Copy link
Contributor Author

parmi93 commented Jun 20, 2023

  • Add a format attribute to the function lwm2m_coap_printf(), so that passed arguments can be checked at compile time.

Do you mean add the format like this (in the liblwm2m.h file)?

__attribute__ ((format (printf, 1, 2))) void lwm2m_coap_printf(const char * format, ...);

When I added this attribute the compiler gave me several warnings in the er-coap-13.c file about the format specifies type (which I fixed). Some of the warnings that I fixed:

./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:658:3: warning: format specifies type 'unsigned long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
  COAP_SERIALIZE_BLOCK_OPTION(  COAP_OPTION_BLOCK2,         block2, "Block2")
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.h:301:43: note: expanded from macro 'COAP_SERIALIZE_BLOCK_OPTION'
      PRINTF(text" [%lu%s (%u B/blk)]\n", coap_pkt->field##_num, coap_pkt->field##_more ? "+" : "", coap_pkt->field##_size); \
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:52:39: note: expanded from macro 'PRINTF'
#define PRINTF(...) lwm2m_coap_printf(__VA_ARGS__)
                                      ^~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:659:3: warning: format specifies type 'unsigned long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
  COAP_SERIALIZE_BLOCK_OPTION(  COAP_OPTION_BLOCK1,         block1, "Block1")
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.h:301:43: note: expanded from macro 'COAP_SERIALIZE_BLOCK_OPTION'
      PRINTF(text" [%lu%s (%u B/blk)]\n", coap_pkt->field##_num, coap_pkt->field##_more ? "+" : "", coap_pkt->field##_size); \
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./lwm2m/wakaama/coap/er-coap-13/er-coap-13.c:52:39: note: expanded from macro 'PRINTF'
#define PRINTF(...) lwm2m_coap_printf(__VA_ARGS__)

I would like to point out that after I fix the warnings in the er-coap-13.c file, my system doesn't crash anymore.
Prior to this fix with CoAP Logs enabled, my system always crashed when a large amount of logs were generated.

I guess this was due to lines of code like this (in c the behaviour of the code is undefined in these cases):
https://github.com/eclipse/wakaama/blob/069b405d101bc0b76dbb794d5e73a9d67e82fef0/coap/er-coap-13/er-coap-13.c#L851-L851

https://github.com/eclipse/wakaama/blob/069b405d101bc0b76dbb794d5e73a9d67e82fef0/coap/er-coap-13/er-coap-13.c#L869-L869

@mlasch
Copy link
Contributor

mlasch commented Feb 8, 2024

We moved the default branch from master to main, can you rebase your PR to the latest main on eclipse/wakaama?

@LukasWoodtli
Copy link
Contributor

I did some big refactoring and improvements for the logging infrastructure in Wakaama: #748. The logging in the CoAP part should eventually be incorporated into the rest of Wakaama logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rettichschnidi rettichschnidi rettichschnidi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@parmi93 @mlasch @LukasWoodtli @rettichschnidi