Compatibility and IRQ fixed for waveform/tone/pwm #4872
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a compiler ambiguity introduced with a floating point frequency option for tone(). Thanks to @Rob58329 for discovering this and proposing the fix. Match original analogWrite behavior by going from 0...1023 (PWMRANGE) and not 0...1024, and also explicitly set the analogWrite pin to an OUTPUT. Thanks to @JAndrassy for finding this. Fixes esp8266#4380 discovered by @cranphin where interrupts were disabled on a stopWaveform(). Remove that completely and bracket the update of non-atomic fields in the structure with disable/enable IRQs for safety.
|
@Rob58329, @JAndrassy , @cranphin if you could give this PR a go and report if it corrects the problems you found, I'd be much obliged. Thx! |
d-a-v
reviewed
Jul 1, 2018
cores/esp8266/Tone.cpp
Outdated
| @@ -70,6 +70,12 @@ void tone(uint8_t _pin, double frequency, unsigned long duration) { | |||
| } | |||
|
|
|||
|
|
|||
| // Fix ambiguous tone() binding when adding in a duration | |||
| void tone(uint8_t _pin, int frequency, unsigned long duration) { | |||
| tone(_pin, frequency, duration); | |||
There was a problem hiding this comment.
Correct me, I think this one is calling itself.
There was a problem hiding this comment.
D'oh! I've been away from the keyboard too long. Will fix that.
Explicitly cast the frequency, when passed in as an int, to an unsigned int. Verified with snippet: tone(D1, (int)1000, 500); tone(D1, (unsigned int)1000, 500); tone(D1, 1000.0, 500); tone(D1, (int)1000); tone(D1, (unsigned int)1000); tone(D1, 1000.0);
d-a-v
approved these changes
Jul 2, 2018
|
Eep, looks good to me, I only read the code, so nothing I can test :) |
Rob58329
referenced
this pull request
Jul 3, 2018
Remove and rewrite all the parts of the core/libraries using TIMER1 and consolidate into a single, shared waveform generation interrupt structure. Tone, analogWrite(), Servo all now just call into this shared resource to perform their tasks so are all compatible and can be used simultaneously. This setup enables multiple tones, analogWrites, servos, and stepper motors to be controlled with reasonable accuracy. It uses both TIMER1 and the internal ESP cycle counter to handle timing of waveform edges. TIMER1 is used in non-reload mode and only edges cause interrupts. The interrupt is started and stopped as required, minimizing overhead when these features are not being used. A generic "startWaveform(pin, high-US, low-US, runtime-US)" and "stopWaveform(pin)" allow for further types of interfaces. Minimum high or low period is ~1 us. Add a tone(float) method, useful when working with lower frequencies. Fixes #4321. Fixes 4349.
|
my test run OK |
|
Thanks for the update, all. |
|
@earlephilhower what is PWMRANGE value? |
|
PWMRANGE is the resolution, i.e. 0...1023 means 1024 different intensity levels. Defined in the headers, but you can override with analogWriteResolution(). |
|
But where is it definied? |
|
in Arduino.h |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a compiler ambiguity introduced with a floating point frequency option
for tone(). Thanks to @Rob58329 for discovering this and proposing the
fix.
Match original analogWrite behavior by going from 0...1023 (PWMRANGE) and
not 0...1024, and also explicitly set the analogWrite pin to an OUTPUT.
Thanks to @JAndrassy for finding this.
Fixes #4830 0 discovered by @cranphin where interrupts were disabled on a
stopWaveform(). Remove that completely and bracket the update of non-atomic
fields in the structure with disable/enable IRQs for safety.