ftdi-rs / ftdi-embedded-hal

Implementation of Rust Embedded HAL traits for FTDI devices
MIT License
36 stars 14 forks source link

generic backend support #2

Closed geomatsi closed 3 years ago

geomatsi commented 3 years ago

Hi @newAM

Work had me busy this week, so this PR is still a WIP that needs a lot of cleanup. On the other hand, overall approach is already shaping up. So it is a good time to get feedback before proceeding with cleanup. By the way, new examples work fine with both libftd2xx and ftdi-rs backends.

Here are some highlights:

Obvious things that need to be modified during cleanup:

Thoughts ? Comments ? Concerns ?

Regards, Sergey

newAM commented 3 years ago

For the errors I was thinking something like this:

use std::error::Error;
use std::fmt;
use std::io;

// might not be possible to do this without GATs
// will have to use the expanded result if this doesn't work yet.
pub type Result<T, E> = std::result::Result<T, FtHalError<E>>;

#[derive(Debug)]
pub enum FtHalError<E: Error> {
    HAL(ErrorKind),
    Io(io::Error),
    Backend(E),
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum ErrorKind {
    InvalidParams,
    InvalidClock,
    BusBusy,
    I2cNoAck,
    GpioPinBusy,
    GpioInvalidPin,
    SpiModeNotSupported,
}

impl ErrorKind {
    fn as_str(&self) -> &str {
        match *self {
            ErrorKind::InvalidParams => "Invalid input params",
            ErrorKind::BusBusy => "Bus is busy",
            ErrorKind::InvalidClock => "Clock is not valid",
            ErrorKind::I2cNoAck => "No ACK from slave",
            ErrorKind::GpioPinBusy => "GPIO pin is already in use",
            ErrorKind::GpioInvalidPin => "No such GPIO pin",
            ErrorKind::SpiModeNotSupported => "Mode not supported",
        }
    }
}

impl<E: Error> fmt::Display for FtHalError<E> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            FtHalError::Io(err) => err.fmt(f),
            FtHalError::Backend(err) => err.fmt(f),
            FtHalError::HAL(err) => write!(f, "A regular error occurred {:?}", err.as_str()),
        }
    }
}

impl<E: Error> From<io::Error> for FtHalError<E> {
    fn from(e: io::Error) -> Self {
        FtHalError::Io(e)
    }
}

#[cfg(feature = "ftdi")]
impl From<ftdi::Error> for FtHalError<ftdi::Error> {
    fn from(e: ftdi::Error) -> Self {
        FtHalError::Backend(e)
    }
}

#[cfg(feature = "libftd2xx")]
impl From<libftd2xx::TimeoutError> for FtHalError<libftd2xx::TimeoutError> {
    fn from(e: libftd2xx::TimeoutError) -> Self {
        FtHalError::Backend(e)
    }
}

This way both FtHalError<ftdi::Error> and FtHalError<libftd2xx::TimeoutError> types can be enabled/disabled independently.

Thoughts?

geomatsi commented 3 years ago

This looks good to me. On the other hand, as far as I remember, during the original discussion we decided to avoid using compile-time backend selection in favor of runtime approach. But this type of error handling brings us back to compile-time features.

newAM commented 3 years ago

This looks good to me. On the other hand, as far as I remember, during the original discussion we decided to avoid using compile-time backend selection in favor of runtime approach. But this type of error handling brings us back to compile-time features.

What I wanted to avoid was having mutually exclusive compile-time backends (e.g. libftdi OR libftd2xx, but not both) because Cargo recommend avoiding this at all costs: https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features

There are rare cases where features may be mutually incompatible with one another. This should be avoided if at all possible, because it requires coordinating all uses of the package in the dependency graph to cooperate to avoid enabling them together.

Compile time features themselves are ok, I wanted to avoid mutually exclusive features because that can leads to difficulties with the rust 2018 feature resolver and feature unification.

Originally I mentioned not having libftd2xx/libftdi as dependencies of the HAL, but that isn't possible with the level of abstraction ftdi-mpsse provides. That changes the original idea because it does mean we will need something like this in the dependencies now:

libftd2xx = { version = "~0.31.0", optional = true }
ftdi = { version = "^0.1", optional = true }

I also wanted to avoid requiring any specific back-end because users should only have to link what they use (only link libftdi if using libftdi, only link libftd2xx if using libftd2xx).

Sorry for the confusion, a lot of the topics I brought up in the original discussion were just ideas (such as not having libftd2xx/libftdi as a dependency). I am flexible on all those ideas! I only have two requirements which I am not flexible on:

  1. This does not use mutually exclusive compile time features.
  2. This does not require any specific backend to compile. It should compile if no feature flags are selected, without linking libftd2xx or libftdi; even though this would be useless in practice.
geomatsi commented 3 years ago

Ok, thanks for clarifying, all makes sense to me. I hope you don't mind if I keep those two features mutually exclusive in examples, like it is done now. This is just to avoid duplicating every example for every backend.

Concerning implementation of the suggested error approach. I need your advice on that because I am not familiar enough with Rust generic data types. Using my simple error implementation was fairly straightforward, e.g. in lib.rs:

impl<Device: MpsseCmdExecutor> FtHal<Device>
    where FtHalError: From<<Device as MpsseCmdExecutor>::Error>,
{
    ...
    pub fn init_default(device: Device) -> Result<FtHal<Device>> {
       ...
    }
    ...
}

In your implementation I don't quite understand how to handle error generic type parameter E.

geomatsi commented 3 years ago

I still have troubles implementing your approach. So I looked around for simpler options. One of them is boxing errors. This approach also does not require explicit backend variants in HAL error enum type. Besides, it appears to be working fine without any changes for the rest of the code:

use std::error::Error;

pub type Result<T> = std::result::Result<T, FtHalError>;

#[derive(Debug)]
pub enum FtHalError {
    HAL(ErrorKind),
    Io(io::Error),
    Backend(Box<dyn Error>)
}

...

#[cfg(feature = "ftdi-lib")]
impl From<ftdi::Error> for FtHalError {
    fn from(e: ftdi::Error) -> Self {
        //FtHalError::FTDI(e)
        FtHalError::Backend(Box::new(e))
    }
}

#[cfg(feature = "ftd2-lib")]
impl From<libftd2xx::TimeoutError> for FtHalError{
    fn from(e: libftd2xx::TimeoutError) -> Self {
        //FtHalError::FTD2XX(e)
        FtHalError::Backend(Box::new(e))
    }
}
newAM commented 3 years ago

I still have troubles implementing your approach. So I looked around for simpler options. One of them is boxing errors. This approach also does not require explicit backend variants in HAL error enum type.

I made a demo branch here where it implements it without boxing for the GPIOs (did not have time today to do SPI and I2C, but they should follow the same pattern): https://github.com/ftdi-rs/ftdi-embedded-hal/tree/generic-backend-support-no-box

geomatsi commented 3 years ago

Thanks for the example. It works just fine. I have updated MR using your error handling approach. Both spi and i2c examples are working fine with appropriate dev branches for libftd2xx-rs and ftdi-rs. This is still WIP: at least init API should be improved and documentation should be added.

geomatsi commented 3 years ago

Looks good to me, just the cleanup items left.

Let me know if you want any help with the cleanup/writing docs, ect.

Work had me busy again, so my progress slowed down a bit. But I hope I will catch up next week with init API and docs. By the way, could you please remind me what needs to be done before libftd2xx can be released ?

newAM commented 3 years ago

could you please remind me what needs to be done before libftd2xx can be released ?

Nothing at the moment, I just have not make a release because it has not been required for anything. When this is close to completion I can make a release of libft2xx so that this can be released too.

geomatsi commented 3 years ago

Hi @newAM

I removed WIP since I added more or less everything that I planned to add in this round. However this PR still can not be merged as-is due to the following reasons:

Anyway, review is welcome )

Regards, Sergey

newAM commented 3 years ago

I had meant to do the review today but I got distracted, sorry 😅 I will get to it this weekend at the latest.

geomatsi commented 3 years ago

Thanks for review !

The docstring in src/lib.rs and README.md are out of sync though, run cargo readme > README.md to fix that.

Thanks for catching. I forgot to update after the last docs fixes. Done and force-pushed.

For libftdi that one looks like it might take a while. We could fork while we wait if you wanted. What are you thoughts on that?

Well, I think we can wait for a while. I will ping the author of ftdi-rs once more during this weekend.

Regards, Sergey

newAM commented 3 years ago

Sounds good!

I am subscribed to the ftdi-rs pull-request as well, when we start to get traction there I will make the libftd2xx release.

geomatsi commented 3 years ago

Hi @newAM ,

I switched to release versions of libftd2xx and ftdi-rs, then added yet another spi example. It looks like we are all set here. Let me know if you have any comments or concerns.

Regards, Sergey

geomatsi commented 3 years ago

Hi @newAM

Want me to create a release after you merge this?

Yes, could you please make a release ? I guess you may want to keep something (history or numbering or crates.io page ?) across ftd2xx-embedded-hal and new ftdi-embedded-hal crate. Next time I will learn the publish procedure to be able to do it without disturbing you.

Regards, Sergey

newAM commented 3 years ago

Yes, could you please make a release ? I guess you may want to keep something (history or numbering or crates.io page ?) across ftd2xx-embedded-hal and new ftdi-embedded-hal crate. Next time I will learn the publish procedure to be able to do it without disturbing you.

It's no trouble :)

Do you want to add your name to the crate authors before the release?

geomatsi commented 3 years ago

Yes, could you please make a release ? I guess you may want to keep something (history or numbering or crates.io page ?) across ftd2xx-embedded-hal and new ftdi-embedded-hal crate. Next time I will learn the publish procedure to be able to do it without disturbing you.

It's no trouble :)

Great. Thanks !

Do you want to add your name to the crate authors before the release?

No need this time. I mostly copy-pasted from your crates )

newAM commented 3 years ago

and it is now live! v0.10.0 is on crates.io