InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.69k stars 921 forks source link

Refactored Ppg for frequency based algorithm. #1486

Closed Ceimour closed 1 year ago

Ceimour commented 1 year ago

Changed driver settings in HRS3300.cpp for better signal to noise at 10Hz. Changed measurementStarted delay to 100ms in HeartRateTask.cpp. Removed unused ppg methods from HeartRateTask.cpp. Added ambient light sensor acquisition and handling in HeartRateTask.cpp. Added FFT and interpolation Ppg support code from Arduino FOSS implementations. Removed currently unused time domain based code. Changed all doubles in PPG/Arduino code to float type so that FPU is used.

Ceimour commented 1 year ago

Complete refactoring the heart rate code. This has been tested against a predicate device and looks to be typically within +/-1bpm when heart rate is stable. This is my first pull request so I would appreciate any guidance on moving forward.

minacode commented 1 year ago

There are files under the Apache License 2.0. Are they compatible with this project?

Edit: seems like adding Apache 2.0 code to GPL code is compatible, but I am no lawyer. Link

minacode commented 1 year ago

Since this PR contains a lot, any sort of documentation helps you to potentially get this through.

For the start it think you should describe a little why you did what you did to get an overview of the different changes. The maintainers don't have much time so presenting your ideas and reasons well is very helpful.

Then we could discuss if we like the changes and if they could maybe separated into smaller pull requests.

Also please tell us what this PR improves.

Ceimour commented 1 year ago

Thanks for taking a look at this!

According to Apache.org (first 2 paragraphs) we are good to use Apache V2.0 within GPLV3, just not the other way around: https://www.apache.org/licenses/GPL-compatibility.html

minacode commented 1 year ago

Ok, cool.

Are you aware that your replies contain some email stuff? No offense, just wanted to tell you in case you didn't notice.

Avamander commented 1 year ago

If replying over email, it's better not to quote the previous message. I've also manually truncated it from people's comments so the thread would look nicer.

Ceimour commented 1 year ago

Motivation: After receiving my new watch I was very impressed with the look, feel and quality of InfiniTime. I expected a hobby project but this something different, something I actually want to use! I experimented with the various features and I noticed the heart rate monitor accuracy issue (like issue 532 below). I set up a dev environment and examined the incoming data. Much of the time it looked really good after Daniel Thompson’s filtering was applied, but then there would sporadic instances of high amplitude noise within the heart rate frequency range. So it’s the old garbage in, garbage out problem. Since I’ve had some experience working in the frequency domain I thought it might be easier to discriminate against the noise using frequency analysis.

Changes:

Improvements:

Cons:

Pull Request: I think this needs to be a single pull request because all the code in the algorithm is integrated to produce a single output. Although it looks like it addresses 532 and coincidentally 1405 and 1092 below.

Applied Algorithm (Ppg::HeartRate): The algorithm is basically similar to the same processing chains found in many papers for extracting information from PPG:

  1. Acquire samples from the HRS3300
    • If acquisition is reset or initial, acquire all 64 samples before processing (before going to step 2)
    • If acquisition is not reset, acquire 8 (overlapWindow) samples (before going to step 2)
  2. Detrend the dataset
    • Remove the slope
    • Convert to the difference between each sample
    • Turns the problematic large, DC shifts into single sample spikes
  3. Bandpass filter the data 30 to 240Hz
    • Removes low frequency data that can interfere with low HR detection
    • Removes the high frequency spikes from the detrending step
  4. Apply a window function
    • Make the signal appear continuous to the FFT
  5. Calculate the FFT (Fast Fourier Transform)
  6. Calculate the power spectrum (ComplexToMagnitude())
  7. Apply result to spectral average
    • Helps to enhance the more frequently occurring heart rate frequency while diminishing random noise
  8. Check signal to noise ratio (SNR)
    • Nothing complex, just compare max to mean
  9. Search for peaks in the spectrum only if SNR is good,
    • Look for peaks above the peak detection threshold (currently 60% of max)
    • If only one peak above threshold, return the peak position in peakLocation
  10. Check peak width
    • Wide peaks can occur from noise or rapidly changing HR
    • If peak is too wide, set peak location to 0.0
  11. Check if HR is within limits (currently 40-230bpm)
    • If out of range set HR to 0.0
  12. If HR == 0.0, reset spectral averaging for next pass
  13. Set Ambient Light Sensor threshold
    • Used in next pass in (Ppg::PreProcess and HeartRateTask.cpp)
    • HeartRateTask will reset the acquisition if ALS threshold is exceeded (goto 1)
  14. Add current peak location (zero or otherwise) to Ppg::HeartRateAverage and assign the result back to peakLocation
  15. Return result
    • 0.0 is valid HR

    • -1 tells HeartRateTask to reset acquisition
    • 0.0 is no valid peak found this pass
  16. Goto 1 Execution time ~20ms

Relevant feature requests: Heart rate measurement accuracy https://github.com/InfiniTimeOrg/InfiniTime/issues/532

Increase the heart rate data logging interval https://github.com/InfiniTimeOrg/InfiniTime/issues/1405

Ring Buffer for Heart Rate Data https://github.com/InfiniTimeOrg/InfiniTime/pull/1092

Potential feature requests that can be addressed in future pull requests: Zero out HRM when stopped https://github.com/InfiniTimeOrg/InfiniTime/issues/682

Bug: Heart rate monitoring continues while charging https://github.com/InfiniTimeOrg/InfiniTime/issues/594

Sleep tracking https://github.com/InfiniTimeOrg/InfiniTime/issues/307

Heart rate measurement is stops, when the screen is turned off https://github.com/InfiniTimeOrg/InfiniTime/issues/183

Please let me know if you have any questions, need more data or if there are any issues with the code. Thanks!

minacode commented 1 year ago

Your description sounds good as far as I read it. More reliable heart rate data would be a very good thing! Can you restore the formatting that it seemed to have when you wrote it? ;) You can just edit the comment if that is possible for you.

Ceimour commented 1 year ago

Motivation: After receiving my new watch I was very impressed with the look, feel and quality of InfiniTime. I expected a hobby project but this is something different, something I actually want to use! I experimented with the various features and I noticed the heart rate monitor accuracy issue (like issue 532 below). I set up a dev environment and examined the incoming data. Much of the time it looked really good after Daniel Thompson’s filtering was applied, but then there would sporadic instances of high amplitude noise within the heart rate frequency range. So it’s the old garbage in, garbage out problem. Since I’ve had some experience working in the frequency domain I thought it might be easier to discriminate against the noise using frequency analysis.

Changes:

Improvements:

Cons:

Pull Request: I think this needs to be a single pull request because all the code in the algorithm is integrated to produce a single output. Although it looks like it addresses 532 and coincidentally 1405 and 1092 below.

Applied Algorithm (Ppg::HeartRate): The algorithm is basically similar to the same processing chains found in many papers for extracting information from PPG:

  1. Acquire samples from the HRS3300
    • If acquisition is reset or initial, acquire all 64 samples before processing (before going to step 2)
    • If acquisition is not reset, acquire 8 (overlapWindow) samples (before going to step 2)
  2. Detrend the dataset
    • Remove the slope
    • Convert to the difference between each sample
    • Turns the problematic large, DC shifts into single sample spikes
  3. Bandpass filter the data 30 to 240Hz
    • Removes low frequency data that can interfere with low HR detection
    • Removes the high frequency spikes from the detrending step
  4. Apply a window function
    • Make the signal appear continuous to the FFT
  5. Calculate the FFT (Fast Fourier Transform)
  6. Calculate the power spectrum (ComplexToMagnitude())
  7. Apply result to spectral average
    • Helps to enhance the more frequently occurring heart rate frequency while diminishing random noise
  8. Check signal to noise ratio (SNR)
    • Nothing complex, just compare max to mean
  9. Search for peaks in the spectrum only if SNR is good,
    • Look for peaks above the peak detection threshold (currently 60% of max)
    • If only one peak above threshold, return the peak position in peakLocation
  10. Check peak width
    • Wide peaks can occur from noise or rapidly changing HR
    • If peak is too wide, set peak location to 0.0
  11. Check if HR is within limits (currently 40-230bpm)
    • If out of range set HR to 0.0
  12. If HR == 0.0, reset spectral averaging for next pass
  13. Set Ambient Light Sensor threshold
    • Used in next pass to determine if ambient light was detected
    • HeartRateTask will reset the acquisition if ALS threshold is exceeded
  14. Add current peak location (zero or otherwise) to Ppg::HeartRateAverage and assign the result back to peakLocation
  15. Return result based on value of peakLocation
    • 0.0 is valid HR

    • -1 tells HeartRateTask to reset acquisition
    • 0.0 is no valid peak found this pass
  16. Goto 1 via HeartRateTask

Execution time ~20ms

Relevant feature requests: Heart rate measurement accuracy https://github.com/InfiniTimeOrg/InfiniTime/issues/532

Increase the heart rate data logging interval https://github.com/InfiniTimeOrg/InfiniTime/issues/1405

Ring Buffer for Heart Rate Data https://github.com/InfiniTimeOrg/InfiniTime/pull/1092

Potential feature requests that can be addressed in future pull requests: Zero out HRM when stopped https://github.com/InfiniTimeOrg/InfiniTime/issues/682

Bug: Heart rate monitoring continues while charging https://github.com/InfiniTimeOrg/InfiniTime/issues/594

Sleep tracking https://github.com/InfiniTimeOrg/InfiniTime/issues/307

Heart rate measurement is stops, when the screen is turned off https://github.com/InfiniTimeOrg/InfiniTime/issues/183

Ceimour commented 1 year ago

Reformatted pull request description. Not sure what the SOP is here, should I delete the old one and this comment as well?

minacode commented 1 year ago

I don't think it is necessary to delete the comments. Thank you for posting it again! 😊 This will help a lot.

yehoshuapw commented 1 year ago

I didn't quite have as much time to test this as I wanted too, so this will be brief (making the hrs better was something I never quite got to, so thanks!)

it seems to have some issue - every about 10 seconds or so, it resets back to Not enough data.

Ceimour commented 1 year ago

Thanks for testing it! I know the signal gets polluted with quite a lot of noise from movement, heaving breathing, fast heart rate, etc., basically anything that adds a lot of energy. I have also noticed that I have hard time getting a reading when it is cold out and there is a large temperature difference between the top of the watch and my wrist. Anyway, if it continues to be an issue the averaging buffer size can be increased to mitigate. I hope that you and others will have time to try it for a longer period. I've only tried it on a couple of other people, so really looking forward to the feedback. We might also want to think of different instructions to the user like, "Not enough data, please remain still..." or something like that.

JF002 commented 1 year ago

Here is the current measurements while running this branch: image

When the sensor is not running, with the medium brightness, the watch use ~10-20mA (15mA avg). When the sensor is running, the watch draws ~12-30mA (20mA avg). So the sensor roughly draws 5mA in average.

How is the average power consumption compared to the previous code?

The original code draws 24mA avg (12 - 40mA) when the sensor is running, so this new implementation draws 4mA less than the original code, which is nice!

Ceimour commented 1 year ago

Basic Algorithm How and Why

How Since we are working in frequency space we are primarily concerned with Nyquist and frequency resolution. The highest analyzed frequency is 4Hz (240bpm) so we need to acquire data at a minimum 8Hz. Frequency resolution is 1/TotalTime(seconds). I wanted to keep the total acquisition time to somewhere around 5 seconds for good UX. So I ended up at 10Hz with a 64 sample buffer resulting in a frequency resolution of 1/6.4s or 0.15625Hz, multiplied by 60 it is 9.375bpm. Yes that's poor resolution for pulse rate results! Thankfully the pulse rate frequency data has a nice Gaussian distribution and we can interpolate fairly accurate values. That coupled with pulse rate result averaging gives values that agree nicely with my off the shelf pulse oximeter. So we can certainly run at higher rates keeping in mind that we still need to acquire for at least 6.4s to keep the existing frequency resolution. It looks like it's taking ~20ms to run the existing algorithm, so the hard limit would be ~50Hz. Keep in mind that to keep the existing frequency resolution we would need to acquire for 6.4s resulting in a larger data buffer.

Why We can get better results using this method because the metrics for determining good versus bad pulse rate data are simpler. We can just look for a single peak above a defined threshold in a power spectrum that has low noise characteristics (simple max to average in this case) and assume that peak is the pulse. Then an output averaging ring buffer is used to store the good pulse rate values and also store failed pulse rates as zero. When the averaging buffer becomes all zeroes, reset the acquisition as if we had just pressed the "Start" button. So basically we just look at the values we think are good results. There are times one may experience incorrect readings but they should be short lived after physically remaining still and allowing good values to fill the buffer.

lman0 commented 1 year ago

any news ? it's the most precise heart rate counter since infinitme creation. it make the pinetime nearly on par with an spo2 meter (for the heart rate part). when i say nearly on par , in fact it's about 99% of the time the same value that on spo2 meter . it's my feeling and when i compare with a owned spo2meter. i use it for 1 week and the value are way better.

sometime it's restart measuring ... is there any way to avoid this @Ceimour ? (is there any movement that triger this behaviour....?) or maybe it will be resolved with #1092 ?

Ceimour commented 1 year ago

@lman0 , thanks for testing this! You are correct that movement can trigger a reset in the data acquisition. For a future improvement I have implemented an adaptive filter that will mitigate this behavior somewhat. But since this pull request is already quite large I hesitate to add it at this point. A reset is produced when the data is noisy (caused primarily from motion, but the sensor sometimes just outputs bad data) for a long period. By long period, I mean more than 10 seconds. The reset text should say "Acquiring, please remain still..." or something like that (another future pull request). The basic philosophy is to attempt to only display the truth. Although it is averaged so it could be the true pulse rate up to 10 seconds in the past. But I think to display values older than that when bad data has been present for over ~10 seconds is misleading. One more thing that can cause a reset is ambient light changes. Try removing the watch when getting a good reading and you should see an immediate reset. I think that in the future with wording and display changes it will provide a better user experience. It's all about user expectations!

Riksu9000 commented 1 year ago

There are still unaddressed review comments and the PR needs to be cleaned up. Someone from the dev team may end up creating a new PR based on these changes, so we can get it merged soon.

Ceimour commented 1 year ago

@lman0 Thanks for sharing the perception that the pulse rate measurement going into reset while in active measurement mode seems confusing. In this recent commit I have removed that bit of code that invokes the reset while measuring. Please give a try and let me know if it feels like the correct behavior. Thanks again!

lman0 commented 1 year ago

@Ceimour can you check why the action crashed? I can't test your new commit since there us no artifact...

Ceimour commented 1 year ago

@lman0 I see that this repeats until exit with failure: Retrieving check runs named Compare build size on InfiniTimeOrg/InfiniTime@25fe200768eb46bae12beffc75664defccddd6e4... Retrieved 0 check runs named Compare build size No completed checks named Compare build size, waiting for 10 seconds...

Riksu9000 commented 1 year ago

The PR needs to be rebased and commits that fix mistakes squashed. The libs should be added in one commit, and modified in another, so we can see the changes made.

Riksu9000 commented 1 year ago

Mark reviews as resolved when you've resolved them, please.

Ceimour commented 1 year ago

@Riksu9000 Thanks for the instruction! I have rebased and now I find that I am not currently on a branch. My git skills are minimal and I am stuck as to how to complete the following command recommended by git: "git push origin HEAD:(name-of-remote-branch)"

Riksu9000 commented 1 year ago

@Ceimour After rebasing you should still be on the same local branch. Because the history of the local and remote branches are different, you have to use the --force flag when pushing.

Ceimour commented 1 year ago

@Riksu9000 Thanks again for the help! I seem to be missing something (new?) related to build size. I get 1 failing check related to this message: "Retrieved 0 check runs named Compare build size"

Riksu9000 commented 1 year ago

You haven't fixed the conflicts. You need to rebase on top of latest develop. Also the commits aren't clean. Libs are added twice and removed once. The formatting of the libs should be left original.

Ceimour commented 1 year ago

@Riksu9000 OK, I think I understand now. Sorry if my confusion is causing you additional and unnecessary work. I know you are quite busy and I really appreciate your feedback and instruction.

lman0 commented 1 year ago

why did you have closed the pr @Ceimour ?

Ceimour commented 1 year ago

@Riksu9000 I did the update. I assumed I had to run Sync to update to the latest develop but that seemed to automatically close the PR. Please let me know if I need to do anything to fix this. Thanks.

Ceimour commented 1 year ago

@lman0 Sorry about that, I'm clearly not getting git! Waiting for help.

lman0 commented 1 year ago

@lman0 Sorry about that, I'm clearly not getting git! Waiting for help.

inside this link is explained how reopen pr (scroll for picture) @Ceimour

Added link with command git https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c it should help you

Ceimour commented 1 year ago

@lman0 I was instructed by @Riksu9000 to force push and that appears to have closed the PR. It looks like I could add commits and then it would enable the Reopen but I will wait for further instruction. If I did something wrong in the rebase force push cycle, I just want to make sure I don't do any more damage!

Avamander commented 1 year ago

It closed because there are no new commits in this branch.

Ceimour commented 1 year ago

@Avamander Thanks for pointing me in the right direction! When I rebased it somehow detached from the branch and the PR appeared to close after pushing. So I went ahead and cloned and was restored to the feature branch. I then made a minor change, committed and pushed. So far I still have the disabled "Reopen" button here. Is there another method of reopening this PR? I see an option in the main Pull requests tab that recognizes that I have a recent push, but it appears that the only option is to create a PR (nothing indicating reopening).

lman0 commented 1 year ago

I have stumbled on this , with git command https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c

But if I you have been instructed by @Riksu9000, you should wait for more precise steps to overthrow this situation.

Ceimour commented 1 year ago

@lman0 Thanks for the link, looks like that will work! Still waiting to see if it is okay to proceed. Thanks again!

JF002 commented 1 year ago

@Ceimour Thanks for your follow-up on this PR, and I'm really sorry you encounter those issues with git... rebasing can be very veeeery confusing sometime.

As of now, the PR appears empty: image

You might want to wait for @Riksu9000 instructions (he's great with those advanced git functionalities), but it looks like the last commit you pushed is 049fbba, which is the last commit on develop. Since your branch contains all your changes, it should still be possible to fix this PR:

When I tried this on my side, there were a few conflicts that needed to be merged (probably because we applied changes to some files after to created your branch). You'll need to fix those conflicts, and then commit and (force)push again your branch to update this PR.

Once new changed appears in the PR, we'll be able to re-open this PR :)

Ceimour commented 1 year ago

@JF002 Thanks! Now I think I see where I went wrong. I will follow your suggestions and try again.

Ceimour commented 1 year ago

@JF002 Thanks again for the instruction. I did as you suggested and as the last step from within branch heartratefreq ran "git push --force". All completed without error. Is there anything else I need to do to get it to reopen?

Ceimour commented 1 year ago

@JF002 I was able to reopen. The reopen button appeared to be greyed out but I tried it and it worked. How should I address the Merge prompt? Update with merge or update with rebase?

pentamassiv commented 1 year ago

Very nice PR! I believe what Riksu9000 wants you to do is to clean up the commit history. Right now the PR contains 8 commits. Some of the commits revert accidental changes you made or other small changes. If this was merged as it is, all these commits would get merged as well. If somebody would investigate a bug and has a look at the last commits, this can be confusing. Git allows you to change old commits (the history). You can change the old commits so that it looks like you created a perfect PR on the first try.

Here is what you could do: Change the history of your last 8 commits: git rebase -i HEAD~8 Running this command gives you a list of commits in your text editor. If you rearrange or change those lines, you are changing the history of your commits. If you change it to the following:

pick 4a30070a Refactored Ppg for frequency based algorithm. Changed driver settings in HRS3300.cpp fo>
fixup 4ed64f5d Reverted Interpolation and arduinoFFT libraries back to author's original in order to >
fixup ffc7ebfc Reverted .vscode to develop 361e381ac32.
fixup 03855831 Moved Arduino libraries to src/libs.
pick 1c8aefc6 Arduino libraries InterpolationLib and arduinoFFT ported to InfiniTime. Changed all dou>
pick bcb1bb50 Fixed arduinoFFT.cpp constants are double, should be float. Modified Ppg.cpp to use ano>
fixup 187df497 Changed magic number to use bpm variable for readability.
fixup 5b7afaa5 Fixed unintended merge from original.

The order of the commits will be changed. Additionally the commits where the line starts with "fixup" will be squashed with the commit from the line above.

You might have to rebase it to the newest "develop" branch by then. You can do that by running: git rebase origin/develop If there were any merge conflicts, you have to resolve them.

Because you changed the history, you will have to "force push" to the repo. That means you have to overwrite your old commits. You can do that by running: git push --force

I hope that helps you a little bit

Ceimour commented 1 year ago

@pentamassiv Thanks for the help! I was finally able to get the rebase done but there is some issue with a post build routine looking for "Compare build size" and it always fails. Hopefully I will work this out soon. Thanks again.

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 407772B 1316B
data 940B 0B
bss 54152B 592B
lman0 commented 1 year ago

hi i have see that the simulator is broken @Ceimour with you pul other wise , your code do create an artifact. maybe you need to do a pr for the new lib you here as well ... what do you think @NeroBurner ?

JF002 commented 1 year ago

@Ceimour be reassured, I do not forget this PR!

I was a bit surprised by the additional flash memory usage of this PR : +10KB. The changes in this PR alone do not explain this big size increase.

So I had a look at the .map file :

So were do those ~8300B come from? I compared map files from main and from this PR and found new entries from libgcc.a and libm.a :

which account for 7296B.

The symboles are

So mostly functions and variables related to float arithmetic. And indeed, in arduinoFFT.cpp we can find calls to sqrtf(), powf(), cosf(),...

Do we have any alternatives here? Could those calculations be done in integer, for example, to avoid linking with those heavy symbols? I'm not saying that we'll refuse this PR because of this, we really want to keep the memory usage (both ram and flash) under control!

Ceimour commented 1 year ago

@JF002 Thanks for the analysis. I was also shocked to see the amount of flash taken by this! I believe I can simplify things to get the size down. I'll let you know when I push changes. Thanks again!

Ceimour commented 1 year ago

@JF002 I was able to get rid of the trig and other functions that were causing the library to be linked. At least I assume that was the case since the size reduced by ~8K. Let me know if I need to fix the rebase. Wasn't sure what to do with the other commits so I left then in there as 'pick'. Thanks!

Ceimour commented 1 year ago

@JF002 Made a mistake on previous push, forgot to rebase on main. This one should be good.

JF002 commented 1 year ago

@Ceimour Amazing job :1st_place_medal: , thank you!

I've just checked the flash memory usage : these changes add a bit less than 2KB, which seems much more reasonable than before !

Regarding RAM usage, I couldn't find any dynamic allocation (which is nice), and the whole HeartRateTask class uses less than 1KB. It's probably more than the previous implementation, but these improvements are probably worth it.

This PR looks good to me. I'll ask @Riksu9000 and @FintasticMan to check whether all their conversations (review) are now solved :)