GeezerVison is showing github.com — Atomic HRS reads by mark9064 · Pull Request #1845 · InfiniTimeOrg/InfiniTime
Conversation
mark9064
commented
Aug 28, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
- 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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Saarsk and NeroBurner reacted with thumbs up emoji
|
Build size and comparison to main:
| Section |
Size |
Difference |
| text |
374216B |
-32B |
| data |
948B |
0B |
| bss |
63480B |
0B |
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Thought I may as well calculate the required buffer length too. Fewer magic numbers
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 :)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
NeroBurner
commented
Sep 21, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Well seems to be back 🙈
found an issue that maybe is related: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116634
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp: In member function ‘Pinetime::Drivers::Hrs3300::PackedHrsAls Pinetime::Drivers::Hrs3300::ReadHrsAls()’:
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:74:83: error: no matching function for call to ‘begin(const Pinetime::Drivers::Hrs3300::Registers [6])’
[build] 74 | constexpr uint8_t baseOffset = static_cast<uint8_t>(*std::min_element(std::begin(dataRegisters), std::end(dataRegisters)));
[build] | ~~~~~~~~~~^~~~~~~~~~~~~~~
[build] In file included from /usr/include/c++/14.2.1/bits/algorithmfwd.h:39,
[build] from /usr/include/c++/14.2.1/bits/stl_algo.h:59,
[build] from /usr/include/c++/14.2.1/algorithm:61,
[build] from /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:8:
[build] /usr/include/c++/14.2.1/initializer_list:88:5: note: candidate: ‘template<class _Tp> constexpr const _Tp* std::begin(initializer_list<_Tp>)’
[build] 88 | begin(initializer_list<_Tp> __ils) noexcept
[build] | ^~~~~
[build] /usr/include/c++/14.2.1/initializer_list:88:5: note: template argument deduction/substitution failed:
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:74:83: note: mismatched types ‘std::initializer_list<_Tp>’ and ‘const Pinetime::Drivers::Hrs3300::Registers*’
[build] 74 | constexpr uint8_t baseOffset = static_cast<uint8_t>(*std::min_element(std::begin(dataRegisters), std::end(dataRegisters)));
[build] | ~~~~~~~~~~^~~~~~~~~~~~~~~
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:74:108: error: no matching function for call to ‘end(const Pinetime::Drivers::Hrs3300::Registers [6])’
[build] 74 | constexpr uint8_t baseOffset = static_cast<uint8_t>(*std::min_element(std::begin(dataRegisters), std::end(dataRegisters)));
[build] | ~~~~~~~~^~~~~~~~~~~~~~~
[build] /usr/include/c++/14.2.1/initializer_list:99:5: note: candidate: ‘template<class _Tp> constexpr const _Tp* std::end(initializer_list<_Tp>)’
[build] 99 | end(initializer_list<_Tp> __ils) noexcept
[build] | ^~~
[build] /usr/include/c++/14.2.1/initializer_list:99:5: note: template argument deduction/substitution failed:
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:74:108: note: mismatched types ‘std::initializer_list<_Tp>’ and ‘const Pinetime::Drivers::Hrs3300::Registers*’
[build] 74 | constexpr uint8_t baseOffset = static_cast<uint8_t>(*std::min_element(std::begin(dataRegisters), std::end(dataRegisters)));
[build] | ~~~~~~~~^~~~~~~~~~~~~~~
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:77:79: error: no matching function for call to ‘begin(const Pinetime::Drivers::Hrs3300::Registers [6])’
[build] 77 | constexpr uint8_t length = static_cast<uint8_t>(*std::max_element(std::begin(dataRegisters), std::end(dataRegisters))) - baseOffset + 1;
[build] | ~~~~~~~~~~^~~~~~~~~~~~~~~
[build] /usr/include/c++/14.2.1/initializer_list:88:5: note: candidate: ‘template<class _Tp> constexpr const _Tp* std::begin(initializer_list<_Tp>)’
[build] 88 | begin(initializer_list<_Tp> __ils) noexcept
[build] | ^~~~~
[build] /usr/include/c++/14.2.1/initializer_list:88:5: note: template argument deduction/substitution failed:
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:77:79: note: mismatched types ‘std::initializer_list<_Tp>’ and ‘const Pinetime::Drivers::Hrs3300::Registers*’
[build] 77 | constexpr uint8_t length = static_cast<uint8_t>(*std::max_element(std::begin(dataRegisters), std::end(dataRegisters))) - baseOffset + 1;
[build] | ~~~~~~~~~~^~~~~~~~~~~~~~~
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:77:104: error: no matching function for call to ‘end(const Pinetime::Drivers::Hrs3300::Registers [6])’
[build] 77 | constexpr uint8_t length = static_cast<uint8_t>(*std::max_element(std::begin(dataRegisters), std::end(dataRegisters))) - baseOffset + 1;
[build] | ~~~~~~~~^~~~~~~~~~~~~~~
[build] /usr/include/c++/14.2.1/initializer_list:99:5: note: candidate: ‘template<class _Tp> constexpr const _Tp* std::end(initializer_list<_Tp>)’
[build] 99 | end(initializer_list<_Tp> __ils) noexcept
[build] | ^~~
[build] /usr/include/c++/14.2.1/initializer_list:99:5: note: template argument deduction/substitution failed:
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:77:104: note: mismatched types ‘std::initializer_list<_Tp>’ and ‘const Pinetime::Drivers::Hrs3300::Registers*’
[build] 77 | constexpr uint8_t length = static_cast<uint8_t>(*std::max_element(std::begin(dataRegisters), std::end(dataRegisters))) - baseOffset + 1;
[build] | ~~~~~~~~^~~~~~~~~~~~~~~
[build] /home/nero/repos/pinetime/InfiniSim/InfiniTime/src/drivers/Hrs3300.cpp:80:15: error: size of array ‘buf’ is not an integral constant-expression
[build] 80 | uint8_t buf[length];
[build] | ^~~~~~
and I get rid of it including <vector> after <algorithm>??? something is definitely going on with gcc 14.2.1 🙈
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Strange that it's intermittent. If adding some headers fixes it I suppose it doesn't hurt to?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
NeroBurner
added a commit
that referenced
this pull request
Sep 22, 2024
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.
We use std::begin, but we don't include any headers that define std::begin
List of headers that define std::begin std::end (from: https://en.cppreference.com/w/cpp/iterator/begin )
Defined in header [<array>](https://en.cppreference.com/w/cpp/header/array)
Defined in header [<deque>](https://en.cppreference.com/w/cpp/header/deque)
Defined in header [<flat_map>](https://en.cppreference.com/w/cpp/header/flat_map)
Defined in header [<flat_set>](https://en.cppreference.com/w/cpp/header/flat_set)
Defined in header [<forward_list>](https://en.cppreference.com/w/cpp/header/forward_list)
Defined in header [<inplace_vector>](https://en.cppreference.com/w/cpp/header/inplace_vector)
Defined in header [<iterator>](https://en.cppreference.com/w/cpp/header/iterator)
Defined in header [<list>](https://en.cppreference.com/w/cpp/header/list)
Defined in header [<map>](https://en.cppreference.com/w/cpp/header/map)
Defined in header [<regex>](https://en.cppreference.com/w/cpp/header/regex)
Defined in header [<set>](https://en.cppreference.com/w/cpp/header/set)
Defined in header [<span>](https://en.cppreference.com/w/cpp/header/span)
Defined in header [<string>](https://en.cppreference.com/w/cpp/header/string)
Defined in header [<string_view>](https://en.cppreference.com/w/cpp/header/string_view)
Defined in header [<unordered_map>](https://en.cppreference.com/w/cpp/header/unordered_map)
Defined in header [<unordered_set>](https://en.cppreference.com/w/cpp/header/unordered_set)
Defined in header [<vector>](https://en.cppreference.com/w/cpp/header/vector)
Opened a PR: #2128
|
mark9064 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
FintasticMan
pushed a commit
that referenced
this pull request
Sep 22, 2024
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>`
Reviewers
JF002
JF002 approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
NeroBurner
left a comment