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

Add a basic RP "read to break" function #2311

Merged
merged 6 commits into from
Jan 19, 2024

Conversation

jamesmunns
Copy link
Contributor

This adds a function called "read_to_break" that receives until one of the following happen:

  1. We receive the whole buffer amount
  2. We receive a line break interrupt

In the latter case, we figure out how many bytes we DID get, and then return that subslice. In the former case, we return the whole subslice.

All other errors are returned the same.

One note:

This is basically a big copy/paste of read. I should probably clean that up, I could do the DMA reg check in a function that basically does:

pub async fn read_to_break<'a>(&mut self, buf: &'a mut [u8]) -> Result<&'a mut [u8], Error> {
    match self.read(buf) {
        Ok(()) => Ok(buf),
        Err(Break) => {
            // get bytes received...
            Ok(&mut buf[..count])
        }
        Err(e) => Err(e)
    }
}

But then we'd have to unwrap self.rx_dma again, which is annoying. Open to thoughts.

@lights0123
Copy link
Contributor

what about calling it read_until_idle like stm32 already does?

pub async fn read_until_idle(&mut self, buffer: &mut [u8]) -> Result<usize, Error>

@jamesmunns
Copy link
Contributor Author

what about calling it read_until_idle like stm32 already does?

So @lights0123, this interface waits for a line break interrupt (the line is held LOW for >2 character times), rather than an idle interrupt (where the line is held HIGH with no data for >32 bit times).

In theory, the RP2040 also has an idle interrupt, but it is difficult to use with DMA (the interrupt doesn't fire if the the FIFO is empty, which it often will be DMA), which is why I wanted to use line breaks instead.

@jamesmunns
Copy link
Contributor Author

Also, I did try reading some other registers, but it seems they get invalidated when the transaction is aborted:

// Begin "James is a chicken" region - I'm not certain if there is ever
// a case where the write addr WOULDN'T exist between the start and end.
// This assert checks that and hasn't fired (yet).
let sval = buffer.as_ptr() as usize;
let eval = sval + buffer.len();
// Note: the `write_addr()` is where the NEXT write would be, BUT we also
// received one extra byte that represents the line break.
let val = ch.regs().write_addr().read() as usize - 1;
assert!((val >= sval) && (val <= eval));
let taken = val - sval;
let txns = ch.regs().trans_count().read();
let a = ch.regs().dbg_ctdreq().read().0;

defmt::println!("taken: {=usize} {=u32} {=u32}", taken, txns, a);

Gives:

...
taken: 70 0 0
taken: 84 0 0
taken: 14 0 0
taken: 28 0 0
taken: 42 0 0
taken: 56 0 0
...

Where the values calculated from write_addr seem to always be correct.

@jamesmunns
Copy link
Contributor Author

Note that raspberrypi/pico-feedback#367 is related to some of the impl details of this PR.

@jamesmunns
Copy link
Contributor Author

Just noting that this should be ready for review whenever.

@jamesmunns
Copy link
Contributor Author

Also noting that my usage of UARTRSR IS consistent with the Arm PrimeCell UART PL011 docs, and it appears this was just a deficiency of the RP2040 datasheet which didn't copy all of the details from the upstream docs:

https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/receive-status-register---error-clear-register--uartrsr-uartecr?lang=en

Comment:

raspberrypi/pico-feedback#367 (comment)

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

I think this is good to go. Earlier review comments seems to be addressed and I don't really have anything to add. Thanks for the excellent code comments, it really helped reviewing the code as well.

@lulf lulf enabled auto-merge January 19, 2024 11:19
@lulf
Copy link
Member

lulf commented Jan 19, 2024

bender run

1 similar comment
@Dirbaio
Copy link
Member

Dirbaio commented Jan 19, 2024

bender run

@Dirbaio
Copy link
Member

Dirbaio commented Jan 19, 2024

@jamesmunns can you rebase so it picks up the new ci jobs?

@jamesmunns
Copy link
Contributor Author

@Dirbaio back at you

@Dirbaio
Copy link
Member

Dirbaio commented Jan 19, 2024

@jamesmunns missing docs :D

@lulf lulf added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@Dirbaio Dirbaio merged commit 1e16661 into embassy-rs:main Jan 19, 2024
@jamesmunns jamesmunns deleted the james/read_to_break branch January 19, 2024 15:04
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.

4 participants