WMT-GmbH / pn532

no_std implementation of the Pn532 protocol using embedded_hal traits
Apache License 2.0
20 stars 19 forks source link

Can't get to pn532 to compile with esp32 esp-hal #21

Open yanshay opened 3 months ago

yanshay commented 3 months ago

I'm trying to get this to work with esp32 using esp-hal (the no_std version of esp32 rust framework) and I can't get a simple application to compile.

I noticed that this library is based on embedded_hal 0.2.7 which was released in Feb 9, 2022, so it's pretty old and I'm using current esp-hal that is compatible with embedded_hal 1.0 .

So is this indeed the case and the code can't really work with today's embedded_hal?

Is there a way to get it to work by doing some extra work of implementing some interfaces or something?

dimpolo commented 3 months ago

Updating to embedded-hal 1.0 has been on my backlog for a while now. Sorry it hasn't happened yet. (I would welcome a PR)

You should be able to write something like this replacing the generic with the concrete ESP I2C type https://github.com/WMT-GmbH/pn532/blob/master/src%2Fi2c.rs#L18-L67

Same goes for SPI

yanshay commented 3 months ago

Updating to embedded-hal 1.0 has been on my backlog for a while now. Sorry it hasn't happened yet. (I would welcome a PR)

I'm willing to take a shot at this. Do you expect it to be complex?

You should be able to write something like this replacing the generic with the concrete ESP I2C type https://github.com/WMT-GmbH/pn532/blob/master/src%2Fi2c.rs#L18-L67

I just did it, but it seems the Timer also needs to be dealt with. I now receive an error, haven't had a chance to look into it yet, so just sharing:


    |
156 | if let Err(e) = pn532.process(&Request::sam_configuration(SAMMode::Normal, false), 0, 50.ms()){
    |                       ^^^^^^^ method cannot be called due to unsatisfied trait bounds
    |
   ::: /Users/my_user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-hal-0.19.0/src/timer/timg.rs:361:1
    |
361 | pub struct Timer<T, DM>
    | ----------------------- doesn't satisfy `_: CountDown`
    |
    = note: the following trait bounds were not satisfied:
            `esp_hal::timer::timg::Timer<TimerX<TIMG0>, Blocking>: embedded_hal::timer::CountDown`
dimpolo commented 3 months ago

Ah yeah, eh1 removed the CountDown trait without a replacement.

In your case using a newtype around the timer and implementing the CountDown trait should get you going.

For a general implementation I'm not sure how to proceed. Maybe we should provide our own timeout trait.

I would guess that a PR won't be much more complicated than your own ESP implementation. But it might take some time to get everything (including the doc tests) compiling

yanshay commented 3 months ago

I managed to get it to compile with eh1 and now getting it to work on ESP32 (not including tests yet). Added a timer trait to take care of the Countdown removal from eh1.

Eventually I managed to get it working but facing several issues, not completely sure if these are ESP32 related or PN532 related or this library related or maybe my eh1 changes related.

  1. On write I get from esp32 i2c timeout errors when I don't set timeout, if I set a large timeout it usually work. I assume it is esp_hal related so will ask also there, but in case it isn't then writing it here as well.

  2. There's a wait_ready issue, it's supposed to read 0x01 before fetching the data that comes next, but the data that comes later also comes with the 0x01 again. I tried doing multiple reads of one byte, and they all bring 0x01 and still the larger buffer read reads the 0x01 at the beginning of the buffer. I also added println! in between so it's not a timing issue as described on the related issue mentioned there. So I modified the read function to skip first byte and now it seems to work (at least with my very limited testing, fetching firmware version), but that's probably not the design. Any ideas what this could be related to? Either, that's how it should work, or it's my PN532, or an esp32/esp_hal issue.

dimpolo commented 3 months ago

Awesome work! Thanks for taking the time.

  1. How large is a large timeout? The pn532 can be quite slow sometimes.
  2. This is expected behavior. If you check the linked i2c implementation you'll see that we also skip the first byte in the read method
yanshay commented 3 months ago

Awesome work! Thanks for taking the time.

1. How large is a large timeout? The pn532 can be quite slow sometimes.

I've place 1000000 there, but they don't specify in the docs what units it is. Also, I'd assume when I put None (which is an option, even the default option) there won't be a timeout. Sometimes I get a timeout even with that value, if I turn off/on the pn532 then it gets fixed.

2. This is expected behavior. If you check the linked `i2c` implementation you'll see that we also skip the first byte in the `read` method

Yes, the way it's done in the original code is by passing two read operations, first is supposed to fetch the 0x01 and the following the rest of the data. What I see is that the first byte indeed comes as 0x01, but then it arrives again in the following read. And even if I do 5 read operations of a single byte that single byte is always 0x01, and until I don't read a larger buffer the buffer has that 0x01 as first byte. For example, the ack reads as (from logs) : [1] then [1, 0, 0, 255, 0, 255] when I increased the first read to two bytes I get [1, 0] then [1, 0, 0, 255, 0, 255] with three bytes first I get [1, 0, 0] then [0, 128, 128, 128, 128, 128] with four bytes first I get [1, 0, 0, 255] then [0, 128, 128, 128, 128, 128]

I don't know whether it is a PN532 I2C issue (is it aware of each read operation and can respond with that first byte over and over again) or an esp32/esp_hal internal buffering bug?

So what I did is to combine the two parts into a single larger read operation and copy the byte starting with the 2nd to the passed buffer. Less elegant since I need to assume a max buffer internally (esp32 max I2C is 32 bytes so I have a limit).

Any ideas on what could cause such behavior?

Another behavior that might be related is the following: In the example code in the README you there's:

if let Ok(uid) = pn532.process(&Request::INLIST_ONE_ISO_A_TARGET, 7, 1000.ms()){
    let result = pn532.process(&Request::ntag_read(10), 17, 50.ms()).unwrap();
    if result[0] == 0x00 {
        println!("page 10: {:?}", &result[1..5]);
    }
}

When I try that, I get an error the BufTooSmall on the page read. I have to use at least 17 bytes buffer. Again, maybe some kind of buffering requirement, if I get it right this is part of the PN532 protocol, not I2C, right? So how does it work in the example provided? Is it a different device?

dimpolo commented 3 months ago

Hm the timeout thing seems strange, but maybe the problems will disappear after we fix the other issues.

For the two reads you'll need to use https://docs.rs/embedded-hal/latest/embedded_hal/i2c/trait.I2c.html#tymethod.transaction instead of two separate reads.

What kind of tag are you reading? Maybe ntag_read is the wrong request for you. Anyways you can increase the const generic N of the Pn532 struct to increase the buffer size.

yanshay commented 3 months ago

For the two reads you'll need to use https://docs.rs/embedded-hal/latest/embedded_hal/i2c/trait.I2c.html#tymethod.transaction instead of two separate reads.

This is what I referred to when I wrote about 'two reads'. The text you see above ([numbers] then [more numbers]) is from logs of the two buffers of that single call to transaction.

What kind of tag are you reading? Maybe ntag_read is the wrong request for you. Anyways you can increase the const generic N of the Pn532 struct to increase the buffer size.

I'm using ntag-215 that I wrote a single record to using NFC tools from an iPhone.

dimpolo commented 3 months ago

Hm this might be a bug in the HAL then. Would you be able to check with a logic analyzer if the following holds:

Data from adjacent operations of the same type are sent after each other without an SP or SR.

The two reads from a single transaction should appear as just one read on the bus

yanshay commented 3 months ago

Hm this might be a bug in the HAL then. Would you be able to check with a logic analyzer if the following holds: That's what I'm thinking as well. I'll ask there.

Would you be able to check with a logic analyzer if the following holds: I don't have a logic analyzer.

Anyway, I now switched to test SPI and ... can't get it to work. It is stuck with wait_ready returning Pending. Values I've seen received there are 255 or 0 or 8 or 4. I've seen this depend on frequency, SPI Mode, if I wired cs to Gnd, etc.

The changes to the spi.rs are really minimal:

  1. Instead of the several trait bounds on SPI, just have SPI: SpiDevice
  2. Instead of transfer, use transfer_in_place
  3. Error changes to embedded_hal spi Error
  4. I guess I can remove the handling of cs since the SpiDevice takes care of that properly, but for now I gave it a pin (either dummy or the real one, both cases didn't work)

Any ideas what to check? I exhausted everything I could think of.

  1. What frequency to set the spi? I used 100kHz so would be low enough to work.
  2. What SPI mode to use (0,1,2,3)? I tried all, but can't test all permutations of everything. 3 I switched esp32 spi to LSB mode as I saw is required (though tried both), also tried with msb_spi feature 4 Should I do any special non trivial wiring on the PN532 module I'm using? (e.g. RSTO can stay disconnected? I'm also not connecting IRQ at thie stage). I connected cs to the pin named ss, is that correct wiring?
  3. I'm using ExclusiveDevice of embedded-hal-bus to create the SpiDevice to pass to the SpiInterface.
  4. I'm using FullDuplex SPI. When using HalfDuplex it doesn't work with. SpiBus/SpiDevice for some reason in esp_hal. Could this be an issue due to using FullDuplex with PN532?
dimpolo commented 3 months ago

You should use Mode 0 and you can set lsb first with https://docs.rs/esp-hal/latest/esp_hal/spi/master/struct.Spi.html#method.with_bit_order but it seems you've done that already.

Frequency shouldn't matter and 100kHz sounds good.

Full duplex and ExclusiveDevice sounds good.

Chip select handling might be the problem. Some devices rely on cs to determine when a frame starts and ends. I don't remember if the pn532 works like that. Could you try using a single transaction instead of multiple write and transfer_in_place calls. That way the cs won't toggle during communications.

yanshay commented 3 months ago

Some devices rely on cs to determine when a frame starts and ends. I don't remember if the pn532 works like that. Could you try using a single transaction instead of multiple write and transfer_in_place calls. That way the cs won't toggle during communications.

Ok, I'll try that. So if this is how the pn532 work, when I connected cs to Gnd it wouldn't recognize data arriving at all?

On another note: This made me decide to get a simple logic analyzer. Do you happen to know to recommend one that isn't expensive yet good for this type of work? I saw there are those cheap ones Salea 8 channel clones, are these recommended? What's the next step up from these?

dimpolo commented 3 months ago

I use one from Kingst and I'm quite happy with it. Don't know how it compares to others.

yanshay commented 3 months ago

I replaced the wait_ready to use transaction, same results. I keep getting 8 as response. I tried changing the data I send in the request and it does change the response, which means the device does react in some way even if not as the code expects. Maybe the 8 has some meaning in PN532? (the ack is supposed to be 1).

I went into the PN532 user manual, and found this diagram:

image

It shows that after a command the host is supposed to poll until it receives status 0x01. It doesn't say it should also write anything to get that status response. And this is indeed how the I2C code looks like. wait_ready only includes read. In the code what's taking place is that the host repeatedly sends STATREAD request, and then 0x00 after that and then reads data back. This is not what I understand from the diagram. I tried only reading status and it didn't work either, but I don't see where the sequence the code performs is documented.

Any ideas what to try next?

dimpolo commented 3 months ago

Yeah I'm also not sure what's going on. I assume you checked the wiring and the dip switch position?

I might have a chance to try your changes on Monday. Could you share what you've got so far?

yanshay commented 3 months ago

I submitted a PR #22 . BTW, regarding I2C on esp-hal they confirmed there's some issue and will solve it, but for now it is still broken there, so I kept my modified version to workaround that issue. https://github.com/esp-rs/esp-hal/issues/1998

On the SPI issues, I found the following on embedded-hal:

At https://rtic.rs/dev/api/embedded_hal/spi/index.html :

CS-to-clock delays

Many chips require a minimum delay between asserting CS and the first SCK edge, and the last SCK edge and deasserting CS. Drivers should NOT use Operation::DelayNs for this, they should instead document that the user should configure the delays when creating the SpiDevice instance, same as they have to configure the SPI frequency and mode. This has a few advantages:

Allows implementations that use hardware-managed CS to program the delay in hardware Allows the end user more flexibility. For example, they can choose to not configure any delay if their MCU is slow enough to “naturally” do the delay (very common if the delay is in the order of nanoseconds).


Could this be related?

yanshay commented 2 months ago

I added async support for I2C (since SPI doesn't work for me) using embedded-hal-async (left the existing semi-async code in there in case of systems without embedded-hal-async support). It's using maybe-async so not a lot of code duplications.

Would you like me to submit a PR?

dimpolo commented 2 months ago

Hey, sorry for the delay... Work has been really busy recently and it'll take me a week or two to get back to this. Async support sounds great but maybe we'll finish #22 first 😅

yanshay commented 2 months ago

I'm not in a rush, it's working for me, also in async 😀 I think the async is part of the eh1 migration but up to you. Let me know when you can get to it.

yanshay commented 2 weeks ago

Any chance this (PR #22) could be merged? I can also submit the async PR and close it all with a single PR.

I've been working with this and also the async version and it's been working great for quite some time. I prefer to use the public repo rather than my private code.