- Notifications
You must be signed in to change notification settings - Fork13.3k
BREAKING - analogWriteRange 8-bit default#7456
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Matching standard Arduino cores, make the default analogWrite() takevalues from 0...255. Users can always use the analogWriteRange() callto change to a different setup.Fixesesp8266#2895
devyte left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This isn't quite right.
- We need to have an implementation of analogWriteResolution(int bits), where bits goes from e.g. 4-10 or whatever, which maps to 0-15 - 0-1023 range. That doesn't mean remove the current analogWriteRange().
- analogWriteRange() needs to have the upper bound checked.
- The init of the default range needs to be changed to be 255 instead of 1023, that part is correct, but...
- We should either have:
- PWMRANGE represent our max capability and have a second define that is the default, e.g.: PWMRANGEDEFAULT
or - have PWMRANGE represent the default of 255 and have a second define that is the max of 1023, e.g.: PWMRANGEMAX
- PWMRANGE doesn't seem to exist in Arduino land, but that's ok
- do we need a res bits getter? I couldn't find anything looking at other core depots
Add a `analogWriteResolution` which takes a number of bits and setsthe range from 0...(1<<bits)-1Remove the PWMRANGE define. It's non-standard and not generally valid(i.e. it's fixed at 1024 of 256, but the real range varies depending onwhat you last set).
earlephilhower commentedJul 26, 2020
DONE
DONE
I suggest a different route: drop the macro. It doesn't track the current state of the machine and it's non-standard.
I say no. The number of bits of PWM resolution is not something that varies over time once set up. No good use case I can see for pulling it out. Would need same thing for range, and what happens when log2(range) has a fractional bit (i.e. |
devyte left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My one concern is that there is no way to tell whether the range/scale setting succeeded from the call itself (no return value), nor is there a getter to check the current value. That's certainly not a problem for cross-core code, because that functionality isn't part of the reference, but still.
I think the docs need to be updated with the min/max values accepted. I can handle that in a separate PR.
Approving.
Also add note about the change and how to fix pre 3.0 applications.
Matching standard Arduino cores, make the default analogWrite() take
values from 0...255. Users can always use the analogWriteRange() call
to change to a different setup.
Fixes#2895