async closures version?

[tommasoclini]

No description provided.

[Finomnis]

Please add tokio as a dev-dependency, otherwise you break no-std compatibility

[Finomnis]

Also, is there a way to support both sync and async simultaneously? Like, a .processor and a .processor_async, together with a .process_byte and a .process_byte_async?

[Finomnis]

If not, I strongly vote for this, @funbiscuit. As async seems to be the future of embedded.

For example, embassy-stm32's USB drivers only support async reads/writes, they do not offer a sync API for good reasons on how their USB implementation works.

It's cumbersome but possible to interact with USB using the current state of this crate, through queues, but it's almost impossible to execute async tasks from handler functions. Which is a very common usecase for an embedded CLI nowadays. And that's exactly what this change would fix.

[tommasoclini]

Also, is there a way to support both sync and async simultaneously? Like, a .processor and a .processor_async, together with a .process_byte and a .process_byte_async?

you can have copies of every function I had to make async

[tommasoclini]

Please add tokio as a dev-dependency, otherwise you break no-std compatibility

I'm not really interested in this crate because I don't need it at the moment, so feel free to work on this yourself, I literally changed like 20 lines of code following my editor to make thism

[funbiscuit]

If not, I strongly vote for this, @funbiscuit. As async seems to be the future of embedded.

That's a tough question, both options (drop sync support or make function duplicates) don't seem good for me. I think you can fork and manually make functions async as shown in this branch. Until a proper solution is found.

[tommasoclini]

actually it's not hard to have both blocking and async version, because blocking code can work with async code, but not viceversa.

usually blocking code is seen as more versatile and with more compatibility, but it's actually the opposite.

something else entirely is blocking code that does not block but has callbacks, like in C without an RTOS, but that's the same pattern as async.

to have a blocking version the caller can just poll once the process function, and if his code is blocking it should be always Poll::ready on the first try.

I added a process_byte_blocking function.

@Finomnis now both blocking and async code are supported.

[Finomnis]

embassy_futures::block_on is dangerous, you have to really be careful to not accidentally create priority inversions with it, otherwise you will create a deadlock. Caused so many hard to debug problems that I banned it from my codebase.

With priority inversions, I mean even threads of the same priority (in RTIC) will not run if you do not .await.

[tommasoclini]

then we can put an error like CodeNotBlocking or something like that?

[Finomnis]

Sounds better, yes :)

[tommasoclini]

@Finomnis what do you think?

[Finomnis]

I would not return Poll, but instead an error if poll is not ready. Poll is not really meant for sync APIs.

Also again, Tokio should be a dev dependency :)

[tommasoclini]

why is Poll not meant for sync APIs?

[Finomnis]

why is Poll not meant for sync APIs?

Because its only intended use is as the return value of the poll function of futures, nothing else

[tommasoclini]

@Finomnis is it fine now?

[Finomnis]

Sounds good :) question is if it will ever get merged though...

[funbiscuit]

As I said before, it does look good if we accept there is only one way to do things - use async/await. I'm not convinced with it and thats backwards incompatible change. So can't merge it as is for now.

[Finomnis]

As I said before, it does look good if we accept there is only one way to do things - use async/await. I'm not convinced with it and thats backwards incompatible change. So can't merge it as is for now.

I expected that already, it's fine :)

[tommasoclini]

I made it backwards compatible; now there is a sync process_byte method and an async process_byte_async method.
Lmk what you guys think @Finomnis @funbiscuit

[Finomnis]

If truly backwards compatible, this file should not change

[tommasoclini]

I forgot to edit the example, sorry, in fact I believe it doesn't build anymore.

[tommasoclini]

Now it should be fully working and backwards compatible.

[codecov]

Codecov Report

❌ Patch coverage is 76.92308% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.27%. Comparing base (3942aa6) to head (cbcc23c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
embedded-cli/src/service.rs	58.82%	7 Missing ⚠️
embedded-cli/src/cli.rs	90.90%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   85.44%   85.27%   -0.18%     
==========================================
  Files          15       15              
  Lines        1381     1419      +38     
==========================================
+ Hits         1180     1210      +30     
- Misses        201      209       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[funbiscuit]

@tommasoclini This looks promising, thanks. I'll take a more in depth look later.

Also I suggest adding at least some basic e2e test (or new example) that uses async version. Copy of existing example would be the best.

[tommasoclini]

@funbiscuit I tried building the arduino example first with the commit right before I started and the latest one of this PR, there's a pretty big size increase:

old:

   text	   data	    bss	    dec	    hex	filename
  18942	      0	     82	  19024	   4a50	arduino-cli.elf

new:

   text	   data	    bss	    dec	    hex	filename
  19734	      0	     82	  19816	   4d68	arduino-cli.elf

Maybe we should wait before merging.
On the desktop example the size is the same though, idk why.

[tommasoclini]

I think we should use maybe_async_cfg2 here.

[tommasoclini]

I'm closing this PR because I have opened #30 which takes a completely different approach.