Update embedded_hal to 1.0.0-alpha.9 #1
Conversation
|
I don't have much time right now, but I'll try to review and test your changes this weekend. I got a rpi and can run the tests directly on the hardware. In general, I'm still waiting for a 1.0 release of embedded hal to finally have some sort of stable baseline. I guess we'll just make a breaking minor version and get your hal update in if everything works as expected. I hope that the wait does not cause you too much trouble |
There was a problem hiding this comment.
Can you please elaborate the reason for this change (remove borrowing interrupt pin)? Originally, the reason to pass the pin by reference was to enable the user to still be able to check the state of the pin to be able to directly detect any changes on the expander.
I can see that this can be a pain to implement in certain cases as a user, as the user needs to ensure that the pin lives as long as the expander struct. It's probably easier and makes more sense to just map an external interrupt on said pin to be notified if something changes on the expander inputs and make the expander struct own the pin.
In this case, it would be totally fine to just consume the interrupt pin as your change does. Also makes the setup easier. I'm fine with this, just wondering if that's the reason you changed it, or I missed something else.
Edit: Actually forget about that, just me being stupid :D Haven't used Rust in a while. The change is fine and makes sense.
src/pin.rs
Outdated
| @@ -16,37 +16,39 @@ use super::Register; | |||
| /// | |||
| /// The [`ExpanderInputPin`] instance can be used with other pieces of software using [`hal`]. | |||
| #[derive(Debug)] | |||
| pub struct ExpanderInputPin<'a, I2C, Io> | |||
| pub struct ExpanderInputPin<'a, I2C, Io, E> | |||
There was a problem hiding this comment.
I don't think it is necessary to expose the E generic here and introduce a new phantomdata type. The E generic is already defined by the error type of the provided I2C generic.
|
@MajorArkwolf I updated the tests and everything is passing on the hardware. Once we clarify what to do with the E generic you exposed this should be good to go. |
|
Oh my apologies, I didn't realise that the removal of the borrowing interrupt pin got pushed onto this PR. I was testing something on my end. I'll have a look into the E, I believe I needed to add it since it was not constrained but it's possible I overlooked something and took the wrong approach. |
No worries, you actually fixed some useless code that way :D so we'll keep that in.
I already tested it without exposed E generic. Works fine on my end. I can push those changes as well if you're OK with it. |
|
Well... I hit this snag when trying to use |
|
Sorry, I haven't looked into this further since embedded_hal will likely break api in the next month or so. You could always pull in from my branch and use it temporarily in the interim. |
Oh my previous message never sent. Yeah feel free to push your changes in. |
|
@TeyKey1 Okay removed the exposing E from ExpanderInputPin and ExpanderOutputPin. It is not clear from one of your previous messages if you want the interrupt pin to stay as a borrowed ref or happy for it to be a copy. I had run into an issue using the ESP32 library because the pin didn't implement a trait (I think it was send) and therefore had issues. Let me know your thoughts, happy either way. |
|
Thank you very much @MajorArkwolf! I'll create a new cargo release today @sjm42 |
|
Oh, thanks a lot, guys! I really appreciate it. |
Reason
esp-idf-hal for esp32 chips is currently using embedded_hal 1.0.0-alpha.9, which is incompatible with 1.0.0-alpha.5. Since downgrading esp-idf-hal is not an option due to needing embedded-svc the next best logical solution was to upgrade pca9535 library.
Work
Testing
No manual testing has been done and unit tests have not been done either. I will try and do manual testing on the esp32 board, if my PCA9535 chip works.