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

implement embedded_io::ReadReady and WriteReady traits for uart peripheral #837

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

antbern
Copy link
Contributor

@antbern antbern commented Aug 22, 2024

Implements embedded_io::ReadReady and WriteReady traits for split and unsplit uart peripheral in terms of the already existing functions for checking if the uart is ready to read or write 🚀

Solves #836

@jannic
Copy link
Member

jannic commented Aug 22, 2024

The PR itself looks good.

However while reviewing it I noticed that we probably have a bug in our implementation of embedded_io::Write:

    fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> {
        self.write_full_blocking(buf);
        Ok(buf.len())
    }

This will block if there's not enough FIFO space to hold the whole buf.

But that's not compatible with the definition of WriteReady: "If this returns true, it’s guaranteed that the next call to Write::write will not block."

As WriteReady can't know how many bytes the next call to write will contain, it can only give that guarantee if write returns without blocking as long as it can write a single byte. So write must not be implemented in terms of write_full_blocking.

(Fixed by #838)

@thejpster thejpster merged commit e29c2c5 into rp-rs:main Aug 22, 2024
22 checks passed
jannic added a commit to jannic/rp-hal that referenced this pull request Aug 24, 2024
The updates from rp-rs#798, rp-rs#837 and rp-rs#838 were only applied to rp2040-hal.
This patch ports them to rp235x-hal.
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.

3 participants