Fix GPIO16 Board Reset; Updated Usage by sommersoft · Pull Request #780 · adafruit/circuitpython

sommersoft · 2018-04-26T01:43:56Z

See Issue #753.

No longer causes a reset when creating the digitalio object. Also updated some of the usage, since a few functions only included normal gpio functions (gpio16 is controlled through RTC register, not GPIO register). Tested both simple output and input usage; both work as expected.

Note to Reviewer(s): PLEASE pay extra attention to common_hal_digitalio_digitalinout_set_value. I'm not totally sure I implemented the conditionals correct with respect to open_drain and else.

…in reset_pins

jerryneedell · 2018-04-26T02:10:58Z

FYI - tested with OLED Featherwing on Feather Huzzah ESP8266 and button B works!

>>> import board
>>> import digitalio
>>> button = digitalio.DigitalInOut(board.GPIO16)
True
>>> button.value    PRESSED BUTTON HERE
False
>>>

tannewt

Awesome work on this! Just a few sanity checks

tannewt · 2018-04-26T06:17:29Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

+        WRITE_PERI_REG(PAD_XPD_DCDC_CONF, (READ_PERI_REG(PAD_XPD_DCDC_CONF) & 0xffffffbc) | 1); 	// mux configuration for XPD_DCDC and rtc_gpio0 connection
+        WRITE_PERI_REG(RTC_GPIO_CONF, READ_PERI_REG(RTC_GPIO_CONF) & ~1);	//mux configuration for out enable
+        WRITE_PERI_REG(RTC_GPIO_ENABLE, READ_PERI_REG(RTC_GPIO_ENABLE) & ~1);	//out disable
+        gpio16_in_use = true;


Mind factoring gpio16_in_use related code into claim_pin and reset_pin like the SAMD port does? https://github.com/adafruit/circuitpython/blob/master/ports/atmel-samd/common-hal/microcontroller/Pin.c#L99

I can certainly try...

Should I also roll adc_in_use into this? Hardly takes any additional effort.

tannewt · 2018-04-26T06:21:30Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

 void common_hal_digitalio_digitalinout_set_value(
        digitalio_digitalinout_obj_t* self, bool value) {
+    if (self->pin->gpio_number == 16) {
+        if (self->open_drain) {


I think this should be self->open_drain && value since you want to be outputting a zero in open drain.

I originally had it that way, but I wasn't sure how/if open_drain && !value would get handled. My theoretical line of thinking was related to set_drive_mode using set_value. Does it need to throw an exception in that case?

So, belay that. I must have confused myself on that. set_drive_mode will only call set_value if the current value is true. I'll update the condition.

tannewt · 2018-04-26T06:22:29Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

-        if (self->open_drain && ((*PIN_DIR) & pin_mask) == 0) {
-            return true;
+        if (self->pin->gpio_number == 16) {
+            return READ_PERI_REG(RTC_GPIO_OUT) & 1;


Does this handle open drain 1 correctly?

With fixing open_drain above, I see why you're asking. And no, it won't, since RTC_GPIO_OUT will return 0.

tannewt · 2018-04-26T06:23:52Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

-            "Pin does not support pull."));
+        // PULL_DOWN is the only hardware pull direction available on GPIO16
+        // since we don't support pull down, just return without attempting
+        // to set pull (which won't work anyway).


Why not throw the exception?

I can add if (pull != PULL_NONE) and throw the exception. I just wanted to remove the confusion when swith_to_input called this, and tossed the exception out even though the pin has been switched to input by the time this is called.

sommersoft · 2018-04-27T02:59:28Z

Open drain update: I think I have the get_value reporting correctly. Tested with a LED hooked up with external pull up:

3V -> LED+  |  LED- -> pull up -> GPIO16

gpio.value = false = LED On
gpio.value = true = LED Off

jerryneedell · 2018-04-27T10:49:57Z

@sommersoft I tried this on GPIO16 but I don't get any response from the LED if I use OPEN_DRAIN. with PUSH_PULL True is ON , False is OFF.

>>> led.drive_mode=digitalio.DriveMode.PUSH_PULL
>>> led.value=False     LED OFF
>>> led.value=True       LED ON
>>> led.value=False      LED OFF
>>> led.drive_mode=digitalio.DriveMode.OPEN_DRAIN
>>> led.value=False     LED OFF
>>> led.value=True       LED OFF
>>> led.drive_mode=digitalio.DriveMode.PUSH_PULL
>>> led.value=False       LED OFF
>>>

DO I need to do something differently?

Here is the completes setup

Adafruit CircuitPython 3.0.0-alpha.6-4-g4e1f7d4-dirty on 2018-04-27; ESP module with ESP8266
>>>
>>>
>>> import board,digitalio
>>> led=digitalio.DigitalInOut(board.GPIO16)
>>> led.switch_to_output()
>>> led.drive_mode
digitalio.DriveMode.PUSH_PULL
>>> led.value=True       LED ON
>>> led.value=False      LED OFF
>>> led.drive_mode=digitalio.DriveMode.OPEN_DRAIN
>>> led.value=True        LED OFF
>>> led.value=False       LED OFF
>>> led.drive_mode=digitalio.DriveMode.PUSH_PULL
>>> led.value=True        LED ON
>>> led.value=False      LED OFF

jerryneedell · 2018-04-27T11:07:48Z

Doh! Nevermind -- connencting 220 ohm resistor to +3 and cathode to GPIO16 works as you described.

led.value=True LED OFF
led.value=False LED ON

for both PUSH_PULL and OPEN DRAIN

tannewt · 2018-04-27T16:54:07Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

-        if (self->open_drain && ((*PIN_DIR) & pin_mask) == 0) {
-            return true;
+        if (self->pin->gpio_number == 16) {
+            if (self->open_drain && (READ_PERI_REG(RTC_GPIO_OUT) | READ_PERI_REG(RTC_GPIO_ENABLE)) == 0) {


Mind removing the bitwise OR in favor of boolean logic? That'll make it easier to read.

tannewt · 2018-04-27T16:54:46Z

ports/esp8266/common-hal/digitalio/DigitalInOut.c

+                WRITE_PERI_REG(PAD_XPD_DCDC_CONF, (READ_PERI_REG(PAD_XPD_DCDC_CONF) & 0xffffffbc) | 1);
+                WRITE_PERI_REG(RTC_GPIO_CONF, READ_PERI_REG(RTC_GPIO_CONF) & ~1);
+                WRITE_PERI_REG(RTC_GPIO_ENABLE, (READ_PERI_REG(RTC_GPIO_ENABLE) & ~1)); // input
+                WRITE_PERI_REG(RTC_GPIO_OUT, (READ_PERI_REG(RTC_GPIO_OUT) & ~1)); // out=0


Can OUT be 1? It'd simplify reading the value.

This block can be indented one less too.

How did that extra indenture sneak in there?... 🤷‍♂️ 😄

Can OUT be 1? It'd simplify reading the value.

I'm not sure, but I can try it. I want to check if it's used to set the pull mode for input, though. Could easily assume that it is config'd in RTC_GPIO_CONF, but assumptions ya know. I'm rolling through the Arduino esp8266 repo to see how they implemented pull down.

After reading through Arduino's handling of GPIO16, I'm confident they use one of the CONF registers. Couldn't find where the calls go from pinMode, but they are definitely shifting values in a full byte. I just tested this with setting OUT & 1 and the same LED test, and didn't get any different results. Also tested switching to 'PUSH_PULL'; 5x5. Will update get_value accordingly.

jepler · 2018-04-30T02:25:25Z

I also tested at 3.0.0-alpha.6-5-g396e4ffc3 and "GPIO16" now works for me as an input. Thanks @sommersoft !

Besides testing as an input, I also tested this:

>>> a = digitalio.DigitalInOut(board.GPIO16)
>>> b = digitalio.DigitalInOut(board.GPIO16)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Pin XPD_DCDC in use```

This is clearly what the code intends but I have to admit that I'm not sure why.

tannewt

Awesome! Thank you so much!

sommersoft added 3 commits April 25, 2018 03:57

now checks for proper pin in is_pin_free; initialize GPIO16 as input …

e70ece4

…in reset_pins

updated GPIO16 construct and handling

bd5a5da

reset GPIO16 to input on deinit

441ce2a

tannewt added bug esp8266 crash labels Apr 26, 2018

tannewt added this to the 3.0 milestone Apr 26, 2018

tannewt requested changes Apr 26, 2018

View reviewed changes

added exception for PULL_UP; corrected open_drain handling

4e1f7d4

tannewt reviewed Apr 27, 2018

View reviewed changes

implemented 'claim_pin' methodology; updated 'open_drain' handling

396e4ff

tannewt approved these changes Apr 30, 2018

View reviewed changes

tannewt merged commit 6855541 into adafruit:master Apr 30, 2018

tannewt mentioned this pull request Apr 30, 2018

Huzzah GPIO16 crash #753

Closed

sommersoft deleted the esp8266_gpio16 branch July 29, 2018 03:20

Conversation

sommersoft commented Apr 26, 2018

Uh oh!

jerryneedell commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sommersoft commented Apr 27, 2018

Uh oh!

jerryneedell commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerryneedell commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jepler commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerryneedell commented Apr 26, 2018 •

edited

Loading

jerryneedell commented Apr 27, 2018 •

edited

Loading

jerryneedell commented Apr 27, 2018 •

edited

Loading

jepler commented Apr 30, 2018 •

edited

Loading