nrf-rs / nrf51-hal

A Rust embedded-hal impl crate for the Nordic nrf51 series of microcontrollers
BSD Zero Clause License
19 stars 13 forks source link

GPIO.split() claims to produce PINnn<Input<Floating>> but doesn't enable the input buffer #20

Open mattheww opened 5 years ago

mattheww commented 5 years ago

GPIO.split() returns a Parts struct, whose members have types like PIN0<Input<Floating>>: https://docs.rs/nrf51-hal/0.6.2/nrf51_hal/gpio/gpio/struct.Parts.html

This means its members claim to implement the embedded_hal::digital::InputPin trait, and so the compiler will let you call is_high() and is_low() on them.

But split() doesn't do anything to enable the input buffer for each pin (which is disabled by default), so these calls won't return meaningful information until you explicitly call one of the .into_xxx_input() methods.

In general the typestate system for these PIN types means the compiler prevents you making mistakes like this, so I think it would be better to either

The second change isn't fully backwards compatible, but if it makes a program stop compiling it's probably revealing a bug.

There's also a weakness that nothing stops you arbitrarily configuring the GPIO pins before calling split(), leading to another way for the typestates to get out of sync with the hardware. Either of the changes above would remove that weakness too.

Nemo157 commented 5 years ago

I took the second approach while experimenting with alternative typestate implementations in embrio-nrf51 (which was strongly inspired by nrf51-hal's implementation). It definitely seems like the correct solution to me.