Skip to content

implement FullDuplex for Spi #122

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

implement FullDuplex for Spi #122

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 75 additions & 22 deletions src/spi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,17 +364,27 @@ where
fn check_read(&mut self) -> nb::Result<(), Error> {
let sr = self.spi.sr.read();

Err(if sr.ovr().bit_is_set() {
nb::Error::Other(Error::Overrun)
self.check_errors()?;

if !sr.rxne().bit_is_set() {
Err(nb::Error::WouldBlock)
} else {
Ok(())
}
}

fn check_errors(&mut self) -> Result<(), Error> {
let sr = self.spi.sr.read();

if sr.ovr().bit_is_set() {
Err(Error::Overrun)
} else if sr.modf().bit_is_set() {
nb::Error::Other(Error::ModeFault)
Err(Error::ModeFault)
} else if sr.crcerr().bit_is_set() {
nb::Error::Other(Error::Crc)
} else if sr.rxne().bit_is_set() {
return Ok(());
Err(Error::Crc)
} else {
nb::Error::WouldBlock
})
Ok(())
}
}

fn send_buffer_size(&mut self) -> u8 {
Expand All @@ -393,17 +403,13 @@ where
fn check_send(&mut self) -> nb::Result<(), Error> {
let sr = self.spi.sr.read();

Err(if sr.ovr().bit_is_set() {
nb::Error::Other(Error::Overrun)
} else if sr.modf().bit_is_set() {
nb::Error::Other(Error::ModeFault)
} else if sr.crcerr().bit_is_set() {
nb::Error::Other(Error::Crc)
} else if sr.txe().bit_is_set() && sr.bsy().bit_is_clear() {
return Ok(());
self.check_errors()?;

if !(sr.txe().bit_is_set() && sr.bsy().bit_is_clear()) {
Err(nb::Error::WouldBlock)
} else {
nb::Error::WouldBlock
})
Ok(())
}
}

fn read_u8(&mut self) -> u8 {
Expand Down Expand Up @@ -453,6 +459,29 @@ where
}
}

impl<SPI, SCKPIN, MISOPIN, MOSIPIN> ::embedded_hal::spi::FullDuplex<u8>
for Spi<SPI, SCKPIN, MISOPIN, MOSIPIN, EightBit>
where
SPI: Deref<Target = SpiRegisterBlock>,
{
type Error = Error;

fn read(&mut self) -> nb::Result<u8, Error> {
self.check_read()?;
Ok(self.read_u8())
}

fn send(&mut self, byte: u8) -> nb::Result<(), Error> {
// We want to transfer bidirectionally, make sure we're in the correct mode
self.set_bidi();

self.check_send()?;
self.send_u8(byte);

self.check_errors().map_err(|e| nb::Error::Other(e))
}
}

impl<SPI, SCKPIN, MISOPIN, MOSIPIN> ::embedded_hal::blocking::spi::Write<u8>
for Spi<SPI, SCKPIN, MISOPIN, MOSIPIN, EightBit>
where
Expand Down Expand Up @@ -481,8 +510,33 @@ where
}

// Do one last status register check before continuing
nb::block!(self.check_send()).ok();
Ok(())
self.check_errors()
}
}

impl<SPI, SCKPIN, MISOPIN, MOSIPIN> ::embedded_hal::spi::FullDuplex<u16>
for Spi<SPI, SCKPIN, MISOPIN, MOSIPIN, SixteenBit>
where
SPI: Deref<Target = SpiRegisterBlock>,
{
type Error = Error;

fn read(&mut self) -> nb::Result<u16, Error> {
self.check_read()?;
Ok(self.read_u16())
}

fn send(&mut self, byte: u16) -> nb::Result<(), Error> {
// We want to transfer bidirectionally, make sure we're in the correct mode
self.set_bidi();

self.check_send()?;
self.send_u16(byte);

match self.check_errors() {
Ok(_) => Ok(()),
Err(e) => Err(nb::Error::Other(e)),
}
Comment on lines +536 to +539
Copy link
Member

@therealprof therealprof Nov 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Couldn't this be written more obviously as:

Suggested change
match self.check_errors() {
Ok(_) => Ok(()),
Err(e) => Err(nb::Error::Other(e)),
}
self.check_errors().map_err(|e| nb::Error::Other(e))

Copy link
Author

Choose a reason for hiding this comment

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

I thought of that, but it seems you can only use it to change the error, not the Result type wrapping everything (Result vs nb::Result here). I get this:

rror[E0308]: mismatched types
   --> src/spi.rs:482:9
    |
475 |     fn send(&mut self, byte: u8) -> nb::Result<(), Error> {
    |                                     --------------------- expected `core::result::Result<(), nb::Error<spi::Error>>` because of return type
...
482 |         self.check_errors().map_err(|e| Err(nb::Error::Other(e)))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `nb::Error`, found enum `core::result::Result`

But maybe I'm not doing something correctly ?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my bad. Cut'n'pasto. Of course it needs to be (corrected above, too):

self.check_errors().map_err(|e| nb::Error::Other(e))

map_err() automatically wraps the value returned from the closure in an Err, cf. here: https://doc.rust-lang.org/src/core/result.rs.html#592

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the map_err isn't needed at all. Check for example the check_send method, where check_errors is called directly with the ? operator. There might be an automatic Into trait implementation for it.

Copy link
Author

Choose a reason for hiding this comment

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

It works with ? yes (and I was surprised), but not if using the statement for returning (ie. without ?). I'll fix the map_err except if you have something better in mind ?

Copy link
Member

Choose a reason for hiding this comment

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

I believe that the map_err isn't needed at all. Check for example the check_send method, where check_errors is called directly with the ? operator. There might be an automatic Into trait implementation for it.

Depends on the signature. If we're replying the right Result type then we can just use it directly. Indeed we can also use ? if we have proper conversion implementations available, however IIRC using ? as the last operation in a function is frowned upon and linted by clippy. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, I missed that it was the last command in function. LGTM then with map_err.

}
}

Expand Down Expand Up @@ -525,7 +579,6 @@ where
}

// Do one last status register check before continuing
nb::block!(self.check_send()).ok();
Ok(())
self.check_errors()
}
}
Loading