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

ESP32-S2: Add IDF pin resets to Microcontroller #3643

Merged
merged 7 commits into from
Dec 15, 2020
Merged

ESP32-S2: Add IDF pin resets to Microcontroller #3643

merged 7 commits into from
Dec 15, 2020

Conversation

hierophect
Copy link
Collaborator

This PR adds the ESP-IDF gpio_reset_pin function to the Microcontroller/Pin module's reset_pin_number and reset_all_pins. This should resolve issues where pins would remain connected to previously used peripherals even after soft reboots or de-inits, since the IDF does not alter the pin matrix for most peripheral de-init functions.

Tested on the Saola 1 Wrover with Neopixel and I2C. Any help testing additional applications for unforeseen problems would be appreciated.

@hierophect hierophect requested a review from tannewt November 3, 2020 21:36
@hierophect hierophect added the espressif applies to multiple Espressif chips label Nov 3, 2020
@tannewt
Copy link
Member

tannewt commented Nov 4, 2020

I'm not sure we want to use gpio_pin_reset because 1) it enables pull up and 2) attaches GPIO to the output. (Source) The pull up behavior doesn't match other ports and we may want to switch back to the default pin function such as JTAG.

We definitely should refine this code though. We do want to reset individual pins.

@hierophect
Copy link
Collaborator Author

I'm not sure we want to use gpio_pin_reset because 1) it enables pull up and 2) attaches GPIO to the output. (Source) The pull up behavior doesn't match other ports and we may want to switch back to the default pin function such as JTAG.

We definitely should refine this code though. We do want to reset individual pins.

We could store a table of default pin setting information, the way we do for the i.MX, and revert to that?

@tannewt
Copy link
Member

tannewt commented Nov 5, 2020

@hierophect I don't think you need that. The ESP32-S2 IOMUX can be set back to digital function 0 which is the default. The GPIO mux can be set back to its default as well.

@hierophect
Copy link
Collaborator Author

@tannewt I've replaced the resets with gpio_matrix_out(pin_number, 0x100, 0, 0);, which is the mux changing magic function used by FastLED alongside the RMT. Seems to work fine for my Saola, but if you've seen another mux alteration function you think would be more appropriate, we could swap it out. Should solve #3715, but I haven't specifically tested Jeff's failing script yet.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Are you sure this resets the IOMUX too? Also, you need to make sure the idf isn't changed.

@hierophect
Copy link
Collaborator Author

The ESP32-S2 IOMUX can be set back to digital function 0 which is the default.

How do you know this/what functions have you seen used to do it? I'm finding the IOMUX to be scant on both online and inline documentation.

@tannewt
Copy link
Member

tannewt commented Nov 25, 2020

For folks following along. The best places to get details about the chip and the IOMUX are the datasheet and the technical reference.

tannewt
tannewt previously approved these changes Dec 7, 2020
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you!

@hierophect
Copy link
Collaborator Author

I do want to note that the IDF includes this comment in the gpio_reset_pin implementation:

//for powersave reasons, the GPIO should not be floating, select pullup

So it's feasible that if someone who really cares about power savings might want to either tweak this in their own version, or implement a new pin modification function that sets the pins to this lower power consuming state.

@hierophect
Copy link
Collaborator Author

@tannewt also, before I merge anything, my latest commit still wasn't quite what you'd asked for, since it still sets the IOMUX to 1 as per the default inside gpio_conf, which is PIN_FUNC_SELECT(io_reg, PIN_FUNC_GPIO);. I got distracted by my board mixup but the alternative I was going to offer is the following:

io_reg = GPIO_PIN_MUX_REG[pin_num];
if (io_reg) {
	gpio_matrix_out(pin_number, 0x100, 0, 0);
	PIN_FUNC_SELECT(io_reg, PIN_FUNC_GPIO);
}

which halts GPIO output and sets the IOMUX to Function 0. But my concern is that Digital Function 0 can default to things like the DAC or SPI, and since they circumvent the GPIO, this could result in the exact kind of behavior we're trying to avoid, where peripherals continue to deliver output to the pin even after a reset. So my recommendation is to use what I've implemented, but I wanted to let you know about this alternative.

@tannewt
Copy link
Member

tannewt commented Dec 8, 2020

I do want to note that the IDF includes this comment in the gpio_reset_pin implementation:

//for powersave reasons, the GPIO should not be floating, select pullup

So it's feasible that if someone who really cares about power savings might want to either tweak this in their own version, or implement a new pin modification function that sets the pins to this lower power consuming state.

We can always do this later. I'm sure there are bigger power savings to do before that.

@tannewt also, before I merge anything, my latest commit still wasn't quite what you'd asked for, since it still sets the IOMUX to 1 as per the default inside gpio_conf, which is PIN_FUNC_SELECT(io_reg, PIN_FUNC_GPIO);. I got distracted by my board mixup but the alternative I was going to offer is the following:

io_reg = GPIO_PIN_MUX_REG[pin_num];
if (io_reg) {
	gpio_matrix_out(pin_number, 0x100, 0, 0);
	PIN_FUNC_SELECT(io_reg, PIN_FUNC_GPIO);
}

which halts GPIO output and sets the IOMUX to Function 0. But my concern is that Digital Function 0 can default to things like the DAC or SPI, and since they circumvent the GPIO, this could result in the exact kind of behavior we're trying to avoid, where peripherals continue to deliver output to the pin even after a reset. So my recommendation is to use what I've implemented, but I wanted to let you know about this alternative.

Digital function 0 is what we should use because UART0 and JTAG are enabled by it. (Not the DAC because it's analog.) It's ok if one of the SPIs is connected. We can fix that later if need be. The other SPI is for flash and ram so we definitely want to leave it.

@hierophect
Copy link
Collaborator Author

hierophect commented Dec 14, 2020

@tannewt I've changed the pin function to 0 in the pin reset process with PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[pin_number], 0);. In testing, this seems to work ok - I'm not sure exactly what you meant by "Not the DAC because it's analog." but in practice the DAC pin shutoff seems to work as expected without a GPIO override (probably due to the dac_output_disable in the de-init and analogout_reset functions). For the SPI pins, I think they're mostly the ones used for Flash, and I don't see weird behavior on 26 with my existing tests. So I think we're good for now, though we should keep an eye out for issues describing unusual behavior on pins after reset.

Screen Shot 2020-12-14 at 3 10 06 PM

@hierophect hierophect requested a review from tannewt December 14, 2020 20:20
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@tannewt tannewt merged commit dc473b2 into adafruit:main Dec 15, 2020
@hierophect hierophect deleted the esp32-pin-reset branch December 15, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
espressif applies to multiple Espressif chips
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants