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

Prototype of an I2C constructor which doesn't own pins #499

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jannic
Copy link
Member

@jannic jannic commented Nov 14, 2022

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
Copy link

cr1901 commented Nov 15, 2022

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
Copy link
Member Author

jannic commented Nov 15, 2022

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
Copy link
Member

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
Copy link

cr1901 commented Nov 15, 2022

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
Copy link
Member

9names commented Nov 15, 2022

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
Copy link
Member Author

jannic commented Nov 15, 2022

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.

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.

4 participants