Waveform stopped by runtime limit in iSR doesn't deinit the timer #7236
Conversation
| while (waveformToDisable) { | ||
| /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ | ||
| if (waveformsEnabled & (1UL << pin)) { | ||
| waveformToDisable = 1UL << pin; |
There was a problem hiding this comment.
Everyone can read 1UL << pin, absolutely no need to introduce an object "mask" for this idiom. Use constness if you really feel the need to.
There was a problem hiding this comment.
Removing a named variable like this, used immediately after definition in the code, is not going to affect GCC's generated code unless it's got a really bad hangover. It's for readability by the next maintainer, only.
…t stopWaveform refuses to do anything if the waveform was stopped by runtime, either. Fixes esp8266#7230.
There was a problem hiding this comment.
stopWaveform is in IRAM because it's possible to turn off a waveform via a digitalWrite/etc. from inside an IRQ.
If you are concerned about generated code size, you should move this out of the O2 code block.
Please note that O2 does not always generate larger code and you'd want to try both ways and then pick the smallest.
OTW, I'm fine w/this PR and moving the delay into the if-clause.
| while (waveformToDisable) { | ||
| /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ | ||
| if (waveformsEnabled & (1UL << pin)) { | ||
| waveformToDisable = 1UL << pin; |
There was a problem hiding this comment.
Removing a named variable like this, used immediately after definition in the code, is not going to affect GCC's generated code unless it's got a really bad hangover. It's for readability by the next maintainer, only.
But
stopWaveform()refuses to do anything about it if the waveform was previously stopped by expired runtime from inside ISR, either. Fix for #7230 from original issue reporter.For some reason
stopWaveform()is in IRAM cache and optimized for performance over size.Also,
stopWaveform()would be explicitly called (if indirectly) from user code, which for waveform with a runtime argument can be reasonably expected to be well adjusted in time to avoid race conditions between ISR-based cutoff vs.stopWaveform().Both facts talk back to #7232 (comment), which curiously mentions that 10µs isn't a big thing - for a function that bloats under gcc -O2 and is in IRAM cache :-)