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

migrate to embedded_hal 1.0 #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yanshay
Copy link

@yanshay yanshay commented Aug 29, 2024

I migrated the code to use embedded_hal 1.0 .

  1. It compiles. I had to add a CountDown which was removed from embedded_hal.
  2. Tested I2C to work with ESP32. It actually required some change due to esp-hal not fully compliant with embedded-hal. This was reported and confirmed there to be an issue they will resolve. Currently the solution assumes max buffer of 32 bytes, probably need to get that value propagated from PN532 which has N as a generic const value. For now, since this isn't final I didn't do it yet.
  3. Tested with SPI, it doesn't work on esp32, I guess it is either due to how I setup the SPI on the esp32. When I get the logic analyzer I'll try to troubleshoot further.

Happy for others to test it with other devices, or if anyone can get it to work with esp32 I'd be happy to know how.

@dimpolo
Copy link
Collaborator

dimpolo commented Aug 29, 2024

Awesome! Thanks so much for your efforts!

For the SPI, could you remove the manual handling of the chip select and the cs field? Also could you perform the actions inside read, write and wait_ready as one transaction?

@yanshay
Copy link
Author

yanshay commented Aug 29, 2024

I can do it, except I can't really test it since SPI doesn't work for me on Esp32, so I could introduce a bug with such changes (mainly the move to transaction).

Removed the cs handling, I think it's better to move the transaction on a system where it is actually running, and test before and after the change that it is working.

@Erik1000
Copy link

Erik1000 commented Nov 9, 2024

@yanshay hey, thanks for your effort! I wanted to use your fork, but I saw that you have copied the CountDown trait from embedded-hal which was removed in 1.0. However, your copied CountDown trait is not implemented by any type and this makes calling Pn532::process etc unused because of Trait bounds on the impl block.
Are you still interested in pushing this PR forward?

@yanshay
Copy link
Author

yanshay commented Nov 9, 2024

If I recall correctly Embedded-Hal dropped CountDown prior to release of embedded-hal 1.0, I couldn't find any established alternatives.
By established alternative I refer to traits that are implemented as part of the various chipset hals.
So instead I decided to copy it into the code and put the (small) burden of implementing it on the user based on the chip they use. That's why you won't see any implementation of it in the code.

Since then, I personally moved to an async solution based on embassy and timeouts need to be implemented differently there.

My end goal is to get the async implementation (for I2C I implemented it) into the repo, updating to embedded-hal 1.0 is a step in that direction.

@Erik1000
Copy link

Erik1000 commented Nov 9, 2024

Yes, I understand. I have switched to i2c yesterday as well since it works better than spi. My goal is to use async too, ill try your async implementation asap

@yanshay
Copy link
Author

yanshay commented Nov 9, 2024

I think I didn't submit a PR with the async version since @dimpolo wanted to take it one step at a time but if you want I can add it to the PR, I think the async version is much more valuable anyway.

@Erik1000
Copy link

Erik1000 commented Nov 9, 2024

Yes I just saw the branch in your fork. Unfortunately, I had to realize that rppal (I use a Raspberry Pi) does not support async yet, so I am stuck on sync i2c for the time being. But I still use your fork (branch of this PR) with a simple timer wrapper for the CountDown trait and it works! So thank you very much!

@gmorell
Copy link

gmorell commented Nov 21, 2024

Yes I just saw the branch in your fork. Unfortunately, I had to realize that rppal (I use a Raspberry Pi) does not support async yet, so I am stuck on sync i2c for the time being. But I still use your fork (branch of this PR) with a simple timer wrapper for the CountDown trait and it works! So thank you very much!

Any chance you could share the wrapper, I am rather new to the embedded_hal stuff and it's not super clear to me how these traits are supposed to work.

@yanshay
Copy link
Author

yanshay commented Nov 21, 2024

Any chance you could share the wrapper, I am rather new to the embedded_hal stuff and it's not super clear to me how these traits are supposed to work.

If you refer to the CountDown, this is what I could find in my tests (I moved to async by now), for ESP32. Maybe it will help you as a starting point.

struct Esp32Timer<T>
where
    T: esp_hal::timer::Timer,
{
    the_timer: T,
    end_time: Instant<u64, 1, 1_000_000>,
}

impl<T> Esp32Timer<T>
where
    T: esp_hal::timer::Timer,
{
    pub fn new(timer: T) -> Self {
        Self {
            the_timer: timer,
            end_time: current_time(),
        }
    }
}

pub enum CountDownError {
    WouldBlock,
}

impl<T> pn532::CountDown for Esp32Timer<T>
where
    T: esp_hal::timer::Timer,
{
    type Error = CountDownError;
    type Time = Duration<u64, 1, 1_000_000>;

    fn start<D: Into<Self::Time>>(&mut self, count: D)
    {
        self.end_time = current_time().checked_add_duration(count.into()).expect("Adding count to time");
    }

    fn wait(&mut self) -> Result<(), Self::Error> {
        if current_time() >= self.end_time {
            Ok(())
        } else {
            Err(CountDownError::WouldBlock)
        }
    }
}

@Erik1000
Copy link

Yes I just saw the branch in your fork. Unfortunately, I had to realize that rppal (I use a Raspberry Pi) does not support async yet, so I am stuck on sync i2c for the time being. But I still use your fork (branch of this PR) with a simple timer wrapper for the CountDown trait and it works! So thank you very much!

Any chance you could share the wrapper, I am rather new to the embedded_hal stuff and it's not super clear to me how these traits are supposed to work.

In my case I was lucky enough that the rppal has implementations for both embedded 1.0 and embedded 0.2 so I just created a wrapper around rppals countdown implementation and implemented the trait from pn532 on this while having the CountDown trait from embedded 0.2 in scope

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.

4 participants