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

Why does ow.write with power=0 pull down the bus? #1130

Closed
wuyuanyi135 opened this issue Mar 7, 2016 · 51 comments
Closed

Why does ow.write with power=0 pull down the bus? #1130

wuyuanyi135 opened this issue Mar 7, 2016 · 51 comments

Comments

@wuyuanyi135
Copy link

Hello. I just found some strange behavior of the onewire library.

I was using pin 2 to communicate with a DS18B20. In the documentation,

ow.write(pin, v, power)
power 1 for wire being held high for parasitically powered devices

I connected the power wire, so I set power as zero.

However, when issuing the commands

Reset -> select rom -> write 0xBE -> read -> read

the result was apparently incorrect (4095 degc).

I used my logic analyzer to see the timing, finding that after write 0xBE the bus was not held high. The following bits were therefore unreadable.

In the source, I spotted * line which probably pulled down the bus. I agree after write operation the bus should be at high-z but what is this line for? I think OW bus should always be held high when idle.

void onewire_write(uint8_t pin, uint8_t v, uint8_t power /* = 0 */) {
  uint8_t bitMask;

  for (bitMask = 0x01; bitMask; bitMask <<= 1) {
      onewire_write_bit(pin, (bitMask & v)?1:0);
  }
  if ( !power) {
    noInterrupts();
    DIRECT_MODE_INPUT(pin);
    DIRECT_WRITE_LOW(pin);    // <---------------------------*
    interrupts();
  }
}

desired behavior with power=1
image

when power=0, the bus was pulled down after write 0xbe, which breaks the next two reading commands
image

@wuyuanyi135
Copy link
Author

Anyone ran into the same problem?

@devyte
Copy link

devyte commented Mar 8, 2016

It's possible nobody is using 1-wire at the moment, or that power=0 wasn't well tested, or something else.
I don't know much about 1-wire, so I doubt I can be of much help, but a quick google puts forth some questions for you:
*Are you sure its a DS18B20 and not a DS18B20-PAR?
*Did you read this, and this, both of which agree on the lower-than-recommended pullup res?
*did you try rebuilding the firmware with your suspect line commented out? Did it work?

From what little I've read of the parasitic vs. non-parasitic power modes, I gather that the bit trains are not supposed to be the same. Then again, what do I know :)

@wuyuanyi135
Copy link
Author

Thanks @devyte

  1. I confirm it is DS18B20. Connecting VCC or not does matter.
  2. This was what I concerned. I found that the internal pull up R was enabled (10KΩ) that was weaker than suggested (<4.7KΩ). But I paralleled another 4.7KΩ so the pull up should be strong enough.
  3. I will try it later.

AFAIK, the bus should hold high when idle, since the transmission is initiated by the falling edge. From the logic analyzer result I can tell the protocol is broken because it cannot parse the bits correctly (The red circle should be HIGH. After writing the bus was falsely set to LOW, causing the slave assume this is a bit. The following bits are therefore misaligned).
image

See after DATA 0xbe. The plateau is not recognized as a bit as that in the above figure.
image

This is the timing series from wiki. If the bus does not stay HIGH after write, the next bit will never be recognized by the slave.
image

The actual implementation of parasite powering mode should be like,

ow.setparasitepin(pin_par) --- pin connect to the MOS
ow.write(pin_bus, 0xff, 1) ---write the bus, activating the MOS to power the bus. Certainly the bus should hold high, otherwise it shorts to GND.
ow.write(pin_bus, 0xff, 0) --- write the bus, without activating the MOS, but after write bus should be HIGH, for the sake of next falling edge.

The MOS is referring that in this figure. (Mine is not DS18B20-PAR but it works the same)
image

If we do not even have the gpio that controlling the MOS set, the power argument does not affect much.

@wuyuanyi135
Copy link
Author

Anyway, maybe I did not give the clear statement of the OW bus. I will build the my firmware to see if it helps

@wuyuanyi135
Copy link
Author

After commenting out DIRECT_WRITE_LOW(pin); it works as I expected, leaving power = 0;
I will claim this is a bug. Currently, power=1 actually not make a "parasite mode", but the basic implementation of onewire protocol. power=0 may not work.
I suggest to remove the power argument, and to make the power=1 as default. Otherwise, we may need another API to specify the GPIO to control the MOS to ensure parasite mode making sense.

@devyte
Copy link

devyte commented Mar 9, 2016

Well, that's good news. But then I have to wonder: why was the power argument implemented at all?
The issue you describe is that DQ is brought low after done writing, which means that the next write gets broken. What would happen then if just before the write the line were brought up, and then the write was done? Shouldn't that also fix the issue?

@wuyuanyi135
Copy link
Author

@devyte
I agree that this argument is redundant. Even though you have strong pull up or a MOS that connect the data line and power rail, the line should hold HIGH throughout the operating time (e.g. conversion time, EEPROM R/W time) rather than only for the time of bus writing. In my mind, if the power is set to 1, there should be an interval argument, which specify how long should the bus being hold HIGH, i.e. next R/W operation should be after interval milliseconds.
The DS18 doc read

The 1-Wire bus must be switched to the strong pullup within 10μs (max) after a Convert T[44h] or Copy Scratchpad [48h] command is issued, and the bus must be held high by the pullup for the duration of the conversion (tCONV) or data transfer (tWR = 10ms). No other activity can take place on the 1-Wire bus while the pullup is enabled.

If you have any other opinion please let me know.

What would happen then if just before the write the line were brought up, and then the write was done? Shouldn't that also fix the issue?

You cannot brought up before write. Basically when any operation is done, the bus is at HIGH. If you pull down the bus after write like the code I am reporting, the devices will falsely assume this falling edge leads a bit. E.g. If you are writing two bytes: 0b00000000 and 0b11111111. After finishing the first byte, the bus is set to LOW. If it is shorter than 500us, it will initiate the next writing, a bit 0. When you bring up the level before next write, the first bit has been shiftted. So what the device get may be 0b00000000 and 0b01111111(the last bit is lost)

if you look at
image

After writing 0xbe, the following falling edge makes the analyzer taking it for a bit. Actually, it is not

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

I wonder if the power argument was supposed to indicate if the devices on the bus were externally powered (i.e. all three of VCC, GND and DATA were connected).

@wuyuanyi135
Copy link
Author

@pjsg I think so, but to implement the power mode, we need to do more. Now even though my module is powered externally, with power=0, it doesn't work.

If this argument is meant to be there, I think at least two functionalities should be implemented

  1. optional specifying the GPIO pin controlling the MOS, if we need more current than the 4.7KOM pull up can supply. if power=0, the GPIO will be set to make the MOS enabled.
  2. specifying the waiting time of bus operation. If power=0, the bus will hold HIGH for interval time before next R/W operation is initiated. This sync delay is not acceptable (ms to s). We may need a buffer and a timer to make it async. But I think this is not that important.

My opinion is to remove this argument.

@devyte
Copy link

devyte commented Mar 9, 2016

@wuyuanyi135 what you say makes sense both logically (falling edge) and software-ly :) Even so, I can't help but wonder why the argument was implemented in the first place...
I think the power value was meant to do something that we either haven't figured out, or was broken at some point and nobody noticed.
Consider the calls here:

  if ( !power) {
    noInterrupts();
    DIRECT_MODE_INPUT(pin);
    DIRECT_WRITE_LOW(pin);
    interrupts();
  }

Please correct me if I'm wrong in the following train of thought, I'm still rather new to the ESP, and again, I know next to nothing of 1-wire :p
After the write operation, if power was set to 0 (i.e.: externally powered devices), then... what? the pin is switched to input, then pulled low? What happens in the ESP if a pin is set to input mode? I would think it floats, which is generally not recommended. Therefore it should be pulled. What happens if it is set to logic low in input mode? what would happen if it were set to logic high? And what is the difference between that and skipping the whole thing, i.e.: when power = 1?
This looks a little like a power-saving trick I did ages ago on an ancient 8-bit atmel, except that there after setting to input I wrote a high level instead of a low to avoid having current flow through the pullup.
So, maybe the code is supposed to look like this for power saving reasons, and this is supposed to be done only when externally powered devices are connected?

  if ( !power) {
    noInterrupts();
    DIRECT_MODE_INPUT(pin);
    DIRECT_WRITE_HIGH(pin); //<--
    interrupts();
  }

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

I think that you are probably right. Can you make the changes, verify that they work, and then submit a pull request?

@wuyuanyi135
Copy link
Author

@pjsg Sure. Do you want me to remove the argument?

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

I would think so -- it would effectively become 'true' all the time. You probably want to keep depower()

@wuyuanyi135
Copy link
Author

@devyte thanks :)
If you look at the comment above onewire_write, you may somewhat get the idea why this argument is even here.

//
// Write a byte. The writing code uses the active drivers to raise the
// pin high, if you need power after the write (e.g. DS18S20 in
// parasite power mode) then set 'power' to 1, otherwise the pin will
// go tri-state at the end of the write to avoid heating in a short or
// other mishap.
//

In the onewire_write_bit function, before the bitbang, the output mode is set to OUTPUT. I don't found any configuration of the push mode, so I assume it is push-pull.
During the writing operation, the slave is in INPUT mode (do not pull down the bus), so it doesn't matter. During reading, the slave will pull down the wire, so if it is PP, the GPIO is shorted. They have done some prevention in onewire_read_bit that brings back gpio to input (float) mode before the slave responds.

What you see in the last lines of onewrie_write, I think, because they know when using parasite mode (power=1), the bus will be dangerously hot (:P) since the pull up is bypassed by another strong pull up. But I think using float input (or say open-drain output HIGH) will serve the protection here.

static void onewire_write_bit(uint8_t pin, uint8_t v)
{
    if (v & 1) {
        noInterrupts();
        DIRECT_WRITE_LOW(pin);
        DIRECT_MODE_OUTPUT(pin);    // drive output low
        delayMicroseconds(10);
        DIRECT_WRITE_HIGH(pin); // drive output high
        interrupts();
        delayMicroseconds(55);
    } else {
        noInterrupts();
        DIRECT_WRITE_LOW(pin);
        DIRECT_MODE_OUTPUT(pin);    // drive output low
        delayMicroseconds(65);
        DIRECT_WRITE_HIGH(pin); // drive output high
        interrupts();
        delayMicroseconds(5);
    }
}

static uint8_t onewire_read_bit(uint8_t pin)
{
    uint8_t r;

    noInterrupts();
    DIRECT_MODE_OUTPUT(pin);
    DIRECT_WRITE_LOW(pin);
    delayMicroseconds(3);
    DIRECT_MODE_INPUT(pin); // let pin float, pull up will raise
    delayMicroseconds(10);
    r = DIRECT_READ(pin);
    interrupts();
    delayMicroseconds(53);
    return r;
}

Anyway, I would suggest to set gpio to input state (float) whatever powering mode is used, for the sake of safety.

void onewire_write(uint8_t pin, uint8_t v,) {
  uint8_t bitMask;

  for (bitMask = 0x01; bitMask; bitMask <<= 1) {
      onewire_write_bit(pin, (bitMask & v)?1:0);
  }
    noInterrupts();
    DIRECT_MODE_INPUT(pin);
    interrupts();
}

Actually I also have the question . I used to code the stm32f0/1 mcu, where the gpio pins have more flexible configuration. For example, I can specify the mode to be either push-pull or open-drain. I wonder if we implemented gpio push mode in platform code?
For example, my mcu's gpio is 3.3V, I can set the bus to open-drain, and attach the pull-up R to 5V. I didn't see anything went wrong. Here if the pin is actually pull-push, to 3.3V, If I attach a pull up to 5V, when I output HIGH (3.3V), what will happen? will it flows backward to 3.3V?
Push-pull

image

Open Drain
image

@wuyuanyi135
Copy link
Author

@devyte You are probably right that float mode will prevent the power consuming and safety issue. But I think,DIRECT_MODE_INPUT(pin); should have put the pin in float mode.
But the fact is that, because of the DIRECT_WRITE_LOW(pin), I detected the falling edge. I didn't see the implementation of the platform code, but somehow the level did change in input mode. In input mode, if I set low level, it become 0; if I set high level, is it pushed high (1) or float (Z)? If it is 1, I would say DIRECT_WRITE_HIGH(pin); is not desired.

@wuyuanyi135
Copy link
Author

@pjsg I think, if I remove the argument, many examples using this library will fail to build. Maybe we leave the argument here but do nothing?

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

Someone using an extra argument on the call is fine -- if the C code doesn't look at it, then it doesn't affect anything.

@devyte
Copy link

devyte commented Mar 9, 2016

@wuyuanyi135

If I attach a pull up to 5V, when I output HIGH (3.3V), what will happen?

Basic Ohm's law. The 5V will be connected to the 3.3V through the 4.7K resistor, so: I = V/R = (5-3.3)/4.7K = 0.36mA over the 4.7K res.

If you always set to input mode, I think the pin floats. So in that case, what happens to a parasite device? Won't it be powered by a floating pin? Wouldn't that cause trouble for the parasite device?
I suspect the logic of the original implementer was something like this:
For external power devices, switch pin to input to conserve power and for safety.
For parasite devices, leave pin powered.

I somehow think that having the power argument is necessary, but I can't really prove it without setting up a testbench :p

What do other solutions do for this? I think I read somewhere that arduino also has 1-wire, how do they handle parasite/non-parasite devices?

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

There seem to be a number of possible hardware approaches:

  • Just connect the 1-wire device to the output pin directly and rely on the internal pullup
  • Use an external 4k7 resistor to pullup the 1-wire bus (either to 3v3 or to 5v)
  • Use an external 3v3 or 5v supply to the power pin on the 1-wire device
  • Use an external transistor to perform the strong pullup.

From reading the datasheet for the DS18B20, it isn't clear which of the above options are actually within spec. During the temperature conversion cycle, you need to be able to supply at least 1mA at at least 3V on the 1-wire bus.

Given that the nodemcu is a 3v3 device, it isn't clear that you can get 3V out of a gpio pin. Without a drive transistor, a 4k7 pullup resistor to 5V is not going to be able to source enough current.

The external power supply (i.e. using the 3v3 from the nodemcu) seems the safest.

Maybe decide what configurations ought to be supported and then decide whether they ca all be driven in the same way, or whether there needs to be two options.....

@devyte
Copy link

devyte commented Mar 9, 2016

@pjsg

Maybe decide what configurations ought to be supported and then decide whether they ca all be driven in the same way, or whether there needs to be two options.....

I was thinking sort of the opposite. We're not designing the device, the electrical capabilities of the ESP are fixed, so the supported electrical configurations for 1-wire are also fixed. My line of thought runs along the line of: the 1-wire code was written by someone who had a certain intent, based on the ESP's electrical capabilities. The 1-wire interface must have worked at some point, I somehow don't think that the code was implemented and never tested at all. So what was that intent, and how would it apply to today's version of the code?
I guess what I mean is that just removing the power argument seems to me to limit the permitted electrical capabilities, and the limit is imposed due to software. This is something I always try to avoid.

Just connect the 1-wire device to the output pin directly and rely on the internal pullup

This is the simplest electrical configuration, and it is supposed to work at least for some 1-wire devices. I'm wondering: how would this work if the pin is switched to input? Can a floating pin correctly power a parasite device between RW cycles?

@TerryE
Copy link
Collaborator

TerryE commented Mar 9, 2016

@eyaleb (in #859) @nickandrew and other developers have raised maybe 20 other issues (see this search list) regularly discuss using the DS18B20s, Surely we need the perspective of someone else who has used DS18B20s successfully here.

I have seen data logging examples based on the DS18B20, so people do use them in anger connected to the ESP modules.

@wuyuanyi135
Copy link
Author

@devyte

If I attach a pull up to 5V, when I output HIGH (3.3V), what will happen?
Basic Ohm's law. The 5V will be connected to the 3.3V through the 4.7K resistor, so: I = V/R = (5-3.3)/4.7K = 0.36mA over the 4.7K res.

I asked my EE friend. He said the PUSH arm (the NPN above) will never be activated, i.e. no current from 5V to 3.3V, because Vbe is negative (I didn't remember clearly the equation he said :( )
In other word, the pull-push mode basically is identical to open-drain, expect that pull-push will be shorted, if you output high while the slave pull down the bus. For open-drain, this will never happen.

If you always set to input mode, I think the pin floats. So in that case, what happens to a parasite device? Won't it be powered by a floating pin? Wouldn't that cause trouble for the parasite device?
I suspect the logic of the original implementer was something like this:
For external power devices, switch pin to input to conserve power and for safety.
For parasite devices, leave pin powered.

No. Although some mcu can drive such a great current, I don't think it is a good idea to draw current from GPIO pin. Just like I2C there is a wire and logic on the bus. When you are using floating mode, if you output high, it is at high-z. The power for parasite device is from the pull up R.

What do other solutions do for this? I think I read somewhere that arduino also has 1-wire, how do
they handle parasite/non-parasite devices?

I may look at the library from arduino.

@wuyuanyi135
Copy link
Author

@devyte

This is the simplest electrical configuration, and it is supposed to work at least for some 1-wire devices. I'm wondering: how would this work if the pin is switched to input? Can a floating pin correctly power a parasite device between RW cycles?

I think the gpio in float state is still pulled up using internal pull up R. gpio.mode(pin, mode, pullup)
I never used parasite mode, but I think your question is reasonable. I will try if the current configuration will work if I only use internal pullup, direct connect to DQ, and connect the gnd, while the pin mode switch to input.

@wuyuanyi135
Copy link
Author

@TerryE I will look at if they ran into the similar problem. Basically, if you see anyone reporting the reading like 4095.08125 degc, or calling ow.write(pin, command, 0) will be the same case.
But I skimmed some issues, it seems everyone was using power=1, probably because the ds18b20 example told so.

@pjsg
Copy link
Member

pjsg commented Mar 9, 2016

The datasheet says that, during the conversion, the device may draw up to 1mA and the voltage must not droop below 3V. This says that the regular 4k7 pullup does not work. In fact, the datasheet talks about the strong pullup (as distinct from the regular pullup). The strong pullup can provide the needed power for the conversion.

Drawing 1mA out of a GPIO pin is fine. An LED on a pin draws much more....

@wuyuanyi135
Copy link
Author

@pjsg Yes. What the GPIO can supply should be far more enough than that required by one module. However, the onewire is a bus protocol. I would probably need more than 4 ds18b20 attached to the bus. Probably the voltage drop will be unacceptable.

@wuyuanyi135
Copy link
Author

@devyte I found that this ow driver is likely to be a modified version of this from arduino library. They defined the power argument, too.
Before I submit any change, I believe I have to talk to the developer for the reason why it is being implemented. But I have never found any code using power=0 mode. Probably it has been a convention that nobody even care if their bus is in parasite or external power. I guess power=0 mode is seldom tested.

@devyte
Copy link

devyte commented Mar 9, 2016

@pjsg I completely agree, a gpio pin should be able to supply parasite power to one or maybe 2 devices. From what I've quickly read online, I think that's the whole point of parasite mode: just plug your 1-wire device and go, forget all else. If you need many devices on the same bus, then of course you have to use external power and add the external pullup/drive fet/etc. But for just one device you shouldn't need to do all that.

@wuyuanyi135 I suspected that the implementation was ported from elsewhere. I'm glad you found that out! From a quick look at the link, it seems to be an avr.
In that case, it's possible the original implementation was broken. It's also possible that the electrical characteristics or even the behavior of the original device aren't the same as the ESP, hence the port of that code to the ESP broke parasite mode. E.g.: I once worked with a device that had different input and output buffers for the pins (they weren't called gpio back then, just io pins). If you set the port to input mode, then the input buffer was connected, and the pins were tri-stated. In that case, if you were to do a write to a pin, then the written value would go to the output buffer, which wasn't connected to the pin, so you wouldn't see an effect for the write until you actually switched the port back to output mode. This is just an example, of course, but in such a case, writing a low after setting input mode could make sense.

This discussion is centered around the ds18b20, but the issue is really a more generic one: parasite power mode for 1-wire seems broken on the ESP for all 1-wire devices.

@wuyuanyi135
Copy link
Author

@devyte Exactly. If you are driving only several onewire slaves consuming current within the limit, the GPIO should be configured to directly drive it.
I am not sure if the implementation of nodemcu allowing output high level in input mode (I assumed it was tri-state). It will make sense, if the power argument means "the slaves are directly driven by the GPIO, rather than external pull up R". Then, this argument means, if power=1 (parasite power), the gpio should be at output mode (push-pull) with HIGH output. if power=0 (external power), the gpio should be at tri-state (say input).
Based on this discussion, we may leave the argument untouched. Only need to remove that line pulling down the wire, when power=0.

void onewire_write(uint8_t pin, uint8_t v, uint8_t power /* = 0 */) {
  uint8_t bitMask;

  for (bitMask = 0x01; bitMask; bitMask <<= 1) {
      onewire_write_bit(pin, (bitMask & v)?1:0);
  }
  if ( !power) {
    noInterrupts();
    DIRECT_MODE_INPUT(pin);      // Put gpio in float when external power is used.
    // DIRECT_WRITE_LOW(pin);    // Remove this line.
    interrupts();
  }
}

This would be ideal. We can finally fix it up and keep the integrity of the API: the old example will still work!

wuyuanyi135 added a commit to wuyuanyi135/nodemcu-firmware that referenced this issue Mar 10, 2016
first commit to fix issue nodemcu#1130

If power = 0, after writing the bus is pulled down, and the slave will take it for a bit. One may have problem like the onewire device returning strange response, (for me, ds18b20 sometimes give temperature 4095.8125 degc). This commit will fix the unexpected situation. 

The power argument is not removed. We believe some use this module without external pull up as the documentation specified. The direct push-pull should be able to drive the slave. In that case, we may not switch the pin to input mode for the push arm supplying the power.
@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

This wiring diagram is incorrect in regards to parasitic mode. When in parasitic mode the VCC should be connected to ground. See figure 6 on page 7 of the datasheet.

https://datasheets.maximintegrated.com/en/ds/DS18B20.pdf

@wuyuanyi135
Copy link
Author

@karrots You are right. Thank you

circ

@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

Also, the notes on parasitic power say it may be necessary to drive DQ to Vcc with a MOSFET to maintain voltage during temperature reading. Maybe this is what the power = 1 means?

So power = 1 if using Vdd is connected to Vcc. and power = 0 if using parasitic.

@wuyuanyi135
Copy link
Author

@karrots its supposed to be. However in the OneWire library apparently there is no intend to control that MOSFET to power the bus. That is why we discussed if we should remove this parameter. What 'parasite' means here, is that the device is driven by the push-pull GPIO directly.

If you look at doc, power=0 actually means external power, and power=1 is parasite mode. The difference is, when power=1, the output maintain high after writing to drive the device.
when power=0, after writing the pin become float. If there is no strong pull-up, the slave do not have enough power to operate parasitically

@wuyuanyi135
Copy link
Author

What I conclude is, when you have strong pull-up, power=0. If there is no pull-up, power=1.

@nickandrew
Copy link
Contributor

The library needs to be modified to control (via a 2nd pin) a strong pull-up, e.g. using a MOSFET.

As currently written, the 'power' argument must always be 1 to hold the bus high between commands. I think the code could be modified to tri-state the output pin if 'power==0'; this would be acceptable for designs which use a pullup resistor on the bus.

@nickandrew
Copy link
Contributor

Also, nice work on the diagrams, @wuyuanyi135.

@wuyuanyi135
Copy link
Author

@nickandrew many thanks :)
I agree we should extend the library to enable the 2-pin mode. But in most scenario I think a strong pull-up R is enough.

I think the code could be modified to tri-state the output pin if 'power==0'; this would be acceptable for designs which use a pullup resistor on the bus.

My PR #1143 fixes exactly the problem. when power==0, the gpio is set to LOW for some reason, which breaks the 1wire protocol.

@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

Driving external MOSFET or not as @nickandrew noted the GPIO pin used must be kept high if the sensor is wired for parasitic mode. No MOSFET would be needed if the pin can maintain the voltage. If the sensor is wired in 3 pin mode there is no issue with the DQ pin going low after requesting a conversion. In fact, during the conversion, you can be communicating with other sensors on the bus. Per the app note in the PDF I linked. If the sensors Vdd is connected to the GPIO pin instead of GND the capacitor is likely not correctly charging and discharging thus causing failed reads.

@wuyuanyi135
Copy link
Author

@karrots

If the sensor is wired in 3 pin mode there is no issue with the DQ pin going low after requesting a conversion.

No, the OW bus must maintain high when it is idle. See the timing sequence
power=0
image
power=1
image

The red cycle is what happened when power=0. The bus is set to low and the devices falsely take it for a bit

Even though the slave devices are not drawing current from the bus, the bus can not be pulled down, otherwise if

  • it is pulled down for less than 0.5ms: the next R/W operation is initiated, slave handles one bit, and the protocol breaks (as what happened in the figures above)
  • it is pulled down for greater than 0.5ms: the devices are reset.

@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

Sure the code assumes there is a pull-up resistor in 3 wire mode. If there isn't then the device is not wired to spec.

I just meant that the code didn't have to halt any further communications on the bus during the conversion process.

@wuyuanyi135
Copy link
Author

@karrots No, the code did not have to wait. :)
Probably many people did not wire the onewire slave properly (to spec) for simply test purpose. It was likely that they even did not connect a pull-up on the bus. I think that's why the author set the power argument, in favor of these devices not connecting according to the standard.

@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

Assuming your right about the library being a clone of the Arduino one the comments state the reason for the power parameter and what its intended states are.

// Write a byte. The writing code uses the active drivers to raise the
// pin high, if you need power after the write (e.g. DS18S20 in
// parasite power mode) then set 'power' to 1, otherwise the pin will
// go tri-state at the end of the write to avoid heating in a short or
// other mishap.

@wuyuanyi135
Copy link
Author

@karrots I read this. I could understand the intention but it was the DIRECT_WRITE_LOW(pin); after putting the pin in tri-state that caused the undesired behavior.
I think this issue has been too overwhelming :). We have discussed

  • What happened due to the bug
  • Which line caused the problem
  • What does power argument mean
  • Should we remove power
  • The different scenario of powering mode

It becomes difficult to find a piece of information. Maybe I need to arrange these information on the top later.

@karrots
Copy link
Contributor

karrots commented Mar 10, 2016

I think the power argument is pretty clear based on the notes.

But as you note there is an issue. It probably has to do with the line you have noted not having the exact same meaning on other architectures this code can run on. It may be worth trying PaulStoffregen/OneWire#8 first before modifying the code too much. This looks like it may address your issue by rewriting the underlying code to match the type of bus OW is supposed to be.

@wuyuanyi135
Copy link
Author

@pstolarz pointed out a critical point in this issue:

In arduino,

    DIRECT_MODE_INPUT(pin);      // Put gpio in float when external power is used.
    DIRECT_WRITE_LOW(pin);    // Remove this line.

simply turn off the internal pull-up R of the GPIO.

In nodemcu, the same code turns out that the pin will be connected to GND.

I have confirmed the desired behavior on an Arduino board. This issue is caused by the platform difference! In input mode, what DIRECT_WRITE_LOW(pin); does is not pulling down the bus! Maybe we need check the platform code to keep the integrity for the sake of porting the Arduino code

@wuyuanyi135
I probably solved your issue. It looks like a bug in your nodemcu library not OneWire, so I think this thread should be closed by the admin to not provide others to confusion. Also the discussion should be switched to nodemcu forum.

--- All the remaining notes are nodemcu specific.

When you say, you observed short to the ground when switching gpio to the input it looked suspicious for me (digital input is characterized by high impedance and even with internal pull-down for open-emitter setup you should not observe ground). So, it looked like low level issue with gpio handling. Indeed, look at app/include/driver/onewire.h:

#define DIRECT_READ(pin) (0x1 & GPIO_INPUT_GET(GPIO_ID_PIN(pin_num[pin])))
#define DIRECT_MODE_INPUT(pin) GPIO_DIS_OUTPUT(pin_num[pin])
#define DIRECT_MODE_OUTPUT(pin)
#define DIRECT_WRITE_LOW(pin) (GPIO_OUTPUT_SET(GPIO_ID_PIN(pin_num[pin]), 0))
#define DIRECT_WRITE_HIGH(pin) (GPIO_OUTPUT_SET(GPIO_ID_PIN(pin_num[pin]), 1))

DIRECT_MODE_OUTPUT() does nothing!
DIRECT_MODE_INPUT() disabled output by GPIO_DIS_OUTPUT() defined in sdk/esp_iot_sdk_v1.4.0/include/gpio.h:

#define GPIO_OUTPUT_SET(gpio_no, bit_value) gpio_output_set((bit_value)<<gpio_no, ((~(bit_value))&0x01)<<gpio_no, 1<<gpio_no,0)
#define GPIO_DIS_OUTPUT(gpio_no) gpio_output_set(0,0,0, 1<<gpio_no)
#define GPIO_INPUT_GET(gpio_no) ((gpio_input_get()>>gpio_no)&BIT0)

gpio_output_set() is closed source but it looks like it simply sets gpio state (set/cleared) and it's mode (input/output). So when you issue:

DIRECT_MODE_INPUT(baseReg, bitmask);
DIRECT_WRITE_LOW(baseReg, bitmask);

It switches gpio to the input state but later switches it back as an output shorted to the ground. This is a bug. I recommend to rethink and rewrite this stuff in your nodemcu lib. E.g. look at OneWire lib where the functions have different implementations:

#define DIRECT_READ(base, mask) ((GPI & (mask)) ? 1 : 0) //GPIO_IN_ADDRESS
#define DIRECT_MODE_INPUT(base, mask) (GPE &= ~(mask)) //GPIO_ENABLE_W1TC_ADDRESS
#define DIRECT_MODE_OUTPUT(base, mask) (GPE |= (mask)) //GPIO_ENABLE_W1TS_ADDRESS
#define DIRECT_WRITE_LOW(base, mask) (GPOC = (mask)) //GPIO_OUT_W1TC_ADDRESS
#define DIRECT_WRITE_HIGH(base, mask) (GPOS = (mask)) //GPIO_OUT_W1TS_ADDRESS

Try playing whit this. It looks better at first glance than in nodemcu. I don't have ESP platform to investigate this issue further.

Good luck
Piotrek

@nickandrew
Copy link
Contributor

In NodeMCU, DIRECT_WRITE_LOW() also sets the pin to output mode.

I think it will be more dangerous to change this function than to leave it be.

@devyte
Copy link

devyte commented Mar 11, 2016

Hi all,

this is exactly what I meant: a reason behind the original code, and that's why I didn't want to just remove the power arg. I'm glad it's getting sorted out!

I agree with @wuyuanyi135 comment about keeping the integrity for the sake of easier porting. Fix should probably be in platform.

@nickandrew why dangerous? I would argue that writing a value to a pin should *not * mean setting the pin to output mode. The pin should keep its mode, which in this case is input, and the operation then wouldn't be dangerous. Worst case the value wouldn't physically go out to the pin, so the user would need to explicitly set to output mode first (which is generally the accepted behavior for small mcus anyways).
Or do you mean dangerous in the sense that a lot of current functionality could break and it would take a lot of effort to get it all sorted out?

@nickandrew
Copy link
Contributor

@devyte The latter; changing the semantics of DIRECT_WRITE_LOW() can have effects across the codebase.

@karrots
Copy link
Contributor

karrots commented Apr 27, 2016

My last comment should have said PaulStoffregen/OneWire#8 instead of PaulStoffregen/OneWire#11. Unsure if these changes match with the ESP8266 architecture.

@pstolarz
Copy link

pstolarz commented Apr 27, 2016

As I wrote in other place cited earlier in this thread - I'd rather suggest to rethink and rewrite low level w1 bus activities of your platform and its alignment with ported ardino w1 lib. This issue is only one sample of problems you may encounter with w1 in the future. Removing the line as @wuyuanyi135 proposed is not a cure in my opinion.

@karrots
Copy link
Contributor

karrots commented Feb 12, 2017

@wuyuanyi135 did this merge fix your issue? If so this issue can be closed.

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

No branches or pull requests

7 participants