Make embedded-hal 0.2 optional #435
Conversation
qwandor
force-pushed
the
embedded-hal
branch
8 times, most recently
from
c70e147
to
f330108
Compare
embedded-hal 0.2 support will be kept for now for compatibility, but users should migrate to embedded-hal 1.0 traits where possible so the old traits shouldn't be in the prelude.
| self, | ||
| initial_output: Level, | ||
| drive: DriveConfig, | ||
| ) -> Pin<Output<PushPull>> { |
There was a problem hiding this comment.
fyi: There's no diff here except fixing formatting. Maybe there should be a format check in CI?
nrf-hal-common/src/saadc.rs
Outdated
| 13 => self.0.ch[0].pselp.write(|w| w.pselp().vddhdiv5()), | ||
| // This can never happen the only analog pins have already been defined | ||
| // PAY CLOSE ATTENTION TO ANY CHANGES TO THIS IMPL OR THE `channel_mappings!` MACRO | ||
| _ => unsafe { unreachable_unchecked() }, |
There was a problem hiding this comment.
The Channel trait seems to be public and safe, which means its channel() function cannot be assumed to be correct. It could either be make an unsafe trait or it could be sealed. But maybe I'm missing some constraint that make it impossible to implement and use a bad Channel.
(Reading the rest of the diff, I can see this code was simply moved. So probably not an issue for this PR.)
There was a problem hiding this comment.
Good point. This isn't new code, but returning an error seems like a better option than causing undefined behaviour.
| rt = ["nrf52811-pac/rt"] | ||
| default = ["rt"] | ||
| default = ["rt", "embedded-hal-02"] |
There was a problem hiding this comment.
I guess embedded-hal-02 is added as a default feature to avoid breaking change. But what about users that disable default features? I'm not sure what the Cargo Book recommends in those cases.
There was a problem hiding this comment.
There will be breaking changes in this release anyway, so I'm not really worried about that. I've included it by default just to make life easier for new users who might be using device drivers that depend on the embedded-hal 0.2 traits. People who care about binary size and build time can disable default features.
xtask/tests/ci.rs
Outdated
| &toml_path, | ||
| "--target", | ||
| target, | ||
| //"--no-default-features", |
There was a problem hiding this comment.
Whoops, well spotted. Uncommented.
U32Ext is already included in nrf_hal_common::prelude.
Return an error if Channel::channel() is invalid rather than causing undefined behaviour. The Channel trait is not sealed, so we can't rely on the implementation being correct.
This required some refactoring as some of the embedded-hal 1.0 implementations depended on the embedded-hal 0.2 implementations.