search

PaulStoffregen / OneWire

Library for Dallas/Maxim 1-Wire Chips
http://www.pjrc.com/teensy/td_libs_OneWire.html
579 stars 382 forks source link

Providing changes to align with open-drain character of the w1 bus #8

Closed pstolarz closed 6 years ago

pstolarz commented 8 years ago

The changes are:

  1. Modifications of low level w1 bus activites (read/write bit/byte/bytes) to align with open-drain character of the w1 bus. Providing direct voltage on the open-drain medium (as in the current version) is asking for troubles and may cause damage of some types of slaves. To mimic open-drain output all high GPIO outputs on the data wire are replaced by switching the data GPIO to the input mode and therefore setting the wire high by the pull-up resistor.
  2. write() and write_bytes() handling of the power supply in the parasitic mode is changed accordingly. From now bus in the high state is always provided with the voltage by the pull-up resistor.
  3. Added touch bus activities. If you need more info about this activity look at Maxim's iButton related spec: http://pdfserv.maximintegrated.com/en/an/AN937.pdf
  4. Provided example for DS temperature sensors using new touch functionality. I'm not sure it's really needed but added it to illustrate the new functionality.

NOTE: The changes were tested on the AVR platform (ATmega328) with DS temp. sensors with and w/o parasitic mode. I'd be grateful for more tests on other platforms and slaves.

PaoloP74 commented 8 years ago

.pde extension is no longer used, change to .ino

ghost commented 7 years ago

This is a pretty awesome change, any reason is hasn't been incorporated yet? :)

PaulStoffregen commented 7 years ago

Not yet tested on non-AVR platforms. Very likely breaks them, since this seems to assume AVR's quirky hardware register features.

pstolarz commented 7 years ago

In what point the code "assumes AVR's quirky hardware register features" and will break non-AVR platforms?

ghost commented 7 years ago

Testing it on other platforms might be sensible, sadly I only have an ESP8266 :) But are you sure about quirky hardware register features? Looking at the code changes I'm pretty sure it doesn't change anything at that level..

ghost commented 7 years ago

So how are we gonna get this integrated? :) Should we ask someone to test? I don't have other platforms.. Is there a Arduino twitter or so we could throw this out to? :) Honestly I think the code is good tho, I don't see any changes related to "assumes AVR's quirky hardware register features" :)

PaulStoffregen commented 7 years ago

It must be tested on other boards.

bzub commented 7 years ago

I've used the OneWire library on ESP-12E (nodemcu) with my home "MicroLan" and have had many issues achieving a stable network. After applying this pull request everything works very well without dropping sensors.

In time I can test against Arduino Uno and atmega328p-pu on the same "MicroLan". Please let me know if there are any extra testing steps I should take and report on.

I use three separate OneWire buses to separate branches of the network. Each is roughly 30m in radius and likely 50m in weight. The topology is primarily Stubbed, with some Star like branches. So it's nowhere near ideal, but perhaps good for testing the limits of a OneWire master. Please note that I had issues even testing on a breadboard with short wires without @pstolarz patch.

PaulStoffregen commented 7 years ago

Sorry, I can't help with this. I don't use ESP8266. I'm depending on the ESP community to submit good pull requests.

gunchev commented 7 years ago

What kind of tests and on what boards are required for the change to be verified?

I have Arduino UNO, Mega, Due, Nano, micro pro, pro and some bare AVRs. On UNO/nano I used the master branch plus this change to communicate with 10 DS18B20 sensors with no problem. 5 sensors were logging data to a CSV file for 5 hours (5xDS18B29 - nano - PC).

PaulStoffregen commented 7 years ago

Generally, if the changes are all within the ifdefs for one type of board, it only needs to be verified on that board.

Changes outside the ifdefs should be avoided. By "avoided", you can read as "unlikely to ever be accepted".

pstolarz commented 7 years ago

@PaulStoffregen - thanks for stating it clear after over 1 year since issuing this PR. I think this statement should be expressed on the main page of the project (so called README) to save time other people issuing PRs to this project.

Since this implementation of one-wire is useless (at least for me) w/o this patch, I'm gonna to create my own fork of this project with more sensible support (you can read as "unlikely to wait over 1 year to reject your PR").

You may safely close this PR, as it has no more sense to hang here.

BTW. Thanks for all people for time spent on testing this PR. I appreciate it!

Testato commented 7 years ago

This is not an "ifdef" PR. It is for all board, it is for 328p also.

orgua commented 6 years ago

there is one critical DIRECT_WRITE_HIGH() in write_bit() that powers the bus. All it takes to make the master open drain compatible is to put a DIRECT_MODE_INPUT() in front of it, or replace it accordingly. 200 LOC for this change are crazy. 6 lines with a proper compileswitch are enough. even if you want to change the behaviour on runtime you just had to pass the power-argument from send() and send_bytes() deeper to send_bit() and act on it.