Heartrate measurements in background #1718
Conversation
|
Build size and comparison to main:
|
|
Was about to implement the same thing! Another idea that I was thinking of was to measure heart rate after a number of steps taken. Perhaps taking measurements after a certain number of steps in a time window. |
This idea should be a setting and not a default, |
It could maybe be an additional trigger for measuring, but not the only one. |
|
I use hr monitor for my sleep. this shouldn't prevent that. |
The code just runs measurements every 5 minutes when the heart rate task is started (through the heart rate screen) and the watch is locked/the screen is off. All the other functionality is exactly as before. The new changes don't interfere with monitoring the heart rate while staying in the heart rate screen. |
|
I noticed during testing that notifications interrupt the background measurement delay and make it reset, so I removed the reset for the Notifications and "just checking the time quickly" won't reset the delay now and the measurements will run more consistently over time. |
And the heart rate sensor likes to use a lot of battery power, so for those feeling battery conscious it would need to be a setting somewhere. |
The background measurements are only running, if the user activates the heart rate measurement in the HR app. So there is no new default that HR measurements are taken in the background all the time, just when activating them through the HR app. The behavior when activating the measurement and leaving the HR app is that when the screen is on the HR is measured all the time. If the screen is off, HR is not measured. My code does not change the case when measurements are activated and the screen is on (stays at "measure as often as possible"). Also when the measurement is not activated in the HR app nothing changes (stays at "never measure"). So the the user still always has the option to just don't activate HR measurements at all, but if they are activated, then additionally to measuring when the screen is on there are also occasional measurements when the screen is off. I'm not sure if it makes sense to have another setting that switches between the current mode and the new "measure additionally in the background" mode. |
|
That sounds pretty reasonable. One other concern is just that it takes so long to take a HR measurement, and the first reading is often wildly inaccurate especially during movement. Another idea is to pause readings when absolutely zero accelerometer movement is detected for the past few readings, i.e. when the watch has not been on one's wrist for a while (of course the accelerometer would still be checked every 5-ish minutes even when off one's wrist and would also resume periodic readings when the watch is woken up). These would both help the battery life and improve accuracy immensely. |
After #1486 is merged, measurements will be almost instantaneous and much more accurate. |
|
Alright, though I still think excessive and no movement detection should be added to preserve some amount of battery life... |
|
I've been testing #1486 for a couple of days now and while the updates are almost instantaneous the first reading usually takes up to 10 seconds for me. I can't say much about the accuracy, but it's similar to the miband 3. I found that for best results, wearing the watch higher up the forearm (about 1/3 of the way) and having it face the inside of the arm works. I was not able to get a measurement restart when I wore it like that (i.e. the reading did not reset to 000, like it does when moving around while having it on the wrist). PSA: It also might be worth checking if the protective film has been removed from the sensor "window". |
patricgruber
force-pushed
the
heartrate-measurements-in-background
branch
from
b967f9b to
faec69e
Compare
|
i'm also testing the new PPG algorithm and sometimes it takes upward of 15-20 seconds to get a fix on the heart rate. i've checked out @patricgruber PR and built it. i'll take it for a spin. also, @pankk thank you for the tip with the film, i'd totally forgotten to remove mine! that might explain the 15-20s delay :) |
|
i'm testing this PR on my device and i'm happy to say it is working quite well. (removing the sensor film reduced the fix time considerably). to start, i tested heart rate characteristic notifications in bluetoothctl: $ bluetoothctl
[bluetoothctl]# devices
Device C9:2D:E5:XX:XX:XX InfiniTime
[bluetoothctl]# pair C9:2D:E5:XX:XX:XX
[bluetoothctl]# connect C9:2D:E5:XX:XX:XX
[InfiniTime]# menu gatt
[InfiniTime]# select-attribute 00002a37-0000-1000-8000-00805f9b34fb
[InfiniTime:/service005b/char005c]# notify oni received notifications almost exactly every five minutes with the screen off: [CHG] Attribute /org/bluez/hci0/dev_C9_2D_E5_XX_XX_XX/service005b/char005c Value:
00 53 .N
00 53 .N after that, i installed nrfConnect (unfortunately, a proprietary app and there doesn't seem to be an open source equivalent) and logged all of the notifications overnight. inspecting the logs, the heart rate accuracy and success rate of notifications seemed very reliable and consistent. awesome! i estimate that total battery drain overnight was less than 10%, but i did not look at the exact percentage before sleep. i will take more accurate battery measurement tonight. |
|
@khimaros could you share the firmware ? |
|
@lman0 -- i'm open to sending this privately if you provide an email address. unfortunately, the build artifact contains private information (usernames and unabridged filesystem paths, possibly more). alternatively, i've written up detailed reproducible build instructions in the comments on this PR: #1761 |
|
So I've been testing this PR along with the related gadgetbridge PR. Super cool to have the HR measurements showing up. A few thoughts on it
Suggestions
Regarding the measurement interval: I was thinking about what it works best as. There are a few options I see regarding when to trigger a measurement
Not sure if I made any sense so interested to hear any thoughts :) |
|
@mark9064 i think these are great suggestions. i'll just mention that, even though the data is stale, it is sometimes useful to be able to quickly turn the watch on to see what the most recent HRM measurement was. maybe it can be displayed in another color to indicate staleness? |
|
FYI, slightly off topic here, but possibly useful for testers of this PR. phyphox also works as a tool for graphing and exporting heart rate data from InfiniTime on android: https://codeberg.org/Freeyourgadget/Gadgetbridge/issues/2383#issuecomment-915776 |
|
Thanks for all the suggestions. Regarding the timeout reset for background measurements: Regarding the reset to 0 when the screen wakes up when ambient light is detected: I personally rather have a slightly older/stale value then no value at all. If I want a fresh value I can just wait, but at least I can just check the screen and see in which region I am, if I had some more or less constant state for at least 10 minutes or so. But if the heart rate is reset to 0, then I don't have that. I have the background measurements that are either sent to companion app or go no where, but I won't ever see them on the watch. I think a good compromise is having a different color for stale values so I know that the values are old, but I also see the last measurement. Regarding the ambient light sensor and running forever: I think implementing a timer to stop the sensor if there is ambient light for more than 10-20 seconds and then just try again for the next "scheduled" measurement seems reasonable. |
|
Sounds sensible overall 👍 One thing to note re running time limits: if it is dark but the PPG has no contact it will still run forever if it just checks ambient light. If the PPG has been running for say 30s with no success the chance it's going to succeed is probably pretty low so I think giving up makes more sense? I guess it depends on how power hungry running the PPG is relatively. I'm planning on making a continuous measurement patch for testing so I should have some ideas on that soonish |
|
@mark9064 Also as side note: my first test was to just let it run continuously in the background and I think it drained around 50% battery or more over night. Also the watch got warm. I think letting it run for that long without breaks is probably not the best idea. But maybe with the updated PPG code made it possible. I have only tested with the old code. |
|
I implemented a timeout of 30s for the background measurement. If there is no data within these 30s, then the measurement is stopped and retried after 10 mins. |
commit d271670 Author: Patric Gruber <[email protected]> Date: Thu May 11 23:49:39 2023 +0200 remove version change in CMakeLists.txt commit faec69e Author: Patric Gruber <[email protected]> Date: Thu May 11 23:47:31 2023 +0200 rebase on main commit c5d2e42 Author: Patric Gruber <[email protected]> Date: Mon Apr 3 21:29:17 2023 +0200 remove background start timestamp reset on sleep commit 7180646 Author: Patric Gruber <[email protected]> Date: Fri Mar 31 12:38:37 2023 +0200 remove version change in CMakeLists.txt commit 9186cd2 Author: Patric Gruber <[email protected]> Date: Fri Mar 31 10:25:36 2023 +0200 increase task delay when waiting in the background to 10s commit a3a30a2 Author: Patric Gruber <[email protected]> Date: Fri Mar 31 10:00:56 2023 +0200 add heart rate measurments in the background
|
So I tried implementing functionality (mark9064@225be81 and mark9064@97d894e) that allows the user to choose between no background measurements, periodic background measurements and continuous measurement. It seems to work as expected but as you noted battery life with continuous measuring is poor (~24h). I don't know where the majority of the power is consumed but it makes sense that it would be the PPG LED as it's on about 50% of the time and has a high drive current. The data is interesting though, sleep zones are clearly visible, but the UI for switching between modes sucks. Not sure if this is a feature users would want or if we should just keep it simple |
|
@mark9064 i'm using your battery life is, expectedly, worse with the continuous mode on. apart from the battery used to power the LED/hardware, i suspect continuous mode is also preventing the main CPU from going to sleep, since it is always busy with the measurement task? this is just a guess, i don't know any of the inside details of InfiniTime/FreeRTOS power management. i agree with you the UI for choosing the HRM mode could be a bit clearer, but as a power user it was intuitive enough. maybe it's just a matter of choosing more descriptive names for the toggle? alternative descriptions: "Always", "Periodic", "Foreground". another option is to make the "background delay" configurable, but then we'd likely need two controls; one for the delay and one to toggle background mode on/off. overall, however, i think this is a great feature and would love to see it land in a release version! it's really incredible to be able to raise my wrist, look at the screen, and see instantaneous heart rate information! i suspect i'm not the only person who would think so. |
|
Finally got time to do some proper power testing using the patchset in my last message |
|
@mark9064 is this using the testing branch at https://github.com/mark9064/InfiniTime/tree/testing ? which measurement mode were you using? periodic? continuous? |
|
I like the idea of the background measurement considering every type of measurement, but I think unlinking the two is the overall best option 👍 |
|
Changed it to do that. But still testing it |
|
What do you think about restricting the foreground measurements to the heart rate app and using background measurements everywhere else, even if the screen is on? I like the idea. It feels a lot more polished for an end user product and should reduce this problem to nearly not existing. Let's do a vote with 👍 and 👎 under this comment. |
|
But if I understand correctly, the only difference between the latest commit and the preferred version in @minacode 's comment is that the HRM sensor is still active when the screen is on, outside of the heartrate app. Is that right? |
|
Hmm, this would mean that with HR on you wouldn't see a current value on your watchface, only if you go to the HR app right? I think this wouldn't be ideal, I would quite like 5 min background measurements but I would also like to see the up to date value if I switch the screen on. I agree that there's a bit more complexity this way, but I think we can accept it as IMO the user experience is better. At least from what I have seen from other smartwatches, if there is a HR indicator on the watchface it immediately begins acquiring HR on device wake |
|
Not to mention that it would mean that you might never get a reading IG your get notifications every 5s because of an active slack conversatio, as the "notification pop up" would interrupt the measiremenevery time.
But the watchface argument is even more crucial. It would lose its purpose.
Besides that, I didn't follow the full conversation, so I am not quite sure I get the issue right now, but making the measurements independent sounds like a complex alignment code would be required which sounds just like a lot of pitfalls/bugs waiting for us.
…--
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
|
In the approach we were talking about the background measurement would happen every, let's say, 10 minutes regardless of if the screen is turned on or off. So you could have your screen on for 30 minutes and you would still get 3 measurements, one every 10 minutes. |
|
I get the continuous measurement when the watch face show the heart rate. We want that. Next iteration: we want the measurement to occur every 5 min (for example) OR if the screen is on. The current state machine can be refactored into a background loop and a screen on/off state where a measurements happens if one of the states triggers it (loop finished or screen on). |
patricgruber
force-pushed
the
heartrate-measurements-in-background
branch
from
e498b17 to
5737b95
Compare
|
I squashed the previous state into one commit and added a new commit that splits up the state machine into 3 bools. You can check the difference and decide which one you prefer. I'm honestly torn between the two. One state value that basically forces to use a switch statement that handles all possible cases vs three bools that have a bit less code. The functionality of the two is not quite the same, since the split version now tries to handle the background loop as independently as possible (because of the split, this is possible now). And there is a small bug, which causes the sensor to keep running for a short while after turning the screen off (with enabled measurement), because the sensor is only stopped in the |
|
Is this ready to be tested? |
|
@CutestNekoAqua Yes it can be tested. I would recommend testing the build from the earlier commit c53abd4 |
|
Hello! Thank you so much to everyone for your work on this feature 🥇 ! I've quickly looked through the previous messages and it looks like this PR in nearly ready to be merged!
Did I forget anything? @patricgruber also mentioned a small bug :
Is this bug critical or can we fix / improve this later on? Regarding the state machine implementation (enum vs 3 boolean values) : I checked both implementations, and the one that use enums looks more readable to me : enum makes it clear that this is a state machine with specific states and transitions. All those switch/case are a bit verbose, but they explicitly show the "legal" and "illegal" transitions. Other than that, are there any other things that need to be done before merging? |
|
|
||
| SettingHeartRate::SettingHeartRate(Pinetime::Controllers::Settings& settingsController) : settingsController {settingsController} { | ||
|
|
||
| lv_obj_t* container1 = lv_cont_create(lv_scr_act(), nullptr); |
There was a problem hiding this comment.
Avoid numbers in variable names - I think just container should be OK here?
src/heartratetask/HeartRateTask.cpp
Outdated
|
|
||
| bool HeartRateTask::ShouldStartBackgroundMeasuring() { | ||
| return GetTicksSinceLastMeasurementStarted() >= GetHeartRateBackgroundMeasurementIntervalInTicks(); | ||
| } |
src/heartratetask/HeartRateTask.cpp
Outdated
| return xTaskGetTickCount() - measurementStart; | ||
| } | ||
|
|
||
| bool HeartRateTask::ShoudStopTryingToGetData() { |
There was a problem hiding this comment.
| bool HeartRateTask::ShoudStopTryingToGetData() { | |
| bool HeartRateTask::ShouldStopTryingToGetData() { |
src/heartratetask/HeartRateTask.h
Outdated
| @@ -3,6 +3,9 @@ | |||
| #include <task.h> | |||
| #include <queue.h> | |||
| #include <components/heartrate/Ppg.h> | |||
| #include "components/settings/Settings.h" | |||
|
|
|||
| #define DURATION_UNTIL_BACKGROUND_MEASUREMENT_IS_STOPPED pdMS_TO_TICKS(30 * 1000) | |||
There was a problem hiding this comment.
This should be a (static) constexpr constant rather than a define
| } | ||
| } | ||
|
|
||
| constexpr std::array<Option, 8> SettingHeartRate::options; |
There was a problem hiding this comment.
Is this declaration needed? I think it is provided by the inclusion of the header file, but I could definitely be wrong on that
src/heartratetask/HeartRateTask.cpp
Outdated
| int ms = settings.GetHeartRateBackgroundMeasurementInterval() * 1000; | ||
| return pdMS_TO_TICKS(ms); |
There was a problem hiding this comment.
| int ms = settings.GetHeartRateBackgroundMeasurementInterval() * 1000; | |
| return pdMS_TO_TICKS(ms); | |
| return settings.GetHeartRateBackgroundMeasurementInterval() * configTICK_RATE_HZ; |
src/heartratetask/HeartRateTask.cpp
Outdated
| return ppgDeltaTms; | ||
| } | ||
|
|
||
| return pdMS_TO_TICKS(100); |
There was a problem hiding this comment.
A comment explaining what case this is would be useful (is this polling for background measuring start?)
src/heartratetask/HeartRateTask.cpp
Outdated
| // This doesn't change the state but resets the measurment timer, which basically starts the next measurment without resetting the | ||
| // sensor. This is basically a fall back to continuous mode, when measurments take too long. |
There was a problem hiding this comment.
| // This doesn't change the state but resets the measurment timer, which basically starts the next measurment without resetting the | |
| // sensor. This is basically a fall back to continuous mode, when measurments take too long. | |
| // This doesn't change the state but resets the measurement timer, which basically starts the next measurement without resetting the | |
| // sensor. This is basically a fall back to continuous mode, when measurements take too long. |
src/heartratetask/HeartRateTask.cpp
Outdated
| bool noDataWithinTimeLimit = bpm == 0 && ShoudStopTryingToGetData(); | ||
| bool dataWithinTimeLimit = bpm != 0; |
There was a problem hiding this comment.
Maybe dataTimeout and foundHr or something similar? Currently dataWithinTimeLimit || noDataWithinTimeLimit reads like it will always be true
|
@mark9064 Thanks for this extensive review! EDIT : Oh it looks like you did most of the review based on the |
patricgruber
force-pushed
the
heartrate-measurements-in-background
branch
from
5737b95 to
a06b882
Compare
patricgruber
force-pushed
the
heartrate-measurements-in-background
branch
from
4609da5 to
6d11797
Compare
patricgruber
force-pushed
the
heartrate-measurements-in-background
branch
from
6d11797 to
5c6b08b
Compare
|
👍 I agree the enum implementation is better, thanks for updating it Can I suggest a simplified 3 state machine? The three states are: The only other state tracked is the background period ( The last measurement time is set to the current time:
Then, whether background measuring is needed can be computed by computing the time since the last measurement and comparing that to the background period i.e. The measurement start time (which allows computing the measurement duration) is set to the current time whenever transitioning out of The state transitions are then as follows: Screen off: Every loop iteration we check whether background measuring is needed The only thing left to compute is the loop sleep time. How does this sound? I think this covers all bases while keeping it simple |
|
@patricgruber I have some more free time now so I'm very happy to pick this ticket up if you'd like? |
|
@mark9064 Thank you for offering that! I currently don't have a lot of time and probably won't have much time in the next months. I also have issues with my hardware, so I can't even test properly right now. |
|
Opened #2322 based on this one with the reworked state machine |
This implements heart rate measurements when the screen is turned off.
The ticket I found for this is: #183
When starting the heart rate measurements through the HeartRate screen and turning off the screen, the heart rate task doesn't stop but keeps running in the background until the heart rate measurement is stopped through the screen again.
The task wait delay is set to ~10 seconds (10k ticks) so the task doesn't run all the time and drains the battery too much.
Already tested it on my PineTime and works great.
Right now it was more a proof of concept, therefore the interval between background measurements is hard-coded to ~5 minutes. But I'd be happy to implement a settings screen to configure the interval or other features that are wanted.