From ad3bf49c7b2864d8f06cedea8ad329e26360f297 Mon Sep 17 00:00:00 2001 From: mark9064 <30447455+mark9064@users.noreply.github.com> Date: Sat, 21 Sep 2024 23:29:15 +0100 Subject: [PATCH] Atomic HRS reads (#1845) - Combine 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 - Remove functions for setting gain / drive that are unused throughout the codebase - Calculate constants with constexpr --- src/components/heartrate/Ppg.cpp | 2 +- src/components/heartrate/Ppg.h | 2 +- src/drivers/Hrs3300.cpp | 57 ++++++++++++++--------------- src/drivers/Hrs3300.h | 10 +++-- src/heartratetask/HeartRateTask.cpp | 3 +- 5 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/components/heartrate/Ppg.cpp b/src/components/heartrate/Ppg.cpp index 3a6988ae..efbed852 100644 --- a/src/components/heartrate/Ppg.cpp +++ b/src/components/heartrate/Ppg.cpp @@ -142,7 +142,7 @@ Ppg::Ppg() { spectrum.fill(0.0f); } -int8_t Ppg::Preprocess(uint32_t hrs, uint32_t als) { +int8_t Ppg::Preprocess(uint16_t hrs, uint16_t als) { if (dataIndex < dataLength) { dataHRS[dataIndex++] = hrs; } diff --git a/src/components/heartrate/Ppg.h b/src/components/heartrate/Ppg.h index 4492b2c2..373e7985 100644 --- a/src/components/heartrate/Ppg.h +++ b/src/components/heartrate/Ppg.h @@ -14,7 +14,7 @@ namespace Pinetime { class Ppg { public: Ppg(); - int8_t Preprocess(uint32_t hrs, uint32_t als); + int8_t Preprocess(uint16_t hrs, uint16_t als); int HeartRate(); void Reset(bool resetDaqBuffer); static constexpr int deltaTms = 100; diff --git a/src/drivers/Hrs3300.cpp b/src/drivers/Hrs3300.cpp index 33889b6f..9c77975e 100644 --- a/src/drivers/Hrs3300.cpp +++ b/src/drivers/Hrs3300.cpp @@ -67,40 +67,37 @@ void Hrs3300::Disable() { WriteRegister(static_cast(Registers::PDriver), 0); } -uint32_t Hrs3300::ReadHrs() { - auto m = ReadRegister(static_cast(Registers::C0DataM)); - auto h = ReadRegister(static_cast(Registers::C0DataH)); - auto l = ReadRegister(static_cast(Registers::C0dataL)); - return ((l & 0x30) << 12) | (m << 8) | ((h & 0x0f) << 4) | (l & 0x0f); -} +Hrs3300::PackedHrsAls Hrs3300::ReadHrsAls() { + constexpr Registers dataRegisters[] = + {Registers::C1dataM, Registers::C0DataM, Registers::C0DataH, Registers::C1dataH, Registers::C1dataL, Registers::C0dataL}; + // Calculate smallest register address + constexpr uint8_t baseOffset = static_cast(*std::min_element(std::begin(dataRegisters), std::end(dataRegisters))); + // Calculate largest address to determine length of read needed + // Add one to largest relative index to find the length + constexpr uint8_t length = static_cast(*std::max_element(std::begin(dataRegisters), std::end(dataRegisters))) - baseOffset + 1; -uint32_t Hrs3300::ReadAls() { - auto m = ReadRegister(static_cast(Registers::C1dataM)); - auto h = ReadRegister(static_cast(Registers::C1dataH)); - auto l = ReadRegister(static_cast(Registers::C1dataL)); - return ((h & 0x3f) << 11) | (m << 3) | (l & 0x07); -} - -void Hrs3300::SetGain(uint8_t gain) { - constexpr uint8_t maxGain = 64U; - gain = std::min(gain, maxGain); - uint8_t hgain = 0; - while ((1 << hgain) < gain) { - ++hgain; + Hrs3300::PackedHrsAls res; + uint8_t buf[length]; + auto ret = twiMaster.Read(twiAddress, baseOffset, buf, length); + if (ret != TwiMaster::ErrorCodes::NoError) { + NRF_LOG_INFO("READ ERROR"); } + // hrs + uint8_t m = static_cast(Registers::C0DataM) - baseOffset; + uint8_t h = static_cast(Registers::C0DataH) - baseOffset; + uint8_t l = static_cast(Registers::C0dataL) - baseOffset; + // There are two extra bits (17 and 18) but they are not read here + // as resolutions >16bit aren't practically useful (too slow) and + // all hrs values throughout InfiniTime are 16bit + res.hrs = (buf[m] << 8) | ((buf[h] & 0x0f) << 4) | (buf[l] & 0x0f); - WriteRegister(static_cast(Registers::Hgain), hgain << 2); -} + // als + m = static_cast(Registers::C1dataM) - baseOffset; + h = static_cast(Registers::C1dataH) - baseOffset; + l = static_cast(Registers::C1dataL) - baseOffset; + res.als = ((buf[h] & 0x3f) << 11) | (buf[m] << 3) | (buf[l] & 0x07); -void Hrs3300::SetDrive(uint8_t drive) { - auto en = ReadRegister(static_cast(Registers::Enable)); - auto pd = ReadRegister(static_cast(Registers::PDriver)); - - en = (en & 0xf7) | ((drive & 2) << 2); - pd = (pd & 0xbf) | ((drive & 1) << 6); - - WriteRegister(static_cast(Registers::Enable), en); - WriteRegister(static_cast(Registers::PDriver), pd); + return res; } void Hrs3300::WriteRegister(uint8_t reg, uint8_t data) { diff --git a/src/drivers/Hrs3300.h b/src/drivers/Hrs3300.h index 8bbdc69a..6f721448 100644 --- a/src/drivers/Hrs3300.h +++ b/src/drivers/Hrs3300.h @@ -21,6 +21,11 @@ namespace Pinetime { Hgain = 0x17 }; + struct PackedHrsAls { + uint16_t hrs; + uint16_t als; + }; + Hrs3300(TwiMaster& twiMaster, uint8_t twiAddress); Hrs3300(const Hrs3300&) = delete; Hrs3300& operator=(const Hrs3300&) = delete; @@ -30,10 +35,7 @@ namespace Pinetime { void Init(); void Enable(); void Disable(); - uint32_t ReadHrs(); - uint32_t ReadAls(); - void SetGain(uint8_t gain); - void SetDrive(uint8_t drive); + PackedHrsAls ReadHrsAls(); private: TwiMaster& twiMaster; diff --git a/src/heartratetask/HeartRateTask.cpp b/src/heartratetask/HeartRateTask.cpp index 9d82d11e..8a5a871b 100644 --- a/src/heartratetask/HeartRateTask.cpp +++ b/src/heartratetask/HeartRateTask.cpp @@ -70,7 +70,8 @@ void HeartRateTask::Work() { } if (measurementStarted) { - int8_t ambient = ppg.Preprocess(heartRateSensor.ReadHrs(), heartRateSensor.ReadAls()); + auto sensorData = heartRateSensor.ReadHrsAls(); + int8_t ambient = ppg.Preprocess(sensorData.hrs, sensorData.als); int bpm = ppg.HeartRate(); // If ambient light detected or a reset requested (bpm < 0)