Send battery level updates via BLE using message queue

[BasedNight]

This pull request changes the implementation of the battery level notifications to send via BLE using a dedicated message queue.

Previously, when battery level updates were broadcast using bt_bas_set_battery_level, these updates were sent on the same workqueue as is used for sending the audio bytes. The volume of audio data being transmitted led to tremendous backpressure, resulting in battery level updates being lost. This caused the battery level to appear to "jump" up and down in the app, as outlined in #2782.

These jumps were most pronounced when charging, probably because the device charges much faster than it discharges. Since the battery level is only updated every 60 seconds, missing even a few of these updates consecutively would cause the battery percentage to jump tremendously.

The same issue is responsible for #3041, for which I will submit another pull request shortly.

NOTE: The bounty mentions the CV1 specifically, but since this bug is caused by code in transport.c, both the DK2 and CV1 are affected. This issue was likely not as noticeable on the DK2 because its battery level update interval is 15 seconds (as opposed to 60). I have tested this fix with my DK2 and it appears to work as intended, but this PR will need to be reviewed by someone who can test it with a CV1.

/claim #2782

[beastoin]

the bat skew on omi cv1 is more pronounced because the battery discharge profile does not cover all states of the battery, and it depends on devices, which are not the same.

however, the inability to transfer bat level is also a valid point; it's not about resolving this issue but more about ensuring that the bat level remains updated.

btw, could you demo (logs are enough)? and also, which priority should we choose? you know the os has several rules for the priority of threads, and we need to choose the best fit for our application.

ref: https://x.com/_thinx/status/1964609753117806849

@BasedNight

[beastoin]

wait, you need the omi cv1 to demo, and I guess you cannot since you don't have the fpc. @aaravgarg, could you send him one?

[aaravgarg]

send me address on discord @BasedNight

[BasedNight]

Requested demo and logs attached. The video shows that the bug also affects DK2, which I tested on. Not very interesting, but you can see the battery % jump from 24% to 27% @ ~1:45 in the video. You can corroborate that with the (main) logs and see that 9/11 of the battery level updates are lost due to the same error:

Battery at 3570 mV (capacity 25%)
[00:00:52.249,206] <wrn> bt_conn: Unable to allocate buffer within timeout
[00:00:52.249,237] <err> bt_att: Unable to allocate buffer for op 0x1b
[00:00:52.249,237] <wrn> bt_gatt: No buffer available to send notification
[00:00:52.249,237] <err> transport: Error updating battery level: -12

I did some tests when charging while the battery was very depleted (~10%) and noted a jump from 12% to 22% due to the lost battery level updates. The jump would be even more noticeable on the CV1 since its battery level update interval is 60 seconds (rather than the 15 second interval of the DK2).

After applying the PR fixes, all 11 expected battery level updates are sent via BT without issue. You can see the absence of the above errors in the attached (PR applied) logs. Like I mentioned before, since the implementation for sending battery level updates via BT are identical for both devices, the PR should fix the bug on DK2 and CV1 alike.

Regarding thread priority—I don't know all the ins-and-outs, but a low-priority preempt thread priority should be fine something as small and infrequent as the battery level updates. I chose 8 somewhat arbitrarily; you can specify any # of preempt and cooperative thread priorities with CONFIG_NUM_PREEMPT_PRIORITIES and CONFIG_NUM_COOP_PRIORITIES respectively, and then use those constants to manage priorities, like so:

// highest priority
#define BATTERY_NOTIFY_PRIO K_PRIO_PREEMPT(1)
// lowest priority
#define BATTERY_NOTIFY_PRIO K_PRIO_PREEMPT(CONFIG_NUM_PREEMPT_PRIORITIES - 1)

That seems most prudent to me, but I'm not very familiar with how other threads are managed in the firmware. Refer to the docs for more.

Battery.Level.Update.Test.mp4

Battery Indicator Logs (main).txt
Battery Indicator Logs (PR applied).txt

[kodjima33]

@BasedNight can you send me in telegram your social media profiles? want to connect (loved your feedback)

[beastoin]

cool, thank you for the log.

the only point that is still bugging me is that reading the bat more frequently will cause larger noise (more dynamic, not stable on adc -> not stable on the bat level) since we need to pull the voltage divider more frequently to read the adc, and that creates noise. that's why we need to choose the best fit threshold.

• omi cv1 takes 2 hours to fully charge; that means in linear (of course it's not[1]) the profile should be 1%+ per 1.2 minutes on charging.

• omi cv1 take 19 hours for draining, that's mean ~ 1% per 11.4 minutes // or without phone ~ 1% per 4.8 minutes.

that's why a 60-second updated rate is fair enough with the rule of thumb that a 100% battery package is passed.

in that case, we can use 45-60s for omi cv1, i suggest, by the way, with the higher priority of the bat, of course, should use 5-6.

the root cause is still the discharge profile, and the voltage conversion is not accurate enough; that's why with every single new device we need to tune the discharge profile to have an accurate bat level.

the code: https://github.com/BasedHardware/omi/blob/main/omi/firmware/omi/src/battery.c#L228

my comment on bat skewed issue https://discord.com/channels/1192313062041067520/1231903583717425153/1408082514140205096

[1]: you could check the discharge profile and my note here: https://x.com/_thinx/status/1964609753117806849

[BasedNight]

To be clear, I am not suggesting polling the battery more often. The only reason I was mentioning the 60-second battery interval of the CV1 was to say that it would cause larger jumps in battery %. So if the CV1 misses 10 updates and succeeds on the 11th, it will have been 10 minutes since the last update and the % may jump a lot. To use the same example, missing 10 battery updates on the DK2 is only ~2.5 minutes without updates, so the battery % would not have changed as much. I believe this made the bug less noticeable on that device.

Now that all the updates will be sent and not dropped, the 60-second interval should be good. I think we can even change the DK2 interval to be 60 seconds as well (to avoid the problems you mentioned with polling more often).

Regarding what you are saying about the discharge profile, is the resulting inaccuracy something that could even be fixed? Do we want to expend the effort to try? I suppose you could charge/discharge the battery several more times, take the average, add more states to battery_states in battery.c, etc., but IMO, it is good enough now.

@beastoin What else do I need to do so we can merge this? Change the thread priority to 6?

[beastoin]

yoh two things:

1. let's test on your omi to make sure everything works as expected. e.g:

• drain the battery to 0%, then plug it in to charge.
• check the percentage, then unplug and check again.
• repeat a few times to see if the battery percentage jumps or not.

2. battery thread priority should be higher (remember, smaller number = higher priority) than the transport thread. thats why i suggested priority 6. but you can also test with your current priority 8 setup, if it works fine, just keep it - no worries.

since the main goal is to fix the battery jumping issue, let's test it a bit more to be sure everything's stable.

@BasedNight

[BasedNight]

Quick update so this doesn't get closed: I got a CV1, just waiting on an FPC cable from the Omi team so I can flash the CFW and test this fix.