-
Notifications
You must be signed in to change notification settings - Fork 182
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
replace Void with Infallible + small fixes #100
Conversation
Ohh, that's a neat feature. The change looks good to me, but the dependency on the void crate should probably be removed from |
I could not remove it entirely due to https://github.com/rust-embedded/embedded-hal/blob/a5c0e3c63e4e431807d491d35dd67d1d43c07c92/src/timer.rs#L77 |
Ah right, makes sense. I guess we might want to submit a PR to fix that in embedded-hal at some point. |
Yes. But in any case it will be breaking change. |
Do we want to merge this now, or wait for the embedded-hal PR to land? If we do it now, we'll get another breaking change after the PR, right? |
This part of changes shouldn't break user code (I hope). |
I think it will, we are changing public API after all. For example, someone might have written wrapper functions around the GPIO pins like this fn do_something_with_pins(...) -> Result<Something, Void> {
pin.set_high()?; // ? Operator no longer works because return type changed from Void to Infallible
} |
Looks like you are right. |
No, I don't think I have. It also seems like kind of a weird thing to do if you understand the point of the Void type. It might not make a difference though. We already have breaking changes staged for the next release, so we might as well throw this in. When embedded-hal updates, that too will be a breaking change (probably). |
I'm pretty much fine with merging this, though the embedded-hal discussion brought up a good question. Do we have a minimum supported rust version? I don't mind "latest stable" at all, but perhaps someone else has concerns about it. @therealprof What do you think? |
Looks like the never type will be included in 1.40 🎉. Do we still want to merge this, and then move to |
By the way is there issue to disable |
That's a good question, it would certainly be nice Also, I may have been too quick with my previous message. The comments on that rust-lang issue suggest that there is still some work to be done for it to stabilize. |
I submitted an issue here: rust-lang/rust#65861 |
I would prefer to stay a couple of versions behind the latest and the greatest. |
Rebased |
Great! would you mind moving the changelog entry to the "Brekaing changes" section? Other than that, I think this is ready to be merged |
Done. |
Lovely, thanks! |
r? @therealprof