Closed jspngh closed 1 year ago
Thanks for the contribution.
I think features are not supposed to de used like that, it is supposed to add functionalities. Did you follow any tutorial?
I think it should be fine. I don't see any other way of supporting two versions of the same crate. For instance, in rp2040-hal they do it the same way.
I think I might have misunderstood your remark.
Did you mean the change in matrix.rs
, which effectively removes eh-0.2 support when the eh-1
feature is enabled?
That's a valid point. I'll take a look at how easy it would be to do it in a purely additive way.
I think the cleanest way may be creating a new trait and providing a blanket impl. In my opinion, I don't think the extra code is worth it and would just provide a switch between eh-1 and eh-0.2, since I don't see a use-case where you want support for both at the same time. Maybe if you're building a library on top of keyberon, but then you should be able to also have a feature that switches between eh-1 and eh-0.2 and propagate that choice to keyberon.
I think this a problem we'll have to solve, not only in this library, but for any driver crate when eh-1 releases. Since many HALs will take time to fully switch to eh-1, you'd want to support both versions. Let me know what you think.
use embedded_hal_02::digital::v2::{InputPin as InputPin_eh02, OutputPin as OutputPin_eh02};
#[cfg(feature = "eh1")]
use embedded_hal_1::digital::{InputPin as InputPin_eh1, OutputPin as OutputPin_eh1};
pub trait InputPin {
type Error;
fn is_high(&self) -> Result<bool, Self::Error>;
fn is_low(&self) -> Result<bool, Self::Error>;
}
impl<P: InputPin_eh02> InputPin for P {
type Error = P::Error;
fn is_high(&self) -> Result<bool, Self::Error> {
self.is_high()
}
fn is_low(&self) -> Result<bool, Self::Error> {
self.is_low()
}
}
#[cfg(feature = "eh1")]
impl<P: InputPin_eh1> InputPin for P {
type Error = P::Error;
fn is_high(&self) -> Result<bool, Self::Error> {
self.is_high()
}
fn is_low(&self) -> Result<bool, Self::Error> {
self.is_low()
}
}
I like it. Please do this, I'll merge with pleasure ;-)
You might want to support both if you have a hal with embedded hal 1 and a io expander over i2c with embedded hal 0.2
I suppose that when embedded hal 1 will be out, embedded hal 0.2 will make a release exporting embedded hal 1 for the identical traits, making the 2 the same.
I've tried a bit more, and the blanket impl for eh-0.2 + eh-1 is not gonna work unfortunately.
I suppose that when embedded hal 1 will be out, embedded hal 0.2 will make a release exporting embedded hal 1 for the identical traits, making the 2 the same.
If that would be the case, then there's no need for this PR. I guess we can wait to see what happens and close this for now.
I just noticed this was still open. I'll close this now and once eh-1.0 lands we could re-evaluate.
This PR adds support for HALs that implement the alpha release of embedded-hal 1.0. When embedded-hal 1.0 releases, it can be made the default with embedded-hal 0.2 being optional.