InfiniTime icon indicating copy to clipboard operation
InfiniTime copied to clipboard

Atomic HRS reads

Open mark9064 opened this issue 2 years ago • 1 comments

  • 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

mark9064 avatar Aug 28 '23 18:08 mark9064

Build size and comparison to main:

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

github-actions[bot] avatar Aug 28 '23 18:08 github-actions[bot]

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

mark9064 avatar Sep 21 '24 20:09 mark9064

Well seems to be back :see_no_evil:

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 :see_no_evil:

NeroBurner avatar Sep 21 '24 22:09 NeroBurner

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

mark9064 avatar Sep 21 '24 22:09 mark9064

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: https://github.com/InfiniTimeOrg/InfiniTime/pull/2128

NeroBurner avatar Sep 22 '24 09:09 NeroBurner