memfault / memfault-firmware-sdk

Memfault SDK for embedded systems. Observability, logging, crash reporting, and OTA all in one service. More information at https://docs.memfault.com.
https://memfault.com
Other
149 stars 75 forks source link

Zephyr legacy path fixups conflict with application/module headers #62

Closed YHusain1 closed 1 year ago

YHusain1 commented 1 year ago

Hi,

We are using memfault SDK for our internal application that uses zephyr rtos. We followed the instructions here: https://docs.memfault.com/docs/mcu/zephyr-guide/

The zephyr RTOS version is 3.4.0, we find the following build error:

are-sdk__ports__zephyr.dir/common/memfault_platform_core.c.obj -MF modules/memfault-firmware-sdk/CMakeFiles/..__modules__lib__memfault-firmware-sdk__ports__zephyr.dir/common/memfault_platform_core.c.obj.d -o modules/memfault-firmware-sdk/CMakeFiles/..__modules__lib__memfault-firmware-sdk__ports__zephyr.dir/common/memfault_platform_core.c.obj -c /root/zephyrproject/modules/lib/memfault-firmware-sdk/ports/zephyr/common/memfault_platform_core.c
/root/zephyrproject/modules/lib/memfault-firmware-sdk/ports/zephyr/common/memfault_platform_core.c:159:22: error: expected ')' before numeric constant
  159 |          APPLICATION,
      |                      ^
      |                      )
ejohnso49 commented 1 year ago

Thanks for reporting this! We will take a look. To help out with this would you be able to share what values the following Kconfigs are using:

YHusain1 commented 1 year ago

We have not set those values. The doc page I linked hadn't mention about it for building the code. What values should I set?

YHusain1 commented 1 year ago
are-sdk__ports__zephyr.dir/v2.4/memfault_fault_handler.c.obj -MF modules/memfault-firmware-sdk/CMakeFiles/..__modules__lib__memfault-firmware-sdk__ports__zephyr.dir/v2.4/memfault_fault_handler.c.obj.d -o modules/memfault-firmware-sdk/CMakeFiles/..__modules__lib__memfault-firmware-sdk__ports__zephyr.dir/v2.4/memfault_fault_handler.c.obj -c /root/zephyrproject/modules/lib/memfault-firmware-sdk/ports/zephyr/v2.4/memfault_fault_handler.c
/root/zephyrproject/modules/lib/memfault-firmware-sdk/ports/zephyr/v2.4/memfault_fault_handler.c:46:47: error: expected ')' before numeric constant
   46 | SYS_INIT(prv_install_nmi_handler, APPLICATION, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT);
      |                                               ^
      |                                               )

^^ this is the other build error

ejohnso49 commented 1 year ago

They're not required, just wanted to check really quick as they're used in that declaration. Thanks for the info, I will take a look!

YHusain1 commented 1 year ago

no problem. I would assume those have default values in the memfault KConfig?

ryanhagen-tmo commented 1 year ago

@ejohnso49 I wanted to point out what @YHusain1 said above, we're using zephyr 3.4.0 and the memfault docs say latest supported is 3.3.0. Although we're not convinced this would cause the build to fail though.

ejohnso49 commented 1 year ago

Thanks for flagging. Yeah, the default values are 40 and n respectively. I'm not able to reproduce at the moment. Would you be able to tell me the Zephyr commit hash you're using to build and the Memfault SDK version you're using?

My build is succeeding and runs correctly, here is my setup if you wanted to try this out as well:

YHusain1 commented 1 year ago

yes, we do have the same commit hash for zephyr, and the right version for Memfault SDK for 1.1.3, The only difference is that your build is for the QEMU cortex m3.

ejohnso49 commented 1 year ago

A couple more questions:

YHusain1 commented 1 year ago

Ok, so narrowed it down to the following include in the memfault source files of memfault_fault_handler.c and memfault_platform_core.c: #include <init.h>

The above is conflicting.

IIRC, I believe Zephyr has now recommended to use #include <zephyr/init.h>

Hence the build error. On changing the memfault source file to include as such, the build passes.

YHusain1 commented 1 year ago

I would suggest to change the include as zephyr/init.h in the memfault repo.

ejohnso49 commented 1 year ago

Ah, thanks for that update. We modify the include paths in our module for backwards compatibility with older Zephyr versions, I'm guessing that is causing the conflict here. For my information, does your codebase have an init.h somewhere that might be included ahead of the Zephyr headers? My guess is that this is being included first by our source and that file doesn't have a definition for SYS_INIT so the compiler essentially substitutes "" in place of it and causes the problem. Your workaround should be fine for now and I'm checking out potential changes we can make in a future release of the SDK

YHusain1 commented 1 year ago

Precisely.

ejohnso49 commented 1 year ago

We just released a new SDK version, 1.2.0. This should address the issue here, please let us know if you hit any issues after updating