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

fix espressif DigitalInOut open-drain #5866

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

dhalbert
Copy link
Collaborator

Open-drain and output both need to be set. Discovered while researching #5865, which is not yet fixed.

Tested with a Saleae and a QT Py ESP32-S2.

@Neradoc
Copy link

Neradoc commented Jan 16, 2022

I tested the PR with a DS18B20 on a Feather S2 and it's detected and gives me temperatures, while latest doesn't. ( #3822 )

@dhalbert
Copy link
Collaborator Author

I tested the PR with a DS18B20 on a Feather S2 and it's detected and gives me temperatures, while latest doesn't. ( #3822 )

That's good! I have a DS18B20 wired up and it's still not working for me, on the QT Py ESP32-S2.

@Neradoc
Copy link

Neradoc commented Jan 16, 2022

My experience with you and I having different results is that tinyuf2 is usually the difference and...

Installing Circuitpython (from the PR) with esptool on the QTPY ESP32S2, the sensor is not detected.
Reinstalling tinyuf2 (latest release) and CP from the PR, the sensor is detected and reports temperatures.
I did a couple of back and forth with consistent results.

I don't know if it's related to the open-drain issue or belongs more to the OneWire issue.

@dhalbert
Copy link
Collaborator Author

Before this PR fix, I saw no signal on the onewire pin with my Saleae, and simple manipulation of the output with a DigitalInOut and open-drain drive did not work. With the .bin from this fix, I do see the signal, but the timings are a bit off (too long). The Saleae display program says that the waveform is in error, and the sensor does not respond.

I'll try the UF2, and see whether the timings are different. Something that TinyUF2 is doing may be changing the clock handling in some way. It might show up in frequency differences for other things, too.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Jan 16, 2022

I tried the UF2 version, and it does not work for me on my QT Py setup, but it gets a lot further, usually, and eventually gets a CRC error. Below is a screenshot showing the two. The top is the UF2 version: the SEARCH ROM command works, and there's a lot more waveform to the right. The bottom is the BIN version. It has a longer delay before the SEARCH ROM (1m vs about 560us), and you can see the SEARCH ROM pulse timings seem rather messed up.

I looked briefly at TinyUF2, and don't immediately see what setup it might be doing that the BIN version is not, but there must be something. I'll inquire further about this. Thanks for thinking to try the UF2 vs the BIN!

onewire-esp32-comparison

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/api-reference/peripherals/gpio.html#_CPPv411gpio_mode_t Is it possible that CircuitPython should be setting GPIO_MODE_INPUT_OUTPUT_OD = ((GPIO_MODE_DEF_INPUT) | (GPIO_MODE_DEF_OUTPUT) | (GPIO_MODE_DEF_OD)) instead? If the state of the pin needs to be read back in, the DEF_INPUT bit may need to be set.

The docs also read a bit like the DEF macro should not be used directly but the MODE enums instead..

@dhalbert
Copy link
Collaborator Author

If the state of the pin needs to be read back in, the DEF_INPUT bit may need to be set.

I was able to print the output value of the pin from the REPL without DEF_INPUT being set. Hmm. The shared-module code for bitbang OneWire does not assume that the pin can be bidirectional: it flips the direction back and forth to write and read.

I agree with you about the macros: I didn't read the doc carefully enough to realize those constants were available.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks, it's clearly a distinct improvement even if it might need to be tweaked again in the future.

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.

Open Drain outputs don't work (also breaks bitbangio.I2C)
3 participants