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.

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

fix(ble): fix race condition for MAC addr device ID in sample #24

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

Conversation

gminn
Copy link
Member

@gminn gminn commented Jun 13, 2024

Summary

The device ID when using the BLE MAC address is Unknown after the system
boots because the device has not been given a MAC address yet at the
time the device ID is set (system init). This change fixes that by setting a
static device id for the peripheral_mds sample app.

Bonus - enabled logging at the INFO level on the peripheral_mds
app so we can run commands like mflt get_device_info and get print-outs.

Test Plan

  • Tested on a nRF52840 DK with the peripheral_mds sample app
uart:~$ *** Booting nRF Connect SDK v3.5.99-ncs1-4965-g3733e7097909 ***
Starting Bluetooth Memfault example
Bluetooth initialized
Advertising successfully started
uart:~$ mflt get_device_info
[00:00:03.995,544] <inf> mflt: S/N: nrf-ble-testdevice
[00:00:03.995,605] <inf> mflt: SW type: nrf-ble-fw
[00:00:03.995,666] <inf> mflt: SW version: 0.0.1+688c47
[00:00:03.995,727] <inf> mflt: HW version: nrf52840dk_nrf52840

Resolves: MCU-433

Copy link
Member Author

gminn commented Jun 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @gminn and the rest of your teammates on Graphite Graphite

@gminn gminn force-pushed the gminn/fix-ble-device-id branch 2 times, most recently from 7ef3b8b to 3c26787 Compare June 18, 2024 15:43
@gminn gminn marked this pull request as ready for review June 18, 2024 15:52
@gminn gminn requested a review from a team June 18, 2024 15:52
Copy link

@ejohnso49 ejohnso49 left a comment

Choose a reason for hiding this comment

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

Thought about this more, and I think I've sadly come to the conclusion that we just shouldn't try to use this for the BLE MAC case. I think that IMEI and network MAC are fine to keep in. Maybe we add a little blurb to the Kconfig to indicate this is for a network MAC (ie nrf7002).

Also I realized that the device serial isn't needed for reboot events 😑 should have remembered that. There's a Memfault config to include device serial with events but it's disabled by default because nearly all devices use the device serial in the chunks POST url.

Copy link
Member Author

gminn commented Jul 22, 2024

Thought about this more, and I think I've sadly come to the conclusion that we just shouldn't try to use this for the BLE MAC case.

To clarify -- you're thinking we remove the example of using the BLE MAC addr for the device id?

@gminn gminn force-pushed the gminn/fix-ble-device-id branch 2 times, most recently from 8e5a36d to 1132580 Compare September 5, 2024 14:46
@github-actions github-actions bot removed the manifest label Sep 5, 2024
@gminn gminn requested a review from ejohnso49 September 5, 2024 14:50
Copy link
Member Author

gminn commented Sep 5, 2024

To clarify -- you're thinking we remove the example of using the BLE MAC addr for the device id?

@ejohnso49 This is back on my radar and I applied your suggestions -- let me know what you think!

modules/memfault-firmware-sdk/Kconfig Outdated Show resolved Hide resolved
  ### Summary
  The device ID when using the MAC address was Unknown after the system
  booted because the device has not been given a MAC address yet at the
  time the device ID is set (system init). This change removes the option
  for using a MAC address as the device id for BLE devices, and sets a
  static device id for the BLE sample app.

  ### Test Plan
  - Tested on a nRF52840 DK with the peripheral_mds sample app
  ```
uart:~$ *** Booting nRF Connect SDK v3.5.99-ncs1-4965-g3733e7097909 ***
Starting Bluetooth Memfault example
Bluetooth initialized
Advertising successfully started
uart:~$ mflt get_device_info
[00:00:03.995,544] <inf> mflt: S/N: nrf-ble-testdevice
[00:00:03.995,605] <inf> mflt: SW type: nrf-ble-fw
[00:00:03.995,666] <inf> mflt: SW version: 0.0.1+688c47
[00:00:03.995,727] <inf> mflt: HW version: nrf52840dk_nrf52840
  ```
@gminn gminn changed the title fix(ble): fix race condition for MAC addr device ID fix(ble): fix race condition for MAC addr device ID in sample Sep 5, 2024
@gminn gminn requested a review from ejohnso49 September 5, 2024 17:43
Copy link

@ejohnso49 ejohnso49 left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this, at least we won't have blank device serial's by default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants