Skip to content

Keep PWM phases constant #7057

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 7 commits into from
Closed

Keep PWM phases constant #7057

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f2c4e9
Make sure PWM phases remain constant
s-hadinger Feb 2, 2020
64c6f04
Remove the entire waveform overshoot compensation
Feb 26, 2020
7f821ce
Avoid thrashing
s-hadinger Mar 3, 2020
d7852d2
Merge branch 'master' of https://github.com/esp8266/Arduino into patch-1
s-hadinger Apr 8, 2020
5db5df6
Better handling of interrupt storm during wifi connection
s-hadinger Apr 8, 2020
4f4b504
Better handling of interrupt storm during wifi connection
s-hadinger Apr 8, 2020
c70e835
Merge branch 'master' of https://github.com/esp8266/Arduino into patch-1
s-hadinger May 6, 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
9 changes: 5 additions & 4 deletions cores/esp8266/core_esp8266_waveform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,23 +256,24 @@ static ICACHE_RAM_ATTR void timer1Interrupt() {
// Check for toggles
int32_t cyclesToGo = wave->nextServiceCycle - now;
if (cyclesToGo < 0) {
cyclesToGo = -((-cyclesToGo) % (wave->nextTimeHighCycles + wave->nextTimeLowCycles));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Logically, I get what you're trying to here. But it seems like this is a no-op unless we have overshot by an entire waveform cycle, no? If it's been less than a waveform cycle then the mod will be a no-op. The mod operator is a relatively expensive one, might consider just ignoring the case for code speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this should be a no-op most of the time. It's a safeguard because if we overshoot one or multiple wafeforms, the PWM will oscillate at 500KHz (1ms up + 1ms down) until the required number of cycles is reached. I don't know if it's even possible, or if 500KHz for a very short amount of time is an issue at all.
We could also add a while (-cyclesToGo > wave->nextTimeHighCycles + wave->nextTimeLowCycles) { cyclesToGo += wave->nextTimeHighCycles + wave->nextTimeLowCycles)}; but that would also add code size to the precious IRAM.

I'm pretty sure this would never occur with PWM, only if someone sets a very short waveform. You can probably drop this for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you comment it out and make a note in the code about why we're skipping, so if something pops up later we can see the reasoning behind the choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this line of code for nom but leaved the two variants in comments.

waveformState ^= mask;
if (waveformState & mask) {
if (i == 16) {
GP16O |= 1; // GPIO16 write slow as it's RMW
} else {
SetGPIO(mask);
}
wave->nextServiceCycle = now + wave->nextTimeHighCycles;
nextEventCycles = min_u32(nextEventCycles, wave->nextTimeHighCycles);
wave->nextServiceCycle = now + wave->nextTimeHighCycles + cyclesToGo;
nextEventCycles = min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What this and the other branch seem to be doing is to be preserving the waveform phase at expense of period jitter.

Could it be that the pulsing you're seeing on some LED boards is due to periods of some pins slightly increasing (when the total on-time is so small that a few 10s of 80mhz instructions would actually increase the %on-time appreciably)? And this, by introducing a feedback on the period makes things average out by continually adding jitter to the last-checked pin so that over large amounts of time the cyclesToGo overrun is subtracted off of the hi/low time?

I think it's a fair thing to do, given that when cyclesToGo < 0 then bad stuff has already happened, actually. Feels like a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, if cyclesToGo is already significantly negative, then we already missed the right timing for the coming edge. Either we keep timing of the next edge, at the expense of phase shifting (original code), or we compensate the missed timing and shorten the next portion to keep phase stable (new version).

Missing one cycle is not visible, whereas shifting the phase creates a visible artefact.

Also min_u32(wave->nextTimeHighCycles + cyclesToGo, 1) is to make sure that we trigger the change at the next microsecond. My first version without the min_u32 resulted in missing an edge at some rare occasion and creating flashes. It looks safe with this now.

Copy link
Contributor

@dok-net dok-net Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry to interfere, but from my work on the concurring PR #7022 I believe recomputing the nextServiceCycle based on now, instead of incrementally, is at the heart of the problem. I am also not even giving the Timer1 a head start anymore, because any delay until a specific pin is reached should in general remain the same for each transition.
So, in the context of this here PR, what happens if you don't do the cyclesToGo compensation you are implementing, but instead of
wave->nextServiceCycle = now + wave->nextTimeHighCycles;
use
wave->nextServiceCycle += wave->nextTimeHighCycles;
? (Same for low cycle)
Never mind cycle timings that are too short to run reliably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will try your PR #7022 not before 2-3 days.
@dok-net IRAM is a very precious resource, I realized your patch is actually smaller in IRAM that the current code with my patch. So that's a super good news.

Copy link
Contributor

Choose a reason for hiding this comment

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

@s-hadinger I'm sorry, though mitigated by fix and feature, the IRAM size is actually larger in my PR, from master

IROM   *: 228428          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27400   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1252  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 680   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25168 )         - zeroed variables      (global, static) in RAM\HEAP

to PR #7022

IROM   *: 228460          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27492   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1252  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 680   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25240 )         - zeroed variables      (global, static) in RAM\HEAP

A prominent fix is that 100% duty or off cycle now work without pulsating on the output.
A specified duration now also leaves the output at the state it is at that moment, so no more force to off when stopped during duty cycle. One can always force thereafter via digitalWrite(pin, <state>), after all. One might call this a breaking change, I consider it a bug fix.
Harder to fix without further increasing RAM usage is maintaining phase when duty cycle changes, but period not: (duty_before+off_before) == (duty_new + off_new). Is this something you consider a requirement?

Copy link
Contributor

Choose a reason for hiding this comment

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

@s-hadinger I think this PR should not be merged, on the grounds that

min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));

is near 100% of the time 1, but otherwise always 0.
A quick inspection seems to indicate that this causes another forced loop-iteration, executing

       } else {
          uint32_t deltaCycles = wave->nextServiceCycle - now;
          nextEventCycles = min_u32(nextEventCycles, deltaCycles);
        }

which more or less revokes the effect of the change in this PR a few CPU cycles later.
So whatever is observed as an improvement is a side-effect of iterating inside the NMI handler.
Am I seriously missing something here headscratch?

Copy link
Collaborator

@earlephilhower earlephilhower Feb 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A prominent fix is that 100% duty or off cycle now work without pulsating on the output.

Err, how would a 100% duty cycle actually get into this section of code, though? The Tone is by definition a 50% cycle and analogWrite checks for 100% duty cycle and just does digitalWrites to avoid any CPU use at all.

A specified duration now also leaves the output at the state it is at that moment, so no more force to off when stopped during duty cycle. One can always force thereafter via digitalWrite(pin, <state>), after all. One might call this a breaking change, I consider it a bug fix.

That would be a breaking change. The original code special-cased it and there are probably HW installations out there depending on this behavior. A 3.0 tweak, not a 2.x one.

Harder to fix without further increasing RAM usage is maintaining phase when duty cycle changes, but period not: (duty_before+off_before) == (duty_new + off_new). Is this something you consider a requirement?

The API specifies a frequency and has no guarantees about phase relationships. You can't know what phases there are, in general, since the app code is running async to the timer...

min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));

As @dok-net says, this does always equal 0 or 1. I think you meant 1 to be microsecondsToClockCycles(1). However, that still means it hits the NMI at 1us and not only on edges which will eat CPU like mad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I know is that min_u32(nextEventCycles, wave->nextTimeHighCycles + cyclesToGo); does not work, because can introduce a negative number causing an edge transition to be skipped. So there's maybe a more elegant way to force it to be non-negative.

Copy link
Contributor

Choose a reason for hiding this comment

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

@s-hadinger a much reworked version is in #7022 now. Changing duty cycle maintains phase if period duration remains the same. Runtime is honored precisely, not only on next incidental timer event. Overall stability, correctness and performance fixes, "works for me" (buzzer, servo, PWM PC fan control). I think that's good for a start.
I don't have measuring equipment to graph timings etc, so basically, my observations that the PR is a major improvement for my use cases is all I can provide in the way of proofing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@earlephilhower

Err, how would a 100% duty cycle actually get into this section of code, though?

First, in the case of direct use of waveform :-) Second, loss of phase for analogWrite()ing min or max values seemed unacceptable to me (magnitude: bug), this here PR seems to confirm that sentiment. I think an explicit from-sketch digitalWrite() to fully stop the PWM generator is correct use.

That would be a breaking change. The original code special-cased it and there are probably HW installations out there depending on this behavior.

Hem. stopWaveform() in master just removes the pin from signal generator, effectively, asynchronously, leaving the pin in a random state, right? OTOH, startWaveform() to set a finish time to an already running waveform cancels the generation at an unpredictable time in master, but is well defined in PR #7022 - at least per my objective. In master, this has nasty effects like in PR #7084 whenever the duty cycle is cut short, for instance. If the expectation is that startWaveform() with a running time is always used on first call for a given inactive pin, and the running time is a multiple of period, then there is no functional change between master and #7022.
Is there a need to stay compatible to broken uses?

} 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->nextTimeLowCycles + cyclesToGo;
nextEventCycles = min_u32(nextEventCycles, min_u32(wave->nextTimeLowCycles + cyclesToGo, 1));
}
} else {
uint32_t deltaCycles = wave->nextServiceCycle - now;
Expand Down