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 to set pin to OUTPUT_OPEN_DRAIN in analogWriteMode #7841

Merged
merged 5 commits into from
Jan 27, 2021

Conversation

klugem
Copy link
Contributor

@klugem klugem commented Jan 25, 2021

I added an optional parameter in the analogWrite() function to allow open-drain PWM signals.
Also see #7836

edit: closes #7836

@earlephilhower
Copy link
Collaborator

The problem w/this PR is that Arduino.h has all its functions as extern "C" which means things like different signatures and default values are not going to compile properly.

Maybe an analogWriteEx(pin, val, setDrive) and a corresponding digitalWriteEx(pin, val, setDrive) or something similar would be a better way forward?

@klugem
Copy link
Contributor Author

klugem commented Jan 25, 2021

The problem w/this PR is that Arduino.h has all its functions as extern "C" which means things like different signatures and default values are not going to compile properly.

Thanks, I just noticed that.

Maybe an analogWriteEx(pin, val, setDrive)

I named it analogWriteMode() but we can rename it to analogWriteEx()

and a corresponding digitalWriteEx(pin, val, setDrive)

I think digitalWrite() does not set pin mode to OUTPUT or does it?

@earlephilhower
Copy link
Collaborator

Maybe an analogWriteEx(pin, val, setDrive)
I named it analogWriteMode() but we can rename it to analogWriteEx()

No problem, your name makes sense.

and a corresponding digitalWriteEx(pin, val, setDrive)
I think digitalWrite() does not set pin mode to OUTPUT or does it?

That's correct, my mistake. I just checked upstream, too, and that's the Arduino way.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM, thx! Need other maintainer to give 👍 to merge since it does add a new API.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

LGTM

@d-a-v d-a-v changed the title allow to set pin to OUTPUT_OPEN_DRAIN in analogWrite allow to set pin to OUTPUT_OPEN_DRAIN in analogWriteMode Jan 26, 2021
@klugem
Copy link
Contributor Author

klugem commented Jan 26, 2021

I just saw that my editor removed a lot of whitespaces in doc/reference.rst and re-added these whitespaces with the last commit... -.-

@earlephilhower
Copy link
Collaborator

@klugem you got the spaces thing fixed, but there's now a regression of a submodule, SoftwareSerial.

Easiest thing to do is go to the libraries/SWSerial directory and git checkout 27477f7 and then go up to the main git repo and redo the git commit -a to match it to current master. Or, if your git-fu is strong, just remove the SWSerial reference in the patch...

@klugem
Copy link
Contributor Author

klugem commented Jan 26, 2021

This was caused because I clicked on "Update branch"...I reverted this commit.

@earlephilhower earlephilhower merged commit f2d83ba into esp8266:master Jan 27, 2021
@dok-net
Copy link
Contributor

dok-net commented May 1, 2021

@earlephilhower This commit is not OK, despite reviews. What is the intended function on an "open" analogWrite? You can't switch the mode in that case due to the "else":

if (analogMap & 1UL << pin) {
  // Per the Arduino docs at https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/
  // val: the duty cycle: between 0 (always off) and 255 (always on).
  // So if val = 0 we have digitalWrite(LOW), if we have val==range we have digitalWrite(HIGH)
 
    analogMap &= ~(1 << pin);
  }
  else {
    if(openDrain) {
      pinMode(pin, OUTPUT_OPEN_DRAIN);
    } else {
      pinMode(pin, OUTPUT);
    }
  }

Plus, the comment about on and off has drifted far from anything that it might apply to :-)

@dok-net
Copy link
Contributor

dok-net commented May 1, 2021

Give me some hint as to how this should behave and I'll PR a fix.
Edit: I'm sorry, I am not going to give myself another headache and build merge conflicts, so the fix is in PR #8008 / #8011 now.

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.

wiring analogWrite() function does not respect previously set pinMode()
4 participants