Heartrate measurements in background #1718

patricgruber · 2023-03-31T09:58:01Z

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.

github-actions · 2023-03-31T10:08:23Z

Build checks have not completed. Possible reasons for this are:

The checks need to be approved by a maintainer
The branch has conflicts
The firmware build has failed

eliedrian · 2023-04-02T05:03:37Z

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.

lman0 · 2023-04-02T13:27:13Z

Another idea that I was thinking of was to measure heart rate after a number of steps taken.

This idea should be a setting and not a default,
Because there is people with leg disability that could use the pine time with the heart rate.
And this people wouldn't be have the heart rate function.
But for fitness related event, without leg disability it could be .

patricgruber · 2023-04-02T15:30:42Z

Another idea that I was thinking of was to measure heart rate after a number of steps taken.

This idea should be a setting and not a default, Because there is people with leg disability that could use the pine time with the heart rate. And this people wouldn't be have the heart rate function. But for fitness related event, without leg disability it could be .

It could maybe be an additional trigger for measuring, but not the only one.

btdogan · 2023-04-03T18:29:58Z

I use hr monitor for my sleep. this shouldn't prevent that.

patricgruber · 2023-04-03T19:27:33Z

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.

patricgruber · 2023-04-03T19:33:46Z

I noticed during testing that notifications interrupt the background measurement delay and make it reset, so I removed the reset for the GoToSleep message.
Now the behavior is slightly different as the background measurement will every 5 minutes from the last background measurement or as soon as the watch is going to sleep. Instead of measuring every 5 minutes starting from the time it goes to sleep.

Notifications and "just checking the time quickly" won't reset the delay now and the measurements will run more consistently over time.

LinuxinaBit · 2023-04-15T21:59:44Z

This idea should be a setting and not a default, Because there is people with leg disability that could use the pine time with the heart rate. And this people wouldn't be have the heart rate function. But for fitness related event, without leg disability it could be .

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.
Maybe in the HR app?

patricgruber · 2023-04-16T21:11:14Z

Maybe in the HR app?

The background measurements are only running, if the user activates the heart rate measurement in the HR app.
If the user doesn't activate the HR measurements in the HR app or if they are disabled again, no background measurement will be taken.

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.
If the HR measurement is not activated in the app no measurement is taken ever.

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").
Only for the case when the measurement is activated in the app AND the screen is off, then the new mode "measure every 5 minutes" is activated.

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.
I think that would be a bit confusing if you have to activate two things in separate places to activate the background measurements. But if that is needed and wanted I'll of course add it.

LinuxinaBit · 2023-04-16T22:32:36Z

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.
Maybe wait for 3 measurements and average the last two, as well as checking for excess movement from the accelerometer before measurement.

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.

Itai-Nelken · 2023-04-17T10:41:07Z

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.

After #1486 is merged, measurements will be almost instantaneous and much more accurate.

LinuxinaBit · 2023-04-18T21:20:48Z

Alright, though I still think excessive and no movement detection should be added to preserve some amount of battery life...

pankk · 2023-04-20T05:58:15Z

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".
(I assume mentioning the PPG PR here is enough and that I don't have to add this feedback to that PR as well.)

khimaros · 2023-05-15T22:19:19Z

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 :)

khimaros · 2023-05-16T13:55:59Z

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 on

i 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.

lman0 · 2023-05-16T13:58:57Z

@khimaros could you share the firmware ?
I would like to test it too

khimaros · 2023-05-16T14:21:53Z

@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

mark9064 · 2023-05-16T16:19:41Z

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

Previous HR values are changed to no longer be cleared upon screen wake with this PR, or upon the PPG algorithm losing fix on the HR
- I think this is confusing as it is difficult to tell when the HR value on screen is fresh and when it is the last reading.
- It's also difficult to tell when it goes from a live reading to holding the last value when the PPG algorithm can't find a value
The running time is unlimited when doing a background measurement
- In the extreme case, if there is no sensor contact, the PPG will run continuously

Suggestions

I think the UI needs to convey that a HR measurement is stale, or it should not show it at all
- Not sure how this applies to all watchfaces
Limit runtime to 15-20s
- Can be done by checking the running duration using the tick count inside HandleSensorData

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

Measurement begins every 5 minutes, ignoring whether the device has been woken or not (close to what is currently implemented)
- In this case it needs to be wary of a possible BackgroundMeasuring -> (screen on) Measuring (total time < limit) -> (screen off) BackgroundWaiting
  - A measurement is then skipped
- Also possible to have Measuring -> background timer expires -> (screen off) BackgroundWaiting -> BackgroundMeasuring
  - i.e the PPG stops and then immediately gets started after, discarding all state and forcing a refix
Measurement runs at least every 5 minutes, taking into account measurements from being awake
- In other words, every time a HR fix is achieved, the measurement timer is set to expire in 5 minutes
- The other condition is that the expiry timer must be reset when a background measurement is attempted but fails due to the time limit
- This way solves the second problem above by design and also seems more sensible to me intuitively
  - I think solving the first problem might require another state?

Not sure if I made any sense so interested to hear any thoughts :)

khimaros · 2023-05-17T02:35:49Z

@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?

khimaros · 2023-05-17T02:41:58Z

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

patricgruber · 2023-05-17T08:27:14Z

Thanks for all the suggestions.

Regarding the timeout reset for background measurements:
Before I implemented the checks so that whenever a measurement started, the timer reset. Which was a problem since just checking the time reset the timer.
I then changed it to the current implementation that only resets the timer when specifically a background measurement is done. This way having the screen up doesn't effect the background measurement at all and so if the watch goes to sleep, the code pretends as if nothing happened and will immediately measure the heart rate if the screen was on for longer than the interval.
A nice change would be to change it so whenever a measurement is done, either in the foreground or background, the interval is reset.

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.

mark9064 · 2023-05-21T23:23:37Z

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

patricgruber · 2023-05-25T20:56:43Z

@mark9064
I would just implement it to always stop trying to measure after 30s, no matter if it is because of ambient light or other reasons.

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.

patricgruber · 2023-05-25T21:43:12Z

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

mark9064 · 2023-05-27T14:05:15Z

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

khimaros · 2023-05-28T19:00:37Z

@mark9064 i'm using your testing branch now on my Pinetime device and it is working quite well. thank you for putting this together and sharing it! i especially like the new raise to wake/lower to sleep functionality. it's very reliable and accurate and i haven't seen any unintended wakes yet.

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.

mark9064 · 2023-07-07T01:28:04Z

Finally got time to do some proper power testing using the patchset in my last message
With low brightness, raise wake and lower to sleep (latest PR versions), sleep mode ~8h/day, wearing watch ~97% of the time: 8 days battery from a full charge. So it would probably be feasible to enable unconditionally if we wanted to, though I think having controls is still nicer

khimaros · 2023-07-07T12:31:55Z

@mark9064 is this using the testing branch at https://github.com/mark9064/InfiniTime/tree/testing ? which measurement mode were you using? periodic? continuous?

patricgruber · 2024-07-11T13:08:15Z

Just rebased to newest main and the build fails with region RAM overflowed with stack.
Seems like the usage increased by 16B but is still within the 64KB

Edit: Fixed it by reducing heap size, like mentioned here: #1867 (comment)
But this probably needs to be fixed

Edit: Should be fixed now

savar · 2024-07-11T15:26:02Z

Just rebased to newest main and the build fails with region RAM overflowed with stack. Seems like the usage increased by 16B but is still within the 64KB

Edit: Fixed it by reducing heap size, like mentioned here: #1867 (comment) But this probably needs to be fixed

Edit: Should be fixed now

Works for me now. Thanks!

I am trying to combine this with OpenTracks to use it as a heart rate monitor. Not sure though if it works correctly.. on a 10s interval the peak was correct, but the avg was with 7bpm aehm.. deadly low.

Tried it once with continuous mode and got peak and avg on 0bmp .. will check on this further.

patricgruber · 2024-07-11T15:31:55Z

To help with understanding the state machine of the heart rate task, I created a state machine diagram (using https://asciiflow.com/) :

               *** Hearrate task state machine ***                
                                                                  
                                                                  
           GoToSleep            StopMeasurement                   
       ┌───────────────┐ ┌─────────────────────────────┐          
       ▼               │ ▼                             │          
    ┌──────┐        ┌──┴──────┐                  ┌─────┴─────┐    
    │      │ WakeUp │         │ StartMeasurement │           │    
    │ Idle ├───────►│ Running ├─────────────────►│ Measuring │    
    │      │        │         │                  │           │    
    └──────┘        └─────────┘                  └─────┬─────┘    
       ▲                                            ▲  │          
       │     StopMeasurement                        │  │          
       ├──────────────────────────────────┐         │  │ GoToSleep
       │                                  │  WakeUp │  │          
       │     ┌────────────────────────────┼─────────┤  │          
       │     │                            │         │  ▼          
    ┌──┴─────┴──────────┐                ┌┴─────────┴──────────┐  
    │                   │    measured    │                     │  
    │ BackgroundWaiting │◄───────────────┤ BackgroundMeasuring │  
    │                   │                │                     │  
    └┬─────────────────┬┘                └─────────────────────┘  
     │        ▲        │                            ▲             
     └────────┘        └────────────────────────────┘             
wait time < interval       wait time >= interval

patricgruber · 2024-07-11T15:38:09Z

I am trying to combine this with OpenTracks to use it as a heart rate monitor. Not sure though if it works correctly.. on a 10s interval the peak was correct, but the avg was with 7bpm aehm.. deadly low.

Tried it once with continuous mode and got peak and avg on 0bmp .. will check on this further.

I'm using Gadgetbridge for the recording of my heartrate and there it works fine. But there a reported heartrate of 0 will be ignored (https://codeberg.org/Freeyourgadget/Gadgetbridge/src/commit/c9326ca4470581a1d0a4ce710c04ffafc0e41164/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/pinetime/PineTimeJFSupport.java#L1151)

minacode · 2024-07-11T16:01:13Z

Could we just not report zeros?

savar · 2024-07-11T17:33:46Z

I am trying to combine this with OpenTracks to use it as a heart rate monitor. Not sure though if it works correctly.. on a 10s interval the peak was correct, but the avg was with 7bpm aehm.. deadly low.
Tried it once with continuous mode and got peak and avg on 0bmp .. will check on this further.

I'm using Gadgetbridge for the recording of my heartrate and there it works fine. But there a reported heartrate of 0 will be ignored (https://codeberg.org/Freeyourgadget/Gadgetbridge/src/commit/c9326ca4470581a1d0a4ce710c04ffafc0e41164/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/pinetime/PineTimeJFSupport.java#L1151)

I would love to continue on this topic but I am not sure this helps with this MR, so @patricgruber please stop us if this goes too far off-topic.

I changed the following and since then the data (but I test further) seems to be correct? (yesterday I had one which was not that correct.. today trying it out for 4-5 times now.. short (2-3mins) and long (20min) are looking good):

diff --git a/src/components/heartrate/HeartRateController.cpp b/src/components/heartrate/HeartRateController.cpp
index e0d69272..29c7fe2d 100644
--- a/src/components/heartrate/HeartRateController.cpp
+++ b/src/components/heartrate/HeartRateController.cpp
@@ -8,8 +8,9 @@ void HeartRateController::Update(HeartRateController::States newState, uint8_t h
   this->state = newState;
   if (this->heartRate != heartRate) {
     this->heartRate = heartRate;
-    service->OnNewHeartRateValue(heartRate);
   }
+
+  service->OnNewHeartRateValue(heartRate);
 }
 
 void HeartRateController::Start() {

but this is probably again not the right MR to discuss/check on this.

UPDATE1/2: deleted (not relevant)

UPDATE3: using bluetoothctl and using the notify thing, I can still see that the measurement sometimes kind of stops.. and then continue again.. the screen was not enabled in the meantime. Once it didn't start for a longer time but started as soon as the screen switched on.. I don't know yet what the issue is.

UPDATE5: At the moment, it looks promising, but I also changed the thing I mentioned in my other comment, so I cannot tell if this actually makes the difference.

src/heartratetask/HeartRateTask.cpp

Co-authored-by: Simon Effenberg <[email protected]>

minacode

Does the measurement work in the background while another app is open?

minacode · 2024-07-20T09:04:40Z

src/heartratetask/HeartRateTask.cpp

+    if (state == States::BackgroundWaiting) {
+      HandleBackgroundWaiting();
+    } else if (state == States::BackgroundMeasuring || state == States::Measuring) {
+      HandleSensorData(&lastBpm);
    }


Why aren't you using a switch here?

uses a switch now

minacode · 2024-07-20T09:27:58Z

src/heartratetask/HeartRateTask.h

+      enum class States {
+        // Screen turned off, heartrate not measured
+        Idle,
+        // Screen turned on, heartrate app open, heartrate not measured
+        Running,
+        // Screen turned on, heartrate app open, heartrate actively measured
+        Measuring,
+        // Screen turned off, heartrate task is waiting until the next measurement should be started
+        BackgroundWaiting,
+        // Screen turned off, heartrate actively measured
+        BackgroundMeasuring
+      };


I think Running is confusing, because it's the state where the measurement is not running.

What do you think about those?

BackgroundIdle ForegroundIdle ForegroundMeasuring BackgroundWaiting BackgroundMeasuring

I don't know if I get all the states correctly, but ForegroundWaiting does not exist? Otherwise we could split the states into two enums (at least logically).

ForegroundRunning (or just Running) means that you have the App open (Running) ~~and you watch for the number on the screen.. in that moment you want to continuously measure the heart rate and therefore there is no reason to have a ForegroundWaiting~~

Makes sense. But according to the comments, Running is the state without active measuring.

yeah sorry, you are right, it is not running in that way... I guess it is mainly because it was used before for that and the HeartRateController is using this state as well. I understand the confusion and maybe it would be good to change it, but it also changes the whole thing further 🤔 .. I am torn.

Let's not keep technical debt because of convenience. This is our chance to give it a more speaking identifier.

I actually agree that technical debt should be cleaned up right now when we have the chance. Otherwise it will probably never be cleaned.
I chose some better names (I think). But if you have other suggestions, it's easily changed

tituscmd · 2024-08-30T12:36:14Z

What is stopping this PR from getting merged? It's one of the, if not the PRs that have changed the usability of my watch my a mile.

minacode · 2024-08-30T13:03:39Z

@JF002 not getting paid for this. There are other things being worked on right now and one can only do so much, I guess.

tituscmd · 2024-08-30T14:11:34Z

I know he isn't getting paid for it. I didn't mean to sound demanding, if that was the case. I was just wondering, since it seems done.

minacode · 2024-08-30T16:30:28Z

Yeah, on second read my message sounds harsh. I didn't mean to, sorry.

Have the last questions/posts been addressed? The more polished the PR is, the less time must be invested from the core maintainers.

…ricgruber/InfiniTime into heartrate-measurements-in-background

patricgruber · 2024-08-30T22:56:12Z

Sorry for the long delay since my last update to the PR. I added the suggested fixes

tituscmd · 2024-08-30T23:08:28Z

Yeah, on second read my message sounds harsh. I didn't mean to, sorry.

Have the last questions/posts been addressed? The more polished the PR is, the less time must be invested from the core maintainers.

Don't worry about sounding harsh, I didn't catch it like that 😄

mark9064

Looking good overall! I haven't reviewed the state machine, but hopefully I will have time to soon

mark9064 · 2024-08-30T23:16:24Z

src/components/settings/Settings.cpp

@@ -8,7 +8,6 @@ Settings::Settings(Pinetime::Controllers::FS& fs) : fs {fs} {
 }

 void Settings::Init() {
-


Accidental whitespace removal?
Or if this is on purpose, also remove from below

Removed the other whitespace

mark9064 · 2024-08-30T23:19:20Z

src/components/settings/Settings.h

+      enum class HeartRateBackgroundMeasurementInterval : uint8_t {
+        Off,
+        Continuous,
+        TenSeconds,


Ten seconds is not a good option to expose: the PPG takes at least 6.4s to get a value and there's honestly no point turning the sensor off for 3.6s, just leave it on and enjoy much more stable heart rate values as the spectral averaging won't get reset every cycle

If that's the case then it would make sense to have a different interval instead. Which interval would you propose instead?
Would 15s make more sense?

I think having 30 as the minimum is OK. The sensor is already allowed to run for up to 30s in the background if it can't find a good signal, in which case 30s mode becomes the same as continuous

The formatting of the settings screen only really works with 2*n items
So it would be nice to have another value instead.
Maybe 15s ? that would be roughly 50% off time and 50% on time for the sensor.
And longer values than 30m doesn't really make sense anymore I think, but I'm also down to add one hour as an option

Hmm how does it look with an odd number of items? If there's a rendering bug it should be fixed, I could take a look

I misinterpreted the timings actually. It's not the measurement-measurement interval as I thought but actually the measurement finished (got value or given up) to measurement start waiting interval

Is that intended? If I enabled this feature at say 1 minute, I would expect a reading to arrive every 1 minute rather than every 1 minute 20 seconds (if it took 20s to find the HR)

If it's not intended then I think 15s is also a bit pointless, but if it is then I think it could just about make sense

There are two reasons I think short measurement times are bad:

It takes a minimum of 6 seconds for the heart rate to be found, and often more if the signal isn't super clean

The heart rate algorithm becomes more accurate the longer it is running

So sensor duty cycles (% of time passing with sensor on) between say 50-100% are a bit pointless IMO, just run the sensor all the time and receive the benefits of always having a value, and that value being more stable and reliable

It is not a perfect interval setting (meaning setting it to 1 minute = one value every minute). It's just a quick and easy solution to get the feature of repetitive measurements in the background.

I think it is difficult to make it more accurate. Since it can't be determined beforehand how long the sensor needs to measure until enough data is captured right? I mean there is an upper limit of 30s (for which the heart rate task just skips the current measurement) and there is a lower limit of around 6s, but that's still a wide range of measuring time. I think the current implementation is easy and quite robust in the regards that you know "If there was a measurement, then after x time another measurement is started"
Otherwise you would need to store the timestamp of the last measurement, and predict how long the sensor needs to take for the next measurement and start preemptively like (predicted time: 10s, interval time: 15s -> actual wait time: 5s) but this just moves the possible wait time from [21s, 44s] to [11s, 34s]. So we could decide to have a hard coded "expected sensor time"
The best approach would be a running average of the last n "sensor times" which would probably be the best predictor, but this adds two additional variables and one additional magic number (window size | n), which increases the added size to the binary. It's not a lot of course, but I had to already move an integer from a instance variable to a local variable that is changed via pointer and do other things to keep the size under the limit.

I'm fine with having a more complicated algorithm, but it seems a lot of effort for probably not a small positive effect, since I assume most people don't need a very precise interval. Also it only really matters for smaller values. If you use a 5 minute interval, another 6-15s is not really that important.

I'm also assuming, that having the sensor running all the time wastes battery. If you just want a HR roughly every 15 seconds, then having 15s wait + 6s measurement means having the sensor 71% of the time off. Which is quite a lot better than having it run all the time. Even if it degrades to 15s wait + 30s measurement, it's still 33% of the time deactivated. Instead of always on.
Even if the output quality might be a bit less accurate. There is always the option of continuous mode if someone wants more and better data.
And the code would have to decide based on the chosen interval setting if the sensor should be running all the time or have breaks in between, which makes it more complicated again.

For longer interval times, this would mean that the sensor is restarted for every measure, but then you again just get the bad "warm-up" data, so if the imprecise data that is retrieved from the sensor that has just been started is a problem, then the longer interval times shouldn't exist anyway (or the the whole PR needs to be a lot more complicated). Because having the sensor running all the time for measurements every 10 minutes seems like a waste of energy.

This PR is open quite for a long time already and I think small improvements can be discussed much more thoroughly and implemented way better after more people had the chance to test it.

But again I'm open for suggestions for making the code better and if anything (like an inaccurate interval time) is a deal breaker, I will of course fix it!

Ah sorry, what I was thinking was actually something different. I agree that trying to predict measurement time would be a bad idea (too complex), and having cases where runtime degrades to continuous is not ideal (for longer periods).

What I meant was storing the start time of the measurement, and then starting the next measurement after start+period, regardless of whether the last measurement succeeded or failed. So if 1 minute measurement is set (and all measurements succeed), after 1 hour 60 measurements will have been made. Currently, if each measurement took 20s, only 45 measurements would be made in an hour. This does cause any measurement period <= MAX_INTERVAL (30s) to degrade to continuous when heart rate cannot be detected.

What do you think of this?

If you're hitting memory limits, shrinking the heap wouldn't be unreasonable BTW, there is currently very little static memory free and the heap will probably be shrunk by 1K or 512B soon anyway

Yes, please start a measurement every x seconds. That is what users will expect to happen.

Implemented as discussed

src/heartratetask/HeartRateTask.cpp

mark9064 · 2024-08-30T23:23:39Z

src/heartratetask/HeartRateTask.h

+      void HandleSensorData(int* lastBpm);
+
+      TickType_t GetHeartRateBackgroundMeasurementIntervalInTicks();
+      bool IsContinuosModeActivated();


Suggested change

bool IsContinuosModeActivated();

bool IsContinuousModeActivated();

mark9064 · 2024-08-30T23:28:17Z

src/heartratetask/HeartRateTask.cpp

+    case HeartRateTask::States::ScreenOnAndStopped:
+      return pdMS_TO_TICKS(100);
+    case HeartRateTask::States::ScreenOffAndWaiting:
+      return pdMS_TO_TICKS(10000);


Does a 10 second delay cause considerable jitter for the 30 second option?

Changed to 1000ms

mark9064 · 2024-08-30T23:30:40Z

src/heartratetask/HeartRateTask.cpp

+    case HeartRateTask::States::ScreenOffAndMeasuring:
+      return ppgDeltaTms;
+    case HeartRateTask::States::ScreenOnAndStopped:
+      return pdMS_TO_TICKS(100);


Can this be portMAX_DELAY? If the screen is on and the measuring is stopped, is it true that the only event that causes a change is StartMeasurement?

Changed to portMAX_DELAY

fallaciousreasoning · 2024-09-03T10:25:20Z

src/components/settings/Settings.h

    private:
      Pinetime::Controllers::FS& fs;

-      static constexpr uint32_t settingsVersion = 0x0007;
+      static constexpr uint32_t settingsVersion = 0x0008;


Does this need to be bumped to 0x0009? Looks like 0x0008 is in main already:
https://github.com/InfiniTimeOrg/InfiniTime/blob/main/src/components/settings/Settings.h#L304

Suggested change

static constexpr uint32_t settingsVersion = 0x0008;

static constexpr uint32_t settingsVersion = 0x0009;

Yes you are right. Thank you!
I will keep this suggested change open until the PR is otherwise ready to merge.

I bumped the settings version a few times already and I don't want to do it every time there is the settings file in main anymore.

patricgruber force-pushed the heartrate-measurements-in-background branch from b967f9b to faec69e Compare May 11, 2023 21:47

patricgruber added 4 commits July 11, 2024 15:06

bump settings version

ffc5f96

fix bug where settings open pair pin screen

7cf4f6e

fix settings screen

6a0276f

refactor heartrate task (switch cases, comments with explanation)

6169263

patricgruber force-pushed the heartrate-measurements-in-background branch from 6324a77 to 6169263 Compare July 11, 2024 13:06

reduce RAM size

e6f0a89

savar reviewed Jul 11, 2024

View reviewed changes

src/heartratetask/HeartRateTask.cpp Outdated Show resolved Hide resolved

keep measuring when transitioning to background

78af44e

Co-authored-by: Simon Effenberg <[email protected]>

minacode reviewed Jul 20, 2024

View reviewed changes

patricgruber added 5 commits August 31, 2024 00:39

use switch case

cedca79

Merge branch 'heartrate-measurements-in-background' of github.com:pat…

0978964

…ricgruber/InfiniTime into heartrate-measurements-in-background

use switch case

71b31c7

remove unnecessary file

b846547

use better state names

9501d36

mark9064 suggested changes Aug 30, 2024

View reviewed changes

integrate code review

7df3999

fallaciousreasoning reviewed Sep 3, 2024

View reviewed changes

use interval as interval, instead of wait time

462ea11

		@@ -8,7 +8,6 @@ Settings::Settings(Pinetime::Controllers::FS& fs) : fs {fs} {
		}

		void Settings::Init() {

	bool IsContinuosModeActivated();
	bool IsContinuousModeActivated();

	static constexpr uint32_t settingsVersion = 0x0008;
	static constexpr uint32_t settingsVersion = 0x0009;

Heartrate measurements in background #1718

Are you sure you want to change the base?

Heartrate measurements in background #1718

Conversation

patricgruber commented Mar 31, 2023

github-actions bot commented Mar 31, 2023 • edited Loading

eliedrian commented Apr 2, 2023

lman0 commented Apr 2, 2023

patricgruber commented Apr 2, 2023

btdogan commented Apr 3, 2023

patricgruber commented Apr 3, 2023

patricgruber commented Apr 3, 2023 • edited Loading

LinuxinaBit commented Apr 15, 2023

patricgruber commented Apr 16, 2023 • edited Loading

LinuxinaBit commented Apr 16, 2023 • edited Loading

Itai-Nelken commented Apr 17, 2023

LinuxinaBit commented Apr 18, 2023

pankk commented Apr 20, 2023

khimaros commented May 15, 2023 • edited Loading

khimaros commented May 16, 2023 • edited Loading

lman0 commented May 16, 2023 • edited Loading

khimaros commented May 16, 2023

mark9064 commented May 16, 2023

khimaros commented May 17, 2023

khimaros commented May 17, 2023

patricgruber commented May 17, 2023

mark9064 commented May 21, 2023

patricgruber commented May 25, 2023

patricgruber commented May 25, 2023 • edited Loading

mark9064 commented May 27, 2023 • edited Loading

khimaros commented May 28, 2023 • edited Loading

mark9064 commented Jul 7, 2023

khimaros commented Jul 7, 2023

patricgruber commented Jul 11, 2024 • edited Loading

savar commented Jul 11, 2024

patricgruber commented Jul 11, 2024

patricgruber commented Jul 11, 2024

minacode commented Jul 11, 2024

savar commented Jul 11, 2024 • edited Loading

minacode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minacode Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

savar Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tituscmd commented Aug 30, 2024

minacode commented Aug 30, 2024

tituscmd commented Aug 30, 2024

minacode commented Aug 30, 2024

patricgruber commented Aug 30, 2024

tituscmd commented Aug 30, 2024

mark9064 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fallaciousreasoning Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 31, 2023 •

edited

Loading

patricgruber commented Apr 3, 2023 •

edited

Loading

patricgruber commented Apr 16, 2023 •

edited

Loading

LinuxinaBit commented Apr 16, 2023 •

edited

Loading

khimaros commented May 15, 2023 •

edited

Loading

khimaros commented May 16, 2023 •

edited

Loading

lman0 commented May 16, 2023 •

edited

Loading

patricgruber commented May 25, 2023 •

edited

Loading

mark9064 commented May 27, 2023 •

edited

Loading

khimaros commented May 28, 2023 •

edited

Loading

patricgruber commented Jul 11, 2024 •

edited

Loading

savar commented Jul 11, 2024 •

edited

Loading

minacode Jul 20, 2024 •

edited

Loading

savar Jul 21, 2024 •

edited

Loading

fallaciousreasoning Sep 3, 2024 •

edited

Loading