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

Use Zephyr 3.2.x full paths for includes #35

Closed percz closed 2 years ago

percz commented 2 years ago

LEGACY_INCLUDE_PATH has already been deprecated in Zephyr 3.2.0.

In order to get Memfault to build, full Zephyr paths are now needed.

CONFIG_LEGACY_INCLUDE_PATH could still be set (prf.conf) for older versions, but shouldn't be needed. The CONFIG_LEGACY_INCLUDE_PATH if statements will need removing after a few more revisions, or could just be ignored already.

noahp commented 2 years ago

Thanks for the patch @percz ! We'll take a look now. Could you provide the version of Zephyr you're using with this change?

percz commented 2 years ago

I've West and GIT pulled the latest and greatest version from Zephyr Project


*** Booting Zephyr OS build zephyr-v3.2.0-1037-g75bb8ce3826f  ***
.....
rtt:~$ [00:00:00.327,392] <inf> mflt: GNU Build ID: ea39837f5196f87fc39838b2dda9a7ed480e3ccf

Officially, the changelog says -

The [CONFIG_LEGACY_INCLUDE_PATH](https://docs.zephyrproject.org/latest/kconfig.html#CONFIG_LEGACY_INCLUDE_PATH) option has been disabled by default, all upstream code and modules have been converted to use <zephyr/...> header paths. The option is still available to facilitate the migration of external applications, but will be removed with the 3.4 release. The [scripts/utils/migrate_includes.py](https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/utils/migrate_includes.py) script is provided to automate the migration.

So I'm not sure why when I tried to build my v3.2.0 (and not 3.4) I ran into issues even with CONFIG_LEGACY_INCLUDE_PATH added as default in Memfaults 'kConfig' file.

chrisc11 commented 2 years ago

I can confirm that this works as expected. I use this PR in ongoing nRF Connect SDK upmerge, however to finish upmerge I would ideally need a new memfault-firmware-sdk release that would include this.

Thanks for the patch @percz and feedback @desowin -- we will work on getting this officially merged back next week!

noahp commented 2 years ago

Hello @percz and @desowin - I've created an alternate version of the patch that tests OK on latest Zephyr main branch (https://github.com/zephyrproject-rtos/zephyr/tree/278120acb600aa03a93dbfb11125882bb0765d40) and that nrf-connect upmerge branch (https://github.com/desowin/nrfconnect-sdk-nrf/tree/20157cb7c349a5eb83410db89a38ea7bdf060840), let me know if it meets your needs!

https://github.com/memfault/memfault-firmware-sdk/pull/36

percz commented 2 years ago

Yeah... looks like I included more includes than needed so thanks for checking the logic. Closing as #36 seems to be addressing the issue.

noahp commented 1 year ago

FYI I included the #36 change as part of the 0.34.1 release: https://github.com/memfault/memfault-firmware-sdk/releases/tag/0.34.1

Let us know if there's any issues there!

jfischer-no commented 1 year ago

Can someone please explain how this is resolved by #36? For example, #include has been deprecated for a long time.