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

Atomic HRS reads #1845

Merged
merged 2 commits into from
Sep 21, 2024
Merged

Atomic HRS reads #1845

merged 2 commits into from
Sep 21, 2024

Conversation

mark9064
Copy link
Contributor

@mark9064 mark9064 commented Aug 28, 2023

  • 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

@github-actions
Copy link

github-actions bot commented Aug 28, 2023

Build size and comparison to main:

Section Size Difference
text 374216B -32B
data 948B 0B
bss 63480B 0B

@JF002 JF002 added this to the 1.15.0 milestone May 12, 2024
Copy link
Contributor

@NeroBurner NeroBurner left a 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

src/drivers/Hrs3300.cpp Outdated Show resolved Hide resolved
src/drivers/Hrs3300.cpp Show resolved Hide resolved
Copy link
Contributor

@NeroBurner NeroBurner left a 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

src/drivers/Hrs3300.cpp Outdated Show resolved Hide resolved
src/drivers/Hrs3300.cpp Show resolved Hide resolved
@mark9064
Copy link
Contributor Author

Thought I may as well calculate the required buffer length too. Fewer magic numbers

Copy link
Contributor

@NeroBurner NeroBurner left a 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 :)

@NeroBurner NeroBurner merged commit ad3bf49 into InfiniTimeOrg:main Sep 21, 2024
7 checks passed
@NeroBurner
Copy link
Contributor

NeroBurner commented Sep 21, 2024

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 🙈

@mark9064
Copy link
Contributor Author

Strange that it's intermittent. If adding some headers fixes it I suppose it doesn't hurt to?

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>`
@NeroBurner
Copy link
Contributor

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

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>`
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.

3 participants