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

Allow waveforms to be specified in clock cycles #7211

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

earlephilhower
Copy link
Collaborator

Allow the PWM to specify sub-microsecond waveform edges, as have been
proposed by @dok-net and me. No other changes intended.

This will increase the linearity at 30 and 40 kHZ PWM rates, but leave
most other things unaffected.

Allow the PWM to specify sub-microsecond waveform edges, as have been
proposed by @dok-net and me.  No other changes intended.

This will increase the linearity at 30 and 40 kHZ PWM rates, but leave
most other things unaffected.
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Does it make sense to bring Servo and Tone to the startWaveformCycles api?

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Apr 13, 2020

I'll throw them in, but it's not going to affect things much. Servos are 20ms periods, so you have 20,000 steps to work with now. Tone is up to 20khz (50us) so it might have some effect.

-edit- Servo's api only allows microseconds or degrees, both as integer. So there's nothing to do there.

@dok-net
Copy link
Contributor

dok-net commented Apr 13, 2020

@earlephilhower @devyte @Tech-TX Let us first consider the physics of CPU cycle resolution vs. µs resolution on the ESP8266 more deeply before possibly wasting time on this.

The TIMER1 can fire at CPU cycle precision, but the ISR has a latency of multi-µs, and a precision of equal or worse than 1µs, even when generating just a single waveform.

BTW, the use of "cycle" is more than mildly confusing here, there's cycle as synonym for a period, there's duty cycle, idle / off cycle, and there's (CPU) clock cycle (a full clock oscillator period) :-) Could we please name any new startWaveform at CPU cycle precision startWaveformClockCycles (unless we agree to my sticking point and there will not be such a function after all)?

My observations are that for long period durations, the CPU cycle precision is achieved due to the TIMER1 resolution, but for duty or off cycles below or near 1µs, it cannot.

On the other hand, attempting to put waveform edges out at CPU cycle resolution, invariably does one performance hurting thing, it increases the number of iterations in the ISR's loop, which contributes to overall system latency and works against the originally intended precision increase.

It would probably be better to align all edges at µs steps (CPU cycle % system_get_cpu_freq() == 0).

@earlephilhower
Copy link
Collaborator Author

The change is a precision vs. accuracy thing. Yes, it's not precise, but it can be relatively accurate with a small (1) number of waveforms. Sending in 12,500ns may not get you exactly 12,500ns to the edge, but it will get you somewhere between what you'd get at 12us and 13us. So you're not artificially quantizing things. For low frequencies, it's a no-op. For 40khz PWM, it can help smooth the jaggies (only 25us per cycle there, so you would only be able to specify 24 different levels from 1-24us otherwise).

Idea is to get the simple, very limited changes in for 2.7 since there is still no consensus on the big changes for 3.0.

@dok-net
Copy link
Contributor

dok-net commented Apr 13, 2020

@earlephilhower To the contrary:
Due to the internal loop latency of the ISR, there is always a quantum, so 12µs, 12500ns, 13µs, in CPU clock cycles, may or may not give the same measurable wall clock time, below or above 12µs, 13µs, in steps of but on average 1µs. The ostensible linearity you see in @Tech-TX pictures is faux and in fact just an artifact of random jitter which masks the real low number of steps.

That said, adding an startWaveformClockCycles() API doesn't hurt, but for now does nothing for the claim that this PR only makes minimal changes to achieve a given goal, because it's not minimal and doesn't achieve the goal.

I've been working hard on final improvements and tunings for the PR #7022 so I don't have hard numbers at hand for this #7211, but if you give it a little time, both @Tech-TX and myself should look into it. Also, I'd hope @s-hadinger will report from LED stripe field tests :-)

@devyte
Copy link
Collaborator

devyte commented Apr 15, 2020

This fixes a bug related to the calculation of the time when edges are supposed to be serviced. The need for it was found experimentally from a linearity graph, and looking at the code it is obvious. The current code imposes an artificial granularity of 1us due to the waveform() arguments. At low frequencies that's ok, but at higher frequencies the calculation error becomes dominant.
This has nothing to do with the inherent overhead between timer IRQ and ISR.
The result of the enhancement has been determined experimentally to be good by looking at the linearity graph of pwm. With one pwm ch @40KHz, the current code shows a staircased graph, while with this fix it is a smooth triangular waveform.
This change is a strict and limited enhancement over current master, and it has been split off from the other PRs due to concerns about other changes in those PRs. That means we know this change is needed, but we're not happy with the experimental results from the other PRs (e.g.: handling of phase). So this goes in now, and investigation into the other concerns continues in the meantime.

@dok-net
Copy link
Contributor

dok-net commented Apr 15, 2020 via email

@dok-net
Copy link
Contributor

dok-net commented Apr 15, 2020 via email

@dok-net
Copy link
Contributor

dok-net commented Apr 16, 2020

Linearity graphs, analogWrite PWM, 40kHz, 1023 steps default:
This is PR7022, commit 166d2a35c24dbe56828229315b0bacee22066bbf:
PR7022_20200416_174633
PR 7211, current branch head:
PR7211_20200416_175427

@earlephilhower
Copy link
Collaborator Author

Thanks for the update, @dok-net! I think we're OK here and can continue in the other PRs to find the best way forward. I believe you've also moved to cycle-level API so I'll merge this and we can then continue on finding the best way forward for 3.0!

@earlephilhower earlephilhower merged commit 1af4ea6 into esp8266:master Apr 16, 2020
@earlephilhower earlephilhower deleted the wvf-to-cycles branch April 16, 2020 21:40
@devyte
Copy link
Collaborator

devyte commented Apr 18, 2020

Per @Tech-TX :
master + #7211 at 80MHz:
image
and same at 160MHz:
image

I think this agrees with @dok-net 's findings above.
So: with this change merged we now have a decent master linearity reference, and we can move on to evaluate other individual pwm enhancements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants