Implement a generic async IO Editor

[m5p3nc3r]

Implement a generic async IO Editor for Embassy

Add a generic Editor that takes async reader/writer for IO. This makes use of the feature 'async_fn_in_trait' which requires nightly so he capability is hidden behind a feature flag and will not change the current functionality in noline.

By using this new Editor, you can implement a std based tokio system, or a no_std based Embassy system for use in embedded systems. An example of the tokio system is included in examples/std. Once the merge is accepted, I will add an example to the embassy crate for RP2040.

[eivindbergem]

I had a quick look, and it generally looks very promising. I have a few nitpicks:

• Move the new editor into no_sync. You can just remove the old tokio-specific async-editor and example, no need to have two implementations when one is sufficient. The new editor can be no_sync::Editor.
• Squash commits. Doesn't have to be a single commit, but as many commit as makes sense.

Once you've fixed this, I'll have a close look and run the code.

[m5p3nc3r]

Hey @eivindbergem, thanks for the feedback.

I was reluctant to replace the no_sync implementation with this new one because of the requirement to have the feature flag 'async_fn_in_trait', which would mandate the use of the +nightly compiler. I did try using the async_trait crate to implement, but it seemed to be incompatible with my solution. I would suggest once this feature is available in the standard compiler release we can move this to be the default implementation.

As for squashing the commits - I did try doing this, but it failed! My git-fu is not that great to be honest, so if you have a pointer to how to do this I will be more than happy to do it.

[eivindbergem]

You make a good point. Let's keep both then. You can put the new one in no_sync::async_trait. I squash commits for a living, so I can do it for you if you want.

[m5p3nc3r]

Ok - I'll do that when I get home from work this evening.

Out of interest, would you also like to see a working example of the code for embassy + rp2040?

[eivindbergem]

Yes, please! And add it to the PR. We can never have too many examples :)

[m5p3nc3r]

Ok - I'll add it to the PR as well, hopefully find time to do this tonight.

[m5p3nc3r]

Ok, I have placed the generic async code into no_sync::async_trait.

Should I create a seperate PR for the embassy example?

[eivindbergem]

The example shows how to use the async editor, so you can put it in this PR.

[m5p3nc3r]

Hey - the example code is in examples/embassy.

Please have a review and let me know what you think. Is shows a multi-function async system with flashing LED and noline attached to the USB Serial driver.

[m5p3nc3r]

I have done a bit of git training and now have the skills to squash this into two commits 🚀

• One for the changes to noline for the generic async implementation, plus an example using Tokio
• The second adding the Embassy example for RPi Pico

I split it into two as the Embassy example is functional, but not perfect yet. I have a PR waiting on embassy to enable async detection of control line changes that will enable the example to detect when the endpoint goes away,.

I believe this is Ok to commit now, and I will update the Embassy example when the upstream changes have been merged.

[eivindbergem]

The code looks very good. I just have some nitpicks:

• It should be clear from the example directory name what platform it is for. A better path would be examples/no_std/rp2040-embassy.
• The example should be added to Github workflows.
• The example shouldn't be under a proprietary license. The library itself is under MPL, but there is no reason to use a Copyleft license for the examples, so they could be under a more permissive license like MIT. The other examples don't have an explicit license, so then I guess they are covered by MPL. I'll create a separate PR to fix that.

[m5p3nc3r]

I'll change the directory, that's fine.

About the CI, the example requires a nightly version of rust, is that ok?

The licence was a copy paste error, I'll remove the explicit license.

[m5p3nc3r]

Hey @eivindbergem, I have addressed issue 1 and 3.

I am not convinced I know enough about github's CI process, especially as it will require the nightly compiler. Are you able to accept this PR and then add the CI later, or would you rather I addressed the CI now?

[m5p3nc3r]

Ok - maybe I am being a bit conservative about the CI. Would something like this work

I have just changed the toolchain from stable to nightly

diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index 579a5e3..e8e5ff9 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -97,3 +97,22 @@ jobs:

     - name: Build
       run: cargo build --verbose
+
+  examples-rp2040-embassy:
+   runs-on: ubuntu-latest
+
+  defaults:
+    run:
+      working-directory: ./examples/no_std/stm32f103
+
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: Install toolchain
+      uses: actions-rs/toolchain@v1
+      with:
+        toolchain: nightly
+        target: thumbv7m-none-eabi
+
+    - name: Build
+      run: cargo build --verbose

[eivindbergem]

Nightly for an example that requires nightly is fine. The diff looks about right, you just need to set the correct working-directory :stuck_out_tongue:

[m5p3nc3r]

Hey @eivindbergem, any chance of getting this reviewed/merged?

[konkers]

I'm interested in this patch for a project I'm working on. Is there anything left to do to merge this? Happy to help move it along.

[konkers]

Also, i believe async-fn-in-trait will be stable in 1.75 so the nightly requirements should not be needed at tht point.

[m5p3nc3r]

Hey @konkers , I believe the patch set to be good now. I am currently using it without issue on an rp2040 using Embassy with an async USB CDC function.

We just need to get this checked and merged by @eivindbergem. I understand that everybody has a day job and maintaining an open source project can be a struggle at times, which is why I have not pushed any further for an upstream review.

[eivindbergem]

Hi. Sorry for not responding. I indeed have a day job, and two kids, so I sometimes get a little overwhelmed :)

@m5p3nc3r I'm not able to start CI. I had issues with this before, with stale PR's, so you might have to push again in.

[m5p3nc3r]

It looks like a number of things have changed in the embedded-hal and embassy bindings in the release of v1.0.0 of these projects. I will do some work to upgrade to this latest version when I have some spare time (I also have a day job and family, so time is limited!)

[m5p3nc3r]

@eivindbergem, I am almost there now. I had to change the use of embedded_hal::serial to embedded_io:: as the serial interface was removed before the v1.0 release. I have it mostly working now, but I have one failure in the unit tests for embedded/sync where string_rx.recv().unwrap() is panicking because string_rx is empty.

Strangely, if I add a second keyboard_tx.send(0xd).unwrap(); just before, string_rx is no longer empty, but the test obviously fails because the input and output strings no longer match.

Do you have any ideas? If I push the still not working MR, can you have a look because I'm not convinced I integrated embedded_io properly.

[m5p3nc3r]

Also - bringing the stm32 example up to embedded-hal v1.0 seems like a big task, and I don't have any hardware to test it on. Is this something that the original author of the stm32 example would be willing to undertake please?

[m5p3nc3r]

Hey @eivindbergem, I think I'm at the limit of my knowledge working with embedded-hal directly now (I have only really used embassy for embedded rust!). I'm not sure the best way to fix the stm32 compilation failure, but I see it also fails in #21 .

Also, the failure in the sync test is probably due to me not integrating embedded_io properly.

Are you able to have a look and see if you have the rust-fu to fix this?

[eivindbergem]

I already started with updating the dependencies in another PR. It's a bit of a mess, and I got stuck at embedded_io as well. I'll give it another try.

[m5p3nc3r]

I had a quick look at trying to get the stm32 example working, but it looks like the primary problem is that the updatream stm32f1xx-hal crate has not yet been updated to the embedded-hal v1.0.0 interfaces.

There is an issue raised on the project to discuss porting to v1.0.0 stm32-rs/stm32f1xx-hal#473, but there does nott seem to be any progress on the stm32f1xxx sieries yet (others seem to be making progress on the update).

Can I suggest that the example is either migrated to an MCU rhat supports the v1.0 embedded-hal, or we disable this example until the upstream hal is updated?

[konkers]

Can I suggest that the example is either migrated to an MCU rhat supports the v1.0 embedded-hal, or we disable this example until the upstream hal is updated?

If you want to go this route, the rp-hal (https://github.com/rp-rs/rp-hal) is updated for embedded-hal v1 and rp2040 devices and boards are widely available.

[eivindbergem]

Dropping the stm32 example in favor of rp2040 is fine with me. I only added the stm32f103 example because that's what I had lying around.

@m5p3nc3r Do you have an rp2040 you can test on?

[m5p3nc3r]

Yes, I have an rp2040, I'll give it a go.

Is it going to be ok for me to use the SoC's uart instead of implementing the USB serial endpoing to make the example easier?

[konkers]

Is it going to be ok for me to use the SoC's uart instead of implementing the USB serial endpoing to make the example easier?

Seems totally fine to me as an example and setting up the USB would be more code that the noline bits :) If you're curious, I've got USB serial and picotool rebooting working in embassy: https://github.com/konkers/rp2040-0816-controller/tree/main/firmware/src/usb

[eivindbergem]

Yes, I have an rp2040, I'll give it a go.

Is it going to be ok for me to use the SoC's uart instead of implementing the USB serial endpoing to make the example easier?

No problem. That might be a more common use case anyway, and last time I used usbd_serial it didn't really work well with an interrupt-driven design, so the example became unnecessarily complex.

[eivindbergem]

Work continues in #22