From baa7e1bf12b58224e9461a5a605dcebabcf9504e Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Mon, 9 Aug 2021 17:45:24 +0300 Subject: [PATCH 1/6] Optimize twiMaster --- src/drivers/TwiMaster.cpp | 43 ++++++++++++++++++--------------------- src/drivers/TwiMaster.h | 4 +++- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index fc9edf81..e4c03d5b 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -9,21 +9,25 @@ using namespace Pinetime::Drivers; // TODO use DMA/IRQ TwiMaster::TwiMaster(const Modules module, const Parameters& params) : module {module}, params {params} { + mutex = xSemaphoreCreateBinary(); +} + +void TwiMaster::ConfigurePins() const { + NRF_GPIO->PIN_CNF[params.pinScl] = + (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | + (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | + (GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | + (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); + + NRF_GPIO->PIN_CNF[params.pinSda] = + (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | + (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | + (GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | + (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); } void TwiMaster::Init() { - if(mutex == nullptr) - mutex = xSemaphoreCreateBinary(); - - NRF_GPIO->PIN_CNF[params.pinScl] = - ((uint32_t) GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | ((uint32_t) GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | - ((uint32_t) GPIO_PIN_CNF_PULL_Pullup << GPIO_PIN_CNF_PULL_Pos) | ((uint32_t) GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | - ((uint32_t) GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); - - NRF_GPIO->PIN_CNF[params.pinSda] = - ((uint32_t) GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | ((uint32_t) GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | - ((uint32_t) GPIO_PIN_CNF_PULL_Pullup << GPIO_PIN_CNF_PULL_Pos) | ((uint32_t) GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | - ((uint32_t) GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); + ConfigurePins(); switch (module) { case Modules::TWIM1: @@ -179,7 +183,8 @@ void TwiMaster::Sleep() { } void TwiMaster::Wakeup() { - Init(); + ConfigurePins(); + twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Enabled << TWIM_ENABLE_ENABLE_Pos); NRF_LOG_INFO("[TWIMASTER] Wakeup"); } @@ -194,16 +199,8 @@ void TwiMaster::FixHwFreezed() { uint32_t twi_state = NRF_TWI1->ENABLE; twiBaseAddress->ENABLE = TWIM_ENABLE_ENABLE_Disabled << TWI_ENABLE_ENABLE_Pos; - NRF_GPIO->PIN_CNF[params.pinScl] = - ((uint32_t) GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | ((uint32_t) GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | - ((uint32_t) GPIO_PIN_CNF_PULL_Pullup << GPIO_PIN_CNF_PULL_Pos) | ((uint32_t) GPIO_PIN_CNF_DRIVE_S0S1 << GPIO_PIN_CNF_DRIVE_Pos) | - ((uint32_t) GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); - - NRF_GPIO->PIN_CNF[params.pinSda] = - ((uint32_t) GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | ((uint32_t) GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | - ((uint32_t) GPIO_PIN_CNF_PULL_Pullup << GPIO_PIN_CNF_PULL_Pos) | ((uint32_t) GPIO_PIN_CNF_DRIVE_S0S1 << GPIO_PIN_CNF_DRIVE_Pos) | - ((uint32_t) GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); + ConfigurePins(); // Re-enable I²C twiBaseAddress->ENABLE = twi_state; -} \ No newline at end of file +} diff --git a/src/drivers/TwiMaster.h b/src/drivers/TwiMaster.h index 6175b99b..b8d36d59 100644 --- a/src/drivers/TwiMaster.h +++ b/src/drivers/TwiMaster.h @@ -26,6 +26,8 @@ namespace Pinetime { void Sleep(); void Wakeup(); + void ConfigurePins() const; + private: ErrorCodes Read(uint8_t deviceAddress, uint8_t* buffer, size_t size, bool stop); ErrorCodes Write(uint8_t deviceAddress, const uint8_t* data, size_t size, bool stop); @@ -41,4 +43,4 @@ namespace Pinetime { static constexpr uint32_t HwFreezedDelay {161000}; }; } -} \ No newline at end of file +} From 2194a339cf55772c944fc1302ca740805d931983 Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Tue, 10 Aug 2021 11:26:43 +0300 Subject: [PATCH 2/6] Only enable the bus when needed --- src/drivers/TwiMaster.cpp | 6 ++++-- src/systemtask/SystemTask.cpp | 9 --------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index e4c03d5b..34aac07f 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -72,8 +72,10 @@ void TwiMaster::Init() { TwiMaster::ErrorCodes TwiMaster::Read(uint8_t deviceAddress, uint8_t registerAddress, uint8_t* data, size_t size) { xSemaphoreTake(mutex, portMAX_DELAY); + Wakeup(); auto ret = Write(deviceAddress, ®isterAddress, 1, false); ret = Read(deviceAddress, data, size, true); + Sleep(); xSemaphoreGive(mutex); return ret; } @@ -81,9 +83,11 @@ TwiMaster::ErrorCodes TwiMaster::Read(uint8_t deviceAddress, uint8_t registerAdd TwiMaster::ErrorCodes TwiMaster::Write(uint8_t deviceAddress, uint8_t registerAddress, const uint8_t* data, size_t size) { ASSERT(size <= maxDataSize); xSemaphoreTake(mutex, portMAX_DELAY); + Wakeup(); internalBuffer[0] = registerAddress; std::memcpy(internalBuffer + 1, data, size); auto ret = Write(deviceAddress, internalBuffer, size + 1, true); + Sleep(); xSemaphoreGive(mutex); return ret; } @@ -179,13 +183,11 @@ void TwiMaster::Sleep() { } nrf_gpio_cfg_default(6); nrf_gpio_cfg_default(7); - NRF_LOG_INFO("[TWIMASTER] Sleep"); } void TwiMaster::Wakeup() { ConfigurePins(); twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Enabled << TWIM_ENABLE_ENABLE_Pos); - NRF_LOG_INFO("[TWIMASTER] Wakeup"); } /* Sometimes, the TWIM device just freeze and never set the event EVENTS_LASTTX. diff --git a/src/systemtask/SystemTask.cpp b/src/systemtask/SystemTask.cpp index 8915ce74..4316330f 100644 --- a/src/systemtask/SystemTask.cpp +++ b/src/systemtask/SystemTask.cpp @@ -219,7 +219,6 @@ void SystemTask::Work() { break; case Messages::GoToRunning: spi.Wakeup(); - twiMaster.Wakeup(); // Double Tap needs the touch screen to be in normal mode if (!settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) { @@ -240,9 +239,7 @@ void SystemTask::Work() { isDimmed = false; break; case Messages::TouchWakeUp: { - twiMaster.Wakeup(); auto touchInfo = touchPanel.GetTouchInfo(); - twiMaster.Sleep(); if (touchInfo.isTouch and ((touchInfo.gesture == Pinetime::Drivers::Cst816S::Gestures::DoubleTap and settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) or (touchInfo.gesture == Pinetime::Drivers::Cst816S::Gestures::SingleTap and @@ -315,7 +312,6 @@ void SystemTask::Work() { if (!settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::DoubleTap)) { touchPanel.Sleep(); } - twiMaster.Sleep(); isSleeping = true; isGoingToSleep = false; @@ -367,17 +363,12 @@ void SystemTask::UpdateMotion() { if (isSleeping && !settingsController.isWakeUpModeOn(Pinetime::Controllers::Settings::WakeUpMode::RaiseWrist)) return; - if (isSleeping) - twiMaster.Wakeup(); - if (stepCounterMustBeReset) { motionSensor.ResetStepCounter(); stepCounterMustBeReset = false; } auto motionValues = motionSensor.Process(); - if (isSleeping) - twiMaster.Sleep(); motionController.IsSensorOk(motionSensor.IsOk()); motionController.Update(motionValues.x, motionValues.y, motionValues.z, motionValues.steps); From 90ea1071d1df4bf80f2a7d62467aca44c8a63d75 Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Tue, 10 Aug 2021 11:52:20 +0300 Subject: [PATCH 3/6] Explicitly disable pullup --- src/drivers/TwiMaster.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index 34aac07f..52f93d85 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -16,12 +16,14 @@ void TwiMaster::ConfigurePins() const { NRF_GPIO->PIN_CNF[params.pinScl] = (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | + (GPIO_PIN_CNF_PULL_Disabled << GPIO_PIN_CNF_PULL_Pos) | (GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); NRF_GPIO->PIN_CNF[params.pinSda] = (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | + (GPIO_PIN_CNF_PULL_Disabled << GPIO_PIN_CNF_PULL_Pos) | (GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); } From 1d341a7aeb16acb45f25b16590630efd655ecd30 Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Thu, 12 Aug 2021 14:06:58 +0300 Subject: [PATCH 4/6] Don't reconfigure pins --- src/drivers/TwiMaster.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index 52f93d85..429a6eb7 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -180,15 +180,10 @@ TwiMaster::ErrorCodes TwiMaster::Write(uint8_t deviceAddress, const uint8_t* dat } void TwiMaster::Sleep() { - while (twiBaseAddress->ENABLE != 0) { - twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Disabled << TWIM_ENABLE_ENABLE_Pos); - } - nrf_gpio_cfg_default(6); - nrf_gpio_cfg_default(7); + twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Disabled << TWIM_ENABLE_ENABLE_Pos); } void TwiMaster::Wakeup() { - ConfigurePins(); twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Enabled << TWIM_ENABLE_ENABLE_Pos); } From 81a36dc31ed22237e3cc06c8f4ba2a5cbcf07f8e Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Mon, 16 Aug 2021 18:26:10 +0300 Subject: [PATCH 5/6] Simplify parameters and cleanup --- src/drivers/TwiMaster.cpp | 43 +++++++++------------------------------ src/drivers/TwiMaster.h | 19 +++++++---------- src/main.cpp | 5 ++--- 3 files changed, 19 insertions(+), 48 deletions(-) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index 429a6eb7..f17d7168 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -8,19 +8,20 @@ using namespace Pinetime::Drivers; // TODO use shortcut to automatically send STOP when receive LastTX, for example // TODO use DMA/IRQ -TwiMaster::TwiMaster(const Modules module, const Parameters& params) : module {module}, params {params} { +TwiMaster::TwiMaster(NRF_TWIM_Type* module, uint32_t frequency, uint8_t pinSda, uint8_t pinScl) + : module {module}, frequency {frequency}, pinSda {pinSda}, pinScl {pinScl} { mutex = xSemaphoreCreateBinary(); } void TwiMaster::ConfigurePins() const { - NRF_GPIO->PIN_CNF[params.pinScl] = + NRF_GPIO->PIN_CNF[pinScl] = (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | (GPIO_PIN_CNF_PULL_Disabled << GPIO_PIN_CNF_PULL_Pos) | (GPIO_PIN_CNF_DRIVE_S0D1 << GPIO_PIN_CNF_DRIVE_Pos) | (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos); - NRF_GPIO->PIN_CNF[params.pinSda] = + NRF_GPIO->PIN_CNF[pinSda] = (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos) | (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos) | (GPIO_PIN_CNF_PULL_Disabled << GPIO_PIN_CNF_PULL_Pos) | @@ -31,28 +32,12 @@ void TwiMaster::ConfigurePins() const { void TwiMaster::Init() { ConfigurePins(); - switch (module) { - case Modules::TWIM1: - twiBaseAddress = NRF_TWIM1; - break; - default: - return; - } + twiBaseAddress = module; - switch (static_cast(params.frequency)) { - case Frequencies::Khz100: - twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K100; - break; - case Frequencies::Khz250: - twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K250; - break; - case Frequencies::Khz400: - twiBaseAddress->FREQUENCY = TWIM_FREQUENCY_FREQUENCY_K400; - break; - } + twiBaseAddress->FREQUENCY = frequency; - twiBaseAddress->PSEL.SCL = params.pinScl; - twiBaseAddress->PSEL.SDA = params.pinSda; + twiBaseAddress->PSEL.SCL = pinScl; + twiBaseAddress->PSEL.SDA = pinSda; twiBaseAddress->EVENTS_LASTRX = 0; twiBaseAddress->EVENTS_STOPPED = 0; twiBaseAddress->EVENTS_LASTTX = 0; @@ -63,12 +48,6 @@ void TwiMaster::Init() { twiBaseAddress->ENABLE = (TWIM_ENABLE_ENABLE_Enabled << TWIM_ENABLE_ENABLE_Pos); - /* // IRQ - NVIC_ClearPendingIRQ(_IRQn); - NVIC_SetPriority(_IRQn, 2); - NVIC_EnableIRQ(_IRQn); - */ - xSemaphoreGive(mutex); } @@ -194,12 +173,10 @@ void TwiMaster::Wakeup() { * */ void TwiMaster::FixHwFreezed() { NRF_LOG_INFO("I2C device frozen, reinitializing it!"); - // Disable I²C + uint32_t twi_state = NRF_TWI1->ENABLE; - twiBaseAddress->ENABLE = TWIM_ENABLE_ENABLE_Disabled << TWI_ENABLE_ENABLE_Pos; - ConfigurePins(); + Sleep(); - // Re-enable I²C twiBaseAddress->ENABLE = twi_state; } diff --git a/src/drivers/TwiMaster.h b/src/drivers/TwiMaster.h index b8d36d59..30ac6c5f 100644 --- a/src/drivers/TwiMaster.h +++ b/src/drivers/TwiMaster.h @@ -8,16 +8,9 @@ namespace Pinetime { namespace Drivers { class TwiMaster { public: - enum class Modules { TWIM1 }; - enum class Frequencies { Khz100, Khz250, Khz400 }; enum class ErrorCodes { NoError, TransactionFailed }; - struct Parameters { - uint32_t frequency; - uint8_t pinSda; - uint8_t pinScl; - }; - TwiMaster(const Modules module, const Parameters& params); + TwiMaster(NRF_TWIM_Type* module, uint32_t frequency, uint8_t pinSda, uint8_t pinScl); void Init(); ErrorCodes Read(uint8_t deviceAddress, uint8_t registerAddress, uint8_t* buffer, size_t size); @@ -26,16 +19,18 @@ namespace Pinetime { void Sleep(); void Wakeup(); - void ConfigurePins() const; - private: ErrorCodes Read(uint8_t deviceAddress, uint8_t* buffer, size_t size, bool stop); ErrorCodes Write(uint8_t deviceAddress, const uint8_t* data, size_t size, bool stop); void FixHwFreezed(); + void ConfigurePins() const; + NRF_TWIM_Type* twiBaseAddress; SemaphoreHandle_t mutex = nullptr; - const Modules module; - const Parameters params; + NRF_TWIM_Type* module; + uint32_t frequency; + uint8_t pinSda; + uint8_t pinScl; static constexpr uint8_t maxDataSize {16}; static constexpr uint8_t registerSize {1}; uint8_t internalBuffer[maxDataSize + registerSize]; diff --git a/src/main.cpp b/src/main.cpp index ffbba5e7..4e94ab19 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -81,9 +81,8 @@ Pinetime::Drivers::SpiNorFlash spiNorFlash {flashSpi}; // The TWI device should work @ up to 400Khz but there is a HW bug which prevent it from // respecting correct timings. According to erratas heet, this magic value makes it run // at ~390Khz with correct timings. -static constexpr uint32_t MaxTwiFrequencyWithoutHardwareBug {0x06200000}; -Pinetime::Drivers::TwiMaster twiMaster {Pinetime::Drivers::TwiMaster::Modules::TWIM1, - Pinetime::Drivers::TwiMaster::Parameters {MaxTwiFrequencyWithoutHardwareBug, pinTwiSda, pinTwiScl}}; +//static constexpr uint32_t MaxTwiFrequencyWithoutHardwareBug {0x06200000}; +Pinetime::Drivers::TwiMaster twiMaster {NRF_TWIM1, TWI_FREQUENCY_FREQUENCY_K250, pinTwiSda, pinTwiScl}; Pinetime::Drivers::Cst816S touchPanel {twiMaster, touchPanelTwiAddress}; #ifdef PINETIME_IS_RECOVERY static constexpr bool isFactory = true; From 40392d7b9199c87de9784cacb1dee5476e5a5b6f Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Wed, 18 Aug 2021 21:40:27 +0300 Subject: [PATCH 6/6] Use highest frequency and move mutex creation to Init --- src/drivers/TwiMaster.cpp | 5 ++++- src/main.cpp | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/drivers/TwiMaster.cpp b/src/drivers/TwiMaster.cpp index f17d7168..76009278 100644 --- a/src/drivers/TwiMaster.cpp +++ b/src/drivers/TwiMaster.cpp @@ -10,7 +10,6 @@ using namespace Pinetime::Drivers; TwiMaster::TwiMaster(NRF_TWIM_Type* module, uint32_t frequency, uint8_t pinSda, uint8_t pinScl) : module {module}, frequency {frequency}, pinSda {pinSda}, pinScl {pinScl} { - mutex = xSemaphoreCreateBinary(); } void TwiMaster::ConfigurePins() const { @@ -30,6 +29,10 @@ void TwiMaster::ConfigurePins() const { } void TwiMaster::Init() { + if (mutex == nullptr) { + mutex = xSemaphoreCreateBinary(); + } + ConfigurePins(); twiBaseAddress = module; diff --git a/src/main.cpp b/src/main.cpp index 4e94ab19..524035fc 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -81,8 +81,8 @@ Pinetime::Drivers::SpiNorFlash spiNorFlash {flashSpi}; // The TWI device should work @ up to 400Khz but there is a HW bug which prevent it from // respecting correct timings. According to erratas heet, this magic value makes it run // at ~390Khz with correct timings. -//static constexpr uint32_t MaxTwiFrequencyWithoutHardwareBug {0x06200000}; -Pinetime::Drivers::TwiMaster twiMaster {NRF_TWIM1, TWI_FREQUENCY_FREQUENCY_K250, pinTwiSda, pinTwiScl}; +static constexpr uint32_t MaxTwiFrequencyWithoutHardwareBug {0x06200000}; +Pinetime::Drivers::TwiMaster twiMaster {NRF_TWIM1, MaxTwiFrequencyWithoutHardwareBug, pinTwiSda, pinTwiScl}; Pinetime::Drivers::Cst816S touchPanel {twiMaster, touchPanelTwiAddress}; #ifdef PINETIME_IS_RECOVERY static constexpr bool isFactory = true;