Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async serial #469

Open
djdisodo opened this issue Dec 22, 2023 · 9 comments
Open

async serial #469

djdisodo opened this issue Dec 22, 2023 · 9 comments

Comments

@djdisodo
Copy link

djdisodo commented Dec 22, 2023

repo

i just uploaded half baked async serial support crate that only supports atmega328p

since implementation is very minimal i think it's possible to merge this thing into avr-hal(feature gated)
i want to as some opinion @Rahix

i didn't implemented async read since i couldn't come up with idea that can cover variety of use cases without overhead

best i could think was to wrap rx interrupt so user can receive byte every interrupt without directly dealing with registers

@Rahix
Copy link
Owner

Rahix commented Dec 25, 2023

Very cool! I think with embedded-hal moving to 1.0, it makes sense to also provide async impls in our HAL where appropriate. I guess we should take a look at other HALs to see how they are treating async traits and follow their path.

i didn't implemented async read since i couldn't come up with idea that can cover variety of use cases without overhead

best i could think was to wrap rx interrupt so user can receive byte every interrupt without directly dealing with registers

same here, how do other HALs handle this?

@djdisodo
Copy link
Author

some of them allocate ram for internal buffer
but avrs have exceptionally small ram

i think we need internal buffer if we want to avoid dropping bytes while not reading, or use some kind of flow control(but other end must support same flow control as well)

however if we need some protocol for serial, it's better to process them on interrupt function to avoid use of two buffer, one that has data, one that has encoded data,

this also applies to writing

it's going to be huge bloat if we try to support all kinds of protocol from interrupt function

@antonio-sc66
Copy link

antonio-sc66 commented Jan 17, 2024

Hi @djdisodo, in Arduino's library for AVRs (C++), the HardwareSerial implementation creates a read buffer and a write buffer with sizes defined on compilation time using #define statements. Interrupts then write or read to and from those buffers.

This buffers are asociated with each object, so everytime a new HardwareSerial instance is created, more RAM gets reserved.

Arduino platform is very popular and most people use the default libraries, so that kind of implementation should work fairly well.

When writing to the RX buffer, two options can be considered:

  • Wrapping mode: Delete oldest byte
  • Discard mode: Discard byte if buffer is full

There's not good or bad option, it will depend on the user needs, but I think discarding is more popular.

About flow control, that has to be implemented in a different abstraction level, not into the reading and writing part.

The problem I see right now with interrupts and elements like buffers is that you have to use global variables, which makes the code harder to manage, specially when dealing with static mut.
@Rahix, is there an alternative way to use interrupt functions?. Can they be integrated into struct implementation, so that they can access the struct fields?

The async version should also work with buffers.

@djdisodo
Copy link
Author

@antonio-sc66

This buffers are asociated with each object
this sounds like it might need alloc

or it would be great if we can pin that buffer or get notified when buffer moved(i'm not sure)

flow control is going to be inefficient if it's implemented in higher abstraction layer on my code

this is because my code doesn't use buffer, every single write() is going to start writing when called stop writing when before return

we don't have to care if it's going to have buffer

When writing to the RX buffer, two options can be considered:

discard and return error when it tries to read discarded bytes
by returning error, we can notify user that something had gone wrong it's going to help user debugging (it's just safe to ensure data integrity)

@antonio-sc66
Copy link

@djdisodo, you don't need alloc to implement those buffers, there are static no_std buffer and cyclic buffer implementations available. Implementing one is not complex either. One thing to keep in mind is that a fixed size has to be chosen at compile time, and it could be different for different MCUs.

Buffers can be uninitialized at first with something like MaybeUninit and then they can be initialized when new() is called on the serial handling struct implementation. They can be droped again by implementing the Drop destructor.

Unfortunately, it's a bit cumbersome to work with globals, specially with Rust type safety and borrowing rules

@djdisodo
Copy link
Author

@antonio-sc66

This buffers are asociated with each object, so everytime a new HardwareSerial instance is created, more RAM gets reserved.

i'm talking about this part
using static buffer will always reserve ram

@djdisodo
Copy link
Author

djdisodo commented Jan 25, 2024

repo
updated version with buffer on same repo

    let serial = UsartDriver::new(dp.USART0, Baudrate::<DefaultClock>::new(57600));

    let mut write = serial.write().unwrap();
    let mut read = serial.read().unwrap();
    let mut buf = [0u8; 8];
    write.write_all(b"loaded").await.unwrap();
    loop {
        let bytes = read.read(&mut buf).await.unwrap();
        write.write_all(&buf[..bytes]).await.unwrap();
    }

you better try this on putty than ravedude

since putty sends not text input but every key press

i tried to write whole myself, ended up being bit messy

also tried to reduce ram usage

by default it has 28bytes of static ram usage(uno has only 1 usart)

async, i tried to run this code alongside with my async lcd clock demo

can't even notice difference even when transmitting ton of texts

review plz

@JonasFocke01
Copy link

@djdisodo What is the content of your env_int file? i want to test your library

@djdisodo
Copy link
Author

@djdisodo What is the content of your env_int file? i want to test your library

https://github.com/djdisodo/env_int

it's this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants