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

BREAKING - analogWriteRange 8-bit default #7456

Merged
merged 8 commits into from
Jul 29, 2020

Conversation

earlephilhower
Copy link
Collaborator

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

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 esp8266#2895
@earlephilhower earlephilhower added this to the 3.0.0 milestone Jul 14, 2020
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.

This isn't quite right.

  1. 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().
  2. analogWriteRange() needs to have the upper bound checked.
  3. The init of the default range needs to be changed to be 255 instead of 1023, that part is correct, but...
  4. 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
  1. PWMRANGE doesn't seem to exist in Arduino land, but that's ok
  2. 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 sets
the range from 0...(1<<bits)-1

Remove 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 on
what you last set).
@earlephilhower
Copy link
Collaborator Author

1. 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().

DONE

2. analogWriteRange() needs to have the upper bound checked.

DONE

3. The init of the default range needs to be changed to be 255 instead of 1023, that part is correct, but...
4. 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
1. PWMRANGE doesn't seem to exist in Arduino land, but that's ok

I suggest a different route: drop the macro. It doesn't track the current state of the machine and it's non-standard.

2. do we need a res bits getter? I couldn't find anything looking at other core depots

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. analogWriteRange(1000);?

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.

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.

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

Successfully merging this pull request may close these issues.

analogWrite is not compliant with standard Arduino API
3 participants