extmod/nimble: Add timeout to deinit. #17247
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17247 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 169 169
Lines 21890 21898 +8
=======================================
+ Hits 21572 21580 +8
Misses 318 318 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
extmod/nimble/modbluetooth_nimble.c
Outdated
| @@ -629,13 +641,12 @@ int mp_bluetooth_init(void) { | |||
|
|
|||
| // Run the scheduler while we wait for stack startup. | |||
| // On non-ringbuffer builds (NimBLE on STM32/Unix) this will also poll the UART and run the event queue. | |||
| mp_uint_t timeout_start_ticks_ms = mp_hal_ticks_ms(); | |||
| volatile mp_uint_t start = mp_hal_ticks_ms(); | |||
There was a problem hiding this comment.
volatile wasn't meant to be left here, I'll remove
|
Should extend this to more closely match the init functionality with regard to raising exceptions on failure |
d33dd07 to
5ddd3d2
Compare
extmod/nimble/modbluetooth_nimble.c
Outdated
| break; | ||
| } | ||
| mp_event_wait_ms(NIMBLE_STARTUP_TIMEOUT - elapsed); | ||
| mp_event_wait_ms(1); |
There was a problem hiding this comment.
I'm not sure exactly why this logic was changed? It should be more efficient to do a sleep for NIMBLE_STARTUP_TIMEOUT - elapsed than many sleeps for 1ms. The idea is that mp_event_wait_ms() can return early if there's a system event to process, so it won't wait that whole time if it doesn't need to.
There was a problem hiding this comment.
I thought this might be over-complicating it for minimal gain, figured it might be better to just simplify it to the same timeout logic used most other places in the codebase.
But yes it's true the original version should be slightly more efficient with likely no charge in code size so I could change birth these timeouts to the original pattern used here.
There was a problem hiding this comment.
With the original mp_event_wait_ms(NIMBLE_STARTUP_TIMEOUT - elapsed); scheme for both timeouts:
text data bss dec hex filename
397964 368 42356 440688 6b970 build-NUCLEO_WB55/firmware.elf
With the "simpler" one here mp_event_wait_ms(1);:
text data bss dec hex filename
397956 368 42356 440680 6b968 build-NUCLEO_WB55/firmware.elf
So the calculated timeout costs 4 bytes of flash (per copy of the logic). Considering this is only on boards that already are big enough to hold the BLE stack that's probably reasonable for more efficient use - BLE application are often battery ones and these kind of optimisations do (slightly) improve power use.
If the BLE radio stops responding before deinit is called the function can get stuck waiting for an event that is never received, particularly if the radio is external or on a separate core. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
5ddd3d2 to
3a4c672
Compare
Summary
If the BLE radio stops responding before deinit is called the function can get stuck waiting for an event that is never received, particularly if the radio is external or on a separate core.
This PR adds a timeout, similar to the timeout already used in the init function. That init function timeout was also
simplified as part of this change.
Relates to #17246
Testing
This has been tested on a stm32wb55 where the deinit was intermittently locking up waiting for the state to change.
This happened on a semi-regular basis on an application automated test suite which was running through hundreds of cycles over a number of hours.
Trade-offs and Alternatives
The main alternative to this is relying on watchdog to break the unit out of the infinite C loop once the failure occurs, but this is not appropriate for all applications.