Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt-in PWM conflict avoidance #496

Merged
merged 40 commits into from
Aug 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2981565
Remove dead code
henrygab Apr 12, 2020
b4d777f
Expose API for count of used/free channels
henrygab Apr 13, 2020
af68035
analogWrite() avoids hostile takeover of PWM peripherals
henrygab Apr 13, 2020
a771af3
remove dead code
henrygab Apr 13, 2020
ab1b904
opt-in ownership of PWM peripheral
henrygab Apr 26, 2020
d469728
Use HwPWM ownership abstraction
henrygab Apr 27, 2020
d5dc053
Use HwPWM ownership abstraction
henrygab May 1, 2020
3efce16
Servo can also use HwPWM ownership abstraction
henrygab May 4, 2020
c58c01d
Add check for PWM being ENABLED
henrygab May 4, 2020
c190318
Use const void* for ownership token
henrygab May 4, 2020
027bcd3
Fix check for connected pins
henrygab May 4, 2020
85e41af
remove dead code
henrygab May 10, 2020
a6e0d2c
Prefer `uintptr_t` to `void const *` for ownership token
henrygab May 11, 2020
0acacf6
Finish conversion back to `uintptr_t` token
henrygab May 11, 2020
145167b
tone() auto-release ownership
henrygab May 13, 2020
bbc4531
Improve logging for end users
henrygab May 14, 2020
aebdca3
Ensure default value for `analogWriteResolution()`
henrygab May 14, 2020
dd06e2f
Fix HwPWM debug logging typo
henrygab May 14, 2020
be9927e
Simplify by auto-detecting when called from ISR
henrygab Jul 11, 2020
37935c7
Remove always_inline attribute
henrygab Jul 11, 2020
e58b39e
Reduce debug output for analog_write()
henrygab Jul 11, 2020
eb55315
Explicit `void` for no parameters
henrygab Jul 11, 2020
a2a4371
Remove static assertion
henrygab Jul 11, 2020
43da173
Update noTone() to be safe to call from ISR
henrygab Jul 11, 2020
d00de2d
Change variable name per @hathach request.
henrygab Jul 11, 2020
d7b18ed
Code review required comment removal
henrygab Jul 11, 2020
29843da
Redefine tone() without ISR
henrygab Aug 3, 2020
e200ee8
Remove deeper debug level 3
henrygab Aug 3, 2020
db5e0d6
Merge branch 'master' into PWM_Conflicts
henrygab Aug 7, 2020
68a9302
Fix off-by-one typo
henrygab Aug 8, 2020
2019117
Merge branch 'PWM_Conflicts' of https://github.com/henrygab/Adafruit_…
henrygab Aug 8, 2020
a566c69
With no ISR, use Mutex (not Binary) semaphore
henrygab Aug 12, 2020
e99b5ea
Use C++11 atomic
henrygab Aug 12, 2020
02045e3
Poe-tay-toe, poe-tah-toe
henrygab Aug 12, 2020
fdf405c
@hathach believes having `noexcept` means code might throw exceptions…
henrygab Aug 12, 2020
4555fc1
Rename struct per @hathach request
henrygab Aug 12, 2020
1ba1299
change casing of class functions per @hathach request
henrygab Aug 12, 2020
a7ffaaa
@hathach doesn't like static_assert
henrygab Aug 12, 2020
5f3d871
@hathach says internal classes should not guard against API misuse ..…
henrygab Aug 12, 2020
53819e5
More removal of `noexcept` markings
henrygab Aug 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 136 additions & 4 deletions cores/nRF5/HardwarePWM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,123 @@ HardwarePWM* HwPWMx[] =
#endif
};

HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm)
#if CFG_DEBUG
bool can_stringify_token(uintptr_t token)
{
_pwm = pwm;
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[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 {
logger.printf(" xx");
}
}
}
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
bool HardwarePWM::takeOwnership(uintptr_t token)
{
bool notInIsr = !isInISR();
if (token == 0) {
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) {
if (notInIsr) {
LOG_LV1("HwPWM", "failing to acquire ownership because already owned by requesting token (cannot take ownership twice)");
}
}
if (this->_owner_token != 0) {
return false;
}
if (this->usedChannelCount() != 0) {
return false;
}
if (this->enabled()) {
return false;
}
// TODO: warn, but do not fail, if taking ownership with IRQs already enabled
// NVIC_GetActive

// Use C++11 atomic CAS operation
uintptr_t newValue = 0U;
return this->_owner_token.compare_exchange_strong(newValue, token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still prefer using freeRTOS mutex, since I have no idea how this atomic function is implemented in ARM arch with preempted RTOS with/without ISR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, you were uncomfortable because the CAS operation was using a GNU C++ macro, which you said was "legacy, mainly targeted Intel CPU".

I definitely looked at rewriting using a mutex. But, if you try, you will see that it quickly gets much more complicated, especially when you have to consider order of initialization for statics, error paths, etc.

The good news is that C++ has defined the atomic operation. Therefore, since it's ensured to work per the C++ standard, I'm not understanding your concern about ARM or whether preemption is enabled in freeRTOS?

From what I could see, the use of the C++ atomic fully addressed your concerns about "legacy" and "mainly targeted Intel CPU".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am new to these atomic operation, unless somebody could point out how these are implemented on ARM, I am not convinced. I suspected it would enable/disable with cpsie/cpsid which is not as ideal as using RTOS critical API. Anyway, I will do the change myself later on.

}
// 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) {
if (notInIsr) {
LOG_LV1("HwPWM", "zero / nullptr is not a valid ownership token (attempted use in releaseOwnership)");
}
return false;
}
if (!this->isOwner(token)) {
if (notInIsr) {
LOG_LV1("HwPWM", "attempt to release ownership when not the current owner");
}
return false;
}
if (this->usedChannelCount() != 0) {
if (notInIsr) {
LOG_LV1("HwPWM", "attempt to release ownership when at least on channel is still connected");
}
return false;
}
if (this->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
// NVIC_GetActive

// 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");
}
return result;
}

HardwarePWM::HardwarePWM(NRF_PWM_Type* pwm) :
_pwm(pwm)
{
_owner_token = 0U;
arrclr(_seq0);

_max_value = 255;
Expand Down Expand Up @@ -204,17 +318,35 @@ 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);

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) const
{
uint8_t usedChannels = 0;
for(int i=0; i<MAX_CHANNELS; i++)
{
if ( (_pwm->PSEL.OUT[i] & PWM_PSEL_OUT_CONNECT_Msk) != (PWM_PSEL_OUT_CONNECT_Disconnected << PWM_PSEL_OUT_CONNECT_Pos) )
{
usedChannels++;
}
}
return usedChannels;
}

uint8_t HardwarePWM::freeChannelCount(void) const
{
return MAX_CHANNELS - usedChannelCount();
}

31 changes: 26 additions & 5 deletions cores/nRF5/HardwarePWM.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

#include "common_inc.h"
#include "nrf.h"
#include <atomic>

#ifdef NRF52840_XXAA
#define HWPWM_MODULE_NUM 4
Expand All @@ -49,7 +50,8 @@ class HardwarePWM
{
private:
enum { MAX_CHANNELS = 4 }; // Max channel per group
NRF_PWM_Type* _pwm;
NRF_PWM_Type * const _pwm;
std::atomic_uintptr_t _owner_token;

uint16_t _seq0[MAX_CHANNELS];

Expand All @@ -67,10 +69,23 @@ 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);
henrygab marked this conversation as resolved.
Show resolved Hide resolved
// 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) const
henrygab marked this conversation as resolved.
Show resolved Hide resolved
{
return this->_owner_token == 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<MAX_CHANNELS; i++)
Expand All @@ -80,7 +95,7 @@ class HardwarePWM
return (-1);
}

bool checkPin(uint8_t pin)
bool checkPin(uint8_t pin) const
{
return pin2channel(pin) >= 0;
}
Expand All @@ -94,8 +109,14 @@ 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(void) const;
uint8_t freeChannelCount(void) const;

static void DebugOutput(Stream& logger);
};

extern HardwarePWM HwPWM0;
Expand Down
Loading