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

Standardise order for setting GPIO pin default values #2942

Merged
merged 13 commits into from
Dec 2, 2023

Conversation

S5NC
Copy link
Contributor

@S5NC S5NC commented Nov 16, 2023

In cases where a digitalWrite is (clearly) called on a pin, and then pinMode is set for that same pin, the two lines are flipped such that the pinMode is set before calling the digitalWrite.

thebentern added a commit to meshtastic/artifacts that referenced this pull request Nov 16, 2023
@S5NC
Copy link
Contributor Author

S5NC commented Nov 16, 2023

Depending on the specific implementation of how the MCU interfaces with the code, the default state may be to leave all IO pins NC, neither pulled high nor low. If this is the case, and by setting the state of the pin via digitalWrite would be the first driving of the pin, then infact having the pinMode call after the digitalWrite could avoid the pin being pulled low or high (whichever is the default) when a pin is set as an output but not assigned a specific value. Ideally the MCU would not force a value once the pin is set as an output, and leave it NC (floating). The internal pullup/pulldown resistors are very weak (not the ones used for inputs, or driving a pin, but the resistance of the pin itself) so these won't have much of an effect if the pin is not set high/low (as in output) or pulled high/low (as you can enable for inputs). However, the MCU may decide to drive the pin low by default. This really varies per MCU. There may also be start-up glitches.

Overall, if it is not critical to NEVER have the pin set as something else, there is no need to check with the MCU for startup glitches, default driving, and set the digitalWrite before the pinMode.

Because of this, I may actually change all "initialising" of pins to their Meshastatic-decided default values to call digitalWrite first (this is only for instances where the output is set adjacently to the mode, as changed in this PR, so not SPI, UART, USB, etc.). It is best to stay on the safe side and avoid any potential HIGH/LOW driving between setting as an output and assigning the output signal, as not to assume that the pin is left floating by the MCU during this gap. The order in which we do pinMode and digitalWrite has no effect on behaviour before this (startup glitches, whether they are forcibly driven / pulled up/down before), so this is beyond the intended scope.

I would like your thoughts on this, should we change all instances as shown in files changed to call pinMode for a pin after digitalWrite has set its initial status? If not for this, then at least for consistency's sake.

@GUVWAF
Copy link
Member

GUVWAF commented Nov 17, 2023

Yes, I think it makes sense to call pinMode before digitalWrite.
Can you run trunk fmt to resolve the CI errors?

Copy link
Contributor

🤖 Pull request artifacts

file commit
pr2942-firmware-2.2.14.db543be.zip db543be

thebentern added a commit to meshtastic/artifacts that referenced this pull request Nov 17, 2023
@S5NC
Copy link
Contributor Author

S5NC commented Dec 2, 2023

On my ESP32-S3 dev board, I measure that turning the board on, if a pin isn't configured by pinMode, that it isn't actively driven. It reads a voltage of 0 V, but when connected to a 3.3 V pullup resistor it reads 3.3 V. I guess this is the same way that a pin configured as an input would act.

I also measure that once pinMode(i, OUTPUT); is run, the pin is actively pulled low. I know it's actively pulled low because I connected a 10 kOhm resistor pulling each pin up to 3.3 V, but it still report nearly 0 V.

I haven't specifically tested strapping pins. Some pins (GPIO 0) have weak resistors pulling up/down. In GPIO 0 (BOOT)'s case, it's a weak internal pullup to 3.3V.

As pins default to being pulled LOW once pinMode is set, I believe the default state should be set before the pinMode is set. I don't have an osciliscope to measure if in this configuration there is a tiny moment where it is pulled low, but I would assume not, that it just applies what is in the IO table (which defaults to 0, but we we set it to 1 (digital) it applies that instead).

This is only the behaviour on the ESP32-S3, other platforms (nRF52, RP2040, etc.) will vary. But we have not seen any problems calling writes right before pinMode is set, so I will change them all to be like this.

The risk I can think of is if there is a low driven relay, this could cause unexpected behaviour. Or an external component is pulled high externally to be turned on at boot, but setting the pinMode before writing HIGH to it anyway could make it power cycle that external component.

@S5NC S5NC changed the title Ensure pinMode is set before pin is used Standardise order for setting GPIO pin default values Dec 2, 2023
@thebentern thebentern merged commit 2544733 into meshtastic:master Dec 2, 2023
3 checks passed
S5NC added a commit to S5NC/meshtastic-firmware that referenced this pull request Dec 2, 2023
@S5NC S5NC deleted the flip-mode-order branch December 2, 2023 16:04
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.

None yet

4 participants