Turn off input_enable for ADC capable pins

[jannic]

This is meant to fix #754.

Not yet tested at all!
Tested using on-target-tests.

[ithinuel]

Since the HW starts with this enabled by default, I’d keep it enabled for pins that are assumed digital by default.

We have a newtype wrapper for ADC pins that does turn of input enable & output disable.

That being said, the main thing I’m not fond of with this solution is that it makes a few pins behave differently "by default" which will cause surprise for sure.
So making all pins to turn off input-enabled and changing that when switching function seems acceptable to me too.

[jannic]

Since the HW starts with this enabled by default, I’d keep it enabled for pins that are assumed digital by default.

We have a newtype wrapper for ADC pins that does turn of input enable & output disable.

That doesn't solve the core issue: The ADC pins should have the input disabled by default. (See erratum RP2040-E6 in the datasheet.)

Both the boot rom (since version RP2040B2) and the C SDK work around it by disabling input enable as soon as possible in the boot process.

For pins actually configured as ADC pins, we already unset input enable.

That being said, the main thing I’m not fond of with this solution is that it makes a few pins behave differently "by default" which will cause surprise for sure. So making all pins to turn off input-enabled and changing that when switching function seems acceptable to me too.

That would be a valid alternative. Did anybody check what bootrom v2 and SDK do?

[jonathanpallant]

Bootrom v2: https://github.com/raspberrypi/pico-bootrom/blob/ef22cd8ede5bc007f81d7f2416b48db90f313434/bootrom/bootrom_rt0.S#L262

[jannic]

And C SDK: https://github.com/raspberrypi/pico-sdk/blob/6a7db34ff63345a7badec79ebea3aaef1712f374/src/rp2_common/pico_runtime/runtime.c#L116-L119

So both clear input enable for pins 26-29 only.

I agree with @ithinuel that it would be nice if all pins had the same startup config. But there's probably a reason that Raspberry only applied the workaround to these pins. Probably to minimize the probability of breaking changes.

Does the same argument count for rp2040-hal? Most rust programs won't notice the difference as long as we implicitly set input enable when selecting a non-adc function on the pin. The C SDK can't be so sure as in C it's more likely that firmware accesses registers directly, without using helper functions.

But it's still possible to break existing software, even if it uses the methods provided by rp2040-hal: For example, PIO can read all pins independent of pin function settings. So it is possible that a firmware (intentionally or unintentionally) doesn't assign a pin to the PIO peripheral, but still reads the input value from a PIO state machine.

[jannic]

@ithinuel @jonathanpallant How shall we progress with this pull request?

As I understand it, switching the ADC pins to "input disabled" early in the boot process is desirable for stability reasons, otherwise they wouldn't have made that change in the C SDK.

The open question is if we should apply that change to the ADC pins only (which minimizes impact on existing users which may somehow rely on the previous behavior), or if we prefer consistency and apply the change to all input pins, as @ithinuel suggested.

I would agree with both approaches, so if both of you agree that it should be applied to all pins, I'll update the pull request accordingly.

[jonathanpallant]

I would make the change to all pins, add a clear note to the CHANGELOG, and make it a breaking change.

[jannic]

I just wrote a small on-target-test, checking that the inputs are really disabled on boot.
Of course, I forgot to call Pins::new because I didn't need the pins in that test (I probed the pad config directly). So the test failed, as disabling the inputs is currently implemented in Pins::new. Which makes me wonder: Is that the right place? On the one hand, it's likely that creating the pin structures is one of the first things being done. But is it guaranteed? Is it early enough?

[ithinuel]

Looks good but had no time to actually test it.