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.

Sign up for GitHub

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

Jump to bottom

I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. #2831

Merged
merged 9 commits into from
Dec 20, 2024
Merged

I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. #2831

merged 9 commits into from
Dec 20, 2024

Conversation

playfulFence
Copy link
Contributor

Description

closes #2767

@playfulFence playfulFence changed the title Upd/wtbeta I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. Dec 18, 2024
bugadani
bugadani reviewed Dec 18, 2024
View reviewed changes
esp-hal/src/i2c/master/mod.rs Outdated
@@ -92,6 +92,9 @@ const MAX_ITERATIONS: u32 = 1_000_000;
pub enum Error {
/// The transmission exceeded the FIFO size.
ExceedingFifo,
#[cfg(any(esp32, esp32s2))]
/// Read limit of 32 bytes or transfer limit of 31 bytes exceeded.
ExceedingTransactionSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from ExceedingFifo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if ExceedingFifo is descriptive enough (?) for this error...
I'm still not sure though, so if you think we can re-use already existing ExceedingFifo - I'll do the fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

ExceedingTransactionSize doesn't immediately make sense to me, as I'm not sure what exactly had been exceeded.

ExceedingTransactionSizeLimit is probably what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not sure if ExceedingFifo is descriptive enough (?) for this error...

It shouldn't ever happen, otherwise we have a bug. The HAL is responsible for papering over length issues if possible. (This also means that a panic is actually not a bad tool here - it's a bug to hit it).

I'd rather not introduce a new error variant for something that we shouldn't let happen in the first place. But trying to read/write longer data than what fits into the FIFO is IMHO pretty much covered by ExceedingFifo. If that isn't descriptive enough, then it should be made more descriptive. Note that we already return that error in setup_write or setup_read for a very similar problem.

bugadani
bugadani reviewed Dec 19, 2024
View reviewed changes
esp-hal/CHANGELOG.md Outdated Show resolved Hide resolved
esp-hal/src/i2c/master/mod.rs Show resolved Hide resolved
esp-hal/src/i2c/master/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/i2c/master/mod.rs Show resolved Hide resolved
@jessebraham jessebraham mentioned this pull request Dec 19, 2024
Merged
bjoernQ
bjoernQ approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Given we will move the validation phase to before we do any register operations (and improve validation in general, provide more helpful logs) this looks good to me (and also those improvements won't affect the public API)

@playfulFence
rebase
f5278f7 
...
bugadani
bugadani reviewed Dec 20, 2024
View reviewed changes
esp-hal/src/i2c/master/mod.rs
pub enum ConfigError {}
pub enum ConfigError {
/// Provided bus frequency is invalid for the current configuration.
InvalidFrequency,
Copy link
Contributor

Choose a reason for hiding this comment

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

Next fun issue, now we have a precedent of TimeoutInvalid, should this be FrequencyInvalid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, yes - that would be better

@playfulFence playfulFence added this pull request to the merge queue Dec 20, 2024
Merged via the queue into esp-rs:main with commit 2862197 Dec 20, 2024
28 checks passed
@playfulFence playfulFence mentioned this pull request Dec 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dominaezzz Dominaezzz Dominaezzz left review comments

@bjoernQ bjoernQ bjoernQ approved these changes

@bugadani bugadani bugadani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics.
4 participants
@playfulFence @bugadani @bjoernQ @Dominaezzz