Ring Buffer for Heart Rate Data #1092
Conversation
we can replace dataIndex in ProcessHeartRate, because it was only being called if dataIndex is 200 anyway (see line 61-67) and the Trough function expects its second argument to be the size and not an index.
changed data Type of heartrate to int, it is only used as an int anyway
|
The change also requires very little extra processing power, one add and one modulo more per array access, the array access is much more expensive in that case. |
I guess nothing prevents us from making UPDATE_HEARTRATE_AFTER and DATA_SIZE powers of two, because i think that 200 is an arbitrary number. Most compilers will automatically optimize the "modulo" operation into an "and" operation. |
|
DATA_SIZE is not arbitrary I think, because the range of valid bpms depends on how many datapoints there are. That's how I read this comment. |
|
As far as I understand (upd: it was a wrong understanding, as explained below), this is because the heart rate is unlikely to exceed 200 bpm. Also it's to count beats in a minute to measure bpm. One can count beats in an arbitrary time period, and then multiply by an adjusting constant. |
|
The fast bpms would show up in the low indices I think. I think the number 200 is chosen because in the process function of PPG the max resulting index could be around 200 (see here, t3+4 would be the highest index (which I got at a max of 210, don't really know why) Making it bigger won't break anything, it might not be very useful though and uses a few bytes of memory. We should maybe still do it though because if t3+4 goes to 210 the heart rate measurement will be completely off (and it will access memory outside of the array before this pr). |
There was a problem hiding this comment.
A couple observations.
The small changes to dataIndex are hard to notice and review. They're also not really necessary.
In ProcessHeartRate(), dataIndex and DATA_SIZE would be the same value without the slight dataIndex change. DATA_SIZE may be slightly more correct, but only before the actual behaviour is changed.
But the more I think about it, this PR doesn't actually change much apart from adding getRingIndex() does it? Maybe it would be better to add and start using the ring buffer in one PR. As it is, these changes are hard to justify. The code improvements would be a nice separate PR.
| lpf {0.11595249, 0.23190498, 0.11595249, -0.72168143, 0.18549138} { | ||
| } | ||
|
|
||
| int Ppg::Compare(int8_t* d1, int shift, size_t count) { |
There was a problem hiding this comment.
Why was this function changed, apart from adding getRingIndex()?
There was a problem hiding this comment.
in the original implementation the pointer was moved forward and then accessed as a 0-based array. That wouldn't work in the ring buffer, because the actual index has to be known to modulo it.
|
First of all thanks for the review :) I agree that the PR doesn't really have a justification to be merged yet, I just implemented it because it might be useful for other future features, which I wanted to start working on (e.g. #1094). At the moment |
|
This definitely seems useful for e.g. higher frequency updates on the display. |
|
@watertrainer are you still planning to work on this? i'm available to help. |
|
Yes, if you have any ideas how to make this useful for the user I'd be more than happy to work on it together. |
|
@watertrainer has this been solved elsewhere? |
|
not that I'm aware of, although there were some heart rate changes a few versions ago, not to sure haven't been following to closely |
This implements a simple Ring Buffer for the Preprocessed Heart Rate Data, so that actually processing it can always take the last 200 datapoints. Before, after 200 datapoints were collected, all the data was discarded and new data was collected. This had, imo, a few issues.
Note that this Pull Request doesn't change any of the user experience/behaviour of the watch yet. A new Heart Rate value is still only calculated and provided every 200 datapoints (4 Seconds at 25Hz, which is the speed I think, although I read here, that each cycle takes 12,5ms, which would mean 80Hz). This pull request would just make future implementations for working with HeartRate data much easier in my opinion.
I also cleaned up and fixed all the clang-tidy complaints in the file I wrote code in, I don't know if that belongs in a different request.
This is also basically my first cpp code, so any help/critique regarding code style etc. is greatly appreciated.