From 2981565830fb132353fb4399daec353aae64acbb Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 12 Apr 2020 15:15:04 -0700 Subject: [PATCH 01/38] Remove dead code --- cores/nRF5/wiring_analog_nRF52.c | 42 -------------------------------- 1 file changed, 42 deletions(-) diff --git a/cores/nRF5/wiring_analog_nRF52.c b/cores/nRF5/wiring_analog_nRF52.c index ee95d3183..9c33c6086 100644 --- a/cores/nRF5/wiring_analog_nRF52.c +++ b/cores/nRF5/wiring_analog_nRF52.c @@ -273,48 +273,6 @@ uint32_t analogRead( uint32_t ulPin ) return mapResolution(value, resolution, readResolution); } -#if 0 -// Right now, PWM output only works on the pins with -// hardware support. These are defined in the appropriate -// pins_*.c file. For the rest of the pins, we default -// to digital output. -void analogWrite( uint32_t ulPin, uint32_t ulValue ) -{ - if (ulPin >= PINS_COUNT) { - return; - } - - ulPin = g_ADigitalPinMap[ulPin]; - - for (int i = 0; i < PWM_COUNT; i++) { - if (pwmChannelPins[i] == 0xFFFFFFFF || pwmChannelPins[i] == ulPin) { - pwmChannelPins[i] = ulPin; - pwmChannelSequence[i] = bit(15) | ulValue; - - NRF_PWM_Type* pwm = pwms[i]; - - pwm->PSEL.OUT[0] = ulPin; - pwm->PSEL.OUT[1] = ulPin; - pwm->PSEL.OUT[2] = ulPin; - pwm->PSEL.OUT[3] = ulPin; - pwm->ENABLE = (PWM_ENABLE_ENABLE_Enabled << PWM_ENABLE_ENABLE_Pos); - pwm->PRESCALER = PWM_PRESCALER_PRESCALER_DIV_1; - pwm->MODE = PWM_MODE_UPDOWN_Up; - pwm->COUNTERTOP = (1 << writeResolution) - 1; - pwm->LOOP = 0; - pwm->DECODER = ((uint32_t)PWM_DECODER_LOAD_Common << PWM_DECODER_LOAD_Pos) | ((uint32_t)PWM_DECODER_MODE_RefreshCount << PWM_DECODER_MODE_Pos); - pwm->SEQ[0].PTR = (uint32_t)&pwmChannelSequence[i]; - pwm->SEQ[0].CNT = 1; - pwm->SEQ[0].REFRESH = 1; - pwm->SEQ[0].ENDDELAY = 0; - pwm->TASKS_SEQSTART[0] = 0x1UL; - - break; - } - } -} -#endif - #ifdef __cplusplus } #endif From b4d777f7f329ac5d8c65e42bc21702f15ed8a8f7 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 12 Apr 2020 17:20:03 -0700 Subject: [PATCH 02/38] Expose API for count of used/free channels --- cores/nRF5/HardwarePWM.cpp | 18 ++++++++++++++++++ cores/nRF5/HardwarePWM.h | 4 ++++ 2 files changed, 22 insertions(+) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 881c054df..9caaa8605 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -218,3 +218,21 @@ uint16_t HardwarePWM::readChannel(uint8_t ch) return (_seq0[ch] & 0x7FFF); } +uint8_t HardwarePWM::usedChannelCount(void) +{ + uint8_t usedChannels = 0; + for(int i=0; iPSEL.OUT[i] & PWM_PSEL_OUT_CONNECT_Msk ) + { + usedChannels++; + } + } + return usedChannels; +} + +uint8_t HardwarePWM::freeChannelCount(void) +{ + return MAX_CHANNELS - usedChannelCount(); +} + diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index bf2691e3f..eaccff1f3 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -96,6 +96,10 @@ class HardwarePWM // Read current set value uint16_t readPin (uint8_t pin); uint16_t readChannel (uint8_t ch); + + // Get count of used / free channels + uint8_t usedChannelCount(); + uint8_t freeChannelCount(); }; extern HardwarePWM HwPWM0; From af680357dd68eeb9da44d7da9e40febd485df454 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 12 Apr 2020 17:24:23 -0700 Subject: [PATCH 03/38] analogWrite() avoids hostile takeover of PWM peripherals --- cores/nRF5/wiring_analog.cpp | 66 +++++++++++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index 570be9eb6..c15e459e1 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -45,13 +45,30 @@ extern "C" { +class INTERNAL_ANALOG_STATE { + public: + uint8_t _lastAnalogWriteResolution = 8; // default value is 8-bit resolution (0..255) + uint8_t _moduleUsedByAnalogWrites[HWPWM_MODULE_NUM]; + INTERNAL_ANALOG_STATE() : _lastAnalogWriteResolution(8) { + for (int i = 0; i < HWPWM_MODULE_NUM; i++) { + _moduleUsedByAnalogWrites[i] = 0; + } + } +}; + +static INTERNAL_ANALOG_STATE _AnalogState; + /** - * This will apply to all PWM Hardware + * This will apply to all PWM Hardware currently used by analogWrite(), + * and automatically apply to future calls to analogWrite(). */ void analogWriteResolution( uint8_t res ) { + // save the resolution for when adding a new instance + _AnalogState._lastAnalogWriteResolution = res; for (int i = 0; isetResolution(res); } } @@ -65,17 +82,50 @@ void analogWriteResolution( uint8_t res ) */ void analogWrite( uint32_t pin, uint32_t value ) { + // If the pin is already in use for analogWrite, this should be fast + // If the pin is not already in use, then it's OK to take slightly more time to setup + + // first attempt to find the pin in any of the HWPWM modules used for analog writes for(int i=0; iaddPin(pin) ) - { - HwPWMx[i]->writePin(pin, value); - return; - } + if (!_AnalogState._moduleUsedByAnalogWrites[i]) continue; + int const ch = HwPWMx[i]->pin2channel(pin); + if (ch < 0) continue; // pin not in use by the PWM instance + // already in use by this PWM instance as channel ch ... + HwPWMx[i]->writeChannel(ch, value); + return; } -} + // Next try adding only to those modules that are already in use + for(int i=0; iaddPin(pin)) continue; + // added to an already-used PWM instance, so write the value + HwPWMx[i]->writePin(pin, value); + return; + } + + // Attempt to acquire a new HwPWMx instance ... but only where + // 1. it's not one already used for analog, and + // 2. it currently has no pins in use. + for(int i=0; iusedChannelCount() != 0) continue; + // added to an already-used PWM instance, so write the value + HwPWMx[i]->setResolution(_AnalogState._lastAnalogWriteResolution); + HwPWMx[i]->writePin(pin, value); + return; + } + + // failed to allocate a HwPWM instance. + // output appropriate debug message. + LOG_LV1("ANA", "Unable to find a free PWM peripheral"); + // TODO: Add additional diagnostics to function at higher log levels + return; } +} // end extern "C" + From a771af3ed6f6b5d0397ab170c85b5ae7a3cbfaf6 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 12 Apr 2020 17:35:04 -0700 Subject: [PATCH 04/38] remove dead code --- cores/nRF5/wiring_analog_nRF52.c | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/cores/nRF5/wiring_analog_nRF52.c b/cores/nRF5/wiring_analog_nRF52.c index 9c33c6086..b8a50fc5e 100644 --- a/cores/nRF5/wiring_analog_nRF52.c +++ b/cores/nRF5/wiring_analog_nRF52.c @@ -33,38 +33,14 @@ static uint32_t saadcGain = SAADC_CH_CONFIG_GAIN_Gain1_6; static bool saadcBurst = SAADC_CH_CONFIG_BURST_Disabled; -#if 0 // Note: Adafruit use seperated HardwarePWM class -#define PWM_COUNT 3 - -static NRF_PWM_Type* pwms[PWM_COUNT] = { - NRF_PWM0, - NRF_PWM1, - NRF_PWM2 -}; - -static uint32_t pwmChannelPins[PWM_COUNT] = { - 0xFFFFFFFF, - 0xFFFFFFFF, - 0xFFFFFFFF -}; -static uint16_t pwmChannelSequence[PWM_COUNT]; -#endif +// Note: Adafruit use seperated HardwarePWM class static int readResolution = 10; -//static int writeResolution = 8; - void analogReadResolution( int res ) { readResolution = res; } -#if 0 -void analogWriteResolution( int res ) -{ - writeResolution = res; -} -#endif - static inline uint32_t mapResolution( uint32_t value, uint32_t from, uint32_t to ) { if ( from == to ) From ab1b90490c2fe5b2ae444a490ff2f43c83e6eb87 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 26 Apr 2020 02:27:38 -0700 Subject: [PATCH 05/38] opt-in ownership of PWM peripheral --- cores/nRF5/HardwarePWM.cpp | 19 +++++++++++++++++++ cores/nRF5/HardwarePWM.h | 13 +++++++++++++ 2 files changed, 32 insertions(+) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 9caaa8605..9169d9c44 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -53,6 +53,25 @@ HardwarePWM* HwPWMx[] = #endif }; +// returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr +bool HardwarePWM::takeOwnership(uintptr_t token) +{ + if (this->_owner_token != 0) return false; // doesn't matter if it's actually a match ... it's not legal to take ownership twice + if (this->usedChannelCount() != 0) return false; // at least one channel is already in use + // use gcc built-in intrinsic to ensure atomicity + // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html + return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); +} +// returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches +bool HardwarePWM::releaseOwnership(uintptr_t token) +{ + if (!this->isOwner(token) ) return false; // don't even look at peripheral + if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins + // use gcc built-in intrinsic to ensure atomicity + // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html + return __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); +} + HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) { _pwm = pwm; diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index eaccff1f3..5bfe69779 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -50,6 +50,7 @@ class HardwarePWM private: enum { MAX_CHANNELS = 4 }; // Max channel per group NRF_PWM_Type* _pwm; + uintptr_t _owner_token = 0; uint16_t _seq0[MAX_CHANNELS]; @@ -67,6 +68,18 @@ class HardwarePWM void setClockDiv(uint8_t div); // value is PWM_PRESCALER_PRESCALER_DIV_x, DIV1 is 16Mhz + // Cooperative ownership sharing + + // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr + bool takeOwnership(uintptr_t token); + // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches + bool releaseOwnership(uintptr_t token); + // allows caller to verify that they own the peripheral + __INLINE bool isOwner(uintptr_t token) __attribute__((__always_inline__)) + { + return this->_owner_token == token; + } + bool addPin (uint8_t pin); bool removePin (uint8_t pin); From d469728574ec3c7318dbbccd70607ac787c7ea7e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 27 Apr 2020 00:58:04 -0700 Subject: [PATCH 06/38] Use HwPWM ownership abstraction --- cores/nRF5/common_func.h | 10 ++++++ cores/nRF5/wiring_analog.cpp | 68 +++++++++++++++++++++--------------- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/cores/nRF5/common_func.h b/cores/nRF5/common_func.h index 60c7cf0d5..c6229b8a2 100644 --- a/cores/nRF5/common_func.h +++ b/cores/nRF5/common_func.h @@ -73,6 +73,8 @@ #define ADA_COUNTER __LINE__ #endif +#include + #define COMMENT_OUT(x) #define memclr(buffer, size) memset(buffer, 0, size) @@ -170,6 +172,14 @@ const char* dbg_err_str(int32_t err_id); // TODO move to other place #define LOG_LV2_BUFFER(...) #endif +#if CFG_DEBUG >= 3 +#define LOG_LV3(...) ADALOG(__VA_ARGS__) +#define LOG_LV3_BUFFER(...) ADALOG_BUFFER(__VA_ARGS__) +#else +#define LOG_LV3(...) +#define LOG_LV3_BUFFER(...) +#endif + #if CFG_DEBUG #define PRINT_LOCATION() PRINTF("%s: %d:\n", __PRETTY_FUNCTION__, __LINE__) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index c15e459e1..e7d7b2f49 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -36,6 +36,7 @@ #include "Arduino.h" + /* Implement analogWrite() and analogWriteResolution() using * HardwarePWM. * @@ -45,18 +46,8 @@ extern "C" { -class INTERNAL_ANALOG_STATE { - public: - uint8_t _lastAnalogWriteResolution = 8; // default value is 8-bit resolution (0..255) - uint8_t _moduleUsedByAnalogWrites[HWPWM_MODULE_NUM]; - INTERNAL_ANALOG_STATE() : _lastAnalogWriteResolution(8) { - for (int i = 0; i < HWPWM_MODULE_NUM; i++) { - _moduleUsedByAnalogWrites[i] = 0; - } - } -}; - -static INTERNAL_ANALOG_STATE _AnalogState; +static uint8_t _lastAnalogWriteResolution; +static const uintptr_t _token = (uintptr_t)(&_lastAnalogWriteResolution); /** * This will apply to all PWM Hardware currently used by analogWrite(), @@ -65,10 +56,10 @@ static INTERNAL_ANALOG_STATE _AnalogState; void analogWriteResolution( uint8_t res ) { // save the resolution for when adding a new instance - _AnalogState._lastAnalogWriteResolution = res; + _lastAnalogWriteResolution = res; for (int i = 0; iisOwner(_token)) continue; HwPWMx[i]->setResolution(res); } } @@ -82,26 +73,42 @@ void analogWriteResolution( uint8_t res ) */ void analogWrite( uint32_t pin, uint32_t value ) { - // If the pin is already in use for analogWrite, this should be fast - // If the pin is not already in use, then it's OK to take slightly more time to setup + /* If the pin is already in use for analogWrite, this should be fast + If the pin is not already in use, then it's OK to take slightly more time to setup + */ - // first attempt to find the pin in any of the HWPWM modules used for analog writes + // first, handle the case where the pin is already in use by analogWrite() for(int i=0; iisOwner(_token)) { + LOG_LV3("ANA", "not currently owner of PWM %d", i); + continue; // skip if not owner of this PWM instance + } + int const ch = HwPWMx[i]->pin2channel(pin); - if (ch < 0) continue; // pin not in use by the PWM instance - // already in use by this PWM instance as channel ch ... + if (ch < 0) { + LOG_LV3("ANA", "pin %d is not used by PWM %d", pin, i); + continue; // pin not in use by this PWM instance + } + LOG_LV2("ANA", "updating pin %" PRIu32 " used by PWM %d", pin, i); HwPWMx[i]->writeChannel(ch, value); return; } - // Next try adding only to those modules that are already in use + // Next, handle the case where can add the pin to a PWM instance already owned by analogWrite() for(int i=0; iaddPin(pin)) continue; - // added to an already-used PWM instance, so write the value + if (!HwPWMx[i]->isOwner(_token)) { + LOG_LV3("ANA", "not currently owner of PWM %d", i); + continue; + } + if (!HwPWMx[i]->addPin(pin)) { + LOG_LV3("ANA", "could not add pin %" PRIu32 " to PWM %d", pin, i); + continue; + } + + // successfully added the pin, so write the value also + LOG_LV2("ANA", "Added pin %" PRIu32 " to already-owned PWM %d", pin, i); HwPWMx[i]->writePin(pin, value); return; } @@ -111,11 +118,16 @@ void analogWrite( uint32_t pin, uint32_t value ) // 2. it currently has no pins in use. for(int i=0; iusedChannelCount() != 0) continue; - // added to an already-used PWM instance, so write the value - HwPWMx[i]->setResolution(_AnalogState._lastAnalogWriteResolution); + if (!HwPWMx[i]->takeOwnership(_token)) { + LOG_LV3("ANA", "Could not take ownership of PWM %d", i); + continue; + } + + // apply the cached analog resolution to newly owned instances + HwPWMx[i]->setResolution(_lastAnalogWriteResolution); + HwPWMx[i]->addPin(pin); HwPWMx[i]->writePin(pin, value); + LOG_LV2("ANA", "took ownership of, and added pin %" PRIu32 " to, PWM %d", pin, i); return; } From d5dc053a34a4f2e83f4c3d98f0fbafb53302b510 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 1 May 2020 16:42:25 -0700 Subject: [PATCH 07/38] Use HwPWM ownership abstraction --- cores/nRF5/Tone.cpp | 59 +++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index e244d3915..20def523a 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -34,35 +34,54 @@ Version Modified By Date Comments 0010 Arduino.org 16/07/27 Added Arduino Primo support *************************************************/ - +#include "Arduino.h" #include "Tone.h" #include "WVariant.h" unsigned long int count_duration=0; volatile bool no_stop = false; uint8_t pin_sound=0; +static uintptr_t _toneToken = (uintptr_t)(&_toneToken); void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { + static_assert(sizeof(unsigned long) == sizeof(uint32_t)); + unsigned int time_per=0; - - if((frequency < 20) | (frequency > 25000)) return; - + + // limit frequency to reasonable audible range + if((frequency < 20) | (frequency > 25000)) { + LOG_LV1("TON", "frequency outside range [20..25000] -- ignoring"); + return; + } + + // Use fixed PWM2 (due to need to connect interrupt) + if (!HwPWMx[2]->isOwner(_toneToken) && + !HwPWMx[2]->takeOwnership(_toneToken)) { + LOG_LV1("TON", "unable to allocate PWM2 to Tone"); + return; + } float per=float(1)/frequency; time_per=per/0.000008; unsigned int duty=time_per/2; - if(duration > 0){ + if(duration > 0) { no_stop = false; float mil=float(duration)/1000; - if(per>mil) + if(per>mil) { count_duration=1; - else + } else { count_duration= mil/per; - } - else + } + } else { no_stop = true; + } + + // PWM configuration depends on the following: + // [ ] time_per + // [ ] duty ( via seq ) + // [ ] pin ( via pins[0] ) // Configure PWM static uint16_t seq_values[]={0}; @@ -97,6 +116,7 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) break; } #else + // Use fixed PWM2, TODO could conflict with other usage uint32_t pins[NRF_PWM_CHANNEL_COUNT]={NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED}; pins[0] = g_ADigitalPinMap[pin]; @@ -110,7 +130,7 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) nrf_pwm_configure(PWMInstance, NRF_PWM_CLK_125kHz, NRF_PWM_MODE_UP, time_per); nrf_pwm_decoder_set(PWMInstance, NRF_PWM_LOAD_COMMON, NRF_PWM_STEP_AUTO); nrf_pwm_sequence_set(PWMInstance, 0, &seq); - nrf_pwm_shorts_enable(PWMInstance, NRF_PWM_SHORT_SEQEND0_STOP_MASK); + nrf_pwm_shorts_enable(PWMInstance, NRF_PWM_SHORT_SEQEND0_STOP_MASK); // shortcut for when SEQ0 ends, PWM output will automatically stop // enable interrupt nrf_pwm_event_clear(PWMInstance, NRF_PWM_EVENT_PWMPERIODEND); @@ -125,6 +145,11 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) void noTone(uint8_t pin) { + if (!HwPWMx[2]->isOwner(_toneToken)) { + LOG_LV1("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); + return; + } + #if 0 uint8_t pwm_type=g_APinDescription[pin].ulPWMChannel; NRF_PWM_Type * PWMInstance = NRF_PWM0; @@ -180,13 +205,15 @@ void PWM2_IRQHandler(void){ nrf_pwm_event_clear(NRF_PWM2, NRF_PWM_EVENT_PWMPERIODEND); if(!no_stop){ count_duration--; - if(count_duration == 0) - noTone(pin_sound); - else - nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); - } - else + if(count_duration == 0) { + nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_STOP); + nrf_pwm_disable(NRF_PWM2); + } else { + nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); + } + } else { nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); + } } #ifdef __cplusplus From 3efce16900e7b16f048f2e1e55eddefaac6d4594 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 3 May 2020 19:39:45 -0700 Subject: [PATCH 08/38] Servo can also use HwPWM ownership abstraction --- libraries/Servo/src/Servo.h | 2 +- libraries/Servo/src/nrf52/Servo.cpp | 108 ++++++++++++++++++---------- 2 files changed, 70 insertions(+), 40 deletions(-) diff --git a/libraries/Servo/src/Servo.h b/libraries/Servo/src/Servo.h index 50979dfbd..28df7462e 100644 --- a/libraries/Servo/src/Servo.h +++ b/libraries/Servo/src/Servo.h @@ -101,7 +101,7 @@ class Servo { public: Servo(); - uint8_t attach(int pin); // attach the given pin to the next free channel, sets pinMode, returns channel number or 0 if failure + uint8_t attach(int pin); // attach the given pin to the next free channel, sets pinMode, returns channel number or INVALID_SERVO if failure (zero is a valid channel number) uint8_t attach(int pin, int min, int max); // as above but also sets min and max values for writes. void detach(); void write(int value); // if value is < 200 its treated as an angle, otherwise as pulse width in microseconds diff --git a/libraries/Servo/src/nrf52/Servo.cpp b/libraries/Servo/src/nrf52/Servo.cpp index b9b86c02f..5966873a7 100644 --- a/libraries/Servo/src/nrf52/Servo.cpp +++ b/libraries/Servo/src/nrf52/Servo.cpp @@ -21,6 +21,7 @@ #include #include +static uintptr_t _servoToken = (uintptr_t)(&_servoToken); static servo_t servos[MAX_SERVOS]; // static array of servo structures uint8_t ServoCount = 0; // the total number of attached servos @@ -32,7 +33,6 @@ Servo::Servo() } else { this->servoIndex = INVALID_SERVO; // too many servos } - this->pwm = NULL; } @@ -43,55 +43,81 @@ uint8_t Servo::attach(int pin) uint8_t Servo::attach(int pin, int min, int max) { - if (this->servoIndex < MAX_SERVOS) { - pinMode(pin, OUTPUT); // set servo pin to output - servos[this->servoIndex].Pin.nbr = pin; - - if (min < MIN_PULSE_WIDTH) min = MIN_PULSE_WIDTH; - if (max > MAX_PULSE_WIDTH) max = MAX_PULSE_WIDTH; - - //fix min if conversion to pulse cycle value is too low - if((min/DUTY_CYCLE_RESOLUTION)*DUTY_CYCLE_RESOLUTIONmin = min; - this->max = max; - - servos[this->servoIndex].Pin.isActive = true; - - // Adafruit, add pin to 1 of available Hw PWM - for(int i=0; iaddPin(pin) ) - { + if (this->servoIndex == INVALID_SERVO) { + return INVALID_SERVO; + } + bool succeeded = false; + pinMode(pin, OUTPUT); // set servo pin to output + servos[this->servoIndex].Pin.nbr = pin; + + if (min < MIN_PULSE_WIDTH) { + min = MIN_PULSE_WIDTH; + } + if (max > MAX_PULSE_WIDTH) { + max = MAX_PULSE_WIDTH; + } + + //fix min if conversion to pulse cycle value is too low + if ( (min/DUTY_CYCLE_RESOLUTION) * DUTY_CYCLE_RESOLUTION < min) { + min += DUTY_CYCLE_RESOLUTION; + } + + this->min = min; + this->max = max; + + servos[this->servoIndex].Pin.isActive = true; + + // Adafruit, add pin to 1 of available Hw PWM + // first, use existing HWPWM modules (already owned by Servo) + for(int i=0; iisOwner(_servoToken) && + HwPWMx[i]->addPin(pin)) { + this->pwm = HwPWMx[i]; + succeeded = true; + break; + } + } + // if could not add to existing owned PWM modules, try to add to a new PWM module + if (!succeeded) { + for(int i=0; itakeOwnership(_servoToken) && + HwPWMx[i]->addPin(pin)) { this->pwm = HwPWMx[i]; + succeeded = true; break; } } + } + if (succeeded) { // do not use this->pwm unless success above! this->pwm->setMaxValue(MAXVALUE); this->pwm->setClockDiv(CLOCKDIV); - } - return this->servoIndex; + return succeeded ? this->servoIndex : INVALID_SERVO; // return INVALID_SERVO on failure (zero is a valid servo index) } void Servo::detach() { - uint8_t const pin = servos[this->servoIndex].Pin.nbr; - - servos[this->servoIndex].Pin.isActive = false; + if (this->servoIndex == INVALID_SERVO) { + return; + } - // remove pin from HW PWM - this->pwm->removePin(pin); + uint8_t const pin = servos[this->servoIndex].Pin.nbr; + servos[this->servoIndex].Pin.isActive = false; + // remove pin from HW PWM + HardwarePWM * pwm = this->pwm; + this->pwm = nullptr; + pwm->removePin(pin); + pwm->releaseOwnership(_servoToken); // ignore failure ... which happens if a pin is still in use, for example } - void Servo::write(int value) -{ - if (value < 0) +{ + if (value < 0) { value = 0; - else if (value > 180) + } else if (value > 180) { value = 180; + } value = map(value, 0, 180, this->min, this->max); writeMicroseconds(value); @@ -100,9 +126,10 @@ void Servo::write(int value) void Servo::writeMicroseconds(int value) { - uint8_t pin = servos[this->servoIndex].Pin.nbr; - - if ( this->pwm ) this->pwm->writePin(pin, value/DUTY_CYCLE_RESOLUTION); + if (this->pwm) { // pwm is only set when this->servoIndex != INVALID_SERVO + uint8_t pin = servos[this->servoIndex].Pin.nbr; + this->pwm->writePin(pin, value/DUTY_CYCLE_RESOLUTION); + } } int Servo::read() // return the value as degrees @@ -112,15 +139,18 @@ int Servo::read() // return the value as degrees int Servo::readMicroseconds() { - uint8_t pin = servos[this->servoIndex].Pin.nbr; - - if ( this->pwm ) return this->pwm->readPin(pin)*DUTY_CYCLE_RESOLUTION; - + if (this->pwm) { // pwm is only set when this->servoIndex != INVALID_SERVO + uint8_t pin = servos[this->servoIndex].Pin.nbr; + return this->pwm->readPin(pin)*DUTY_CYCLE_RESOLUTION; + } return 0; } bool Servo::attached() { + if (this->servoIndex == INVALID_SERVO) { + return false; + } return servos[this->servoIndex].Pin.isActive; } From c58c01df68a395fb134759e44eb6fc7829e51b3b Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 3 May 2020 20:00:22 -0700 Subject: [PATCH 09/38] Add check for PWM being ENABLED --- cores/nRF5/HardwarePWM.cpp | 2 ++ libraries/Servo/src/nrf52/Servo.cpp | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 9169d9c44..fc8ceb00f 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -58,6 +58,7 @@ bool HardwarePWM::takeOwnership(uintptr_t token) { if (this->_owner_token != 0) return false; // doesn't matter if it's actually a match ... it's not legal to take ownership twice if (this->usedChannelCount() != 0) return false; // at least one channel is already in use + if (this->enabled ) return false; // if it's enabled, do not allow new ownership, even with no pins in use // use gcc built-in intrinsic to ensure atomicity // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); @@ -67,6 +68,7 @@ bool HardwarePWM::releaseOwnership(uintptr_t token) { if (!this->isOwner(token) ) return false; // don't even look at peripheral if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins + if ( this->enabled ) return false; // if it's enabled, do not allow ownership to be released, even with no pins in use // use gcc built-in intrinsic to ensure atomicity // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html return __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); diff --git a/libraries/Servo/src/nrf52/Servo.cpp b/libraries/Servo/src/nrf52/Servo.cpp index 5966873a7..bd3584d40 100644 --- a/libraries/Servo/src/nrf52/Servo.cpp +++ b/libraries/Servo/src/nrf52/Servo.cpp @@ -108,7 +108,10 @@ void Servo::detach() HardwarePWM * pwm = this->pwm; this->pwm = nullptr; pwm->removePin(pin); - pwm->releaseOwnership(_servoToken); // ignore failure ... which happens if a pin is still in use, for example + if (pwm->usedChannelCount() == 0) { + pwm->stop(); // disables peripheral so can release ownership + pwm->releaseOwnership(_servoToken); + } } void Servo::write(int value) From c190318bb9af4d342333c22fb708065effd63e2b Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 3 May 2020 20:23:13 -0700 Subject: [PATCH 10/38] Use const void* for ownership token --- cores/nRF5/HardwarePWM.cpp | 8 ++++---- cores/nRF5/HardwarePWM.h | 9 +++++---- cores/nRF5/Tone.cpp | 2 +- cores/nRF5/wiring_analog.cpp | 10 +++++----- libraries/Servo/src/nrf52/Servo.cpp | 2 +- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index fc8ceb00f..85db29611 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -54,21 +54,21 @@ HardwarePWM* HwPWMx[] = }; // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr -bool HardwarePWM::takeOwnership(uintptr_t token) +bool HardwarePWM::takeOwnership(void const * token) { if (this->_owner_token != 0) return false; // doesn't matter if it's actually a match ... it's not legal to take ownership twice if (this->usedChannelCount() != 0) return false; // at least one channel is already in use - if (this->enabled ) return false; // if it's enabled, do not allow new ownership, even with no pins in use + if (this->enabled() ) return false; // if it's enabled, do not allow new ownership, even with no pins in use // use gcc built-in intrinsic to ensure atomicity // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); } // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches -bool HardwarePWM::releaseOwnership(uintptr_t token) +bool HardwarePWM::releaseOwnership(void const * token) { if (!this->isOwner(token) ) return false; // don't even look at peripheral if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins - if ( this->enabled ) return false; // if it's enabled, do not allow ownership to be released, even with no pins in use + if ( this->enabled() ) return false; // if it's enabled, do not allow ownership to be released, even with no pins in use // use gcc built-in intrinsic to ensure atomicity // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html return __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index 5bfe69779..59751466e 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -50,7 +50,7 @@ class HardwarePWM private: enum { MAX_CHANNELS = 4 }; // Max channel per group NRF_PWM_Type* _pwm; - uintptr_t _owner_token = 0; + void const * _owner_token = nullptr; uint16_t _seq0[MAX_CHANNELS]; @@ -71,11 +71,12 @@ class HardwarePWM // Cooperative ownership sharing // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr - bool takeOwnership(uintptr_t token); + bool takeOwnership (void const * token); // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches - bool releaseOwnership(uintptr_t token); + bool releaseOwnership(void const * token); + // allows caller to verify that they own the peripheral - __INLINE bool isOwner(uintptr_t token) __attribute__((__always_inline__)) + __INLINE bool isOwner(void const * token) __attribute__((__always_inline__)) { return this->_owner_token == token; } diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 20def523a..5f073d4ed 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -41,7 +41,7 @@ Version Modified By Date Comments unsigned long int count_duration=0; volatile bool no_stop = false; uint8_t pin_sound=0; -static uintptr_t _toneToken = (uintptr_t)(&_toneToken); +static char const * _toneToken = "Tone"; void tone(uint8_t pin, unsigned int frequency, unsigned long duration) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index e7d7b2f49..79ccf4084 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -47,7 +47,7 @@ extern "C" { static uint8_t _lastAnalogWriteResolution; -static const uintptr_t _token = (uintptr_t)(&_lastAnalogWriteResolution); +static char const * _analogToken = "analog"; /** * This will apply to all PWM Hardware currently used by analogWrite(), @@ -59,7 +59,7 @@ void analogWriteResolution( uint8_t res ) _lastAnalogWriteResolution = res; for (int i = 0; iisOwner(_token)) continue; + if (!HwPWMx[i]->isOwner(_analogToken)) continue; HwPWMx[i]->setResolution(res); } } @@ -80,7 +80,7 @@ void analogWrite( uint32_t pin, uint32_t value ) // first, handle the case where the pin is already in use by analogWrite() for(int i=0; iisOwner(_token)) { + if (!HwPWMx[i]->isOwner(_analogToken)) { LOG_LV3("ANA", "not currently owner of PWM %d", i); continue; // skip if not owner of this PWM instance } @@ -98,7 +98,7 @@ void analogWrite( uint32_t pin, uint32_t value ) // Next, handle the case where can add the pin to a PWM instance already owned by analogWrite() for(int i=0; iisOwner(_token)) { + if (!HwPWMx[i]->isOwner(_analogToken)) { LOG_LV3("ANA", "not currently owner of PWM %d", i); continue; } @@ -118,7 +118,7 @@ void analogWrite( uint32_t pin, uint32_t value ) // 2. it currently has no pins in use. for(int i=0; itakeOwnership(_token)) { + if (!HwPWMx[i]->takeOwnership(_analogToken)) { LOG_LV3("ANA", "Could not take ownership of PWM %d", i); continue; } diff --git a/libraries/Servo/src/nrf52/Servo.cpp b/libraries/Servo/src/nrf52/Servo.cpp index bd3584d40..adbcf4553 100644 --- a/libraries/Servo/src/nrf52/Servo.cpp +++ b/libraries/Servo/src/nrf52/Servo.cpp @@ -21,7 +21,7 @@ #include #include -static uintptr_t _servoToken = (uintptr_t)(&_servoToken); +static char const * _servoToken = "Servo"; static servo_t servos[MAX_SERVOS]; // static array of servo structures uint8_t ServoCount = 0; // the total number of attached servos From 027bcd348cde77859f7241d21d0e14a88e532a1c Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 3 May 2020 23:46:34 -0700 Subject: [PATCH 11/38] Fix check for connected pins --- cores/nRF5/HardwarePWM.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 85db29611..cd070aa3f 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -244,7 +244,7 @@ uint8_t HardwarePWM::usedChannelCount(void) uint8_t usedChannels = 0; for(int i=0; iPSEL.OUT[i] & PWM_PSEL_OUT_CONNECT_Msk ) + if ( (_pwm->PSEL.OUT[i] & PWM_PSEL_OUT_CONNECT_Msk) != (PWM_PSEL_OUT_CONNECT_Disconnected << PWM_PSEL_OUT_CONNECT_Pos) ) { usedChannels++; } From 85e41afe270ccd1c6dcba3995402ca527a75fe5e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 10 May 2020 00:02:06 -0700 Subject: [PATCH 12/38] remove dead code --- cores/nRF5/Tone.cpp | 70 --------------------------------------------- 1 file changed, 70 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 5f073d4ed..30b6eacfc 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -78,11 +78,6 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) no_stop = true; } - // PWM configuration depends on the following: - // [ ] time_per - // [ ] duty ( via seq ) - // [ ] pin ( via pins[0] ) - // Configure PWM static uint16_t seq_values[]={0}; //In each value, the most significant bit (15) determines the polarity of the output @@ -95,35 +90,12 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) 0 }; -#if 0 - //assign pin to pwm channel - look at WVariant.h for details about ulPWMChannel attribute - uint8_t pwm_type=g_APinDescription[pin].ulPWMChannel; - if(pwm_type == NOT_ON_PWM) - return; - - uint32_t pins[NRF_PWM_CHANNEL_COUNT]={NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED}; - pins[pwm_type & 0x0F]=g_APinDescription[pin].ulPin; - IRQn_Type IntNo = PWM0_IRQn; - NRF_PWM_Type * PWMInstance = NRF_PWM0; - switch(pwm_type &0xF0){ - case 16://0x10 - PWMInstance = NRF_PWM1; - IntNo = PWM1_IRQn; - break; - case 32://0x20 - PWMInstance = NRF_PWM2; - IntNo = PWM2_IRQn; - break; - } -#else - // Use fixed PWM2, TODO could conflict with other usage uint32_t pins[NRF_PWM_CHANNEL_COUNT]={NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED}; pins[0] = g_ADigitalPinMap[pin]; IRQn_Type IntNo = PWM2_IRQn; NRF_PWM_Type * PWMInstance = NRF_PWM2; -#endif nrf_pwm_pins_set(PWMInstance, pins); nrf_pwm_enable(PWMInstance); @@ -150,21 +122,7 @@ void noTone(uint8_t pin) return; } -#if 0 - uint8_t pwm_type=g_APinDescription[pin].ulPWMChannel; - NRF_PWM_Type * PWMInstance = NRF_PWM0; - switch(pwm_type &0xF0){ - case 16://0x10 - PWMInstance = NRF_PWM1; - break; - case 32://0x20 - PWMInstance = NRF_PWM2; - break; - } -#else NRF_PWM_Type * PWMInstance = NRF_PWM2; -#endif - nrf_pwm_task_trigger(PWMInstance, NRF_PWM_TASK_STOP); nrf_pwm_disable(PWMInstance); } @@ -173,34 +131,6 @@ void noTone(uint8_t pin) extern "C"{ #endif -#if 0 -void PWM0_IRQHandler(void){ - nrf_pwm_event_clear(NRF_PWM0, NRF_PWM_EVENT_PWMPERIODEND); - if(!no_stop){ - count_duration--; - if(count_duration == 0) - noTone(pin_sound); - else - nrf_pwm_task_trigger(NRF_PWM0, NRF_PWM_TASK_SEQSTART0); - } - else - nrf_pwm_task_trigger(NRF_PWM0, NRF_PWM_TASK_SEQSTART0); -} - -void PWM1_IRQHandler(void){ - nrf_pwm_event_clear(NRF_PWM1, NRF_PWM_EVENT_PWMPERIODEND); - if(!no_stop){ - count_duration--; - if(count_duration == 0) - noTone(pin_sound); - else - nrf_pwm_task_trigger(NRF_PWM1, NRF_PWM_TASK_SEQSTART0); - } - else - nrf_pwm_task_trigger(NRF_PWM1, NRF_PWM_TASK_SEQSTART0); -} -#endif - void PWM2_IRQHandler(void){ nrf_pwm_event_clear(NRF_PWM2, NRF_PWM_EVENT_PWMPERIODEND); if(!no_stop){ From a6e0d2c925691ab5f588702455b952e2161410e4 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 10 May 2020 18:14:10 -0700 Subject: [PATCH 13/38] Prefer `uintptr_t` to `void const *` for ownership token --- cores/nRF5/HardwarePWM.cpp | 59 ++++++++++++++++++++++++++++++++------ cores/nRF5/HardwarePWM.h | 32 ++++++++++++++------- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index cd070aa3f..6c354fb0a 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -53,9 +53,51 @@ HardwarePWM* HwPWMx[] = #endif }; +bool can_stringify_token(uintptr_t token) +{ + uint8_t * t = (uint8_t *)&token; + for (size_t i = 0; i < sizeof(uintptr_t); ++i, ++t) + { + uint8_t x = *t; + if ((x < 0x20) || (x > 0x7E)) return false; + } + return true; +} + +void HardwarePWM::DebugOutput(Stream& logger) +{ + const size_t count = arrcount(HwPWMx); + logger.printf("HwPWM Debug:"); + for (size_t i = 0; i < count; i++) { + HardwarePWM const * pwm = HwPWMx[i]; + uintptr_t token = pwm->_owner_token; + logger.printf(" || %d:", i); + if (can_stringify_token(token)) { + uint8_t * t = (uint8_t*)(&token); + static_assert(sizeof(uintptr_t) == 4); + logger.printf(" \"%c%c%c%c\"", t[0], t[1], t[2], t[3] ); + } else { + static_assert(sizeof(uintptr_t) == 4); + logger.printf(" %08x ", token); + } + for (size_t j = 0; j < MAX_CHANNELS; j++) { + uint32_t r = pwm->_pwm->PSEL.OUT[i]; // only read it once + if ( (r & PWM_PSEL_OUT_CONNECT_Msk) != (PWM_PSEL_OUT_CONNECT_Disconnected << PWM_PSEL_OUT_CONNECT_Pos) ) { + logger.printf(" %02x", r & 0x1F); + } else { + logger.printf(" xx"); + } + } + } + logger.printf("\n"); +} + + + // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr -bool HardwarePWM::takeOwnership(void const * token) +bool HardwarePWM::takeOwnership(uintptr_t token) { + if (token == 0) return false; // cannot take ownership with nullptr if (this->_owner_token != 0) return false; // doesn't matter if it's actually a match ... it's not legal to take ownership twice if (this->usedChannelCount() != 0) return false; // at least one channel is already in use if (this->enabled() ) return false; // if it's enabled, do not allow new ownership, even with no pins in use @@ -64,8 +106,9 @@ bool HardwarePWM::takeOwnership(void const * token) return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); } // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches -bool HardwarePWM::releaseOwnership(void const * token) +bool HardwarePWM::releaseOwnership(uintptr_t token) { + if (token == 0) return false; // cannot release ownership with nullptr if (!this->isOwner(token) ) return false; // don't even look at peripheral if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins if ( this->enabled() ) return false; // if it's enabled, do not allow ownership to be released, even with no pins in use @@ -74,9 +117,9 @@ bool HardwarePWM::releaseOwnership(void const * token) return __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); } -HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) +HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) : + _pwm(pwm) { - _pwm = pwm; arrclr(_seq0); _max_value = 255; @@ -225,7 +268,7 @@ bool HardwarePWM::writePin(uint8_t pin, uint16_t value, bool inverted) return writeChannel(ch, value, inverted); } -uint16_t HardwarePWM::readPin(uint8_t pin) +uint16_t HardwarePWM::readPin(uint8_t pin) const { int ch = pin2channel(pin); VERIFY( ch >= 0, 0); @@ -233,13 +276,13 @@ uint16_t HardwarePWM::readPin(uint8_t pin) return readChannel(ch); } -uint16_t HardwarePWM::readChannel(uint8_t ch) +uint16_t HardwarePWM::readChannel(uint8_t ch) const { // remove inverted bit return (_seq0[ch] & 0x7FFF); } -uint8_t HardwarePWM::usedChannelCount(void) +uint8_t HardwarePWM::usedChannelCount(void) const { uint8_t usedChannels = 0; for(int i=0; i_owner_token == token; } + __INLINE bool takeOwnership (void const * token) + { return takeOwnership((uintptr_t)token); } + __INLINE bool releaseOwnership(void const * token) + { return releaseOwnership((uintptr_t)token); } + __INLINE bool isOwner (void const * token) const + __attribute__ ((__always_inline__)) + { return isOwner((uintptr_t)token); } bool addPin (uint8_t pin); bool removePin (uint8_t pin); - int pin2channel(uint8_t pin) + int pin2channel(uint8_t pin) const { pin = g_ADigitalPinMap[pin]; for(int i=0; i= 0; } @@ -108,12 +115,15 @@ class HardwarePWM bool writeChannel(uint8_t ch , uint16_t value, bool inverted = false); // Read current set value - uint16_t readPin (uint8_t pin); - uint16_t readChannel (uint8_t ch); + uint16_t readPin (uint8_t pin) const; + uint16_t readChannel (uint8_t ch) const; // Get count of used / free channels - uint8_t usedChannelCount(); - uint8_t freeChannelCount(); + uint8_t usedChannelCount() const; + uint8_t freeChannelCount() const; + + // for debug/validation + static void DebugOutput(Stream& logger); }; extern HardwarePWM HwPWM0; From 0acacf67b813c7689ccb2113d88e5f806518d57e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 10 May 2020 18:52:54 -0700 Subject: [PATCH 14/38] Finish conversion back to `uintptr_t` token --- cores/nRF5/HardwarePWM.cpp | 4 ++-- cores/nRF5/HardwarePWM.h | 13 ++++--------- cores/nRF5/Tone.cpp | 3 +-- cores/nRF5/wiring_analog.cpp | 2 +- libraries/Servo/src/nrf52/Servo.cpp | 2 +- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 6c354fb0a..bf68dcb77 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -53,6 +53,7 @@ HardwarePWM* HwPWMx[] = #endif }; +#if CFG_DEBUG bool can_stringify_token(uintptr_t token) { uint8_t * t = (uint8_t *)&token; @@ -91,8 +92,7 @@ void HardwarePWM::DebugOutput(Stream& logger) } logger.printf("\n"); } - - +#endif // CFG_DEBUG // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr bool HardwarePWM::takeOwnership(uintptr_t token) diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index 7457d1642..fbf9a672a 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -76,17 +76,11 @@ class HardwarePWM bool releaseOwnership(uintptr_t token); // allows caller to verify that they own the peripheral - __INLINE bool isOwner(uintptr_t token) const __attribute__((__always_inline__)) + __INLINE bool isOwner(uintptr_t token) const + __attribute__ ((__always_inline__)) { return this->_owner_token == token; } - __INLINE bool takeOwnership (void const * token) - { return takeOwnership((uintptr_t)token); } - __INLINE bool releaseOwnership(void const * token) - { return releaseOwnership((uintptr_t)token); } - __INLINE bool isOwner (void const * token) const - __attribute__ ((__always_inline__)) - { return isOwner((uintptr_t)token); } bool addPin (uint8_t pin); bool removePin (uint8_t pin); @@ -122,8 +116,9 @@ class HardwarePWM uint8_t usedChannelCount() const; uint8_t freeChannelCount() const; - // for debug/validation +#if CFG_DEBUG static void DebugOutput(Stream& logger); +#endif // CFG_DEBUG }; extern HardwarePWM HwPWM0; diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 30b6eacfc..ed70680ec 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -41,8 +41,7 @@ Version Modified By Date Comments unsigned long int count_duration=0; volatile bool no_stop = false; uint8_t pin_sound=0; -static char const * _toneToken = "Tone"; - +static uintptr_t _toneToken = 0x656e6f54; // 'T' 'o' 'n' 'e' void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index 79ccf4084..3d2aadb78 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -47,7 +47,7 @@ extern "C" { static uint8_t _lastAnalogWriteResolution; -static char const * _analogToken = "analog"; +static uintptr_t _analogToken = 0x676f6c41; // 'A' 'l' 'o' 'g' /** * This will apply to all PWM Hardware currently used by analogWrite(), diff --git a/libraries/Servo/src/nrf52/Servo.cpp b/libraries/Servo/src/nrf52/Servo.cpp index adbcf4553..0d4f6b167 100644 --- a/libraries/Servo/src/nrf52/Servo.cpp +++ b/libraries/Servo/src/nrf52/Servo.cpp @@ -21,7 +21,7 @@ #include #include -static char const * _servoToken = "Servo"; +static uintptr_t _servoToken = 0x76726553; // 'S' 'e' 'r' 'v' static servo_t servos[MAX_SERVOS]; // static array of servo structures uint8_t ServoCount = 0; // the total number of attached servos From 145167b579532f50035bde115517a9138d8b66f6 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Wed, 13 May 2020 10:35:05 -0700 Subject: [PATCH 15/38] tone() auto-release ownership --- cores/nRF5/HardwarePWM.cpp | 11 ++++- cores/nRF5/HardwarePWM.h | 4 +- cores/nRF5/Tone.cpp | 84 +++++++++++++++++++++++--------------- 3 files changed, 65 insertions(+), 34 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index bf68dcb77..0e2b88d4e 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -65,6 +65,7 @@ bool can_stringify_token(uintptr_t token) return true; } + void HardwarePWM::DebugOutput(Stream& logger) { const size_t count = arrcount(HwPWMx); @@ -79,7 +80,7 @@ void HardwarePWM::DebugOutput(Stream& logger) logger.printf(" \"%c%c%c%c\"", t[0], t[1], t[2], t[3] ); } else { static_assert(sizeof(uintptr_t) == 4); - logger.printf(" %08x ", token); + logger.printf(" %08x", token); } for (size_t j = 0; j < MAX_CHANNELS; j++) { uint32_t r = pwm->_pwm->PSEL.OUT[i]; // only read it once @@ -108,6 +109,14 @@ bool HardwarePWM::takeOwnership(uintptr_t token) // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool HardwarePWM::releaseOwnership(uintptr_t token) { + if (!releaseOwnershipFromISR(token)) { + LOG_LV1("HwPWM", "Failed to release ownership of PWM %p using token %x", _pwm, token); + return false; + } + return true; +} +bool HardwarePWM::releaseOwnershipFromISR(uintptr_t token) { + // Do not do any logging if called from ISR ... if (token == 0) return false; // cannot release ownership with nullptr if (!this->isOwner(token) ) return false; // don't even look at peripheral if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index fbf9a672a..e4c9ecf15 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -50,7 +50,7 @@ class HardwarePWM private: enum { MAX_CHANNELS = 4 }; // Max channel per group NRF_PWM_Type * const _pwm; - uintptr_t _owner_token = 0; + volatile uintptr_t _owner_token = 0; uint16_t _seq0[MAX_CHANNELS]; @@ -74,6 +74,8 @@ class HardwarePWM bool takeOwnership (uintptr_t token); // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool releaseOwnership(uintptr_t token); + // As above, but ensured safe to call from ISR + bool releaseOwnershipFromISR(uintptr_t token); // allows caller to verify that they own the peripheral __INLINE bool isOwner(uintptr_t token) const diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index ed70680ec..690267988 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -38,15 +38,23 @@ Version Modified By Date Comments #include "Tone.h" #include "WVariant.h" -unsigned long int count_duration=0; +volatile unsigned long int count_duration=0; volatile bool no_stop = false; uint8_t pin_sound=0; static uintptr_t _toneToken = 0x656e6f54; // 'T' 'o' 'n' 'e' +// NOTE: Currently hard-coded to only use PWM2 ... +// These are the relvant hard-coded variables +// (plus the ISR PWM2_IRQHandler) +static IRQn_Type const _IntNo = PWM2_IRQn; +static NRF_PWM_Type * const _PWMInstance = NRF_PWM2; +static HardwarePWM * const _HwPWM = HwPWMx[2]; + void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { static_assert(sizeof(unsigned long) == sizeof(uint32_t)); - + bool new_no_stop; + unsigned long int new_count_duration = (unsigned long int)-1L; unsigned int time_per=0; // limit frequency to reasonable audible range @@ -55,9 +63,15 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) return; } + // set nostop to true to avoid race condition. + // Specifically, race between a tone finishing + // after checking for ownership (which releases ownership) + // No effect if a tone is not playing.... + no_stop = true; + // Use fixed PWM2 (due to need to connect interrupt) - if (!HwPWMx[2]->isOwner(_toneToken) && - !HwPWMx[2]->takeOwnership(_toneToken)) { + if (!_HwPWM->isOwner(_toneToken) && + !_HwPWM->takeOwnership(_toneToken)) { LOG_LV1("TON", "unable to allocate PWM2 to Tone"); return; } @@ -66,15 +80,15 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) time_per=per/0.000008; unsigned int duty=time_per/2; if(duration > 0) { - no_stop = false; + new_no_stop = false; float mil=float(duration)/1000; if(per>mil) { - count_duration=1; + new_count_duration = 1; } else { - count_duration= mil/per; + new_count_duration = mil/per; } } else { - no_stop = true; + new_no_stop = true; } // Configure PWM @@ -88,42 +102,45 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) 0, 0 }; - - // Use fixed PWM2, TODO could conflict with other usage + uint32_t pins[NRF_PWM_CHANNEL_COUNT]={NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED}; pins[0] = g_ADigitalPinMap[pin]; - IRQn_Type IntNo = PWM2_IRQn; - NRF_PWM_Type * PWMInstance = NRF_PWM2; - - nrf_pwm_pins_set(PWMInstance, pins); - nrf_pwm_enable(PWMInstance); - nrf_pwm_configure(PWMInstance, NRF_PWM_CLK_125kHz, NRF_PWM_MODE_UP, time_per); - nrf_pwm_decoder_set(PWMInstance, NRF_PWM_LOAD_COMMON, NRF_PWM_STEP_AUTO); - nrf_pwm_sequence_set(PWMInstance, 0, &seq); - nrf_pwm_shorts_enable(PWMInstance, NRF_PWM_SHORT_SEQEND0_STOP_MASK); // shortcut for when SEQ0 ends, PWM output will automatically stop - // enable interrupt - nrf_pwm_event_clear(PWMInstance, NRF_PWM_EVENT_PWMPERIODEND); - nrf_pwm_int_enable(PWMInstance, NRF_PWM_INT_PWMPERIODEND_MASK); - NVIC_SetPriority(IntNo, 6); //low priority - NVIC_ClearPendingIRQ(IntNo); - NVIC_EnableIRQ(IntNo); - - nrf_pwm_task_trigger(PWMInstance, NRF_PWM_TASK_SEQSTART0); + count_duration = 0x6FFF; // large enough to avoid hitting zero in next few lines + no_stop = new_no_stop; + + nrf_pwm_pins_set(_PWMInstance, pins); + nrf_pwm_enable(_PWMInstance); + nrf_pwm_configure(_PWMInstance, NRF_PWM_CLK_125kHz, NRF_PWM_MODE_UP, time_per); + nrf_pwm_decoder_set(_PWMInstance, NRF_PWM_LOAD_COMMON, NRF_PWM_STEP_AUTO); + nrf_pwm_sequence_set(_PWMInstance, 0, &seq); + nrf_pwm_shorts_enable(_PWMInstance, NRF_PWM_SHORT_SEQEND0_STOP_MASK); // shortcut for when SEQ0 ends, PWM output will automatically stop + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_PWMPERIODEND); + nrf_pwm_int_enable(_PWMInstance, NRF_PWM_INT_PWMPERIODEND_MASK); + NVIC_SetPriority(_IntNo, 6); //low priority + NVIC_ClearPendingIRQ(_IntNo); + NVIC_EnableIRQ(_IntNo); + count_duration = new_count_duration; + nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_SEQSTART0); } void noTone(uint8_t pin) { - if (!HwPWMx[2]->isOwner(_toneToken)) { + if (!_HwPWM->isOwner(_toneToken)) { LOG_LV1("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); return; } - - NRF_PWM_Type * PWMInstance = NRF_PWM2; - nrf_pwm_task_trigger(PWMInstance, NRF_PWM_TASK_STOP); - nrf_pwm_disable(PWMInstance); + nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_STOP); + nrf_pwm_disable(_PWMInstance); + _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; + NVIC_DisableIRQ(_IntNo); + _HwPWM->releaseOwnership(_toneToken); + if (_HwPWM->isOwner(_toneToken)) { + LOG_LV1("TON", "stopped tone, but failed to release ownership of PWM peripheral?"); + return; + } } #ifdef __cplusplus @@ -137,6 +154,9 @@ void PWM2_IRQHandler(void){ if(count_duration == 0) { nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_STOP); nrf_pwm_disable(NRF_PWM2); + _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; + NVIC_DisableIRQ(PWM2_IRQn); + _HwPWM->releaseOwnershipFromISR(_toneToken); } else { nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); } From bbc453156eac9e010127f68562ce75824de7e650 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Wed, 13 May 2020 17:22:47 -0700 Subject: [PATCH 16/38] Improve logging for end users --- cores/nRF5/HardwarePWM.cpp | 54 ++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 0e2b88d4e..84a43c05c 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -65,7 +65,6 @@ bool can_stringify_token(uintptr_t token) return true; } - void HardwarePWM::DebugOutput(Stream& logger) { const size_t count = arrcount(HwPWMx); @@ -98,10 +97,28 @@ void HardwarePWM::DebugOutput(Stream& logger) // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr bool HardwarePWM::takeOwnership(uintptr_t token) { - if (token == 0) return false; // cannot take ownership with nullptr - if (this->_owner_token != 0) return false; // doesn't matter if it's actually a match ... it's not legal to take ownership twice - if (this->usedChannelCount() != 0) return false; // at least one channel is already in use - if (this->enabled() ) return false; // if it's enabled, do not allow new ownership, even with no pins in use + if (token == 0) { + LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in takeOwnership)"); + return false; // cannot take ownership with nullptr + } + if (token == this->_owner_token) { + LOG_LV1("HwPWM", "failing to acquire ownership because already owned by requesting token (cannot take ownership twice)"); + } + if (this->_owner_token != 0) { + LOG_LV3("HwPWM", "failing to acquire ownership because already owned by other token"); + return false; + } + if (this->usedChannelCount() != 0) { + LOG_LV3("HwPWM", "failing to acquire ownership because at least one channel connected"); + return false; + } + if (this->enabled()) { + LOG_LV3("HwPWM", "failing to acquire ownership because peripheral is already enabled"); + return false; + } + // TODO: warn, but do not fail, if taking ownership with IRQs already enabled + // NVIC_GetActive + // use gcc built-in intrinsic to ensure atomicity // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); @@ -109,11 +126,32 @@ bool HardwarePWM::takeOwnership(uintptr_t token) // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool HardwarePWM::releaseOwnership(uintptr_t token) { - if (!releaseOwnershipFromISR(token)) { - LOG_LV1("HwPWM", "Failed to release ownership of PWM %p using token %x", _pwm, token); + if (token == 0) { + LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in releaseOwnership)"); return false; } - return true; + if (!this->isOwner(token)) { + LOG_LV1("HwPWM", "attempt to release ownership when not the current owner"); + return false; + } + if (this->usedChannelCount() != 0) { + LOG_LV1("HwPWM", "attempt to release ownership when at least on channel is still connected"); + return false; + } + if (this->enabled()) { + LOG_LV1("HwPWM", "attempt to release ownership when PWM peripheral is still enabled"); + return false; // if it's enabled, do not allow ownership to be released, even with no pins in use + } + // TODO: warn, but do not fail, if releasing ownership with IRQs enabled + // NVIC_GetActive + + // use gcc built-in intrinsic to ensure atomicity + // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html + bool result = __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); + if (!result) { + LOG_LV1("HwPWM", "race condition resulted in failure to acquire ownership"); + } + return result; } bool HardwarePWM::releaseOwnershipFromISR(uintptr_t token) { // Do not do any logging if called from ISR ... From aebdca3f639f6978f423e890da9d5169e718cae0 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Wed, 13 May 2020 22:41:16 -0700 Subject: [PATCH 17/38] Ensure default value for `analogWriteResolution()` --- cores/nRF5/wiring_analog.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index 3d2aadb78..b2e88115f 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -46,7 +46,7 @@ extern "C" { -static uint8_t _lastAnalogWriteResolution; +static uint8_t _lastAnalogWriteResolution = 8; // default is 256 levels static uintptr_t _analogToken = 0x676f6c41; // 'A' 'l' 'o' 'g' /** From dd06e2f05a9332ef9b799511e0c395702cd5e769 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Thu, 14 May 2020 00:58:59 -0700 Subject: [PATCH 18/38] Fix HwPWM debug logging typo --- cores/nRF5/HardwarePWM.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 84a43c05c..fbc2ec736 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -82,7 +82,7 @@ void HardwarePWM::DebugOutput(Stream& logger) logger.printf(" %08x", token); } for (size_t j = 0; j < MAX_CHANNELS; j++) { - uint32_t r = pwm->_pwm->PSEL.OUT[i]; // only read it once + uint32_t r = pwm->_pwm->PSEL.OUT[j]; // only read it once if ( (r & PWM_PSEL_OUT_CONNECT_Msk) != (PWM_PSEL_OUT_CONNECT_Disconnected << PWM_PSEL_OUT_CONNECT_Pos) ) { logger.printf(" %02x", r & 0x1F); } else { From be9927e497830f2e419839b9454bcf6971b565c0 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 17:34:20 -0700 Subject: [PATCH 19/38] Simplify by auto-detecting when called from ISR Thanks to @hathach for recommendation: https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/496#discussion_r452712103 --- cores/nRF5/HardwarePWM.cpp | 48 +++++++++++++++++++++++--------------- cores/nRF5/HardwarePWM.h | 2 -- cores/nRF5/Tone.cpp | 2 +- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index fbc2ec736..306220449 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -97,23 +97,34 @@ void HardwarePWM::DebugOutput(Stream& logger) // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr bool HardwarePWM::takeOwnership(uintptr_t token) { + bool notInIsr = !isInISR(); if (token == 0) { - LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in takeOwnership)"); + if (notInIsr) { + LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in takeOwnership)"); + } return false; // cannot take ownership with nullptr } if (token == this->_owner_token) { - LOG_LV1("HwPWM", "failing to acquire ownership because already owned by requesting token (cannot take ownership twice)"); + if (notInIsr) { + LOG_LV1("HwPWM", "failing to acquire ownership because already owned by requesting token (cannot take ownership twice)"); + } } if (this->_owner_token != 0) { - LOG_LV3("HwPWM", "failing to acquire ownership because already owned by other token"); + if (notInIsr) { + LOG_LV3("HwPWM", "failing to acquire ownership because already owned by other token"); + } return false; } if (this->usedChannelCount() != 0) { - LOG_LV3("HwPWM", "failing to acquire ownership because at least one channel connected"); + if (notInIsr) { + LOG_LV3("HwPWM", "failing to acquire ownership because at least one channel connected"); + } return false; } if (this->enabled()) { - LOG_LV3("HwPWM", "failing to acquire ownership because peripheral is already enabled"); + if (notInIsr) { + LOG_LV3("HwPWM", "failing to acquire ownership because peripheral is already enabled"); + } return false; } // TODO: warn, but do not fail, if taking ownership with IRQs already enabled @@ -126,20 +137,29 @@ bool HardwarePWM::takeOwnership(uintptr_t token) // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool HardwarePWM::releaseOwnership(uintptr_t token) { + bool notInIsr = !isInISR(); if (token == 0) { - LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in releaseOwnership)"); + if (notInIsr) { + LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in releaseOwnership)"); + } return false; } if (!this->isOwner(token)) { - LOG_LV1("HwPWM", "attempt to release ownership when not the current owner"); + if (notInIsr) { + LOG_LV1("HwPWM", "attempt to release ownership when not the current owner"); + } return false; } if (this->usedChannelCount() != 0) { - LOG_LV1("HwPWM", "attempt to release ownership when at least on channel is still connected"); + if (notInIsr) { + LOG_LV1("HwPWM", "attempt to release ownership when at least on channel is still connected"); + } return false; } if (this->enabled()) { - LOG_LV1("HwPWM", "attempt to release ownership when PWM peripheral is still enabled"); + if (notInIsr) { + LOG_LV1("HwPWM", "attempt to release ownership when PWM peripheral is still enabled"); + } return false; // if it's enabled, do not allow ownership to be released, even with no pins in use } // TODO: warn, but do not fail, if releasing ownership with IRQs enabled @@ -153,16 +173,6 @@ bool HardwarePWM::releaseOwnership(uintptr_t token) } return result; } -bool HardwarePWM::releaseOwnershipFromISR(uintptr_t token) { - // Do not do any logging if called from ISR ... - if (token == 0) return false; // cannot release ownership with nullptr - if (!this->isOwner(token) ) return false; // don't even look at peripheral - if ( this->usedChannelCount() != 0) return false; // fail if any channels still have pins - if ( this->enabled() ) return false; // if it's enabled, do not allow ownership to be released, even with no pins in use - // use gcc built-in intrinsic to ensure atomicity - // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html - return __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); -} HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) : _pwm(pwm) diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index e4c9ecf15..d3bda2337 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -74,8 +74,6 @@ class HardwarePWM bool takeOwnership (uintptr_t token); // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool releaseOwnership(uintptr_t token); - // As above, but ensured safe to call from ISR - bool releaseOwnershipFromISR(uintptr_t token); // allows caller to verify that they own the peripheral __INLINE bool isOwner(uintptr_t token) const diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 690267988..15fb03b05 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -156,7 +156,7 @@ void PWM2_IRQHandler(void){ nrf_pwm_disable(NRF_PWM2); _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; NVIC_DisableIRQ(PWM2_IRQn); - _HwPWM->releaseOwnershipFromISR(_toneToken); + _HwPWM->releaseOwnership(_toneToken); } else { nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); } From 37935c7938288dd97f6dd781be0a9d258ddd8740 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 19:31:58 -0700 Subject: [PATCH 20/38] Remove always_inline attribute Per @hathach request https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/496#discussion_r452713305 --- cores/nRF5/HardwarePWM.h | 1 - 1 file changed, 1 deletion(-) diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index d3bda2337..25e60e93a 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -77,7 +77,6 @@ class HardwarePWM // allows caller to verify that they own the peripheral __INLINE bool isOwner(uintptr_t token) const - __attribute__ ((__always_inline__)) { return this->_owner_token == token; } From e58b39e2ded01799764d5c1ad86dc43577b4db10 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 19:53:48 -0700 Subject: [PATCH 21/38] Reduce debug output for analog_write() Remove all output when the pin is already configured and everything works. --- cores/nRF5/wiring_analog.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index b2e88115f..22ece4240 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -81,16 +81,13 @@ void analogWrite( uint32_t pin, uint32_t value ) for(int i=0; iisOwner(_analogToken)) { - LOG_LV3("ANA", "not currently owner of PWM %d", i); continue; // skip if not owner of this PWM instance } int const ch = HwPWMx[i]->pin2channel(pin); if (ch < 0) { - LOG_LV3("ANA", "pin %d is not used by PWM %d", pin, i); continue; // pin not in use by this PWM instance } - LOG_LV2("ANA", "updating pin %" PRIu32 " used by PWM %d", pin, i); HwPWMx[i]->writeChannel(ch, value); return; } From eb55315d4fad7775f614c3e0d2610e526462ca2c Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 20:49:14 -0700 Subject: [PATCH 22/38] Explicit `void` for no parameters Per @hathach comments https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/496/files/d7e005cc99d4db22a3e6940279e5a59b995f247e#r452772810 --- cores/nRF5/HardwarePWM.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index 25e60e93a..46bcd3349 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -112,8 +112,8 @@ class HardwarePWM uint16_t readChannel (uint8_t ch) const; // Get count of used / free channels - uint8_t usedChannelCount() const; - uint8_t freeChannelCount() const; + uint8_t usedChannelCount(void) const; + uint8_t freeChannelCount(void) const; #if CFG_DEBUG static void DebugOutput(Stream& logger); From a2a43719c583f7fb2d6dfa38ad10274e6537a388 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 20:53:59 -0700 Subject: [PATCH 23/38] Remove static assertion Per @hathach request https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/496/files/d7e005cc99d4db22a3e6940279e5a59b995f247e?file-filters%5B%5D=.c&file-filters%5B%5D=.cpp#r452765910 --- cores/nRF5/Tone.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 15fb03b05..57af183e8 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -52,7 +52,6 @@ static HardwarePWM * const _HwPWM = HwPWMx[2]; void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { - static_assert(sizeof(unsigned long) == sizeof(uint32_t)); bool new_no_stop; unsigned long int new_count_duration = (unsigned long int)-1L; unsigned int time_per=0; From 43da1739a37346fc4f9b8395345f987542ade2f2 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 21:23:14 -0700 Subject: [PATCH 24/38] Update noTone() to be safe to call from ISR --- cores/nRF5/Tone.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 57af183e8..9cdf312b4 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -127,8 +127,12 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) void noTone(uint8_t pin) { + bool notInIsr = !isInISR(); + if (!_HwPWM->isOwner(_toneToken)) { - LOG_LV1("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); + if (notInIsr) { + LOG_LV1("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); + } return; } nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_STOP); @@ -137,7 +141,9 @@ void noTone(uint8_t pin) NVIC_DisableIRQ(_IntNo); _HwPWM->releaseOwnership(_toneToken); if (_HwPWM->isOwner(_toneToken)) { - LOG_LV1("TON", "stopped tone, but failed to release ownership of PWM peripheral?"); + if (notInIsr) { + LOG_LV1("TON", "stopped tone, but failed to release ownership of PWM peripheral?"); + } return; } } @@ -151,11 +157,8 @@ void PWM2_IRQHandler(void){ if(!no_stop){ count_duration--; if(count_duration == 0) { - nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_STOP); - nrf_pwm_disable(NRF_PWM2); - _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; - NVIC_DisableIRQ(PWM2_IRQn); - _HwPWM->releaseOwnership(_toneToken); + uint8_t pin = _PWMInstance->PSEL.OUT[0]; + noTone(pin); } else { nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); } From d00de2d783d9b2ccc0cadc7e37381e3ee2c5c857 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 21:27:42 -0700 Subject: [PATCH 25/38] Change variable name per @hathach request. --- cores/nRF5/wiring_analog.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index 22ece4240..5267a64cf 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -46,7 +46,7 @@ extern "C" { -static uint8_t _lastAnalogWriteResolution = 8; // default is 256 levels +static uint8_t _analogResolution = 8; // default is 256 levels static uintptr_t _analogToken = 0x676f6c41; // 'A' 'l' 'o' 'g' /** @@ -56,7 +56,7 @@ static uintptr_t _analogToken = 0x676f6c41; // 'A' 'l' 'o' 'g' void analogWriteResolution( uint8_t res ) { // save the resolution for when adding a new instance - _lastAnalogWriteResolution = res; + _analogResolution = res; for (int i = 0; iisOwner(_analogToken)) continue; @@ -121,7 +121,7 @@ void analogWrite( uint32_t pin, uint32_t value ) } // apply the cached analog resolution to newly owned instances - HwPWMx[i]->setResolution(_lastAnalogWriteResolution); + HwPWMx[i]->setResolution(_analogResolution); HwPWMx[i]->addPin(pin); HwPWMx[i]->writePin(pin, value); LOG_LV2("ANA", "took ownership of, and added pin %" PRIu32 " to, PWM %d", pin, i); From d7b18ed40f4104465d413da7ad42a5b1a39a95b7 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jul 2020 21:41:27 -0700 Subject: [PATCH 26/38] Code review required comment removal --- libraries/Servo/src/nrf52/Servo.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/Servo/src/nrf52/Servo.cpp b/libraries/Servo/src/nrf52/Servo.cpp index 0d4f6b167..029955502 100644 --- a/libraries/Servo/src/nrf52/Servo.cpp +++ b/libraries/Servo/src/nrf52/Servo.cpp @@ -129,7 +129,7 @@ void Servo::write(int value) void Servo::writeMicroseconds(int value) { - if (this->pwm) { // pwm is only set when this->servoIndex != INVALID_SERVO + if (this->pwm) { uint8_t pin = servos[this->servoIndex].Pin.nbr; this->pwm->writePin(pin, value/DUTY_CYCLE_RESOLUTION); } @@ -142,7 +142,7 @@ int Servo::read() // return the value as degrees int Servo::readMicroseconds() { - if (this->pwm) { // pwm is only set when this->servoIndex != INVALID_SERVO + if (this->pwm) { uint8_t pin = servos[this->servoIndex].Pin.nbr; return this->pwm->readPin(pin)*DUTY_CYCLE_RESOLUTION; } From 29843dacb0c34c9ee151f1f05046d591544452a0 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 3 Aug 2020 01:40:29 -0700 Subject: [PATCH 27/38] Redefine tone() without ISR Other minor changes include: * Enable RTT option for output (requires platform.local.txt or boards.local.txt) * Allow setting compiler optimization (requires platform.local.txt or boards.local.txt) --- cores/nRF5/HardwarePWM.h | 2 + cores/nRF5/Tone.cpp | 526 +++++++++++++++++++++++++++++---------- cores/nRF5/Tone.h | 39 ++- cores/nRF5/main.cpp | 9 +- platform.txt | 7 +- 5 files changed, 452 insertions(+), 131 deletions(-) diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index 46bcd3349..1afa0c37d 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -117,6 +117,8 @@ class HardwarePWM #if CFG_DEBUG static void DebugOutput(Stream& logger); +#else + static void inline DebugOutput(Stream& logger) { (void)logger; }; #endif // CFG_DEBUG }; diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 9cdf312b4..4aff67413 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -1,22 +1,22 @@ /* Tone.cpp - A Tone Generator Library +A Tone Generator Library - Written by Brett Hagman +Written by Brett Hagman - This library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License as published by the Free Software Foundation; either +version 2.1 of the License, or (at your option) any later version. - This library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. - You should have received a copy of the GNU Lesser General Public - License along with this library; if not, write to the Free Software - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Version Modified By Date Comments ------- ----------- -------- -------- @@ -37,136 +37,408 @@ Version Modified By Date Comments #include "Arduino.h" #include "Tone.h" #include "WVariant.h" - -volatile unsigned long int count_duration=0; -volatile bool no_stop = false; -uint8_t pin_sound=0; -static uintptr_t _toneToken = 0x656e6f54; // 'T' 'o' 'n' 'e' +#include "limits.h" // NOTE: Currently hard-coded to only use PWM2 ... // These are the relvant hard-coded variables -// (plus the ISR PWM2_IRQHandler) -static IRQn_Type const _IntNo = PWM2_IRQn; +// TODO: Consider allowing dynamic use of any available PWM peripheral (but start with #2 for compatibility) static NRF_PWM_Type * const _PWMInstance = NRF_PWM2; static HardwarePWM * const _HwPWM = HwPWMx[2]; +// Defined a struct, to simplify validation testing ... also provides context when debugging +class TONE_PWM_CONFIG { + private: + const nrf_pwm_clk_t clock_frequency = NRF_PWM_CLK_125kHz; + const nrf_pwm_mode_t pwm_mode = NRF_PWM_MODE_UP; + const nrf_pwm_dec_step_t step_mode = NRF_PWM_STEP_AUTO; + const nrf_pwm_dec_load_t load_mode = NRF_PWM_LOAD_COMMON; + const uintptr_t toneToken = 0x656e6f54; //< 'T' 'o' 'n' 'e' + private: + uint64_t pulse_count; //< total number of PWM pulses + uint32_t seq0_refresh; //< count of pulses for each SEQ0 iteration + uint32_t seq1_refresh; //< count of pulses for each SEQ1 iteration + uint16_t loop_count; //< how many times to restart SEQ0/SEQ1? + uint16_t time_period; //< how many clock cycles allocated to each PWM pulse? + uint16_t duty_with_polarity; //< SEQ[N].PTR will point here, length == 1 + uint8_t arduino_pin; //< the arduino pin for playback + uint8_t nrf_pin; //< the nrf pin for playback + nrf_pwm_task_t task_to_start; //< Whether to start playback at SEQ0 or SEQ1 + nrf_pwm_short_mask_t shorts; //< shortcuts to enable + boolean is_initialized; //< defaults to uninitialized + + public: + bool EnsurePwmPeripheralOwnership(void) noexcept; + bool InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count, uint16_t time_period) noexcept; + bool ApplyConfiguration(uint32_t pin) noexcept; + bool StartPlayback(void) noexcept; + bool StopPlayback(bool releaseOwnership = false) noexcept; + bool ApplyConfigurationAndStartPlayback(uint32_t pin) noexcept; + uint64_t CalculateExpectedPulseCount(void) noexcept; +}; +TONE_PWM_CONFIG _pwm_config; + +inline static bool _is_pwm_enabled(NRF_PWM_Type const * pwm_instance) { + bool isEnabled = + (pwm_instance->ENABLE & PWM_ENABLE_ENABLE_Msk) == + (PWM_ENABLE_ENABLE_Enabled << PWM_ENABLE_ENABLE_Pos); + return isEnabled; +} + +/* These two functions were verified as equivalent to + the prior calculations (except where the old floating-point + based code resulted in rounding errors ... and thus there + are some one-off differences ... but the integer-based + functions are more accurate). + + These functions entirely avoid floating point math, and all + the nasty edge cases they can create... (NaN, INF, etc.) + + See https://gist.github.com/henrygab/6b570ebd51354bf247633c72b8dc383b + for code that compares the new lambdas to the old calculations. +*/ +constexpr inline static uint16_t _calculate_time_period(uint32_t frequency) throw() { + // range for frequency == [20..25000], + // so range of result == [ 5..62500] + // which fits in 16 bits. + return 125000 / frequency; +}; +constexpr inline static uint64_t _calculate_pulse_count(uint32_t frequency, uint32_t duration) throw() { + // range for frequency == [20..25000], + // range for duration == [ 1..0xFFFF_FFFF] + // so range of result == [ 1..0x18_FFFF_FFE7] (requires 37 bits) + static_assert(sizeof(unsigned long long) >= sizeof(uint64_t)); + return + (duration == 0) ? + 0 : + ((duration < 1000ULL) && (duration * frequency < 1000ULL)) ? + 1ULL : + (UINT64_MAX / frequency < duration) ? + (duration / 1000ULL) * frequency : + (((uint64_t)duration) * frequency / 1000ULL); +}; +inline static int _bits_used(unsigned long x) noexcept { + if (0 == x) return 0; + unsigned int result = 0; + do { + result++; + } while (x >>= 1); + return result; +} +inline static int _bits_used(unsigned long long x) noexcept { + if (0 == x) return 0; + unsigned int result = 0; + do { + result++; + } while (x >>= 1); + return result; +} + +/* +* In older versions of the BSP, tone() would cause an interrupt for every cycle, tracking whether +* the playback should be infinite or duration-based via two global, volatile variables, "nostop" +* and "count_duration". The interrupt would then trigger the STARTSEQ0 task, or stop playback +* by calling noTone(). +* +* What is available to configure looping with minimal memory usage: +* 1. SEQ[n].REFRESH === how many PWM period before loading next sample from sequence +* Thus, setting to 99 will cause 100 pulses per item +* Treat as a 23-bit value. +* 2. LOOP === how many times to loop back to SEQ[0] +* SEQ[0] will play the same count if started PWM at SEQ[0] +* SEQ[0] will play one fewer times if started PWM at SEQ[1] +* Treat as a 15-bit value. +* +* Therefore, between REFRESH and LOOP, can support up to 40-bit repeat WITHOUT INTERRUPTS. +* +* The value of duration is given in milliseconds. +* Frequency is limited to range [20 ... 25000]. +* +* Therefore, maximum pulse count is limited as follows: +* (32-bit duration) * (20 ... 25000) / 1000UL +* (0xFFFF_FFFF) * 25 +* 0x18_FFFF_FFE7 ... which is 37 bits +* +* Therefore, all possible values for tone() can be supported +* via a single one-time configuration of the PWM peripheral. +* +* PWM peripheral can be de-allocated by sketch call to noTone(). +* +* Design: +* 0. At each call to tone(), unconditionally stop any existing playback. +* 1. For infinite duration, configure large REFRESH and LOOP (to minimize reading of RAM / AHB traffic), +* and setup shortcut to repeat infinitely. +* 2. For specified duration, configure both SEQ0 and SEQ1: +* 1a. SEQ[1].REFRESH <-- total count % _iterations_per_loop +* 1b. SEQ[0].REFRESH <-- _iterations_per_loop - SEQ[1].CNT +* 1c. LOOP <-- duration_count / _iterations_per_loop +* +* Result: Zero CPU usage, minimal AHB traffic +*/ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { - bool new_no_stop; - unsigned long int new_count_duration = (unsigned long int)-1L; - unsigned int time_per=0; - - // limit frequency to reasonable audible range - if((frequency < 20) | (frequency > 25000)) { - LOG_LV1("TON", "frequency outside range [20..25000] -- ignoring"); - return; - } - - // set nostop to true to avoid race condition. - // Specifically, race between a tone finishing - // after checking for ownership (which releases ownership) - // No effect if a tone is not playing.... - no_stop = true; - - // Use fixed PWM2 (due to need to connect interrupt) - if (!_HwPWM->isOwner(_toneToken) && - !_HwPWM->takeOwnership(_toneToken)) { - LOG_LV1("TON", "unable to allocate PWM2 to Tone"); - return; - } - - float per=float(1)/frequency; - time_per=per/0.000008; - unsigned int duty=time_per/2; - if(duration > 0) { - new_no_stop = false; - float mil=float(duration)/1000; - if(per>mil) { - new_count_duration = 1; - } else { - new_count_duration = mil/per; - } - } else { - new_no_stop = true; - } - - // Configure PWM - static uint16_t seq_values[]={0}; - //In each value, the most significant bit (15) determines the polarity of the output - //0x8000 is MSB = 1 - seq_values[0]= duty | 0x8000; - nrf_pwm_sequence_t const seq={ - seq_values, - NRF_PWM_VALUES_LENGTH(seq_values), - 0, - 0 + static_assert(sizeof(unsigned long int) <= sizeof(uint32_t)); + static_assert(sizeof(unsigned int) <= sizeof(uint32_t)); + + // Used only to protect calls against simultaneous multiple calls to tone(). + // Using a function-local static to avoid accidental reference from ISR or elsewhere, + // and to simplify ensuring the semaphore gets initialized. + static StaticSemaphore_t _tone_semaphore_allocation; + static auto init_semaphore = [] () throw() { //< use a lambda to both initialize AND give the mutex + SemaphoreHandle_t handle = xSemaphoreCreateBinaryStatic(&_tone_semaphore_allocation); + auto mustSucceed = xSemaphoreGive(handle); + (void)mustSucceed; + NRFX_ASSERT(mustSucceed == pdTRUE); + return handle; }; + static SemaphoreHandle_t _tone_semaphore = init_semaphore(); - uint32_t pins[NRF_PWM_CHANNEL_COUNT]={NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED, NRF_PWM_PIN_NOT_CONNECTED}; - pins[0] = g_ADigitalPinMap[pin]; - - // enable interrupt - count_duration = 0x6FFF; // large enough to avoid hitting zero in next few lines - no_stop = new_no_stop; - - nrf_pwm_pins_set(_PWMInstance, pins); - nrf_pwm_enable(_PWMInstance); - nrf_pwm_configure(_PWMInstance, NRF_PWM_CLK_125kHz, NRF_PWM_MODE_UP, time_per); - nrf_pwm_decoder_set(_PWMInstance, NRF_PWM_LOAD_COMMON, NRF_PWM_STEP_AUTO); - nrf_pwm_sequence_set(_PWMInstance, 0, &seq); - nrf_pwm_shorts_enable(_PWMInstance, NRF_PWM_SHORT_SEQEND0_STOP_MASK); // shortcut for when SEQ0 ends, PWM output will automatically stop - nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_PWMPERIODEND); - nrf_pwm_int_enable(_PWMInstance, NRF_PWM_INT_PWMPERIODEND_MASK); - NVIC_SetPriority(_IntNo, 6); //low priority - NVIC_ClearPendingIRQ(_IntNo); - NVIC_EnableIRQ(_IntNo); - count_duration = new_count_duration; - nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_SEQSTART0); -} + // limit frequency to reasonable audible range + if((frequency < 20) | (frequency > 25000)) { + LOG_LV1("TON", "frequency outside range [20..25000] -- ignoring"); + return; + } + if(xSemaphoreTake(_tone_semaphore, portMAX_DELAY) != pdTRUE) { + LOG_LV1("TON", "error acquiring semaphore (should never occur?)"); + return; + } + uint64_t pulse_count = _calculate_pulse_count(frequency, duration); + uint16_t time_period = _calculate_time_period(frequency); + if (!_pwm_config.EnsurePwmPeripheralOwnership()) { + LOG_LV1("TON", "Unable to acquire PWM peripheral"); + } else if (!_pwm_config.StopPlayback()) { + LOG_LV1("TON", "Unable to stop playback"); + } else if (!_pwm_config.InitializeFromPulseCountAndTimePeriod(pulse_count, time_period)) { + LOG_LV1("TON", "Failed calculating configuration"); + } else if (!_pwm_config.ApplyConfiguration(pin)) { + LOG_LV1("TON", "Failed applying configuration"); + } else if (!_pwm_config.StartPlayback()) { + LOG_LV1("TON", "Failed attempting to start PWM peripheral"); + } else { + //LOG_LV2("TON", "Started playback of tone at frequency %d duration %ld", frequency, duration); + } + xSemaphoreGive(_tone_semaphore); + return; +} void noTone(uint8_t pin) { - bool notInIsr = !isInISR(); - - if (!_HwPWM->isOwner(_toneToken)) { - if (notInIsr) { - LOG_LV1("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); - } - return; - } - nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_STOP); - nrf_pwm_disable(_PWMInstance); - _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; - NVIC_DisableIRQ(_IntNo); - _HwPWM->releaseOwnership(_toneToken); - if (_HwPWM->isOwner(_toneToken)) { - if (notInIsr) { - LOG_LV1("TON", "stopped tone, but failed to release ownership of PWM peripheral?"); - } - return; - } + ( void )pin; // avoid unreferenced parameter compiler warning + _pwm_config.StopPlayback(true); // release ownership of PWM peripheral } -#ifdef __cplusplus -extern "C"{ -#endif - -void PWM2_IRQHandler(void){ - nrf_pwm_event_clear(NRF_PWM2, NRF_PWM_EVENT_PWMPERIODEND); - if(!no_stop){ - count_duration--; - if(count_duration == 0) { - uint8_t pin = _PWMInstance->PSEL.OUT[0]; - noTone(pin); - } else { - nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); - } - } else { - nrf_pwm_task_trigger(NRF_PWM2, NRF_PWM_TASK_SEQSTART0); - } +bool TONE_PWM_CONFIG::EnsurePwmPeripheralOwnership(void) { + if (!_HwPWM->isOwner(TONE_PWM_CONFIG::toneToken) && !_HwPWM->takeOwnership(TONE_PWM_CONFIG::toneToken)) { + LOG_LV1("TON", "unable to allocate PWM2 to Tone"); + return false; + } + return true; } -#ifdef __cplusplus +// +// The final loop's SEQ1 will ALWAYS output one pulse ... +// In other words, SEQ1's refresh is *IGNORED* for the final loop. +// +// Visually, with each sequence length set to one as is done with tone(): +// ====================================================================== +// +// Starting at SEQ0, loopCnt = 2, SEQ0 refresh == 4, SEQ1 refresh = 2: +// +// [----SEQ0-----] [SEQ1-] [----SEQ0-----] [1] +// 0 0 0 0 1 1 0 0 0 0 1 +// +// ====================================================================== +// +// Starting as SEQ1, loopCnt = 2, SEQ0 refresh == 4, SEQ1 refresh = 2: +// +// [SEQ1-] [----SEQ0-----] [1] +// 1 1 0 0 0 0 1 +// +// ====================================================================== +// +// Therefore, the total count of pulses that will be emitted by the +// PWM peripheral (per the configuration of tone() API): +// +// COUNT = (SEQ0.CNT * (SEQ0.REFRESH+1); +// COUNT += (SEQ1.CNT * (SEQ1.REFRESH+1); +// COUNT *= (loopCount-1); // the number of times SEQ0 and SEQ1 both run +// COUNT += (start at SEQ0) ? (SEQ0.CNT * (SEQ0.REFRESH+1) : 0; +// COUNT += 1; // final SEQ1 emits single pulse +// +bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) noexcept { + + if (_bits_used(pulse_count_x) > 37) { + LOG_LV1("TON", "pulse count is limited to 37 bit long value"); + return false; + } + + this->pulse_count = pulse_count_x; + this->time_period = time_period; + this->duty_with_polarity = 0x8000U | (time_period / 2U); + + if (this->pulse_count == 0) { + LOG_LV3("TON", "Infinite duration requested\n"); + + this->seq0_refresh = 0xFFFFFFU; // 24-bit maximum value + this->seq1_refresh = 0xFFFFFFU; // 24-bit maximum value + this->loop_count = 0xFFFFU; // 16-bit maximum value + this->task_to_start = NRF_PWM_TASK_SEQSTART0; + this->shorts = NRF_PWM_SHORT_LOOPSDONE_SEQSTART0_MASK; + } + else if (this->pulse_count == 1) { + LOG_LV3("TON", "Edge case: exactly one pulse\n"); + // yes, this is an edge case + this->seq0_refresh = 0; + this->seq1_refresh = 0; + this->loop_count = 1; + this->task_to_start = NRF_PWM_TASK_SEQSTART1; + this->shorts = NRF_PWM_SHORT_LOOPSDONE_STOP_MASK; + } + else { + LOG_LV3("TON", "Non-infinite duration of at least two pulses requested\n"); + + // This is the interesting section. + // + // To ensure refresh value stays within 24 bits, the maximum number of bits + // for the pulse_count is ((24 * 3) / 2) + 1 == (72/2) + 1 == 36 + 1 == 37. + // + // Validation: + // 37 * 2 / 3 == 74 / 3 == 24 (OK) -- leaves 13 bits for loop count + // 38 * 2 / 3 == 76 / 3 == 25 (fail) -- leaves 13 bits for loop count + // + // NOTE: theoretically possible to support up to 40 bit pulse count, but + // would require more complex logic. + // + unsigned int bits_needed = _bits_used(this->pulse_count); // bits_used is now in range [2..37] + + // split the number of bits between refresh and loop_count in 2:1 ratio + // so that, no matter what inputs are given, guaranteed to have interrupt-free solution + unsigned int bits_for_refresh = bits_needed * 2 / 3; // range is [1 .. 24] + unsigned int bits_for_loop_count = bits_needed - bits_for_refresh; // range is [1 .. 13] + + // NOTE: Due to final SEQ1 outputting exactly one pulse, may need one additional bit for loop count + // ... but that will still be within the 16 bits available, because top of range is 13 bits. + LOG_LV3("TON", "Using %d bits for refresh, and %d bits for loop_count\n", bits_for_refresh, bits_for_loop_count); + + // now determine how many PWM pulses should occur per loop (when both SEQ0 and SEQ1 are played) + uint32_t total_refresh_count = 1 << bits_for_refresh; // range is [2 .. 2^24] + uint32_t full_loops = (this->pulse_count - 1) / total_refresh_count; // == loopCount - 1 + + LOG_LV3("TON", "total_refresh_count is 0x%" PRIx32 " (%" PRIu32 ")\n", total_refresh_count, total_refresh_count); + LOG_LV3("TON", "full_loops is 0x%" PRIx32 " (%" PRIu32 ")\n", full_loops, full_loops); + + // if (pulses - 1) % total_refresh_count == 0, then start at SEQ1 and split refresh evenly + // else, start at SEQ0, and set SEQ0 to extra pulses needed... + uint32_t extraPulsesNeededIfStartingAtSequence1 = (this->pulse_count - 1) % total_refresh_count; + uint32_t seq0_count; + + if (extraPulsesNeededIfStartingAtSequence1 == 0) { + LOG_LV3("TON", "Pulse count (minus one) is exact multiple of total_refresh_count -- starting at SEQ1\n"); + seq0_count = total_refresh_count / 2; // range is [1 .. 2^23] + this->task_to_start = NRF_PWM_TASK_SEQSTART1; + } + else { + LOG_LV3("TON", "Pulse count (minus one) requires extra %" PRIu32 " pulses ... setting SEQ0 to that value\n", extraPulsesNeededIfStartingAtSequence1); + seq0_count = extraPulsesNeededIfStartingAtSequence1; + this->task_to_start = NRF_PWM_TASK_SEQSTART0; + } + this->loop_count = full_loops + 1; + this->seq0_refresh = seq0_count - 1; + this->seq1_refresh = (total_refresh_count - seq0_count) - 1; + + LOG_LV3("TON", "seq0_count is %" PRIu32 ", so refresh will be set to %" PRIu32 "\n", seq0_count, this->seq0_refresh); + LOG_LV3("TON", "seq1_count is %" PRIu32 ", so refresh will be set to %" PRIu32 "\n", (total_refresh_count - seq0_count), this->seq1_refresh); + + this->shorts = NRF_PWM_SHORT_LOOPSDONE_STOP_MASK; + } + this->is_initialized = true; + return true; +} +bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) noexcept { + if (!this->is_initialized) { + return false; + } + if (pin >= PINS_COUNT) { + return false; + } + if (!this->EnsurePwmPeripheralOwnership()) { + return false; + } + this->StopPlayback(false); + + this->arduino_pin = pin; + this->nrf_pin = g_ADigitalPinMap[pin]; + + uint32_t pins[NRF_PWM_CHANNEL_COUNT] = { + this->nrf_pin, + NRF_PWM_PIN_NOT_CONNECTED, + NRF_PWM_PIN_NOT_CONNECTED, + NRF_PWM_PIN_NOT_CONNECTED + }; + + nrf_pwm_pins_set(_PWMInstance, pins); // must set pins before enabling + nrf_pwm_enable(_PWMInstance); + nrf_pwm_configure(_PWMInstance, TONE_PWM_CONFIG::clock_frequency, TONE_PWM_CONFIG::pwm_mode, this->time_period); + nrf_pwm_decoder_set(_PWMInstance, TONE_PWM_CONFIG::load_mode, TONE_PWM_CONFIG::step_mode); + nrf_pwm_shorts_set(_PWMInstance, this->shorts); + nrf_pwm_int_set(_PWMInstance, 0); + + // static sequence value ... This is the value actually used + // during playback ... do NOT modify without semaphore *and* + // having ensured playback has stopped! + static uint16_t seq_values[1] = { this->duty_with_polarity }; + + nrf_pwm_seq_ptr_set(_PWMInstance, 0, &seq_values[0]); + nrf_pwm_seq_ptr_set(_PWMInstance, 1, &seq_values[0]); + nrf_pwm_seq_cnt_set(_PWMInstance, 0, 1); + nrf_pwm_seq_cnt_set(_PWMInstance, 1, 1); + + nrf_pwm_seq_refresh_set(_PWMInstance, 0, seq0_refresh); + nrf_pwm_seq_refresh_set(_PWMInstance, 1, seq1_refresh); + nrf_pwm_seq_end_delay_set(_PWMInstance, 0, 0); + nrf_pwm_seq_end_delay_set(_PWMInstance, 1, 0); + nrf_pwm_loop_set(_PWMInstance, loop_count); + + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_STOPPED); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_SEQSTARTED0); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_SEQSTARTED1); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_SEQEND0); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_SEQEND1); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_PWMPERIODEND); + nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_LOOPSDONE); + return true; +} +bool TONE_PWM_CONFIG::StartPlayback(void) noexcept { + if (!this->is_initialized) { + LOG_LV1("TON", "Cannot start playback without first initializing"); + return false; + } + if (!this->EnsurePwmPeripheralOwnership()) { + LOG_LV1("TON", "PWM peripheral not available for playback"); + return false; + } + nrf_pwm_task_trigger(_PWMInstance, this->task_to_start); + return true; +} +bool TONE_PWM_CONFIG::StopPlayback(bool releaseOwnership) { + + bool notInIsr = !isInISR(); + + if (!_HwPWM->isOwner(TONE_PWM_CONFIG::toneToken)) { + if (notInIsr) { + LOG_LV2("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); + } + return false; + } + // ensure stopped and then disable + if (_is_pwm_enabled(_PWMInstance)) { + nrf_pwm_task_trigger(_PWMInstance, NRF_PWM_TASK_STOP); + nrf_pwm_disable(_PWMInstance); + _PWMInstance->PSEL.OUT[0] = NRF_PWM_PIN_NOT_CONNECTED; + } + + if (releaseOwnership) { + _HwPWM->releaseOwnership(TONE_PWM_CONFIG::toneToken); + } + return true; } -#endif diff --git a/cores/nRF5/Tone.h b/cores/nRF5/Tone.h index 8874a1ced..064cfad33 100644 --- a/cores/nRF5/Tone.h +++ b/cores/nRF5/Tone.h @@ -15,6 +15,7 @@ License along with this library; if not, write to the Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +/** \file Tone.h */ #ifndef _WIRING_TONE_ #define _WIRING_TONE_ @@ -25,8 +26,44 @@ #include "wiring_digital.h" #include "nrf_pwm.h" - +/** + * \brief Generate a tone (PWM) output on the specified pin. + * + * \param[in] pin The Arduino pin for output + * + * \param[in] frequency The frequency, in hertz, of the requested tone. + * + * \param[in] duration An optional duration, in milliseconds (default: 0 == infinite) + * + * Generates a square wave of the specified frequency (and 50% duty cycle) + * on a pin. A duration can be specified, otherwise the wave continues until a call + * to `noTone()`. The pin can be connected to a piezo buzzer or other speaker to + * play tones. + * + * The `tone()` API has the following contracts, which were defined by + * the original Arduino code: + * + * 1. at most one `tone()` can be generated at a time + * 2. so long as the `pin` stays the same, `tone()` can be called repeatedly + * 3. a call to `noTone()` is required prior to calling `tone()` with a different pin. + * + * The third requirement is not enforced in the nRF52 BSP. Instead, if a call + * to `tone()` is made with a different pin, a call to `noTone()` occurs automatically, + * simplifying use somewhat. + * + * For the nRF52, the allowed range for parameter `frequency` is `[20..25000]`. + * + * \see noTone + */ void tone(uint8_t pin, unsigned int frequency, unsigned long duration = 0); +/** + * \brief Stops generation of a tone (PWM) output. + * + * \param pin For the nRF52, this argument is ignored + * + * Stops the generation of a square wave triggered by `tone()`. + * This function has no effect if no tone is being generated. + */ void noTone(uint8_t pin); diff --git a/cores/nRF5/main.cpp b/cores/nRF5/main.cpp index 98e3a2918..18e7432cd 100644 --- a/cores/nRF5/main.cpp +++ b/cores/nRF5/main.cpp @@ -15,6 +15,9 @@ #define ARDUINO_MAIN #include "Arduino.h" +#if (CFG_LOGGER == 2) + #include +#endif // DEBUG Level 1 #if CFG_DEBUG @@ -106,11 +109,15 @@ extern "C" int _write (int fd, const void *buf, size_t count) { (void) fd; - +#if (CFG_LOGGER == 2) + unsigned numBytes = count; + SEGGER_RTT_Write(0, buf, numBytes); +#else if ( Serial ) { return Serial.write( (const uint8_t *) buf, count); } +#endif return 0; } diff --git a/platform.txt b/platform.txt index f8f47382c..ce3cd4b9b 100644 --- a/platform.txt +++ b/platform.txt @@ -28,11 +28,14 @@ compiler.warning_flags.default= compiler.warning_flags.more=-Wall compiler.warning_flags.all=-Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-pointer-arith +# Allow changing optimization settings via platform.local.txt / boards.local.txt +compiler.optimization_flag=-Ofast + compiler.path={runtime.tools.arm-none-eabi-gcc.path}/bin/ compiler.c.cmd=arm-none-eabi-gcc compiler.c.flags=-mcpu={build.mcu} -mthumb -c -g {compiler.warning_flags} {build.float_flags} -std=gnu11 -ffunction-sections -fdata-sections -nostdlib --param max-inline-insns-single=500 -MMD compiler.c.elf.cmd=arm-none-eabi-gcc -compiler.c.elf.flags=-Ofast -Wl,--gc-sections -save-temps +compiler.c.elf.flags={compiler.optimization_flag} -Wl,--gc-sections -save-temps compiler.S.cmd=arm-none-eabi-gcc compiler.S.flags=-c -g -x assembler-with-cpp compiler.cpp.cmd=arm-none-eabi-g++ @@ -60,7 +63,7 @@ nordic.path={build.core.path}/nordic # build.logger_flags and build.sysview_flags and intentionally empty, # to allow modification via a user's own boards.local.txt or platform.local.txt files. -build.flags.nrf= -DSOFTDEVICE_PRESENT -DARDUINO_NRF52_ADAFRUIT -DNRF52_SERIES -DLFS_NAME_MAX=64 -Ofast {build.debug_flags} {build.logger_flags} {build.sysview_flags} "-I{build.core.path}/cmsis/Core/Include" "-I{nordic.path}" "-I{nordic.path}/nrfx" "-I{nordic.path}/nrfx/hal" "-I{nordic.path}/nrfx/mdk" "-I{nordic.path}/nrfx/soc" "-I{nordic.path}/nrfx/drivers/include" "-I{nordic.path}/nrfx/drivers/src" "-I{nordic.path}/softdevice/{build.sd_name}_nrf52_{build.sd_version}_API/include" "-I{nordic.path}/softdevice/{build.sd_name}_nrf52_{build.sd_version}_API/include/nrf52" "-I{rtos.path}/Source/include" "-I{rtos.path}/config" "-I{rtos.path}/portable/GCC/nrf52" "-I{rtos.path}/portable/CMSIS/nrf52" "-I{build.core.path}/sysview/SEGGER" "-I{build.core.path}/sysview/Config" "-I{build.core.path}/TinyUSB" "-I{build.core.path}/TinyUSB/Adafruit_TinyUSB_ArduinoCore" "-I{build.core.path}/TinyUSB/Adafruit_TinyUSB_ArduinoCore/tinyusb/src" +build.flags.nrf= -DSOFTDEVICE_PRESENT -DARDUINO_NRF52_ADAFRUIT -DNRF52_SERIES -DLFS_NAME_MAX=64 {compiler.optimization_flag} {build.debug_flags} {build.logger_flags} {build.sysview_flags} "-I{build.core.path}/cmsis/Core/Include" "-I{nordic.path}" "-I{nordic.path}/nrfx" "-I{nordic.path}/nrfx/hal" "-I{nordic.path}/nrfx/mdk" "-I{nordic.path}/nrfx/soc" "-I{nordic.path}/nrfx/drivers/include" "-I{nordic.path}/nrfx/drivers/src" "-I{nordic.path}/softdevice/{build.sd_name}_nrf52_{build.sd_version}_API/include" "-I{nordic.path}/softdevice/{build.sd_name}_nrf52_{build.sd_version}_API/include/nrf52" "-I{rtos.path}/Source/include" "-I{rtos.path}/config" "-I{rtos.path}/portable/GCC/nrf52" "-I{rtos.path}/portable/CMSIS/nrf52" "-I{build.core.path}/sysview/SEGGER" "-I{build.core.path}/sysview/Config" "-I{build.core.path}/TinyUSB" "-I{build.core.path}/TinyUSB/Adafruit_TinyUSB_ArduinoCore" "-I{build.core.path}/TinyUSB/Adafruit_TinyUSB_ArduinoCore/tinyusb/src" # usb flags build.flags.usb= -DUSBCON -DUSE_TINYUSB -DUSB_VID={build.vid} -DUSB_PID={build.pid} '-DUSB_MANUFACTURER={build.usb_manufacturer}' '-DUSB_PRODUCT={build.usb_product}' From e200ee88debf120796411ba17551455429399d20 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Mon, 3 Aug 2020 02:32:08 -0700 Subject: [PATCH 28/38] Remove deeper debug level 3 --- cores/nRF5/HardwarePWM.cpp | 9 --------- cores/nRF5/Tone.cpp | 17 +---------------- cores/nRF5/common_func.h | 8 -------- cores/nRF5/wiring_analog.cpp | 3 --- 4 files changed, 1 insertion(+), 36 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 306220449..e29f538a1 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -110,21 +110,12 @@ bool HardwarePWM::takeOwnership(uintptr_t token) } } if (this->_owner_token != 0) { - if (notInIsr) { - LOG_LV3("HwPWM", "failing to acquire ownership because already owned by other token"); - } return false; } if (this->usedChannelCount() != 0) { - if (notInIsr) { - LOG_LV3("HwPWM", "failing to acquire ownership because at least one channel connected"); - } return false; } if (this->enabled()) { - if (notInIsr) { - LOG_LV3("HwPWM", "failing to acquire ownership because peripheral is already enabled"); - } return false; } // TODO: warn, but do not fail, if taking ownership with IRQs already enabled diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 4aff67413..70e365bae 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -277,8 +277,6 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count this->duty_with_polarity = 0x8000U | (time_period / 2U); if (this->pulse_count == 0) { - LOG_LV3("TON", "Infinite duration requested\n"); - this->seq0_refresh = 0xFFFFFFU; // 24-bit maximum value this->seq1_refresh = 0xFFFFFFU; // 24-bit maximum value this->loop_count = 0xFFFFU; // 16-bit maximum value @@ -286,8 +284,7 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count this->shorts = NRF_PWM_SHORT_LOOPSDONE_SEQSTART0_MASK; } else if (this->pulse_count == 1) { - LOG_LV3("TON", "Edge case: exactly one pulse\n"); - // yes, this is an edge case + // yes, this is an edge case; e.g., frequency == 100, duration == 100 causes this this->seq0_refresh = 0; this->seq1_refresh = 0; this->loop_count = 1; @@ -295,8 +292,6 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count this->shorts = NRF_PWM_SHORT_LOOPSDONE_STOP_MASK; } else { - LOG_LV3("TON", "Non-infinite duration of at least two pulses requested\n"); - // This is the interesting section. // // To ensure refresh value stays within 24 bits, the maximum number of bits @@ -318,37 +313,27 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count // NOTE: Due to final SEQ1 outputting exactly one pulse, may need one additional bit for loop count // ... but that will still be within the 16 bits available, because top of range is 13 bits. - LOG_LV3("TON", "Using %d bits for refresh, and %d bits for loop_count\n", bits_for_refresh, bits_for_loop_count); // now determine how many PWM pulses should occur per loop (when both SEQ0 and SEQ1 are played) uint32_t total_refresh_count = 1 << bits_for_refresh; // range is [2 .. 2^24] uint32_t full_loops = (this->pulse_count - 1) / total_refresh_count; // == loopCount - 1 - LOG_LV3("TON", "total_refresh_count is 0x%" PRIx32 " (%" PRIu32 ")\n", total_refresh_count, total_refresh_count); - LOG_LV3("TON", "full_loops is 0x%" PRIx32 " (%" PRIu32 ")\n", full_loops, full_loops); - // if (pulses - 1) % total_refresh_count == 0, then start at SEQ1 and split refresh evenly // else, start at SEQ0, and set SEQ0 to extra pulses needed... uint32_t extraPulsesNeededIfStartingAtSequence1 = (this->pulse_count - 1) % total_refresh_count; uint32_t seq0_count; if (extraPulsesNeededIfStartingAtSequence1 == 0) { - LOG_LV3("TON", "Pulse count (minus one) is exact multiple of total_refresh_count -- starting at SEQ1\n"); seq0_count = total_refresh_count / 2; // range is [1 .. 2^23] this->task_to_start = NRF_PWM_TASK_SEQSTART1; } else { - LOG_LV3("TON", "Pulse count (minus one) requires extra %" PRIu32 " pulses ... setting SEQ0 to that value\n", extraPulsesNeededIfStartingAtSequence1); seq0_count = extraPulsesNeededIfStartingAtSequence1; this->task_to_start = NRF_PWM_TASK_SEQSTART0; } this->loop_count = full_loops + 1; this->seq0_refresh = seq0_count - 1; this->seq1_refresh = (total_refresh_count - seq0_count) - 1; - - LOG_LV3("TON", "seq0_count is %" PRIu32 ", so refresh will be set to %" PRIu32 "\n", seq0_count, this->seq0_refresh); - LOG_LV3("TON", "seq1_count is %" PRIu32 ", so refresh will be set to %" PRIu32 "\n", (total_refresh_count - seq0_count), this->seq1_refresh); - this->shorts = NRF_PWM_SHORT_LOOPSDONE_STOP_MASK; } this->is_initialized = true; diff --git a/cores/nRF5/common_func.h b/cores/nRF5/common_func.h index c6229b8a2..ad42c6260 100644 --- a/cores/nRF5/common_func.h +++ b/cores/nRF5/common_func.h @@ -172,14 +172,6 @@ const char* dbg_err_str(int32_t err_id); // TODO move to other place #define LOG_LV2_BUFFER(...) #endif -#if CFG_DEBUG >= 3 -#define LOG_LV3(...) ADALOG(__VA_ARGS__) -#define LOG_LV3_BUFFER(...) ADALOG_BUFFER(__VA_ARGS__) -#else -#define LOG_LV3(...) -#define LOG_LV3_BUFFER(...) -#endif - #if CFG_DEBUG #define PRINT_LOCATION() PRINTF("%s: %d:\n", __PRETTY_FUNCTION__, __LINE__) diff --git a/cores/nRF5/wiring_analog.cpp b/cores/nRF5/wiring_analog.cpp index 5267a64cf..7c1ac04ff 100644 --- a/cores/nRF5/wiring_analog.cpp +++ b/cores/nRF5/wiring_analog.cpp @@ -96,11 +96,9 @@ void analogWrite( uint32_t pin, uint32_t value ) for(int i=0; iisOwner(_analogToken)) { - LOG_LV3("ANA", "not currently owner of PWM %d", i); continue; } if (!HwPWMx[i]->addPin(pin)) { - LOG_LV3("ANA", "could not add pin %" PRIu32 " to PWM %d", pin, i); continue; } @@ -116,7 +114,6 @@ void analogWrite( uint32_t pin, uint32_t value ) for(int i=0; itakeOwnership(_analogToken)) { - LOG_LV3("ANA", "Could not take ownership of PWM %d", i); continue; } From 68a9302a79be30df27acab2cb0a72b5cf119c411 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sat, 8 Aug 2020 11:20:36 -0700 Subject: [PATCH 29/38] Fix off-by-one typo --- cores/nRF5/Tone.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 70e365bae..ac85f27af 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -309,7 +309,7 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count // split the number of bits between refresh and loop_count in 2:1 ratio // so that, no matter what inputs are given, guaranteed to have interrupt-free solution unsigned int bits_for_refresh = bits_needed * 2 / 3; // range is [1 .. 24] - unsigned int bits_for_loop_count = bits_needed - bits_for_refresh; // range is [1 .. 13] + //unsigned int bits_for_loop_count = bits_needed - bits_for_refresh; // range is [1 .. 13] // NOTE: Due to final SEQ1 outputting exactly one pulse, may need one additional bit for loop count // ... but that will still be within the 16 bits available, because top of range is 13 bits. @@ -371,10 +371,11 @@ bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) noexcept { // static sequence value ... This is the value actually used // during playback ... do NOT modify without semaphore *and* // having ensured playback has stopped! - static uint16_t seq_values[1] = { this->duty_with_polarity }; + static uint16_t seq_values; // static value + seq_values = this->duty_with_polarity; - nrf_pwm_seq_ptr_set(_PWMInstance, 0, &seq_values[0]); - nrf_pwm_seq_ptr_set(_PWMInstance, 1, &seq_values[0]); + nrf_pwm_seq_ptr_set(_PWMInstance, 0, &seq_values); + nrf_pwm_seq_ptr_set(_PWMInstance, 1, &seq_values); nrf_pwm_seq_cnt_set(_PWMInstance, 0, 1); nrf_pwm_seq_cnt_set(_PWMInstance, 1, 1); From a566c693d9fbea5f97137b8a92e36b76cb05226e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 20:20:49 -0700 Subject: [PATCH 30/38] With no ISR, use Mutex (not Binary) semaphore --- cores/nRF5/Tone.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index ac85f27af..2b04688eb 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -184,7 +184,7 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) // and to simplify ensuring the semaphore gets initialized. static StaticSemaphore_t _tone_semaphore_allocation; static auto init_semaphore = [] () throw() { //< use a lambda to both initialize AND give the mutex - SemaphoreHandle_t handle = xSemaphoreCreateBinaryStatic(&_tone_semaphore_allocation); + SemaphoreHandle_t handle = xSemaphoreCreateMutexStatic(&_tone_semaphore_allocation); auto mustSucceed = xSemaphoreGive(handle); (void)mustSucceed; NRFX_ASSERT(mustSucceed == pdTRUE); From e99b5ea40e7649fb582f2492d7f4ccc303c9f620 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 22:24:37 -0700 Subject: [PATCH 31/38] Use C++11 atomic --- cores/nRF5/HardwarePWM.cpp | 12 ++++++------ cores/nRF5/HardwarePWM.h | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index e29f538a1..6acf51ec0 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -121,9 +121,9 @@ bool HardwarePWM::takeOwnership(uintptr_t token) // TODO: warn, but do not fail, if taking ownership with IRQs already enabled // NVIC_GetActive - // use gcc built-in intrinsic to ensure atomicity - // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html - return __sync_bool_compare_and_swap(&(this->_owner_token), 0, token); + // Use C++11 atomic CAS operation + uintptr_t newValue = 0U; + return this->_owner_token.compare_exchange_strong(newValue, token); } // returns true ONLY when (1) no PWM channel has a pin attached, and (2) the owner token matches bool HardwarePWM::releaseOwnership(uintptr_t token) @@ -156,9 +156,8 @@ bool HardwarePWM::releaseOwnership(uintptr_t token) // TODO: warn, but do not fail, if releasing ownership with IRQs enabled // NVIC_GetActive - // use gcc built-in intrinsic to ensure atomicity - // See https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html - bool result = __sync_bool_compare_and_swap(&(this->_owner_token), token, 0); + // Use C++11 atomic CAS operation + bool result = this->_owner_token.compare_exchange_strong(token, 0U); if (!result) { LOG_LV1("HwPWM", "race condition resulted in failure to acquire ownership"); } @@ -168,6 +167,7 @@ bool HardwarePWM::releaseOwnership(uintptr_t token) HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) : _pwm(pwm) { + _owner_token = 0U; arrclr(_seq0); _max_value = 255; diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index 1afa0c37d..c2d8f4e4e 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -38,6 +38,7 @@ #include "common_inc.h" #include "nrf.h" +#include #ifdef NRF52840_XXAA #define HWPWM_MODULE_NUM 4 @@ -50,7 +51,7 @@ class HardwarePWM private: enum { MAX_CHANNELS = 4 }; // Max channel per group NRF_PWM_Type * const _pwm; - volatile uintptr_t _owner_token = 0; + std::atomic_uintptr_t _owner_token; uint16_t _seq0[MAX_CHANNELS]; From 02045e3b300d00569c6d6acc4e121d65fef41a6a Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 22:35:19 -0700 Subject: [PATCH 32/38] Poe-tay-toe, poe-tah-toe --- cores/nRF5/HardwarePWM.cpp | 2 ++ cores/nRF5/HardwarePWM.h | 4 ---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/cores/nRF5/HardwarePWM.cpp b/cores/nRF5/HardwarePWM.cpp index 6acf51ec0..f9927727d 100644 --- a/cores/nRF5/HardwarePWM.cpp +++ b/cores/nRF5/HardwarePWM.cpp @@ -92,6 +92,8 @@ void HardwarePWM::DebugOutput(Stream& logger) } logger.printf("\n"); } +#else +void HardwarePWM::DebugOutput(Stream& logger) {} #endif // CFG_DEBUG // returns true ONLY when (1) no PWM channel has a pin, and (2) the owner token is nullptr diff --git a/cores/nRF5/HardwarePWM.h b/cores/nRF5/HardwarePWM.h index c2d8f4e4e..024a8c848 100644 --- a/cores/nRF5/HardwarePWM.h +++ b/cores/nRF5/HardwarePWM.h @@ -116,11 +116,7 @@ class HardwarePWM uint8_t usedChannelCount(void) const; uint8_t freeChannelCount(void) const; -#if CFG_DEBUG static void DebugOutput(Stream& logger); -#else - static void inline DebugOutput(Stream& logger) { (void)logger; }; -#endif // CFG_DEBUG }; extern HardwarePWM HwPWM0; From fdf405c1ba88da5ee4f4626e167ec9f3a84b7cd7 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 22:57:50 -0700 Subject: [PATCH 33/38] @hathach believes having `noexcept` means code might throw exceptions. As it has zero effect in this codebase, I am removing to avoid arguments. --- cores/nRF5/Tone.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 2b04688eb..7c35f1bc5 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -67,13 +67,13 @@ class TONE_PWM_CONFIG { boolean is_initialized; //< defaults to uninitialized public: - bool EnsurePwmPeripheralOwnership(void) noexcept; - bool InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count, uint16_t time_period) noexcept; - bool ApplyConfiguration(uint32_t pin) noexcept; - bool StartPlayback(void) noexcept; - bool StopPlayback(bool releaseOwnership = false) noexcept; - bool ApplyConfigurationAndStartPlayback(uint32_t pin) noexcept; - uint64_t CalculateExpectedPulseCount(void) noexcept; + bool EnsurePwmPeripheralOwnership(void); + bool InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count, uint16_t time_period); + bool ApplyConfiguration(uint32_t pin); + bool StartPlayback(void); + bool StopPlayback(bool releaseOwnership = false); + bool ApplyConfigurationAndStartPlayback(uint32_t pin); + uint64_t CalculateExpectedPulseCount(void); }; TONE_PWM_CONFIG _pwm_config; @@ -116,7 +116,7 @@ constexpr inline static uint64_t _calculate_pulse_count(uint32_t frequency, uint (duration / 1000ULL) * frequency : (((uint64_t)duration) * frequency / 1000ULL); }; -inline static int _bits_used(unsigned long x) noexcept { +inline static int _bits_used(unsigned long x) { if (0 == x) return 0; unsigned int result = 0; do { @@ -124,7 +124,7 @@ inline static int _bits_used(unsigned long x) noexcept { } while (x >>= 1); return result; } -inline static int _bits_used(unsigned long long x) noexcept { +inline static int _bits_used(unsigned long long x) { if (0 == x) return 0; unsigned int result = 0; do { @@ -265,7 +265,7 @@ bool TONE_PWM_CONFIG::EnsurePwmPeripheralOwnership(void) { // COUNT += (start at SEQ0) ? (SEQ0.CNT * (SEQ0.REFRESH+1) : 0; // COUNT += 1; // final SEQ1 emits single pulse // -bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) noexcept { +bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) { if (_bits_used(pulse_count_x) > 37) { LOG_LV1("TON", "pulse count is limited to 37 bit long value"); @@ -339,7 +339,7 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count this->is_initialized = true; return true; } -bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) noexcept { +bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) { if (!this->is_initialized) { return false; } @@ -394,7 +394,7 @@ bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) noexcept { nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_LOOPSDONE); return true; } -bool TONE_PWM_CONFIG::StartPlayback(void) noexcept { +bool TONE_PWM_CONFIG::StartPlayback(void) { if (!this->is_initialized) { LOG_LV1("TON", "Cannot start playback without first initializing"); return false; From 4555fc1b0a137b83b2eee77148399793aee5684b Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 23:00:37 -0700 Subject: [PATCH 34/38] Rename struct per @hathach request --- cores/nRF5/Tone.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 7c35f1bc5..bcd1d2da8 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -46,7 +46,7 @@ static NRF_PWM_Type * const _PWMInstance = NRF_PWM2; static HardwarePWM * const _HwPWM = HwPWMx[2]; // Defined a struct, to simplify validation testing ... also provides context when debugging -class TONE_PWM_CONFIG { +class TonePwmConfig { private: const nrf_pwm_clk_t clock_frequency = NRF_PWM_CLK_125kHz; const nrf_pwm_mode_t pwm_mode = NRF_PWM_MODE_UP; @@ -75,7 +75,7 @@ class TONE_PWM_CONFIG { bool ApplyConfigurationAndStartPlayback(uint32_t pin); uint64_t CalculateExpectedPulseCount(void); }; -TONE_PWM_CONFIG _pwm_config; +TonePwmConfig _pwm_config; inline static bool _is_pwm_enabled(NRF_PWM_Type const * pwm_instance) { bool isEnabled = @@ -227,8 +227,8 @@ void noTone(uint8_t pin) _pwm_config.StopPlayback(true); // release ownership of PWM peripheral } -bool TONE_PWM_CONFIG::EnsurePwmPeripheralOwnership(void) { - if (!_HwPWM->isOwner(TONE_PWM_CONFIG::toneToken) && !_HwPWM->takeOwnership(TONE_PWM_CONFIG::toneToken)) { +bool TonePwmConfig::EnsurePwmPeripheralOwnership(void) { + if (!_HwPWM->isOwner(TonePwmConfig::toneToken) && !_HwPWM->takeOwnership(TonePwmConfig::toneToken)) { LOG_LV1("TON", "unable to allocate PWM2 to Tone"); return false; } @@ -265,7 +265,7 @@ bool TONE_PWM_CONFIG::EnsurePwmPeripheralOwnership(void) { // COUNT += (start at SEQ0) ? (SEQ0.CNT * (SEQ0.REFRESH+1) : 0; // COUNT += 1; // final SEQ1 emits single pulse // -bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) { +bool TonePwmConfig::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) { if (_bits_used(pulse_count_x) > 37) { LOG_LV1("TON", "pulse count is limited to 37 bit long value"); @@ -339,7 +339,7 @@ bool TONE_PWM_CONFIG::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count this->is_initialized = true; return true; } -bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) { +bool TonePwmConfig::ApplyConfiguration(uint32_t pin) { if (!this->is_initialized) { return false; } @@ -363,8 +363,8 @@ bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) { nrf_pwm_pins_set(_PWMInstance, pins); // must set pins before enabling nrf_pwm_enable(_PWMInstance); - nrf_pwm_configure(_PWMInstance, TONE_PWM_CONFIG::clock_frequency, TONE_PWM_CONFIG::pwm_mode, this->time_period); - nrf_pwm_decoder_set(_PWMInstance, TONE_PWM_CONFIG::load_mode, TONE_PWM_CONFIG::step_mode); + nrf_pwm_configure(_PWMInstance, TonePwmConfig::clock_frequency, TonePwmConfig::pwm_mode, this->time_period); + nrf_pwm_decoder_set(_PWMInstance, TonePwmConfig::load_mode, TonePwmConfig::step_mode); nrf_pwm_shorts_set(_PWMInstance, this->shorts); nrf_pwm_int_set(_PWMInstance, 0); @@ -394,7 +394,7 @@ bool TONE_PWM_CONFIG::ApplyConfiguration(uint32_t pin) { nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_LOOPSDONE); return true; } -bool TONE_PWM_CONFIG::StartPlayback(void) { +bool TonePwmConfig::StartPlayback(void) { if (!this->is_initialized) { LOG_LV1("TON", "Cannot start playback without first initializing"); return false; @@ -406,11 +406,11 @@ bool TONE_PWM_CONFIG::StartPlayback(void) { nrf_pwm_task_trigger(_PWMInstance, this->task_to_start); return true; } -bool TONE_PWM_CONFIG::StopPlayback(bool releaseOwnership) { +bool TonePwmConfig::StopPlayback(bool releaseOwnership) { bool notInIsr = !isInISR(); - if (!_HwPWM->isOwner(TONE_PWM_CONFIG::toneToken)) { + if (!_HwPWM->isOwner(TonePwmConfig::toneToken)) { if (notInIsr) { LOG_LV2("TON", "Attempt to set noTone when not the owner of the PWM peripheral. Ignoring call...."); } @@ -424,7 +424,7 @@ bool TONE_PWM_CONFIG::StopPlayback(bool releaseOwnership) { } if (releaseOwnership) { - _HwPWM->releaseOwnership(TONE_PWM_CONFIG::toneToken); + _HwPWM->releaseOwnership(TonePwmConfig::toneToken); } return true; } From 1ba129906163a7eb1326806c686d2c4ee3027b31 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 23:05:08 -0700 Subject: [PATCH 35/38] change casing of class functions per @hathach request --- cores/nRF5/Tone.cpp | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index bcd1d2da8..b60748acf 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -67,13 +67,11 @@ class TonePwmConfig { boolean is_initialized; //< defaults to uninitialized public: - bool EnsurePwmPeripheralOwnership(void); - bool InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count, uint16_t time_period); - bool ApplyConfiguration(uint32_t pin); - bool StartPlayback(void); - bool StopPlayback(bool releaseOwnership = false); - bool ApplyConfigurationAndStartPlayback(uint32_t pin); - uint64_t CalculateExpectedPulseCount(void); + bool ensurePwmPeripheralOwnership(void); + bool initializeFromPulseCountAndTimePeriod(uint64_t pulse_count, uint16_t time_period); + bool applyConfiguration(uint32_t pin); + bool startPlayback(void); + bool stopPlayback(bool releaseOwnership = false); }; TonePwmConfig _pwm_config; @@ -205,15 +203,15 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) uint64_t pulse_count = _calculate_pulse_count(frequency, duration); uint16_t time_period = _calculate_time_period(frequency); - if (!_pwm_config.EnsurePwmPeripheralOwnership()) { + if (!_pwm_config.ensurePwmPeripheralOwnership()) { LOG_LV1("TON", "Unable to acquire PWM peripheral"); - } else if (!_pwm_config.StopPlayback()) { + } else if (!_pwm_config.stopPlayback()) { LOG_LV1("TON", "Unable to stop playback"); - } else if (!_pwm_config.InitializeFromPulseCountAndTimePeriod(pulse_count, time_period)) { + } else if (!_pwm_config.initializeFromPulseCountAndTimePeriod(pulse_count, time_period)) { LOG_LV1("TON", "Failed calculating configuration"); - } else if (!_pwm_config.ApplyConfiguration(pin)) { + } else if (!_pwm_config.applyConfiguration(pin)) { LOG_LV1("TON", "Failed applying configuration"); - } else if (!_pwm_config.StartPlayback()) { + } else if (!_pwm_config.startPlayback()) { LOG_LV1("TON", "Failed attempting to start PWM peripheral"); } else { //LOG_LV2("TON", "Started playback of tone at frequency %d duration %ld", frequency, duration); @@ -224,10 +222,10 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) void noTone(uint8_t pin) { ( void )pin; // avoid unreferenced parameter compiler warning - _pwm_config.StopPlayback(true); // release ownership of PWM peripheral + _pwm_config.stopPlayback(true); // release ownership of PWM peripheral } -bool TonePwmConfig::EnsurePwmPeripheralOwnership(void) { +bool TonePwmConfig::ensurePwmPeripheralOwnership(void) { if (!_HwPWM->isOwner(TonePwmConfig::toneToken) && !_HwPWM->takeOwnership(TonePwmConfig::toneToken)) { LOG_LV1("TON", "unable to allocate PWM2 to Tone"); return false; @@ -265,7 +263,7 @@ bool TonePwmConfig::EnsurePwmPeripheralOwnership(void) { // COUNT += (start at SEQ0) ? (SEQ0.CNT * (SEQ0.REFRESH+1) : 0; // COUNT += 1; // final SEQ1 emits single pulse // -bool TonePwmConfig::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) { +bool TonePwmConfig::initializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x, uint16_t time_period) { if (_bits_used(pulse_count_x) > 37) { LOG_LV1("TON", "pulse count is limited to 37 bit long value"); @@ -339,17 +337,17 @@ bool TonePwmConfig::InitializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x this->is_initialized = true; return true; } -bool TonePwmConfig::ApplyConfiguration(uint32_t pin) { +bool TonePwmConfig::applyConfiguration(uint32_t pin) { if (!this->is_initialized) { return false; } if (pin >= PINS_COUNT) { return false; } - if (!this->EnsurePwmPeripheralOwnership()) { + if (!this->ensurePwmPeripheralOwnership()) { return false; } - this->StopPlayback(false); + this->stopPlayback(false); this->arduino_pin = pin; this->nrf_pin = g_ADigitalPinMap[pin]; @@ -394,19 +392,19 @@ bool TonePwmConfig::ApplyConfiguration(uint32_t pin) { nrf_pwm_event_clear(_PWMInstance, NRF_PWM_EVENT_LOOPSDONE); return true; } -bool TonePwmConfig::StartPlayback(void) { +bool TonePwmConfig::startPlayback(void) { if (!this->is_initialized) { LOG_LV1("TON", "Cannot start playback without first initializing"); return false; } - if (!this->EnsurePwmPeripheralOwnership()) { + if (!this->ensurePwmPeripheralOwnership()) { LOG_LV1("TON", "PWM peripheral not available for playback"); return false; } nrf_pwm_task_trigger(_PWMInstance, this->task_to_start); return true; } -bool TonePwmConfig::StopPlayback(bool releaseOwnership) { +bool TonePwmConfig::stopPlayback(bool releaseOwnership) { bool notInIsr = !isInISR(); From a7ffaaadbd073a4d9f4cb9254fa448dccbc7cc1e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 23:35:59 -0700 Subject: [PATCH 36/38] @hathach doesn't like static_assert --- cores/nRF5/Tone.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index b60748acf..5bd1c9dd9 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -104,7 +104,6 @@ constexpr inline static uint64_t _calculate_pulse_count(uint32_t frequency, uint // range for frequency == [20..25000], // range for duration == [ 1..0xFFFF_FFFF] // so range of result == [ 1..0x18_FFFF_FFE7] (requires 37 bits) - static_assert(sizeof(unsigned long long) >= sizeof(uint64_t)); return (duration == 0) ? 0 : @@ -174,9 +173,6 @@ inline static int _bits_used(unsigned long long x) { */ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) { - static_assert(sizeof(unsigned long int) <= sizeof(uint32_t)); - static_assert(sizeof(unsigned int) <= sizeof(uint32_t)); - // Used only to protect calls against simultaneous multiple calls to tone(). // Using a function-local static to avoid accidental reference from ISR or elsewhere, // and to simplify ensuring the semaphore gets initialized. From 5f3d871d91d6fed7a359e35fd707ee42c644b9ed Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Tue, 11 Aug 2020 23:42:17 -0700 Subject: [PATCH 37/38] @hathach says internal classes should not guard against API misuse ... I disagree but he is the gatekeeper --- cores/nRF5/Tone.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 5bd1c9dd9..36e6bdbb7 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -64,7 +64,6 @@ class TonePwmConfig { uint8_t nrf_pin; //< the nrf pin for playback nrf_pwm_task_t task_to_start; //< Whether to start playback at SEQ0 or SEQ1 nrf_pwm_short_mask_t shorts; //< shortcuts to enable - boolean is_initialized; //< defaults to uninitialized public: bool ensurePwmPeripheralOwnership(void); @@ -330,13 +329,9 @@ bool TonePwmConfig::initializeFromPulseCountAndTimePeriod(uint64_t pulse_count_x this->seq1_refresh = (total_refresh_count - seq0_count) - 1; this->shorts = NRF_PWM_SHORT_LOOPSDONE_STOP_MASK; } - this->is_initialized = true; return true; } bool TonePwmConfig::applyConfiguration(uint32_t pin) { - if (!this->is_initialized) { - return false; - } if (pin >= PINS_COUNT) { return false; } @@ -389,10 +384,6 @@ bool TonePwmConfig::applyConfiguration(uint32_t pin) { return true; } bool TonePwmConfig::startPlayback(void) { - if (!this->is_initialized) { - LOG_LV1("TON", "Cannot start playback without first initializing"); - return false; - } if (!this->ensurePwmPeripheralOwnership()) { LOG_LV1("TON", "PWM peripheral not available for playback"); return false; From 53819e509156299d9d172268dac074abb8ccdda4 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Wed, 12 Aug 2020 10:35:35 -0700 Subject: [PATCH 38/38] More removal of `noexcept` markings --- cores/nRF5/Tone.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cores/nRF5/Tone.cpp b/cores/nRF5/Tone.cpp index 36e6bdbb7..c2321d8d9 100644 --- a/cores/nRF5/Tone.cpp +++ b/cores/nRF5/Tone.cpp @@ -93,13 +93,13 @@ inline static bool _is_pwm_enabled(NRF_PWM_Type const * pwm_instance) { See https://gist.github.com/henrygab/6b570ebd51354bf247633c72b8dc383b for code that compares the new lambdas to the old calculations. */ -constexpr inline static uint16_t _calculate_time_period(uint32_t frequency) throw() { +constexpr inline static uint16_t _calculate_time_period(uint32_t frequency) { // range for frequency == [20..25000], // so range of result == [ 5..62500] // which fits in 16 bits. return 125000 / frequency; }; -constexpr inline static uint64_t _calculate_pulse_count(uint32_t frequency, uint32_t duration) throw() { +constexpr inline static uint64_t _calculate_pulse_count(uint32_t frequency, uint32_t duration) { // range for frequency == [20..25000], // range for duration == [ 1..0xFFFF_FFFF] // so range of result == [ 1..0x18_FFFF_FFE7] (requires 37 bits) @@ -176,7 +176,7 @@ void tone(uint8_t pin, unsigned int frequency, unsigned long duration) // Using a function-local static to avoid accidental reference from ISR or elsewhere, // and to simplify ensuring the semaphore gets initialized. static StaticSemaphore_t _tone_semaphore_allocation; - static auto init_semaphore = [] () throw() { //< use a lambda to both initialize AND give the mutex + static auto init_semaphore = [] () { //< use a lambda to both initialize AND give the mutex SemaphoreHandle_t handle = xSemaphoreCreateMutexStatic(&_tone_semaphore_allocation); auto mustSucceed = xSemaphoreGive(handle); (void)mustSucceed;