Enable pull-up resistor in OneWire library #1226
Conversation
|
@ADiea @avr39-ripe Can you check this PR? |
| @@ -124,7 +124,7 @@ OneWire::OneWire(uint8_t pin) | |||
|
|
|||
| void OneWire::begin() | |||
| { | |||
| pinMode(pin, INPUT); | |||
| pinMode(pin, INPUT_PULLUP); | |||
| noPullup(pin); | |||
There was a problem hiding this comment.
This looks like it will disable the pullup. To me this looks put here on purpose maybe the Arduino way is not to enable pullups?. Also be aware that if you enable pullups and also have pullups they will go in parallel making the equivalent smaller in value that together with line capacitance might not be appropiate for given sck frequency.
I would not touch lib code if it is working an tested as it probably is consistent with Arduino way. After onewire::begin you can manually set the pins to pullup in your client code.
|
I am not sure what is the default Arduino behavior here but I guess the default way is to disable pullups because there is a specifically disable pullups in the begin(). I would not enable them by default to not interfere with already present external pullups. If your application requires pullups internally I would advise of setting sda/sck pullups after ::begin() function is called. This will ensure everyone else is not affected by the change :) |
| @@ -112,7 +112,7 @@ | |||
| #define IO_REG_TYPE uint16_t | |||
| #define IO_REG_ASM | |||
| #define DIRECT_READ(base, mask) (digitalRead(mask) ? 1 : 0) | |||
| #define DIRECT_MODE_INPUT(base, mask) pinMode(mask, INPUT) | |||
| #define DIRECT_MODE_INPUT(base, mask) pinMode(mask, INPUT_PULLUP) | |||
There was a problem hiding this comment.
This is the really interesting change
|
Hi @ADiea, |
|
Ok, I've read up on the topic and I agree now that it's probably not good to change it unconditionally:
However, it works pretty well with the internal pull-up if the sensor is close to the controller, therefore I'd like to change the PR to make it configurable. Is that something you'd merge? |
|
Please understand, sometimes You need strong pullup with external
transistor, so dont force it in library.
Maybe add it as parameter in constructor.
21.08.2017 12:04 PM "Max Weller" <[email protected]> napisał(a):
… Hi @ADiea <https://github.com/adiea>,
it is not possible to do it only in my application, without changing the
library. The really important change is the one I highlighted above. The
OneWire protocol is implemented by the library switching between Input and
Output Mode all the time. If I would just change it to INPUT_PULLUP after
my call to Onewire::begin(), it would be overwritten in no time. The change
in the begin function is not important, the change in the headers is.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1226 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHOUx-QcCJ-5G36rmgfD439NlFsNsOcks5saVYmgaJpZM4O8p26>
.
|
|
Thinking more about this, I don't see a problem with just enabling the internal pull-ups all the time. If a "stronger" pull-up is needed in an application, we can just put an external pull-up resistor. It will be in parallel to the internal pull-up, lowering the resistance, which should not hurt. Therefore I think it would help in many applications where the sensor is close to the ESP, making the external pull-up unnecessary, while being no problem in the applications where an external pullup is used. |
mikee47
force-pushed
the
develop
branch
2 times, most recently
from
7b19c1b to
295a5f3
Compare
Enable the internal pull-up resistor for the GPIO used by OneWire library. OneWire requires by its design a pull-up resistor, and if the internal one is not enabled an external resistor would be required.