Fix various errors in examples #19
Conversation
Made the necessary changes to get this example to compile. This includes replacing the ".." placeholders with comments. I also changed the formatting of the `else if` block in `read`, to make it easier to include a placeholder comment in there.
Fix some problems in the `Mutex` example. Please note that it still doesn't compile, as it's obviously rather abstract and incomplete, in order to demonstrate the principle.
Please note that it still doesn't compile, partly because parts of it are pseudo-code, partly because of futures-related issues that I don't understand, and don't currently have the time to get into.
It still doesn't compile, due to the fake `Mutex` and `CircularBuffer` types, as well as the ".." placeholders, but I've fixed the other errors.
|
I've pushed some additional fixes for problems that I found while working on #20. I've amended the existing commits, no new commits have been added. |
There was a problem hiding this comment.
Thanks for the PR, @hannobraun.
Most changes look good to me but I left some comments about what doesn't seem correct.
re: testing this. These doc comments could be un-ignore-d to include them in the test suite but it's tricky and it can be done in a different PR.
src/lib.rs
Outdated
| //! /// A synchronized serial interface | ||
| //! // NOTE This is a global singleton | ||
| //! pub struct Serial1; | ||
| //! pub struct Serial1(Mutex<..>); |
There was a problem hiding this comment.
If this contains a field then it's no longer a global singleton. It's also not required to have this field. See below
src/lib.rs
Outdated
| //! type Error = !; | ||
| //! | ||
| //! fn read(&self) -> Result<u8, nb::Error<Self::Error>> { | ||
| //! hal::serial::Read::read(&Serial1(USART1.lock())) |
There was a problem hiding this comment.
This can use the Serial struct from before like this hal::serial::Read::read(&Serial(&*USART1.lock()))
There was a problem hiding this comment.
Ah, got it. I missed the point of that example before.
src/lib.rs
Outdated
| //! use stm32f103xx_hal_impl::{Serial, Timer}; | ||
| //! | ||
| //! /// `futures` version of `Timer.wait` | ||
| //! /// | ||
| //! /// This returns a future that must be polled to completion | ||
| //! fn wait<T>(timer: &T) -> impl Future<Item = (), Error = !> | ||
| //! fn wait<T>(timer: T) -> impl Future<Item = (), Error = !> |
There was a problem hiding this comment.
With this signature the function will destroy the timer object. The pattern that the futures ecosystem uses to work around this issue is to return the input by value in Item. Thus the return type should be impl Future<Item = T, Error = !>.
There was a problem hiding this comment.
I don't think this pattern will work with poll_fn. Here's what I got now:
fn wait<T>(timer: T) -> impl Future<Item = T, Error = !>
where
T: hal::Timer,
{
future::poll_fn(|| {
try_nb!(timer.wait());
Ok(Async::Ready(timer))
})
}And here's how it fails:
error[E0507]: cannot move out of captured outer variable in an `FnMut` closure
--> src/lib.rs:29:25
|
23 | fn wait<T>(timer: T) -> impl Future<Item = T, Error = !>
| ----- captured outer variable
...
29 | Ok(Async::Ready(timer))
| ^^^^^ cannot move out of captured outer variable in an `FnMut` closure
Which makes sense, as poll_fn takes an FnMut closure.
Of course I could re-create poll_fn/PollFn, but this seems a bit to involved for an example like this. What do you think?
There was a problem hiding this comment.
Yeah, I think you'll have to use loop_fn instead.
There was a problem hiding this comment.
Thanks for the help! I think I got it now.
src/lib.rs
Outdated
| //! Ok(Async::Ready(try_nb!(timer.wait()))) | ||
| //! }) | ||
| //! } | ||
| //! | ||
| //! /// `futures` version of `Serial.read` | ||
| //! /// | ||
| //! /// This returns a future that must be polled to completion | ||
| //! fn read<S>(serial: &S) -> impl Future<Item = u8, Error = Error> | ||
| //! fn read<S>(serial: S) -> impl Future<Item = u8, Error = S::Error> |
src/lib.rs
Outdated
| //! Ok(Async::Ready(try_nb!(serial.read()))) | ||
| //! }) | ||
| //! } | ||
| //! | ||
| //! /// `futures` version of `Serial.write` | ||
| //! /// | ||
| //! /// This returns a future that must be polled to completion | ||
| //! fn write<S>(byte: u8) -> impl Future<Item = (), Error = Error> | ||
| //! fn write<S>(serial: S, byte: u8) -> impl Future<Item = (), Error = S::Error> |
|
Thanks for the review! I've pushed a commit addressing your first comment. I didn't adress your comments on the futures example (see the comment I left in reply to yours). |
|
I've amended my last commit and force-pushed. It addresses all the review comments now. In case the changes are okay: Do you want me to leave it like that, or should I squash the changes from the last commit into the original commits? |
|
Thanks, @hannobraun. @homunkulus r+ |
|
📌 Commit dbafdc7 has been approved by |
|
☀️ Test successful - status-travis |
|
Thank you! |
I've been working on picking up the stalled #16. Since this PR includes a change request regarding an example, I wanted to make sure to properly test all of them, but found out that they're kind of a mess. There were a lot of build errors that a user copying them into a new project would face.
I fixed a lot of those errors, and left others in there. The commits have more details about that. I do think that this PR, while clearly an improvement, is just papering over the deeper issue: That the examples are ignored (as in, not compiled during
cargo test), and therefore prone to break and become out of date. I think we should find a way to change that, but that is beyond the scope of this humble pull request.