allow to set pin to OUTPUT_OPEN_DRAIN in analogWriteMode by klugem · Pull Request #7841 · esp8266/Arduino (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation10 Commits5 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

klugem

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

edit: closes #7836

Michael Kluge added 2 commits

January 25, 2021 20:40

@earlephilhower

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

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

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.

earlephilhower

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.

d-a-v

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 titleallow to set pin to OUTPUT_OPEN_DRAIN in analogWrite allow to set pin to OUTPUT_OPEN_DRAIN in analogWriteMode

Jan 26, 2021

@klugem

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

@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

@klugem

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

@earlephilhower

@dok-net

@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

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.