funbiscuit / embedded-cli-rs

CLI in Rust with autocompletion, subcommands, options, help and history for embedded systems (like Arduino or STM32)
Apache License 2.0
79 stars 4 forks source link

Async Writer? #13

Closed MabezDev closed 2 months ago

MabezDev commented 6 months ago

Great library, I particularly love the attention you put into removing panicking branches! I'm looking forward to using this in an upcoming project.

Whilst it would be possible to use the blocking writer (the amount of data on a cli is quite small, so small blocking time), it would be better for my application (running async only) if the writer had the ability to write out bytes asynchronously. Have you considered how to handle this?

Finally, is there a reason you didn't use the embedded_io::Read trait to do the input byte processing? With the Read trait you could still process things one byte at a time internally if needed. By using both traits, it would be really easy to have a CLI over serial, and a telnet connection for example.

funbiscuit commented 6 months ago

I think I tried to use embedded_io::Read trait in some initial versions of lib, but that didn't meet some of my requirements. As I put in readme:

Some of the API might be a bit ugly, but I don't see a better solution for now. If you have suggestions - open an Issue or a Pull Request.

I didn't work with async in embedded yet, maybe you can suggest how api should look? It's okay if that would be a breaking change. The same goes for embedded_io::Read trait. If you have an idea of how that should look from user perspective - feel free to suggest.

aloebs29 commented 3 months ago

Re: Async support, the builder API might support an async_writer() method which takes an object implementing the embedded-io-async Write trait. Then you would just write to the async writer with something like cli.async_writer().write_str("Hello").await?.

I'm not sure how you'd handle formatting with ufmt though..

As far as the reader portion of the API goes, I don't think you'd need to do anything to add async support, as the user would be awaiting whatever async read method they're using on their I/O peripheral, and then calling cli.process_byte as they would with the current API.

funbiscuit commented 3 months ago

Then you would just write to the async writer with something like cli.async_writer().write_str("Hello").await?.

I think that's not as easy as it seems. Library itself should use this async writer under the hood (inside process_byte method). As I see it, library should have both async and sync interfaces to work correctly. And that's x2 to the code, or somewhere near it. Maybe I should try to decouple write logic so it would be easier to use async writer. Last time I tried to do that I've got a much heavier binary (more than twice as much flash size on arduino example). And I think that's not worth it.

aloebs29 commented 3 months ago

Ahh, yeah I didn't think about the fact that the library would need to use the async write under the hood. This is definitely one of those situations where "function coloring" adds a lot of friction.

For my use case (which is running the CLI from an async task) I ended up just using an in-memory buffer as my writer, and then the async task that manages the underlying peripheral just transfers bytes from that buffer to the peripheral. I would have needed to do something similar even if there was an async API, as I wanted my CLI to be owned by a separate task than the one servicing the peripheral, so it wasn't much of an inconvenience for me.

funbiscuit commented 3 months ago

You have a very good approach with buffers. Initially I tried to embed rx/tx buffers into cli itself, but then found it not very convenient. So using external rx/tx buffers is much better and solves the issue with missing async writer. Probably one day I'll document that in readme or somewhere else.

MabezDev commented 2 months ago

I also went with the buffer approach, I think it works quite well without duplicating code here in the cli.