Skip to content

Embedded error #218

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

Closed
wants to merge 6 commits into from
Closed

Embedded error #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
940af7c
Added embedded_error to dependencies and re-export in prelude
therealprof Sep 28, 2020
5e2eaa0
Use embedded_error::GpioError as Error type in GPIO
therealprof Sep 28, 2020
a3db2bb
Use embedded_error::SerialError as Error type in serial implementation
therealprof Sep 28, 2020
c19179e
Use embedded_error::SpiError as Error type in SPI implementation
therealprof Sep 28, 2020
0ef237e
Use embedded_error::I2cError as Error type in I2C implementation
therealprof Sep 28, 2020
d70cde9
Add entry to CHANGELOG.md
therealprof Sep 28, 2020
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Changed

- [breaking-change] Depend on `embedded-error` to provide universal error types
- [breaking-change] Updated synopsys-usb-otg dependency to v0.2.0.
- Cleanups to the Sdio driver, some hw independent functionality moved to the new sdio-host library.
- [breaking-change] Sdio is disabled by default, enable with the `sdio` feature flag.
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ stm32f4 = "0.11"
synopsys-usb-otg = { version = "0.2.0", features = ["cortex-m"], optional = true }
sdio-host = { version = "0.5.0", optional = true }
embedded-dma = "0.1.0"
embedded-error = "0.6"

[dependencies.bare-metal]
version = "0.2.5"
Expand Down
14 changes: 7 additions & 7 deletions src/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ macro_rules! gpio {
/// GPIO
pub mod $gpiox {
use core::marker::PhantomData;
use core::convert::Infallible;
use crate::prelude::*;

use embedded_hal::digital::v2::{InputPin, OutputPin, StatefulOutputPin, toggleable};
use crate::pac::$GPIOX;
Expand Down Expand Up @@ -294,7 +294,7 @@ macro_rules! gpio {
}

impl<MODE> OutputPin for $PXx<Output<MODE>> {
type Error = Infallible;
type Error = GpioError;
Copy link
Contributor

Choose a reason for hiding this comment

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

For the other errors, I see the advantages, but replacing infallible by some non empty error, that's not great. I personally into_ok (a personal implementation for stable) on these errors, and it would not be possible after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't make it uniform then we can't use this universally with drivers and applications, this is just a preview of what's to come in embedded-hal. There're plenty of cases like on a Raspberry Pi or behind GPIO expanders where GPIO is not infallible.

What is into_ok? And how is it different from .ok()? I don't think applications will change a lot but there might be a slight size overhead (not sure, really, Infallible is also not quite ideal). If you have any ideas for improvement, I'd be all ears.

Copy link
Contributor

Choose a reason for hiding this comment

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

In which case would you need to know the error? Can you point on real example? I suppose some intermediate driver between embedded_hal and another driver where you have to manage some kind of error?

https://doc.rust-lang.org/core/result/enum.Result.html#method.into_ok and my "waiting to stabilise implementation" https://github.com/TeXitoi/keyseebee/blob/master/firmware/src/main.rs#L32-L42

That's an infallible .ok().unwrap(). As I know I don't have to manage error, I can remove the complexity of managing them for nothing. And it won't be possible anymore with this modification.

Maybe we can have Error: Into<GpioError> and Infallible can implement this conversion. But it will complexify the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

In which case would you need to know the error? Can you point on real example? I suppose some intermediate driver between embedded_hal and another driver where you have to manage some kind of error?

Yes, for example. No, I don't have an example yet but support for fallible GPIOs has been requested a few times and was incorporated into embedded-hal (aka the digital::v2 debacle).

Maybe we can have Error: Into and Infallible can implement this conversion. But it will complexify the interface.

When we change the Error types in embedded-hal the bounds will be Into<GpioError> + Clone + Debug. I don't think we can implement Into<GpioError> for Infallible but if you have any idea how to solve that your input would certainly be appreciated over at rust-embedded/embedded-hal#229.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implementing Into for Infallible is easy: you just have to implement From, that you should implement if possible instead of Into anyway: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=86a6dabe6ccc820f756c03718d5e4cc4

Will try to look at the linked issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do a PR for implementing From for *Error

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be careful with doing that for the other Error. Many (most?) of the other peripherals are impossible to implement correctly in an Infallible way and I'd be wary to add a loophole to avoid a correct implementation out of the gate; we can always add it later if it makes sense.

Copy link
Member

@nickray nickray Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm not mistaken, only embedded-error can implement From<Infallible> for its types, and at least mathematically/logically, this conversion should exist (and there's only one implementation "up to canonical isomorphism"...). So I think the implementation should be added, with warnings about (peripheral) implementation correctness, no?

Copy link
Member Author

@therealprof therealprof Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nickray Yes, adding it to embedded-error and using Infallible at least for GPIO is what we were talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made a different proposition: rust-embedded/embedded-hal#229 (comment)

I'll wait a bit the feedbacks before doing a PR to embedded-error, if you don't mind.


fn set_high(&mut self) -> Result<(), Self::Error> {
// NOTE(unsafe) atomic write to a stateless register
Expand Down Expand Up @@ -323,7 +323,7 @@ macro_rules! gpio {
impl<MODE> toggleable::Default for $PXx<Output<MODE>> {}

impl<MODE> InputPin for $PXx<Output<MODE>> {
type Error = Infallible;
type Error = GpioError;

fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
Expand All @@ -336,7 +336,7 @@ macro_rules! gpio {
}

impl<MODE> InputPin for $PXx<Input<MODE>> {
type Error = Infallible;
type Error = GpioError;

fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
Expand Down Expand Up @@ -755,7 +755,7 @@ macro_rules! gpio {
}

impl<MODE> OutputPin for $PXi<Output<MODE>> {
type Error = Infallible;
type Error = GpioError;

fn set_high(&mut self) -> Result<(), Self::Error> {
// NOTE(unsafe) atomic write to a stateless register
Expand Down Expand Up @@ -784,7 +784,7 @@ macro_rules! gpio {
impl<MODE> toggleable::Default for $PXi<Output<MODE>> {}

impl<MODE> InputPin for $PXi<Output<MODE>> {
type Error = Infallible;
type Error = GpioError;

fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
Expand All @@ -797,7 +797,7 @@ macro_rules! gpio {
}

impl<MODE> InputPin for $PXi<Input<MODE>> {
type Error = Infallible;
type Error = GpioError;

fn is_high(&self) -> Result<bool, Self::Error> {
self.is_low().map(|v| !v)
Expand Down
66 changes: 29 additions & 37 deletions src/i2c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use core::ops::Deref;
use embedded_hal::blocking::i2c::{Read, Write, WriteRead};

use crate::prelude::*;

use crate::{bb, pac::i2c1};

#[cfg(any(
Expand Down Expand Up @@ -546,16 +548,6 @@ impl PinScl<FMPI2C> for PF14<AlternateOD<AF4>> {}
#[cfg(any(feature = "stm32f413", feature = "stm32f423",))]
impl PinScl<FMPI2C> for PF15<AlternateOD<AF4>> {}

#[derive(Debug)]
pub enum Error {
OVERRUN,
NACK,
TIMEOUT,
BUS,
CRC,
ARBITRATION,
}

#[cfg(any(
feature = "stm32f401",
feature = "stm32f405",
Expand Down Expand Up @@ -786,39 +778,39 @@ where
self.i2c.cr1.modify(|_, w| w.pe().set_bit());
}

fn check_and_clear_error_flags(&self) -> Result<i2c1::sr1::R, Error> {
fn check_and_clear_error_flags(&self) -> Result<i2c1::sr1::R, I2cError> {
// Note that flags should only be cleared once they have been registered. If flags are
// cleared otherwise, there may be an inherent race condition and flags may be missed.
let sr1 = self.i2c.sr1.read();

if sr1.timeout().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.timeout().clear_bit());
return Err(Error::TIMEOUT);
return Err(I2cError::Timeout);
}

if sr1.pecerr().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.pecerr().clear_bit());
return Err(Error::CRC);
return Err(I2cError::PacketErrorChecking);
}

if sr1.ovr().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.ovr().clear_bit());
return Err(Error::OVERRUN);
return Err(I2cError::Overrun);
}

if sr1.af().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.af().clear_bit());
return Err(Error::NACK);
return Err(I2cError::NACK);
}

if sr1.arlo().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.arlo().clear_bit());
return Err(Error::ARBITRATION);
return Err(I2cError::ArbitrationLoss);
}

if sr1.berr().bit_is_set() {
self.i2c.sr1.modify(|_, w| w.berr().clear_bit());
return Err(Error::BUS);
return Err(I2cError::Bus);
}

Ok(sr1)
Expand All @@ -830,18 +822,18 @@ where
}

trait I2cCommon {
fn write_bytes(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error>;
fn write_bytes(&mut self, addr: u8, bytes: &[u8]) -> Result<(), I2cError>;

fn send_byte(&self, byte: u8) -> Result<(), Error>;
fn send_byte(&self, byte: u8) -> Result<(), I2cError>;

fn recv_byte(&self) -> Result<u8, Error>;
fn recv_byte(&self) -> Result<u8, I2cError>;
}

impl<I2C, PINS> I2cCommon for I2c<I2C, PINS>
where
I2C: Deref<Target = i2c1::RegisterBlock>,
{
fn write_bytes(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
fn write_bytes(&mut self, addr: u8, bytes: &[u8]) -> Result<(), I2cError> {
// Send a START condition
self.i2c.cr1.modify(|_, w| w.start().set_bit());

Expand Down Expand Up @@ -882,7 +874,7 @@ where
Ok(())
}

fn send_byte(&self, byte: u8) -> Result<(), Error> {
fn send_byte(&self, byte: u8) -> Result<(), I2cError> {
// Wait until we're ready for sending
while {
// Check for any I2C errors. If a NACK occurs, the ADDR bit will never be set.
Expand All @@ -901,7 +893,7 @@ where
Ok(())
}

fn recv_byte(&self) -> Result<u8, Error> {
fn recv_byte(&self) -> Result<u8, I2cError> {
while {
// Check for any potential error conditions.
self.check_and_clear_error_flags()?;
Expand All @@ -918,7 +910,7 @@ impl<I2C, PINS> WriteRead for I2c<I2C, PINS>
where
I2C: Deref<Target = i2c1::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> {
self.write_bytes(addr, bytes)?;
Expand All @@ -932,7 +924,7 @@ impl<I2C, PINS> Write for I2c<I2C, PINS>
where
I2C: Deref<Target = i2c1::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> {
self.write_bytes(addr, bytes)?;
Expand All @@ -952,7 +944,7 @@ impl<I2C, PINS> Read for I2c<I2C, PINS>
where
I2C: Deref<Target = i2c1::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> {
if let Some((last, buffer)) = buffer.split_last_mut() {
Expand Down Expand Up @@ -1003,7 +995,7 @@ where
// Fallthrough is success
Ok(())
} else {
Err(Error::OVERRUN)
Err(Self::Error::Overrun)
}
}
}
Expand Down Expand Up @@ -1073,19 +1065,19 @@ where
(self.i2c, self.pins)
}

fn check_and_clear_error_flags(&self, isr: &fmpi2c::isr::R) -> Result<(), Error> {
fn check_and_clear_error_flags(&self, isr: &fmpi2c::isr::R) -> Result<(), I2cError> {
// If we received a NACK, then this is an error
if isr.nackf().bit_is_set() {
self.i2c
.icr
.write(|w| w.stopcf().set_bit().nackcf().set_bit());
return Err(Error::NACK);
return Err(I2cError::NACK);
}

Ok(())
}

fn send_byte(&self, byte: u8) -> Result<(), Error> {
fn send_byte(&self, byte: u8) -> Result<(), I2cError> {
// Wait until we're ready for sending
while {
let isr = self.i2c.isr.read();
Expand All @@ -1100,7 +1092,7 @@ where
Ok(())
}

fn recv_byte(&self) -> Result<u8, Error> {
fn recv_byte(&self) -> Result<u8, I2cError> {
while {
let isr = self.i2c.isr.read();
self.check_and_clear_error_flags(&isr)?;
Expand All @@ -1117,9 +1109,9 @@ impl<I2C, PINS> WriteRead for FMPI2c<I2C, PINS>
where
I2C: Deref<Target = fmpi2c::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Error> {
fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> {
// Set up current slave address for writing and disable autoending
self.i2c.cr2.modify(|_, w| unsafe {
w.sadd1_7()
Expand Down Expand Up @@ -1187,9 +1179,9 @@ impl<I2C, PINS> Read for FMPI2c<I2C, PINS>
where
I2C: Deref<Target = fmpi2c::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Error> {
fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> {
// Set up current address for reading
self.i2c.cr2.modify(|_, w| unsafe {
w.sadd1_7()
Expand Down Expand Up @@ -1223,9 +1215,9 @@ impl<I2C, PINS> Write for FMPI2c<I2C, PINS>
where
I2C: Deref<Target = fmpi2c::RegisterBlock>,
{
type Error = Error;
type Error = I2cError;

fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> {
// Set up current slave address for writing and enable autoending
self.i2c.cr2.modify(|_, w| unsafe {
w.sadd1_7()
Expand Down
1 change: 1 addition & 0 deletions src/prelude.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub use embedded_error::*;
pub use embedded_hal::digital::v2::InputPin as _embedded_hal_digital_v2_InputPin;
pub use embedded_hal::digital::v2::OutputPin as _embedded_hal_digital_v2_OutputPin;
pub use embedded_hal::digital::v2::StatefulOutputPin as _embedded_hal_digital_v2_StatefulOutputPin;
Expand Down
Loading