Skip to content

Conversation

@alexiionescu
Copy link
Contributor

serial uart read for embassy async

Also fix an issue with async write on large buffers, was panicking with Err Would Block. Discovered when testing reading and echoing back a large reader buffer.
Tested on ESP32C3 (WROOM) only using a separate project. Examples do not work on riscv at the moment (see issue #604)

I will use this on a separate larger project that requires a serial bridge between PC and a network of ESP32C3 (WROOM) communicating via ESP-NOW. I might revisit it and fix some issues if I find any.

NOTE: embassy_hal_async does not have a Read trait for serial, only Write traits, so I could not follow the same implementation pattern as in the write case. The new function is called read, not sure if it is Ok, there was not a read function for nb implementation. If you think is to generic, I can change it to read_async

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@alexiionescu alexiionescu changed the title Async read Async serial uart read Jun 22, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! This is a good first iteration but I have a few comments I think we should address :).

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nearly there, just a few more little bits to address :)

.await;

while self.uart.get_rx_fifo_count() > 0 && read_bytes < buf.len() {
buf[read_bytes] = unsafe {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use read() here? We could do while let Ok(byte) = self.read() instead of duplicating some code.

where
T: Instance,
{
pub async fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document that read might not fill buf completely.

Copy link
Contributor Author

@alexiionescu alexiionescu Jun 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will also insert an error case Error::ReadBufferFull when provided buffer size is not enough.

@MabezDev
Copy link
Member

Did you mean to close this @alexiionescu?

@alexiionescu
Copy link
Contributor Author

alexiionescu commented Jun 24, 2023

No, I messed up my pull request branch by merging the latest from upstream in order to run the examples.
Bad Idea !!!
Now I have rebased to the initial commit before merge and staged all my modifications again. After I am done I need to force push it again I guess and discard all other changes.I read this might re-open the PR?
Any better idea what to do in this case?

Also someone has rewrite the examples to use the same type on all examples and I have already merged that changed

    interrupt::enable(Interrupt::UART0, interrupt::Priority::Priority1).unwrap();

I can keep the merging, although I guess the CI will fail.

@alexiionescu
Copy link
Contributor Author

I have pushed the commit before the force-push closed. I don't think I can re-open it myself.

@alexiionescu
Copy link
Contributor Author

I have also pushed my latest changes based on your review. Please let me know If I need to make a new PR or you can re-open this.

@alexiionescu alexiionescu mentioned this pull request Jun 26, 2023
8 tasks
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

Successfully merging this pull request may close these issues.

2 participants