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

Investigate C-INTERMEDIATE for all stabilized drivers #2820

Open
bugadani opened this issue Dec 16, 2024 · 2 comments
Open

Investigate C-INTERMEDIATE for all stabilized drivers #2820

bugadani opened this issue Dec 16, 2024 · 2 comments

Comments

@bugadani
Copy link
Contributor

cc #2784

@bjoernQ
Copy link
Contributor

bjoernQ commented Dec 19, 2024

To avoid breaking changes in future:

To resolve this I guess for the non-error case we need to carefully look at every function returning ! or Result<(),E> (or where Result::T is a simple type)

For the error case we could consider that every error variant contains a payload, e.g. MaxDmaTransferSizeExceeded(MaxDmaTransferSizeExceededInfo), - initially that's just an empty struct (non-exhaustive) - if we ever consider to provide more information we could extend that struct without a breaking change

However, I'm not sure that is worth it

@MabezDev MabezDev removed the status:needs-attention This should be prioritized label Dec 19, 2024
@MabezDev
Copy link
Member

So I believe this only applies to I2C & SPI, as they are the two drivers with the concept of a transaction.

I think to take @bjoernQ's ideas a bit further we should:

We should change the return type of spi/i2c transactions to be Result<(), TransactionError>, where TransactionError looks like:

pub struct TransactionError {
   inner: Error,
}

Initially we'd only expose the error inside TransactionError, but it would create a path for us to give some more context to the error when multiple operations may have occurred. For example adding sucessful_operations: usize and a method to access it.

The thing is, embedded-hal only allows us to have on error type for the whole driver, so this will never be available with the eh api. So here we have the option of marking the inherent transaction methods on the drivers as unstable, and we can punt this to post 1.0. Thoughts?

@MabezDev MabezDev added investigation:done and removed investigation Not needed for 1.0, but don't know if we can support without breaking the driver. labels Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

3 participants