Prototype of an I2C constructor which doesn't own pins

[jannic]

Just a simple PoC showing that the existing I2C struct is flexible enough to be extended with a variant that doesn't own SCL/SDA pins. That way, it would be easy to use I2C with DynPins.

For now I only tested that this still builds. And the function naming needs to be improved. (Also, lots of unnecessary code duplication in controller.rs)

@cr1901: Would that, in combination with #482, solve #481?

[cr1901]

I have a proof-of-concept driver that's not client code that shows what I want to do. Is it possible you could create a branch with these changes on top of #482 so that I can push the repo with the proof-of-concept?

[jannic]

Is it possible you could create a branch with these changes on top of #482 so that I can push the repo with the proof-of-concept?

Sure, just merged both branches together:
https://github.com/jannic/rp-hal/tree/prototype_i2c_without_static_pins_with_dyn_gpio

[ithinuel]

I think the point of the peripheral taking ownership of the pins is to be able to guaranty that the pins will remain in a valid state for the peripheral to function as expected.
So IMHO, the peripheral should still take ownership (or at the very least an &mut) of the DynPin and instead of relying on the compiler to validate the state of the pin, the driver should do that at runtime.

This imply validating that the pair of dynpin is valid for the given peripheral and that their state (function) will remain the same until the peripheral gets released.

[cr1901]

I think the point of the peripheral taking ownership of the pins is to be able to guaranty that the pins will remain in a valid state for the peripheral to function as expected.

I guess my follow-up question is "why doesn't the SPI peripheral do this"?

[9names]

SPI peripheral driver predates us switching to the atsamd pin traits. Really needs to be updated to match everything else, but there's always so many things to fix.

[jannic]

First, a general question: Do we really need to guarantee that the pin mode is and stays correct?
Sure, the default way of configuring a peripheral should be as easy to use as possible, and that includes making sure that the pin configuration is correct.
But that should not make other use cases impossible. So if the use case @cr1901 has can only be solved by not checking the pin config at all, in my opinion we should provide such an interface. (With appropriate warnings in the doc comment.)

Rationale:

• It is useful for some use case
• It is not unsound - the rust program would not do something bad if the pin config was wrong
• A pin not being in the correct function mode would be similar to a pin not connected at the hardware layer - something we can't prevent by software safeguards
• We already don't prevent all possible software misconfigurations:
  • The user might have chosen the wrong pin (but only if that pin is able to provide the requested function)
  • There might be another pin configured to the same peripheral (eg. both GPIO0 and GPIO4 might be configured to I2C0 SDA at the same time, which would probably not work as expected.)
  • Pin configurations like set_output_enable_override would also prevent proper function of the peripheral, and we don't prevent that.

That said, if all reasonable use cases can be handled with a variant that owns the DynPins, and checks (or activates) the correct pin function, we should do that. In fact, the main reason I didn't do it for this proof of concept was that this would have been slightly more work.