-
-
Notifications
You must be signed in to change notification settings - Fork 955
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
Atomic HRS reads #1845
Atomic HRS reads #1845
Conversation
mark9064
commented
Aug 28, 2023
•
edited
Loading
edited
- Combines the reading of all HRS3300 registers into one I2C read so data is not partial
- Downsizes both HRS and ALS to 16bit as the sensor does not generate larger than 16bit values in its current configuration
- Increasing the resolution by 1 bit doubles the sensor acquisition time, since we are already at 10Hz we are never going to use a higher resolution
- The PPG algorithm buffers for ALS/HRS are already 16bit anyway
- Removes functions for setting gain / drive that are unused throughout the codebase
Build size and comparison to main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some compile time checks, just to be sure the algorithm isn't (silently) destroyed when changing the registers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the constexpr documentation please and then we're good to go from my side
907367d
to
7518252
Compare
Thought I may as well calculate the required buffer length too. Fewer magic numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporarily hit a compiler bug in with InfiniSim with gcc 14.2.1. Worked on gcc 14.1.1 and gcc 13.3.0 and and clang 18.1.8. Reinstalled 14.2.1 to generate the error message... and gone.
So looks good to me. Especially the constexpr stuff :)
Well seems to be back 🙈 found an issue that maybe is related: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116634
and I get rid of it including |
Strange that it's intermittent. If adding some headers fixes it I suppose it doesn't hurt to? |
Fix for Hrs3300 PR about Atomic HRS reads: #1845 We use `std::begin` and `std::end`, but we don't include one of the headers that define those functions. See https://en.cppreference.com/w/cpp/iterator/begin for a list of headers that define `std::begin` and `std::end`. Starting with GCC 14 this leads to a compilation error presumably because they cleaned up their headers. Fix code by inlcuding `<iterator>`
after more investigating I can say, not a compiler bug. GCC 14 is now more correct and somewhere cleaned up their stlib headers. List of headers that define
Opened a PR: #2128 |
Fix for Hrs3300 PR about Atomic HRS reads: #1845 We use `std::begin` and `std::end`, but we don't include one of the headers that define those functions. See https://en.cppreference.com/w/cpp/iterator/begin for a list of headers that define `std::begin` and `std::end`. Starting with GCC 14 this leads to a compilation error presumably because they cleaned up their headers. Fix code by inlcuding `<iterator>`