Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heart rate sensor - HRS gain changed to x8 #531

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

hatmajster
Copy link
Contributor

TL/DR Heart rate sensor doesn't work for me, its random. Changed HRS gain from x64 to 4x and now it works much better. However I don't know if this is good/proper value, so I created a setting to let everyone customize it.

So as for few other people ( https://forum.pine64.org/showthread.php?tid=13727 and #397 ) heart rate sensor was not working for me. Actually it wasn't working at all, I could leave the clock on the desk and the desk would have pulse ranging from 40 to 220. To me it was completely random no matter if I had it on hand or not. Though it seems like some folks have it working about fine, so it probably depends on something.

Checking documentation from here: http://www.tianyihexin.com/pic/file/20170627/20170627154877337733.pdf I've noticed that there is a gain setting for HRS. Current setting was 0x10, which means its x64, kinda extreme to me - seems far away from the last "sane" value - 8x. Though, worth noting, x64 seems to be recommended value.

I changed gain to 4x and for me it fixed HRS, now I have measurements that I would expect. Its a one line change in Hrs3300.cpp

However to better tinker with gain and test more values I created an option to customize it. All values seem to me to be working almost the same, as I have no real reference. I can only say that my cheap tomtom wristband show almost the same results now, +/- 5.

So my proposal is to use/test this new setting, and maybe it will be possible to find better compromise (because as I read, few people are happy with x64). After testing and picking the "good" value, the setting will probably be obsolete. Well maybe even can be removed from this PR... If my change will really help.

PS. I also have some debug showings implemented, but decided to not commit them here...

@hatmajster hatmajster changed the title Hrs3300: added gain setting option and changed default gain to x4 Heart rate sensor - customizable gain to have better measurements Jul 27, 2021
@kieranc
Copy link
Contributor

kieranc commented Jul 28, 2021

Nice work! I'm testing it, on 4x setting the readout looks a lot better than it used to, very much more stable.

@hatmajster
Copy link
Contributor Author

Thanks, though its a very simple change of value;) My intention through this PR was mostly to let users pick their own best gain, as I saw myself that default just doesn't work for me

@raulgool
Copy link

raulgool commented Aug 7, 2021

I have tested this functionality too. It works well.

With default value, HR measured by InfiniTime was always the second or third harmonic of the actual heart rate.
With reduced gain (4 or 8), the heart rate values are much more stable.

@spaetz
Copy link

spaetz commented Sep 11, 2021

As per the discussion in the issue, if the value is an improvement, make it the default rather than introducing a setting where most people will have no clue what to use.

@kieranc
Copy link
Contributor

kieranc commented Sep 13, 2021

So is the consensus that 4x gain works best for most people? It does for me, has anyone found that a different value works better for them?

I think this is suitable for inclusion with 1.5, either with the settings app or just 4x gain - most people seem to find the current setting doesn't work very well.

@hatmajster
Copy link
Contributor Author

Yeah, but I recently found that for me 4x doesn't work too well when I am running. I can't get proper measurements due to my crappy phone, but 8x seems to be working a lot better. I would say we should change it to 8x as the second best value (x64 is in fact recommended by the producer).

As I written above, I agree, this setting should probably not be added to Infinitime, it makes simple things complicated. However, its quite a nice way to experiment and pick best option. Maybe some voting should be made to make a final decision.

However I see that Daniel Thompson just made a change to 8x in wasp-os: wasp-os/wasp-os@bbf7d3a, and people seem to be happy with it. Maybe we should just follow soon;)

@lman0
Copy link

lman0 commented Sep 21, 2021

hi
a question : is the gain level related to the skin colour or it's not?
if that's the case maybe instead of number it would be better to turn number as a slider between 'white' , 'black' skin level.
it would much more readable

@tmilburn
Copy link

Instead of decreasing the gain, should we decrease the LED drive using the PDRIVE setting? This will result in battery life savings when heat rate is switched on.

I think there must be better algorithms for determining the best combination of LED intensity and amplifier gain. https://www.nxp.com/docs/en/application-note/AN4327.pdf has a description of NXP's calibration algorithm which involves periodically checking that the baseline reading is within expected limits and if not then adjusted the LED intensity (and in our case the amplifier gain).

And this all ties into the PPG code which takes the sensor readings, filters them and finds the peaks. The filtering stage includes an automatic gain control which could have feedback back into the heart rate sensor's gain and LED intensity. I would be interested in getting feedback from @daniel-thompson who designed the algorithms.

@daniel-thompson
Copy link

And this all ties into the PPG code which takes the sensor readings, filters them and
finds the peaks. The filtering stage includes an automatic gain control which could
have feedback back into the heart rate sensor's gain and LED intensity. I would be
interested in getting feedback from @daniel-thompson who designed the algorithms.

An automatic control of the sensor gain can't be controlled from the digital AGC since that happens after the initial high-pass filtering. You would need to run additional filtering steps at the front of the chain to manage the hardware gain control.

@tmilburn
Copy link

tmilburn commented Oct 4, 2021

And this all ties into the PPG code which takes the sensor readings, filters them and
finds the peaks. The filtering stage includes an automatic gain control which could
have feedback back into the heart rate sensor's gain and LED intensity. I would be
interested in getting feedback from @daniel-thompson who designed the algorithms.

An automatic control of the sensor gain can't be controlled from the digital AGC since that happens after the initial high-pass filtering. You would need to run additional filtering steps at the front of the chain to manage the hardware gain control.

If the high pass filter has 0dB gain at low frequencies then surely this would still work?

@daniel-thompson
Copy link

daniel-thompson commented Oct 4, 2021

An automatic control of the sensor gain can't be controlled from the digital AGC since that happens after the initial high-pass filtering. You would need to run additional filtering steps at the front of the chain to manage the hardware gain control.

If the high pass filter has 0dB gain at low frequencies then surely this would still work?

The magnitude of the high frequency components is tiny compared to the ADC range and the natural DC drift of the PPG signal. You are welcome to experiment but I doubt anything useful can be discovered about whether it is safe to change the input gain after the HPF.

@JF002
Copy link
Collaborator

JF002 commented Oct 4, 2021

@daniel-thompson Nice to see you here :)

@hatmajster If I read the datasheet correctly, it seems like the only valid values for the gain are x1, x2, x4, x8 and x64:
image

So I think using a enum class with those values instead of an uint32 in Settings. That would make the code more strongly typed, more readable, less error prone, and the UI only allows those values anyway.

Also, I agree we should try to find the value that works for the most people as the default value, until we find a way to maybe auto-tune it in the future ;)

@geekbozu
Copy link
Member

geekbozu commented Oct 16, 2021

@hatmajster any plans to update/resolve these conflicts? This is on my review list "soon"(tm).
If you do not have time/interest I can fork and clean it up so we can move this along :)

@hatmajster
Copy link
Contributor Author

Sorry, didn't have time, but I finally rebased the code for the newest version, also changed the configuration from simple int to enum. Tested, should be okay I hope. Gain increasing logic is now in Settings, should be probably in SettingHeartRate.cpp

Now 8x is default, as 2x-8x doesn't seem to differ so much for people, but I for myself think 8x is best when I move my hand a lot (for example while running).

I would also like to remind again, that ideally this PR shouldn't be merged. I just made it for folks to try multiple options of gain to maybe find one best. I did not checked yet, but if Daniel's change to 8x went okay, maybe InfiniTime should also just switch?

@geekbozu
Copy link
Member

I approved the work Flow, There will be a DFU artifact generate which can be used to test this.

@JF002 JF002 added this to the 1.9.0 milestone Jan 19, 2022
@JF002
Copy link
Collaborator

JF002 commented Jan 19, 2022

Sorry for ignoring this PR for so long!
A few people told me that setting the gain to specific values seem to improve the readings of the HR sensor.
Is there a specific value that seem to work for "most of the users" we could set by default? If enough people are happy with the default value, maybe we can dismiss the setting app.

@yehoshuapw
Copy link
Contributor

It does seem (as also done in wasp-os), (and according to both my personal testing, and a friend - and messages all over the place) that 8 works very well.
(it's actually better then I excepted without any filtering from movement)

@trman
Copy link

trman commented Jan 19, 2022

hi
i am for 8 as gain it's much better!
and since the waspos main dev didn't have received issue about the fact that putting the gain to 8 is bad ,
maybe it's time for infinitime to put the HR gain to 8 as well
(in order to have better and faster measurement)

@raulgool
Copy link

raulgool commented Jan 19, 2022

I have used this and for me 4, 8 work better than default one.
Also mentioned earlier here #531 (comment)

@daniel-thompson
Copy link

Is there a specific value that seem to work for "most of the users" we could set by default? If enough people are happy with the default value, maybe we can dismiss the setting app.

Overall I'd recommend changing the default to x8. It is the most conservative change (given the original was x64) and I've not heard anybody saying bad things about it!

As you may remember from other Issues I'm generally against presenting a gain option to users. Mostly because it reduces user pressure to get the default right... and, in the long term, converging on good defaults is better for the whole PineTime ecosystem.

On the other hand I did note that Infinitime now has a filesystem. Providing a "Developer Options" section (similar to Android although maybe with less of an expedition to enable it) with an option that can enable raw sensor data logging to the filesystem would be a great setting to have. Raw sensor data (from a diverse range of subjects) could be used to tweak the existing algorithm, develop alternatives and, if the x8 did turn out not to be optimal, would help us understand why.

@JF002
Copy link
Collaborator

JF002 commented Jan 20, 2022

Thanks everyone! I guess we'll set the default gain to x8 and see how it goes, then!

As you may remember from other Issues I'm generally against presenting a gain option to users. Mostly because it reduces user pressure to get the default right... and, in the long term, converging on good defaults is better for the whole PineTime ecosystem.

That's exactly my opinion too! That and the fact the InfiniTime already have quite a lot of settings and they might become confusing for the users. And settings pages also take space in ROM so...

On the other hand I did note that Infinitime now has a filesystem. Providing a "Developer Options" section (similar to Android although maybe with less of an expedition to enable it) with an option that can enable raw sensor data logging to the filesystem would be a great setting to have. Raw sensor data (from a diverse range of subjects) could be used to tweak the existing algorithm, develop alternatives and, if the x8 did turn out not to be optimal, would help us understand why.

Yes, InfiniTime implements a file system and also provides file system API over BLE so we can actually fetch those data on a computer. The only missing piece is the code that logs HR to a file. That's a really good idea.
Oh btw, there's also a PR that adds serial over BLE (BLE NUS?). That could be useful to monitor HR values in real time.

@kieranc
Copy link
Contributor

kieranc commented Jan 20, 2022

Fixed 8x seems a good option to me. @hatmajster if you'd like credit for your work can you make a PR which just sets the gain to 8x?

@wandering-hacker
Copy link

This is perfect! I have had something like this bouncing around in my mind, I'll have to test this out.

@hatmajster
Copy link
Contributor Author

Sorry this was so long that I kind of forgotten about it;) Yes, I will change the PR to just be 8x today, meaning removing all customization stuff introduced here.

@kieranc
Copy link
Contributor

kieranc commented Jan 24, 2022

Understandable, don't worry. I was thinking it might be easier to simply create a new PR to keep the commit log clean, but it's up you you, it can always be squashed during merge.

@hatmajster
Copy link
Contributor Author

hatmajster commented Jan 24, 2022

Rebased, removed customisation, left only x8 gain, compiled, flashed, testing in progress. Seems to be working for now.
Oh, and if for whatever reason anybody wants the customisation, old version is left on my fork in heart-rate-gain-setting-old branch.

@hatmajster
Copy link
Contributor Author

hatmajster commented Jan 25, 2022

Fixed just the comment, to give a hint why the value is non default. Don't know why I didn't save file yesterday...
Anyway, tested for a day, seems to be working correctly. Well its only a simple value change and was even like this for some time here so... ;)

@hatmajster hatmajster changed the title Heart rate sensor - customizable gain to have better measurements Heart rate sensor - HRS gain changed to x8 Jan 26, 2022
@JF002
Copy link
Collaborator

JF002 commented Jan 26, 2022

@hatmajster Thank you very much for those changes. I'm sorry you had to remove most of the changes from the first iteration of this PR!
Fortunately, you kept the code in case anyone else need it in the future! Here's the link : https://github.com/hatmajster/InfiniTime/tree/heart-rate-gain-setting-old

@JF002 JF002 merged commit 71c895d into InfiniTimeOrg:develop Jan 26, 2022
@mc0e
Copy link

mc0e commented Apr 14, 2022

hi a question : is the gain level related to the skin colour or it's not? if that's the case maybe instead of number it would be better to turn number as a slider between 'white' , 'black' skin level. it would much more readable

Can I just highlight the above quoted question from @lman0? Differences in skin pigmentation is also my best guess as to why the sensor might provide different gain options. In polling what setting works for best for most people here, do we have an adequate representation of people with different skin pigmentation?

If this is important, and to avoid presenting users with extra options, could the phone itself adjust the hardware gain setting? The software automatic gain control is already doing the work of extracting info about the range of values it's seeing, so it seems like it shouldn't be difficult to use that to recognise when the hardware gain setting is not ideal, and raise or lower the hardware gain as required? We just don't want to do that in the middle of data collection without invalidating that measurement period for heart-rate measurement.

@kieranc
Copy link
Contributor

kieranc commented Apr 14, 2022

Can I just highlight the above quoted question from @lman0? Differences in skin pigmentation is also my best guess as to why the sensor might provide different gain options. In polling what setting works for best for most people here, do we have an adequate representation of people with different skin pigmentation?

It's an interesting question for sure, it has been discussed a little, we don't really have a lot of data to work with other than people reporting that the 64x setting provided largely random data and 8x being a significant improvement, for most. One user with pale skin reported that the 8x gain was no improvement for them, and another with dark skin said the same. Hair quantity and band tightness also seem to impact the readings. I think unless someone wants to find some suitable test subjects and use a build with variable gain to get real data, the current setting is acceptable for now and an improvement over the previous one. I believe the sensor can also adjust the light intensity, but this is just another variable which would need significant testing to know if it's useful or not. Writing a nice algorithm which monitors the data and adjusts gain and intensity on the fly to get the best quality data would be great, but it's way beyond my abilities :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.