spi: enforce all traits have the same Error type.#331
Merged
Conversation
|
r? @therealprof (rust-highfive has picked a reviewer for you, use r? to override) |
This was referenced Dec 19, 2021
Closed
Member
Author
|
Working HAL implementation for nRF chips here embassy-rs/embassy#552. Updating was quite straightforward as the HAL already used the same error type for all impls. No issues discovered. |
burrbull
previously approved these changes
Dec 20, 2021
eldruin
previously approved these changes
Dec 20, 2021
eldruin
left a comment
eldruin
left a comment
Member
There was a problem hiding this comment.
Sounds like a good idea to me.
Opinions @rust-embedded/hal?
This was referenced Dec 20, 2021
Dirbaio
changed the title
Dirbaio
force-pushed
the
spi-same-error
branch
from
2c40bbf to
37edca7
Compare
Member
Author
|
Friendly ping @eldruin @therealprof @ryankurte I guess if no one has any concerns in 23 days it's good to go? Rebased |
Contributor
bors Bot
added a commit
that referenced
this pull request
Jan 12, 2022
338: digital: enforce all traits have the same Error type. r=therealprof a=Dirbaio Equivalent of #331 for GPIO traits. This one is a bit trickier, so it maybe warrants some discussion (this is the reason I've opened everything as separate PR's, sorry for the spam!): - The bounds on `IoPin` are becoming very cursed, but I *think* they're correct... Is there some HAL out there implementing it, so that I can verify this doesn't break it? - This forces the "input" and "output" errors to be the same. Perhaps we want to only unify Output and Input separately? In practice I don't think this will be an issue, as structs usually impl only Input or Output but not both (except with IoPin maybe?) Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously discussed in #323, split off to its own PR now.
This PR enforces all SPI trait impls for a given type use the same Error associated type, by moving it to its own ErrorType trait.
How breaking is this?
I believe this is not very breaking in practice (it won't make upgrading existing code to 1.0 harder), because:
SpiWriteError, SpiReadError, SpiTransferError, SpiTransactionalError...Why is this good?
Traits being able to have different Error types is IMO a case of "bad freedom" for the HALs. Today's traits don't stop HALs from implementing them with different error types, but this would cause them to be incompatible with drivers with these bounds. If traits force error types to be the same, the problem is gone.
What about other traits?
I believe this should be also applied to the other traits. I propose discussing this in the context of SPI here, and if the consensus is it's good I'll send PRs doing the same for the other traits.