Refactor to use embedded_io and embedded_io_async

m5p3nc3r commented 4 months ago

Hi guys

This is a bit of a major refactor to use embedded_io and embedded_io_async as a consistent interface for the sync and async interfaces. It build on the work I did in #17 and should replace that PR if this refactor is accepted.

There are some benefits of this new approach

The Editor logic between sync and async is now identical
There is no need for Tokio in the core noline implementation, only in the example
The sync and no_sync (now renamed sync_editor and async_editor) are simplified, with IO logic moved into [a]sync_io

I have only worked to get the examples for std/std-sync and std/std-async-tokio working, but wanted to check in to see if this approach was going in the right direction.

There is still a lot to do:

Update the comments to reflect the new code structure
Update the no_std examples to use the new code structure
Re-enable the tests

Please let me know what you think, and I will complete the work if you think this worthwhile.

eivindbergem commented 4 months ago

This is really good work! I had a quick look, and this is very much the going in the right direction, so please continue :smile:

m5p3nc3r commented 4 months ago

Hey guys

I am really struggling with implementing the Sync example. The reason is that USB devices can return UsbError::WouldBlock, in which case I would need to call the poll function again. In the original example, this is worked around by lending the UsbDevice instance to the SerialWrapper class. This feels wrong, as in a multi-function device, you really want to be calling poll in an outer loop as this really has nothing to do with the SerialWrapper functionality.

The way I am calling poll looks like this at the moment

let mut editor = loop {
        if !usb_dev.poll(&mut [io.inner().serial])
            || !io.inner().serial.dtr()
            || !io.inner().serial.rts()
        {
            continue;
        }

        break EditorBuilder::new_static::<128>()
            .with_static_history::<128>()
            .build_sync(&mut io)
            .unwrap();
    };

Which feels like the correct way to do this. I only lend the serial CDC instance into SerialWrapper.

The correct way to implement this would be to add embedded_io::ReadReady and embedded_io::WriteReady, but those would also need to have access to the UsbDevice instance to call poll(...) on, which also feels broken.

My suggestion would be that using noline in a sync/blocking configuration with an async backend is not a sensible configuration?

The other angle to this argument would be that you would not have a multi-function USB device if you are using noline in a blocking way as this would not work.

Your thoughts??

eivindbergem commented 4 months ago

I'm not sure I have much to contribute with here. From what I remember usbd-serial didn't really support interrupt-driven usage as it was poll based. What I implemented was mostly a way of working around these limitations. I see that usbd-serial has had some released since then – I remember the developer working on a big rewrite – so maybe things have changed in this regard.

In any case, you likely have a better understanding of this at the moment. So, just go ahead and do what you think is the most sensible :)

m5p3nc3r commented 4 months ago

Ok, so I have the std[sync/async] and nostd[sync/async] examples working now.

I am moving on to re-integrating the tests. I have one question for you. Now that I have moved over to using embedded_io::ErrorKind as the default error type, and we need to enable the std feature in embedded_io to get the From trait implementation for std::io::Error, what is the best way to do this. I believe there are two options;

1, make std a default feature, this will be a breaking change in the configuration, but it will mean that you can simply run carto test and have it work as expected. This also have the benefit of having the intellisense working properly for the test cases. 2, force you to run cargo test --features=std to enable the embedded_io crate to be build correctly. The problem with this is that the rust analyser will think there are errors in the test code.

I plan to go with number 1 as I believe it is the most rusty way to do this. Let me know your thoughts.

m5p3nc3r commented 4 months ago

One last question - we have discussed this before, but I wanted to check before I made the change.

Are you Ok with me removing the stm32 example? I don't have any hardware and am unable to test is. I have sync and async examples using the rp2040. Or would somebody with the stm32 board be happy to update the example code?

eivindbergem commented 4 months ago

Ok, so I have the std[sync/async] and nostd[sync/async] examples working now.

I am moving on to re-integrating the tests. I have one question for you. Now that I have moved over to using embedded_io::ErrorKind as the default error type, and we need to enable the std feature in embedded_io to get the From trait implementation for std::io::Error, what is the best way to do this. I believe there are two options;

1, make std a default feature, this will be a breaking change in the configuration, but it will mean that you can simply run carto test and have it work as expected. This also have the benefit of having the intellisense working properly for the test cases. 2, force you to run cargo test --features=std to enable the embedded_io crate to be build correctly. The problem with this is that the rust analyser will think there are errors in the test code.

I plan to go with number 1 as I believe it is the most rusty way to do this. Let me know your thoughts.

Another solution here is to use #![cfg_attr(not(test), no_std)] to have std in tests.

eivindbergem commented 4 months ago

One last question - we have discussed this before, but I wanted to check before I made the change.

Are you Ok with me removing the stm32 example? I don't have any hardware and am unable to test is. I have sync and async examples using the rp2040. Or would somebody with the stm32 board be happy to update the example code?

You can just remove it for now, and maybe I'll add a new one back in later.

m5p3nc3r commented 4 months ago

Ok, so I have the std[sync/async] and nostd[sync/async] examples working now. I am moving on to re-integrating the tests. I have one question for you. Now that I have moved over to using embedded_io::ErrorKind as the default error type, and we need to enable the std feature in embedded_io to get the From trait implementation for std::io::Error, what is the best way to do this. I believe there are two options; 1, make std a default feature, this will be a breaking change in the configuration, but it will mean that you can simply run carto test and have it work as expected. This also have the benefit of having the intellisense working properly for the test cases. 2, force you to run cargo test --features=std to enable the embedded_io crate to be build correctly. The problem with this is that the rust analyser will think there are errors in the test code. I plan to go with number 1 as I believe it is the most rusty way to do this. Let me know your thoughts.

Another solution here is to use #![cfg_attr(not(test), no_std)] to have std in tests.

I have noticed that the github actions runner for tests uses cargo test --verbose --all-features which means that the CI should work. But if you wanted to run them manually, it would only work if you run the same command - cargo test would fail.

m5p3nc3r commented 4 months ago

@eivindbergem I think it is ready now. Should I move it ro 'ready for review'?

I appreciate there is a lot of churn in this, but I think its a sensible change.

m5p3nc3r commented 4 months ago

@eivindbergem - Thats everything green on the CI now, I don't know how to remove the required test for examples-stm32f103, so it still shows as not completed. Is this something you can resolve?

eivindbergem commented 4 months ago

@eivindbergem - Thats everything green on the CI now, I don't know how to remove the required test for examples-stm32f103, so it still shows as not completed. Is this something you can resolve?

It says here that one test haven't completed yet, but I don't see it in the list. Not sure what's going on.

eivindbergem commented 4 months ago

@eivindbergem - Thats everything green on the CI now, I don't know how to remove the required test for examples-stm32f103, so it still shows as not completed. Is this something you can resolve?

It says here that one test haven't completed yet, but I don't see it in the list. Not sure what's going on.

Figured it out, I had set the stm32 test to required in project settings.

m5p3nc3r commented 4 months ago

Awesome - let me know if there is anything else you want me to do to the code before merging.

eivindbergem commented 3 months ago

It looks very good now! I can squash and merge now, if you're ready.

m5p3nc3r commented 3 months ago

Sure - I'm already using it in my embassy project, all seems fine :o)

rustne-kretser / noline

Refactor to use embedded_io and embedded_io_async #22