-
Notifications
You must be signed in to change notification settings - Fork 1.4k
modules: memfault: Use hw_id for default Memfault device id
#25313
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
modules: memfault: Use hw_id for default Memfault device id
#25313
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 9e29a26b845624af50ae5d90fac5a37016062a05 more detailssdk-nrf:
Github labels
List of changed files detected by CI (8)Outputs:ToolchainVersion: df3cc9d822 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
d598ab4 to
413d137
Compare
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-25313/nrf/releases_and_maturity/migration/migration_guide_3.2.html |
413d137 to
28da197
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR simplifies the Memfault device ID configuration by consolidating the device ID source options and improving defaults based on system configuration.
- Replaced separate
CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEIandCONFIG_MEMFAULT_NCS_DEVICE_ID_NET_MACoptions with a unifiedCONFIG_MEMFAULT_NCS_DEVICE_ID_HW_IDthat uses the hw_id library - Set intelligent defaults for the HW ID source based on available hardware (IMEI for modem-equipped devices, BLE MAC for Bluetooth devices, network MAC for specific boards)
- Deprecated the old
CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEIoption while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/debug/memfault/boards/thingy91x_nrf9151_ns.conf | Removed explicit CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI configuration as it's now automatically selected based on NRF_MODEM_LIB |
| samples/debug/memfault/boards/nrf9151dk_nrf9151_ns.conf | Removed explicit CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI configuration as it's now automatically selected based on NRF_MODEM_LIB |
| samples/cellular/modem_shell/overlay-memfault.conf | Removed explicit CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI configuration as it's now automatically selected based on NRF_MODEM_LIB |
| samples/bluetooth/peripheral_mds/prj.conf | Removed hardcoded device ID as BLE MAC will now be used automatically based on BT being enabled |
| modules/memfault-firmware-sdk/memfault_integration.c | Updated preprocessor conditions to use new CONFIG_MEMFAULT_NCS_DEVICE_ID_HW_ID alongside deprecated IMEI option |
| modules/memfault-firmware-sdk/Kconfig | Introduced CONFIG_MEMFAULT_NCS_DEVICE_ID_HW_ID as the new default, deprecated IMEI option, and added intelligent HW ID source defaults |
| doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst | Documented the device ID configuration changes and new default behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
87a9436 to
10e5533
Compare
10e5533 to
94e9e1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
94e9e1a to
a22afd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Removed a metric for the tracking Bluetooth TX thread unused stack ``ncs_bt_tx_unused_stack``. | ||
| The thread in question was removed in Zephyr v3.7.0. | ||
|
|
||
| * Simplified the options for ``CONFIG_MEMFAULT_NCS_DEVICE_ID_*``, which sets the Memfault Device Serial: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comprehensive description of the changes is good, but just wondering if most of this should be in the migration guide instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agree. Would the below look better?
In the changelog, provide the below details:
- Simplified the options for
CONFIG_MEMFAULT_NCS_DEVICE_ID_*, which sets the Memfault Device Serial.
Additionally, when usinghw_idas the Memfault Device Serial source, the defaulthw_idsource is modified depending on the system configuration
SeeMigration guide for nRF Connect SDK v3.2.0_ for more details.
The above could be moved under the updated heading and this whole section could be moved to the 3.2 migration guide - https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/releases_and_maturity/migration/migration_guide_3.2.rst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, I'll make this change! thanks both <3
| # the nRF7002 DK | ||
| default HW_ID_LIBRARY_SOURCE_NET_MAC if BOARD_NRF7002DK_NRF5340_CPUAPP | ||
| # And finally, use the BLE MAC address if Bluetooth is enabled | ||
| default HW_ID_LIBRARY_SOURCE_BLE_MAC if BT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is actually not ideal under certain conditions- it uses bt_le_oob_get_local(), which will generate a new Private Resolvable Address :(. It may be better to use UID instead, @maxd-nordic can you comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be a mistake in my hwid implementation then :o
87c8de2 to
2582eca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rename the option and the name string to adhere to Bluetooth naming convention. See original suggestion here: nrfconnect#25313 (comment) Signed-off-by: Noah Pendleton <[email protected]>
2582eca to
c6e8ee5
Compare
Rename the option and the name string to adhere to Bluetooth naming convention. See original suggestion here: nrfconnect#25313 (comment) Signed-off-by: Noah Pendleton <[email protected]>
Rename the option and the name string to adhere to Bluetooth naming convention. See original suggestion here: nrfconnect#25313 (comment) Signed-off-by: Noah Pendleton <[email protected]>
c6e8ee5 to
fb123cc
Compare
|
This is now ready for final review, appreciate all the excellent review feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
Rename the option and the name string to adhere to Bluetooth naming convention. See original suggestion here: nrfconnect#25313 (comment) Signed-off-by: Noah Pendleton <[email protected]>
Set the default Memfault Device Serial to use the `hw_id` library. The available options for built-in Memfault Device Serial are now: - `CONFIG_MEMFAULT_NCS_DEVICE_ID_HW_ID` (new, and default) - Use ``hw_id`` provided device ID, which is also what nRF Cloud uses for device identity. See the :ref:`lib_hw_id` library for options for device ID source. - `CONFIG_MEMFAULT_NCS_DEVICE_ID_STATIC` - Used to set a custom build-time defined static device ID, primarily useful for testing. - `CONFIG_MEMFAULT_NCS_DEVICE_ID_RUNTIME` - Use a runtime-applied device ID, commonly used when the serial number of the device is written into settings at manufacturing time, for example. - `CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI` (deprecated) - Use the LTE modem IMEI as the device ID. - `CONFIG_MEMFAULT_NCS_DEVICE_ID_NET_MAC` (deprecated) - Use the network interface MAC address as the device ID. Signed-off-by: Noah Pendleton <[email protected]>
f96885f to
9e29a26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gminn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great clean up. I have a few non-blocking questions on defaults!
Set the default Memfault Device Serial to use the
hw_idlibrary. Theavailable options for built-in Memfault Device Serial are now:
CONFIG_MEMFAULT_NCS_DEVICE_ID_HW_ID(new, and default) - Usehw_idprovided device ID, which is also what nRF Cloud uses fordevice identity. See the :ref:
lib_hw_idlibrary for options fordevice ID source.
CONFIG_MEMFAULT_NCS_DEVICE_ID_STATIC- Used to set a custombuild-time defined static device ID, primarily useful for testing.
CONFIG_MEMFAULT_NCS_DEVICE_ID_RUNTIME- Use a runtime-applied deviceID, commonly used when the serial number of the device is written into
settings at manufacturing time, for example.
CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI(deprecated) - Use the LTE modemIMEI as the device ID.
CONFIG_MEMFAULT_NCS_DEVICE_ID_NET_MAC(deprecated) - Use the networkinterface MAC address as the device ID.