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

Add KIND contant to Interface trait #169

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

rfuest
Copy link
Collaborator

@rfuest rfuest commented Jan 22, 2025

This PR adds the interface kind constant to the Interface trait as discussed in #167.

I've decided to not extend the InitError enum and just panic instead. It should only be possible to encounter this error during development time, when the programmer wasn't aware that the combination of controller model, color format, and interface kind isn't supported.

@rfuest rfuest requested a review from almindor as a code owner January 22, 2025 02:44
Copy link
Owner

@almindor almindor left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Owner

@almindor almindor left a comment

Choose a reason for hiding this comment

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

Hmm actually thinking about this, I wonder if forcing panic is the wrong call here. I think I'd sleep better if we Model::init errored out properly in this case.

I'm not sure how to best approach that given how Interface::Error is currently generic. My main concern is that kernels can have basic expectations such as "drivers do not panic" and I think that should be upheld, even in init code.

@rfuest
Copy link
Collaborator Author

rfuest commented Jan 23, 2025

Hmm actually thinking about this, I wonder if forcing panic is the wrong call here. I think I'd sleep better if we Model::init errored out properly in this case.

I'm not sure how to best approach that given how Interface::Error is currently generic. My main concern is that kernels can have basic expectations such as "drivers do not panic" and I think that should be upheld, even in init code.

I don't think that this would have been an issue, because the compiler would have been able to resolve the assertions at compile time and remove them from the final binary.

I've changed the code to return a ModelInitError from Model::init, which is a variant of InitError that doesn't contain the reset pin error. Using InitError directly would have required adding an additional type parameter for the reset pin error to Model::init. This didn't feel right because Model::init has no access to the reset pin.

If you are concerned about possible panics we should also look at Builder::display_size, which is much more likely to panic IMO, because the parameters might not be known at compile time. We could move the size check to Builder::init and also return a InitError::InvalidConfiguration in that case. But this is outside the scope of this PR.

@rfuest rfuest requested a review from almindor January 23, 2025 16:56
@almindor
Copy link
Owner

If you are concerned about possible panics we should also look at Builder::display_size, which is much more likely to panic IMO, because the parameters might not be known at compile time. We could move the size check to Builder::init and also return a InitError::InvalidConfiguration in that case. But this is outside the scope of this PR.

Agreed, I'll make an issue for that

@almindor almindor merged commit 6fa0504 into almindor:master Jan 23, 2025
15 checks passed
@rfuest rfuest deleted the interface-kind branch January 27, 2025 15:05
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.

2 participants