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

Make it possible to construct DynPin from scratch #482

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

9names
Copy link
Member

@9names 9names commented Oct 23, 2022

Basic functionality for #481

@cr1901
Copy link

cr1901 commented Nov 14, 2022

I finally got around to testing this... I don't think it's going to be enough; ultimately what I need is "a way to pass in DynPins to e.g. the I2C peripheral, which take type-level Pins. There's a try_from for DynPin, but the problem is I don't know which type-level pin I need to convert to until runtime.

The SPI peripheral doesn't seem to take in any pins to initialize, so I can safely use that with DynPin. What can be done about e.g. I2c and UartPeripheral?

@jannic
Copy link
Member

jannic commented Nov 14, 2022

ultimately what I need is "a way to pass in DynPins to e.g. the I2C peripheral, which take type-level Pins. There's a try_from for DynPin, but the problem is I don't know which type-level pin I need to convert to until runtime.

That one looks like a separate (but related) issue:
Currently, struct I2C {...} encodes the actual pin used in its type. So it's impossible to create an instance without knowing the pins at compile time.

I think this requirement is too strict. I don't see an inherent reason why it shouldn't be possible to construct an I2C peripheral with DynPins. Of course, that either needs some run-time checks (not zero cost - but really not expensive either), or would allow the user to create an I2C peripheral with nonsensical pins. Even the latter wouldn't be a catastrophe, it's not unsound or such, it just won't work.

@@ -305,7 +305,7 @@ impl DynPin {
/// must be at most one corresponding [`DynPin`] in existence at any given
/// time. Violating this requirement is `unsafe`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side note, as it wasn't changed in this PR: The comment "Violating this requirement is unsafe" is a little bit off. The function is unsafe because the caller must guarantee that DynPins are unique. Violating such a condition is not unsafe, it's plain wrong. It may cause UB, or other bugs. But saying that violation the safety conditions of an unsafe function is unsafe doesn't convey any useful meaning.

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.

3 participants