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

Upgrade to embedded-hal 1.0.0-alpha.6 #26

Closed
wants to merge 3 commits into from
Closed

Upgrade to embedded-hal 1.0.0-alpha.6 #26

wants to merge 3 commits into from

Conversation

andresv
Copy link
Contributor

@andresv andresv commented Dec 22, 2021

This PR updates everything to embedded-hal 1.0.0-alpha.6. 1.0.0 is around the corner so those traits are more less what to expect from v1.0.

This version also adds CAN traits. So this is prerequisite for adding CAN support with #27.

@ivmarkov
Copy link
Collaborator

I assume, once we migrate to alpha6, it would still be possible to use crates that depend on embedded-hal V0.2 via the embedded-hal-compat crate?

If you can confirm that?

The background of my question is that there are gazillion of LED drivers in the embedded-graphics ecosystem, that currently depend on embedded-hal 0.2 and I don't see them migrating overnight once 1.0 is released. On the other hand, I don't want that we are losing compatibility with them.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Dec 29, 2021

Basically, we need at least reverse compatibility for GPIO, I2C and SPI, as this is what these drivers usually rely on.

@andresv
Copy link
Contributor Author

andresv commented Dec 29, 2021

I will look into embedded-hal-compat.
At the moment I am still working on CAN. If this is finished then I'll check how to deal with backwards compatibility.

@ivmarkov
Copy link
Collaborator

I will look into embedded-hal-compat. At the moment I am still working on CAN. If this is finished then I'll check how to deal with backwards compatibility.

I assume not 'if' but 'when'. :)
Also, forgot to say - thank you for this contribution! I don't really expect compatibility issues by utilizing the compat crate - just something we should check before merging.

Let me know once you think this diff would be ready for merging.

@andresv andresv mentioned this pull request Dec 29, 2021
@andresv
Copy link
Contributor Author

andresv commented Dec 29, 2021

I looked into embedded-hal-compat. It indeed allows to use 0.2 drivers. Tried with st7789:

use display_interface_spi::SPIInterfaceNoCS;
use embedded_hal_compat::ReverseCompat;

let config = <spi::config::Config as Default>::default()
    .baudrate(26.MHz().into())
    .bit_order(spi::config::BitOrder::MSBFirst);

let di = SPIInterfaceNoCS::new(
    spi::Master::<spi::SPI2, _, _, _, _>::new(
        peripherals.spi2,
        spi::Pins {
            sclk: pins.gpio18,
            sdo: pins.gpio19,
            sdi: Option::<gpio::Gpio21<gpio::Unknown>>::None,
            cs: Some(pins.gpio6),
        },
        config,
    )?
    .reverse(),
    pins.gpio27.into_output()?.reverse(),
);

let mut display = st7789::ST7789::new(
    di,
    pins.gpio23.into_output()?.reverse(),
    // SP7789V is designed to drive 240x320 screens, even though the TTGO physical screen is smaller
    240,
    320,
);

Notice .reverse() here and there. This makes 1.0 traits compatible with 0.2.

Essentially esp-idf-hal itself is not going to use embedded-hal-compat, it must be used on application side.

@ivmarkov
Copy link
Collaborator

Essentially esp-idf-hal itself is not going to use embedded-hal-compat, it must be used on application side.

That should be fine.

@andresv
Copy link
Contributor Author

andresv commented Dec 30, 2021

@ivmarkov, it should be now ready to merge.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 1, 2022

@andresv Before merging this PR, I would like to at least announce that this is coming at our regular ESP32 meeting. And probably solicit some feedback, as to e.g. what are the plans in relation to embedded-hal versions for the bare-metal HAL crate (esp32-hal) (@MabezDev @jessebraham - ping). Not that we have to absolutely be using the same embedded-hal version across bare-metal and ESP-IDF HALs, but would be good to be working towards a common vision.

Fortunately, the next meeting is just behind the corner - Jan 04. Since it is open for everyone, you can attend too - that is - if you wish, given that you have a stake in this development.

Please ping me on the Matrix channel in private, so that I can forward you the meeting.

By the way - in addition to CANBUS support - there is now one more reason - albeit minor - to migrate to alpha6/1.0: The ADC support that I just landed cannot be implemented for 'degraded' pins (GpioPin<>), because in V0.2, adc::Channel::channel() does not take &self. They've fixed this in post V0.2 releases :)

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

As discussed in the one (and only) inline review comment, we need to improve a bit on the error handling code.

src/i2c.rs Outdated Show resolved Hide resolved
ivmarkov and others added 2 commits January 3, 2022 11:11
This version has CAN traits. So this is prerequisite for adding CAN support.
@@ -563,7 +561,7 @@ impl<UART: Uart> serial::Read<u8> for Rx<UART> {
match unsafe { uart_read_bytes(UART::port(), &mut buf as *mut u8 as *mut _, 1, 0) } {
1 => Ok(buf),
0 => Err(nb::Error::WouldBlock),
err => Err(nb::Error::Other(EspError::from(err as i32).unwrap())),
_ => Err(nb::Error::Other(serial::ErrorKind::Other)),
Copy link
Contributor Author

@andresv andresv Jan 3, 2022

Choose a reason for hiding this comment

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

Notice that new error type is not used here because these functions do not return esp_error_t.

@andresv
Copy link
Contributor Author

andresv commented Jan 3, 2022

@ivmarkov, take a look, errors are now handled as you proposed.

@andresv andresv requested a review from ivmarkov January 3, 2022 14:48
@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 5, 2022

@ivmarkov, take a look, errors are now handled as you proposed.

Sorry for the delay, I've been busy in standing up a new branch, where your alpha6 + canbus code is in, but additionally, this branch would simultaneously support embedded-hal alpha6 and embedded-hal 0.2. Why?

  • The statement that embedded-hal 1.0 is "just around the corner" wasn't accepted that well :). Seemingly it has been "just around the corner" for an year and a half now, with 4 issues still unresolved (I thought it is one, but they are really 4)...
  • The 'compat' crate has incomplete support for reversing, i.e. not all 0.2 traits are supported
  • Folks just feel uneasy that we'll put an extra burden on our users in the form of an additional crate

Anyway. Work progressing nicely, I have i2c, spi and adc done that way, gpio and delay remaining.

But in the meantime... have you seen this?

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

Changes look great - thanks a lot for that work!
Now, regarding merging:

  • I've already merged your alpha6-compat commits into the embedded-hal-1-compat branch
  • Then I and then extended these with backwards compatibility with embedded-hal 0.2
  • Then I merged your CAN driver (thanks again!)
  • Then I've slightly reshuffled the code so that if/when can support pops up in a new V0.2 release, we can implement it by uncommenting a few blocks of code

What I would really appreciate is if you can test the committed CAN driver in branch embedded-hal-1-compat. It should work, but then again I've moved your code around a bit for the V0.2 compatibility

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 6, 2022

@ivmarkov ivmarkov closed this Jan 6, 2022
usbalbin pushed a commit to usbalbin/esp-idf-hal that referenced this pull request May 5, 2022
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.

2 participants