-
Notifications
You must be signed in to change notification settings - Fork 221
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
Derive common traits for various types in uart
and i2c
drivers
#2825
Conversation
I opted to manually implement it like https://github.com/esp-rs/esp-hal/pull/2823/files#diff-f2b53d5313d6241262548c66be719b207cf49feadd833841f499ed4afdf1dfe2R441 . What do you think about that? |
This PR also closes #2788, as it implements |
2e93fec
to
79df783
Compare
79df783
to
1a1bfe7
Compare
|
||
impl core::fmt::Display for ConfigError { | ||
fn fmt(&self, _f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a little insane, but #2831 will add some variants and so once this is merged, it can be rebased on top of these changes and a proper implementation can be added for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would strum::Display
make sense here, as well?
|
1a1bfe7
to
211c38b
Compare
impl core::fmt::Display for Error { | ||
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
match self { | ||
Error::ExceedingFifo => write!(f, "The transmission exceeded the FIFO size"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are crates that can turn docstrings into Display output (displaydoc or docsplay). Should we try and use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess thiserror
does support no_std
now, but I'm fairly indifferent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
211c38b
to
3fb4ec0
Compare
Some notes related to the traits mentioned in the corresponding issues:
Clone
,Copy
,PartialEq
andEq
are automatically derived byEnumSetType
already, and therefore not required foruart::UartInterrupt
fugit::Rate
does not implementHash
, soi2c::master::Config
cannot derive itAtomicWaker
implements no traits, thereforei2c::master::State
cannot eitherRegarding the implementation of
Display
:i2c::master::ConfigError
has no variants, so implementingDisplay
is meaningless for it (i.e. what would we actually display?)strum::Display
on this type if requestedDisplay
look like fori2c::master::Config
Display
look like fori2c::master::I2c
Happy to make the
Display
-related changes, just would like to come to a consensus with regards to how this should look so we can be consistent across drivers.Closes #2776
Closes #2810
Closes #2788