Background HR

[mark9064]

Largely based on #1718

Changes:

• 4 state machine
• Heart rate display avoids stale values being shown for a long time in some scenarios
  • does not cover all scenarios, but hopefully good enough and as close to the current behaviour as possible while still making sense for background measurement
• Constant frequency HR measurements that take into account successful screen on measurements (and also long periods of unsuccessful screen on measurements)
• Refactored a few annoying bits of the PPG code like lastBpm and the bodge to get correct not enough data messages
• Removed the 15s mode. The sensor already takes ~6s to get a measurement in ideal conditions so it makes sense to use continuous - the sensor on time will be very similar and proper history averaging with continuous means measurement accuracy is better
• Misc code bits such as using std::optional more where sensible. I haven't used it in the settings struct as the layout is compiler dependent AFAIK

Accuracy improvements (second commit)

• Sampling is constant frequency instead of delay based
• Raised the priority of the HR task to 1 so that it does not get out-competed by DisplayApp

This is still more complex than I'd like it to be

[github-actions]

Build size and comparison to main:

Section	Size	Difference
text	381180B	1112B
data	944B	0B
bss	22552B	8B

Run in InfiniEmu

[mark9064]

Sim fix should be trivial, just a method rename it seems

[tituscmd]

Merged this onto my main device and will try it out compared to the "old"(er) system. Anything to look out for in particular?

[mark9064]

I'm most interested in hearing whether it feels natural in terms of how it behaves when you have screen on measurements interacting with background measurements. Also any bugs ofc :)
Thanks for testing!

[tituscmd]

Two things I can definitely say already:

1. I'm a big fan of the feature that, after about 30 seconds with not enough data, the measurement gets set to failed. That way the amount of wrong measurements has been reduced a good bit
2. I don't know if this is actually a causation, but I haven't had a reboot in 19hrs on a busy day like today where I usually get a reboot every few hours. This might have been fixed by a bug that was in the old version of background HR, or maybe I messed up the merge when I first merged background HR onto my device (long ago, more towards the beginning of my PineTime journey so I was definitely less experienced and the chances of me having messed up there aren't small)

I'll keep an eye out on 2. and on bugs in general of course.

PS: Now that measurements can fail, the value will be set to 0 and look awkward on the watch face. I solved it like this and I think this might be a good idea going forward (of course, again, the amount of dashes (-) is debatable)

[tituscmd]

Thinking about it, it would actually make a lot of sense if this updated background HR fixes my reboots:

Most of the reboots I have encountered have been when I am moving. I usually get a lot on the bike, and not so many just sitting at home. Sometimes when I'm sick and don't go out at all, I noticed uptimes of up to multiple days. All of this points to the idea that the reboots have something to do with me moving.
I originally thought that something was up with the accelerometer - of course I did, because the accelerometer is specifically for movement. But now that I thought about the updated background HR more today, maybe that's not the case.

The HR sensor on PineTime is notoriously not very good, if it works at all, when moving. That leads me to believe, that maybe the background HR was responsible for my reboots.
The new version of background HR (this PR) has the fail save function to mark measurements as failed when there hasn't been enough data for 30 seconds.
On the old version, I could imagine me being on the bike, a background measurement gets called and, since I'm moving so much, it can't gather enough data and gets stuck in that measurement loop and eventually crashes.

So maybe the fail save function in this PR has fixed that. Has anybody noticed similar behavior?

[mark9064]

PS: Now that measurements can fail, the value will be set to 0 and look awkward on the watch face. I solved it like this and I think this might be a good idea going forward (of course, again, the amount of dashes (-) is debatable)

I'd very much welcome a PR which replaces zero with - everywhere. And TBH we should also refactor it all to expect std::optional<uint8_t> rather than uint8_t so we can discard the hacky zero value altogether

[WhyNotHugo]

PS: Now that measurements can fail, the value will be set to 0 and look awkward on the watch face. I solved it like this and I think this might be a good idea going forward (of course, again, the amount of dashes (-) is debatable)

I'd very much welcome a PR which replaces zero with - everywhere. And TBH we should also refactor it all to expect std::optional<uint8_t> rather than uint8_t so we can discard the hacky zero value altogether

See: #2342

[JF002]

This looks very similar to what I POC'ed a while ago.

This is still more complex than I'd like it to be
Yeah, I also thought I could make this simpler, but... complexity increased as soon as I tried to handle all use-cases as well :-)

And I think this PR makes a great use of std::optional, which is great!

[mark9064]

OK should be good to go now. Will try get InfiniSim PR up soon

[WhyNotHugo]

@mark9064 InfiniTimeOrg/InfiniSim#180 might serve as a base :)

[Koman10]

@JF002 Hoi, ik zou graag even privé met je praten. Heb je een momentje?

[mark9064]

Sim PR is up InfiniTimeOrg/InfiniSim#184

Compiles locally
Merge this first, then bump the sim InfiniTime version? (@NeroBurner let me know what works best for you, I don't use the sim too often 😅 )

[NeroBurner]

either make the sim work for both code versions or I'll merge the sim PR after this one is merged