Nimble: Reduce power usage, add compatibility to LFCLK calibration

[StarGate01]

This PR has been broken out of #1050 .

• Reduce BLE power usage by only powering up radio peripherals and the high frequency clock when needed (BLE_LL_RFMGMT_ENABLE_TIME parameter)
• Use NRF SDK to access high frequency clock, in order to not interfere with potential LFRC calibration.

[trman]

does this pr , allow better battery usage @StarGate01 ?
how much is battery time gain ?

[StarGate01]

@trman Refer to https://mynewt.apache.org/v1_7_0/network/ble_setup/ble_lp_clock.html#crystal-settle-time-configuration

Note that changing this time will impact battery life with the amount depending on the application. The HFXO draws a fairly large amount of current when running so keeping this time as small as possible will reduce overall current drain.

BLE_XTAL_SETTLE_TIME has been deprecated and migrated to BLE_LL_RFMGMT_ENABLE_TIME (see https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/src/libs/mynewt-nimble/nimble/controller/syscfg.yml#L407=)

I have not yet performed long-running power measurements, my Pinetime is going strong for days. I'll report back when I have reliable measurements, these tests can take days or weeks.

[kieranc]

I've been using this for a couple of weeks, the battery life isn't enormously different but it has been performing well and bluetooth connectivity has been reliable as ever. The battery graph in gadgetbridge looks less steep and I've been getting about 5 days use down to 50% which is when I usually charge it.

[StarGate01]

@kieranc Thank you for testing! I did not perform large-scale power measurements for the unmodified Infinitime versions - I am glad to see that your experience matches mine.

[Riksu9000]

InfiniTime formatting rules shouldn't apply to external libs. I'm updating the workflow to ignore the libs folder.

[StarGate01]

I see that the formatting rules have been updated in 6171c9d , the CI format checks should pass now - but I don't have permission to re-trigger them.

[Riksu9000]

The checks use the workflow files from your branch, so it must be rebased to fix it. This will also allow the checks to be started.

[JF002]

Thanks for this PR @StarGate01 ! I really appreciate all the efforts made to improve battery life!

Have you considered upstreaming those changes to nimble? Many other project would probably be interested in reducing power usage!

Also, are the changes to the log and to the critical section needed? ble_npl_hw_is_in_critical() does not seem to be used.

[StarGate01]

@JF002 Thanks!

Have you considered upstreaming those changes to nimble?

Yes, see apache/mynewt-nimble#1207 . Kind of hard, since Nimble does not use the NRF SDK in that way (yet). A documentation addition would be nice to have in any case.

changes to [...] the critical section

Yes, especially ble_npl_hw_is_in_critical is required by the RF management system (enabled via MYNEWT_VAL_BLE_LL_RFMGMT_ENABLE_TIME). The code won't compile otherwise.

It might be worth looking at apache/mynewt-nimble#1021 to further improve the critical section functions eventually. However, I don't know that much about the inner working of FreeRTOS interrupt handling, and the current solution works great.

changes to the log

These changes make it possible to in-depth debug Nimble. This is disabled by default, and can be enabled by changing these lines in the CMake file: https://github.com/InfiniTimeOrg/InfiniTime/pull/1051/files#diff-148715d6ea0c0ea0a346af3f6bd610d010d490eca35ac6a9b408748f7ca9e3f4R807 . This is part of my PR for improved debugging support.

[JF002]

Yes, see apache/mynewt-nimble#1207

Awesome! Great work! I would very much like to keep nimble as close as possible to the original code so that we keep the possibility to switch to a submodule someday :)
Is there anything we can do to help integrating this change in NimBLE?

[StarGate01]

My best idea to integrate the NRF SDK LF clock calibaration compatibility into Nimble was this: apache/mynewt-nimble#1207 (comment) , essentially to define a weak fallback symbol which can get overloaded by the linker if the NRF SDK is used. I admit that this is kind of a hack, but it should work - although I have not tested it.

Ultimatly, there should be a PR created in Nimble to address this, and an amendment made to the documentation at https://mynewt.apache.org/latest/network/ble_setup/ble_lp_clock.html , to warn users of potential incompatibilies with code that switches the HF clock (e.g. the NRF SDK LF calibaration routines).

I have paused work on upstreaming this to Nimble due to the unclear state of the NRF SDK usage, and because I got no feedback on my weak symbol idea yet - however, to be fair, I don't have it verified to be working.

Maybe working on this PR only makes sense once Nimble is a proper submodule of InfiniTime, since the testing of the NRF SDK interactions becomes a lot easier then.

I propose to continue the potential discussion concerning the Nimble upsteraming in apache/mynewt-nimble#1207 .

[trman]

i if i undertand right @StarGate01 @JF002 , it's not possible to merge this part on nimble because it's against their rule
of not be tightly tied to nrf sdk ?

so it can be only be done here ?

[StarGate01]

Nimble just does not use the NRF SDK currently, so the required functions do not exist.

I proposed a potential solution in apache/mynewt-nimble#1207 (comment) , but this is hard to test and verify. Once InfiniTime uses Nimble as a proper submodule, this change would be much easier to upstream into Nimble.

In the meantime, this can be merged into our "fork" of Nimble.

[Avamander]

Once InfiniTime uses Nimble as a proper submodule, this change would be much easier to upstream into Nimble.

Nimble's configuration handling is currently a significant pain for that to happen. We'd have to patch Nimble using CMake, which isn't very nice.

[StarGate01]

@Riksu9000 Since you added the needs-testing tag to this PR - how should we go about testing? Is there anything specific I could do?

[Riksu9000]

We should test this PR to make sure everything still works as expected and measure the decrease in power usage if possible.

[StarGate01]

I plan on running detailed power analysis using the NRF-PPK2 once I receive it. Until then, functional tests will have to do - I can confirm that this patch runs great on all my devices.

[StarGate01]

@Riksu9000 @trman @JF002 Using the PPK2, I performed several tests to determine the power consumption impact of this PR.

A detailed analysis and logdata is available at https://github.com/StarGate01/p8b-infinitime/tree/master/poweranalysis .

The Nimble patches lead to a mean average current reduction of -0.188 mA across all tests, which is -12.7% on average.

This should enable a theoretical increased battery runtime of about 114.5%.

Let me know what you think, and also if you find any errors in my calculations.

[Riksu9000]

Battery life improvements are great. I think we can merge this and see if any issues pop up during 1.11.0 dev.

[JF002]

We'll merge this branch now that 1.10 has been released so that we can easily check for potential side-effects (and potentially battery live improvements).