Skip to content

Preserve phase relationship when waveform updated #7192

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Preserve phase relationship when waveform updated #7192

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37d9fce
Preserve phase relationship when waveform updated
earlephilhower Apr 4, 2020
6450989
Allow clock-cycle granularity for analogWrite
earlephilhower Apr 4, 2020
e75da87
Actually use the Cycles not the US call in analogWrite
earlephilhower Apr 4, 2020
6fc6b80
Use InterruptLock and not manual locking
earlephilhower Apr 5, 2020
393cc85
Fix potential race condition when Tone() updating
earlephilhower Apr 5, 2020
4b2562a
Adjust to a lockfree cycle update logic
earlephilhower Apr 6, 2020
545fa10
Move waveform vars to a struct to save IRAM
earlephilhower Apr 6, 2020
371ef22
Favor phase over duty cycle
earlephilhower Apr 7, 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
41 changes: 28 additions & 13 deletions cores/esp8266/core_esp8266_waveform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
*/

#include <Arduino.h>
#include "ets_sys.h"
#include "interrupts.h"
#include "core_esp8266_waveform.h"

extern "C" {
Expand All @@ -54,8 +54,10 @@ extern "C" {
typedef struct {
uint32_t nextServiceCycle; // ESP cycle timer when a transition required
uint32_t expiryCycle; // For time-limited waveform, the cycle when this waveform must stop
uint32_t nextTimeHighCycles; // Copy over low->high to keep smooth waveform
uint32_t nextTimeLowCycles; // Copy over high->low to keep smooth waveform
uint32_t timeHighCycles; // Currently running waveform period
uint32_t timeLowCycles; //
uint32_t gotoTimeHighCycles; // Copied over on the next period to preserve phase
uint32_t gotoTimeLowCycles; //
} Waveform;

static Waveform waveform[17]; // State of all possible pins
Expand Down Expand Up @@ -112,20 +114,30 @@ void setTimer1Callback(uint32_t (*fn)()) {
// waveform smoothly on next low->high transition. For immediate change, stopWaveform()
// first, then it will immediately begin.
int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS) {
return startWaveformCycles(pin, microsecondsToClockCycles(timeHighUS), microsecondsToClockCycles(timeLowUS), microsecondsToClockCycles(runTimeUS));
}

int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCycles, uint32_t runTimeCycles) {
if ((pin > 16) || isFlashInterfacePin(pin)) {
return false;
}
Waveform *wave = &waveform[pin];
// Adjust to shave off some of the IRQ time, approximately
wave->nextTimeHighCycles = microsecondsToClockCycles(timeHighUS);
wave->nextTimeLowCycles = microsecondsToClockCycles(timeLowUS);
wave->expiryCycle = runTimeUS ? GetCycleCount() + microsecondsToClockCycles(runTimeUS) : 0;
if (runTimeUS && !wave->expiryCycle) {

wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0;
if (runTimeCycles && !wave->expiryCycle) {
wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it
}

uint32_t mask = 1<<pin;
if (!(waveformEnabled & mask)) {
if (waveformEnabled & mask) {
esp8266::InterruptLock il; // Make the variable sets atomic
wave->gotoTimeLowCycles = timeLowCycles;
wave->gotoTimeHighCycles = timeHighCycles;
} else { // if (!(waveformEnabled & mask))
wave->timeHighCycles = timeHighCycles;
wave->timeLowCycles = timeLowCycles;
wave->gotoTimeHighCycles = wave->timeHighCycles;
wave->gotoTimeLowCycles = wave->timeLowCycles;
// Actually set the pin high or low in the IRQ service to guarantee times
wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1);
waveformToEnable |= mask;
Expand Down Expand Up @@ -263,16 +275,19 @@ static ICACHE_RAM_ATTR void timer1Interrupt() {
} else {
SetGPIO(mask);
}
wave->nextServiceCycle = now + wave->nextTimeHighCycles;
nextEventCycles = min_u32(nextEventCycles, wave->nextTimeHighCycles);
wave->nextServiceCycle = now + wave->timeHighCycles;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is causing a drift in the waveform, because it doesn't consider the time elapsed between the timer firing and the measurement of now (assuming a single waveform).
Instead, consider:
´´´cpp
wave->nextServiceCycle += wave->timeHighCycles;
´´´
which comes from #7057, as pointed out by @dok-net.

nextEventCycles = min_u32(nextEventCycles, wave->timeHighCycles);
} else {
if (i == 16) {
GP16O &= ~1; // GPIO16 write slow as it's RMW
} else {
ClearGPIO(mask);
}
wave->nextServiceCycle = now + wave->nextTimeLowCycles;
nextEventCycles = min_u32(nextEventCycles, wave->nextTimeLowCycles);
wave->nextServiceCycle = now + wave->timeLowCycles;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previosu comment, consider:
´´´cpp
wave->nextServiceCycle += wave->timeHighCycles;
´´´

nextEventCycles = min_u32(nextEventCycles, wave->timeLowCycles);
// Copy over next full-cycle timings
wave->timeHighCycles = wave->gotoTimeHighCycles;
wave->timeLowCycles = wave->gotoTimeLowCycles;
}
} else {
uint32_t deltaCycles = wave->nextServiceCycle - now;
Expand Down
2 changes: 2 additions & 0 deletions cores/esp8266/core_esp8266_waveform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ extern "C" {
// If runtimeUS > 0 then automatically stop it after that many usecs.
// Returns true or false on success or failure.
int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS);
// Same as above, but pass in CPU clock cycles instead of microseconds
int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCycles, uint32_t runTimeCycles);
// Stop a waveform, if any, on the specified pin.
// Returns true or false on success or failure.
int stopWaveform(uint8_t pin);
Expand Down
5 changes: 3 additions & 2 deletions cores/esp8266/core_esp8266_wiring_pwm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "core_esp8266_waveform.h"

extern "C" {
#include "user_interface.h"

static uint32_t analogMap = 0;
static int32_t analogScale = PWMRANGE;
Expand All @@ -50,7 +51,7 @@ extern void __analogWrite(uint8_t pin, int val) {
if (pin > 16) {
return;
}
uint32_t analogPeriod = 1000000L / analogFreq;
uint32_t analogPeriod = (1000000L * system_get_cpu_freq()) / analogFreq;
if (val < 0) {
val = 0;
} else if (val > analogScale) {
Expand All @@ -68,7 +69,7 @@ extern void __analogWrite(uint8_t pin, int val) {
stopWaveform(pin);
digitalWrite(pin, LOW);
} else {
if (startWaveform(pin, high, low, 0)) {
if (startWaveformCycles(pin, high, low, 0)) {
analogMap |= (1 << pin);
}
}
Expand Down