From b953002f8f917b96139f198a2628ba3d36d0a0db Mon Sep 17 00:00:00 2001 From: Olliver Schinagl Date: Thu, 20 Oct 2016 17:36:04 +0200 Subject: [PATCH 1/3] drivers: pn532_i2c: Clarify preamble and start byte The pn532 documentation differs slightly from the included ascii art documentation on how a packet looks like. The preamble was omitted however the postamble is mentioned. This patch adds the Preamble to the ascii frame documentation. The code later refers incorrectly to the start byte as the preamble. This variable was renamed to more descriptively state that it is the preambe and the start bytes. Signed-off-by: Olliver Schinagl --- libnfc/chips/pn53x-internal.h | 48 ++++++++++++++++++++--------------- libnfc/drivers/pn532_i2c.c | 11 +++++--- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/libnfc/chips/pn53x-internal.h b/libnfc/chips/pn53x-internal.h index c43934a..e4869cd 100644 --- a/libnfc/chips/pn53x-internal.h +++ b/libnfc/chips/pn53x-internal.h @@ -81,32 +81,38 @@ #define TgGetTargetStatus 0x8A /** @note PN53x's normal frame: + * See the PN532 (firmware) user manual, section 6.2.1.1: Normal information + * frame, figure 13. Normal information frame, page 28 rev. 02 - 2007-11-07. * - * .-- Start - * | .-- Packet length - * | | .-- Length checksum - * | | | .-- Direction (D4 Host to PN, D5 PN to Host) - * | | | | .-- Code - * | | | | | .-- Packet checksum - * | | | | | | .-- Postamble - * V | | | | | | - * ----- V V V V V V - * 00 FF 02 FE D4 02 2A 00 + * .-- Preamble + * | .-- Start + * | | .-- Packet length + * | | | .-- Length checksum + * | | | | .-- Direction (D4 Host to PN, D5 PN to Host) + * | | | | | .-- Code + * | | | | | | .-- Packet checksum + * | | | | | | | .-- Postamble + * | V | | | | | | + * V ----- V V V V V V + * 00 00 FF 02 FE D4 02 2A 00 */ /** @note PN53x's extended frame: + * See the PN532 (firmware) user manual, section 6.2.1.2: Extended information + * frame, figure 14. Normal information frame, page 29 rev. 02 - 2007-11-07. * - * .-- Start - * | .-- Fixed to FF to enable extended frame - * | | .-- Packet length - * | | | .-- Length checksum - * | | | | .-- Direction (D4 Host to PN, D5 PN to Host) - * | | | | | .-- Code - * | | | | | | .-- Packet checksum - * | | | | | | | .-- Postamble - * V V V | | | | | - * ----- ----- ----- V V V V V - * 00 FF FF FF 00 02 FE D4 02 2A 00 + * .-- Preamble + * | .-- Start + * | | .-- Fixed to FF to enable extended frame + * | | | .-- Packet length + * | | | | .-- Length checksum + * | | | | | .-- Direction (D4 Host to PN, D5 PN to Host) + * | | | | | | .-- Code + * | | | | | | | .-- Packet checksum + * | | | | | | | | .-- Postamble + * | V V V | | | | | + * V ----- ----- ----- V V V V V + * 00 00 FF FF FF 00 02 FE D4 02 2A 00 */ /** diff --git a/libnfc/drivers/pn532_i2c.c b/libnfc/drivers/pn532_i2c.c index b62c7de..fe3345c 100644 --- a/libnfc/drivers/pn532_i2c.c +++ b/libnfc/drivers/pn532_i2c.c @@ -75,6 +75,10 @@ const struct timespec rdyDelay = { .tv_nsec = PN532_RDY_LOOP_DELAY * 1000 * 1000 }; +/* preamble and start bytes, see pn532-internal.h for details */ +const uint8_t pn53x_preamble_and_start[] = { 0x00, 0x00, 0xff }; +#define PN53X_PREAMBLE_AND_START_LEN (sizeof(pn53x_preamble_and_start) / sizeof(pn53x_preamble_and_start[0])) + /* Private Functions Prototypes */ static nfc_device *pn532_i2c_open(const nfc_context *context, const nfc_connstring connstring); @@ -334,9 +338,10 @@ pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int break; }; - uint8_t abtFrame[PN532_BUFFER_LEN] = { 0x00, 0x00, 0xff }; // Every packet must start with "00 00 ff" + uint8_t abtFrame[PN532_BUFFER_LEN]; size_t szFrame = 0; + memcpy(abtFrame, pn53x_preamble_and_start, PN53X_PREAMBLE_AND_START_LEN); // Every packet must start with the preamble and start bytes. if ((res = pn53x_build_frame(abtFrame, &szFrame, pbtData, szData)) < 0) { pnd->last_error = res; return pnd->last_error; @@ -477,9 +482,7 @@ pn532_i2c_receive(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLen, int goto error; } - const uint8_t pn53x_preamble[3] = { 0x00, 0x00, 0xff }; - - if (0 != (memcmp(frameBuf, pn53x_preamble, 3))) { + if (0 != (memcmp(frameBuf, pn53x_preamble_and_start, PN53X_PREAMBLE_AND_START_LEN))) { log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR, "%s", "Frame preamble+start code mismatch"); pnd->last_error = NFC_EIO; goto error; From 8f8f780c2bdd95b8609b43d37f9f4982825851d4 Mon Sep 17 00:00:00 2001 From: Olliver Schinagl Date: Fri, 21 Oct 2016 10:48:56 +0200 Subject: [PATCH 2/3] drivers: pn532_i2c: Respect proper timing specifications The pn532 user manual states that after a i2c stop condition and before a i2c start condition there should be a delay of minimally 1.3 milliseconds. This is probably a limitation of the i2c peripheral or the firmware. In any case, each i2c_read and i2c_write creates the packets which are complemented with start/stop markers. It is thus required to take care of timing in these two functions. We solve this by wrapping the lower i2c_read and i2c_write functions for the pn532, as this requirement is not for all chips. Currently, we keep time using local variable, and thus the code is not thread-safe. With libnfc being single threaded and only one instances of libnfc can open a bus anyway, this is not yet a problem. Signed-off-by: Olliver Schinagl --- libnfc/drivers/pn532_i2c.c | 81 +++++++++++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 14 deletions(-) diff --git a/libnfc/drivers/pn532_i2c.c b/libnfc/drivers/pn532_i2c.c index fe3345c..6315954 100644 --- a/libnfc/drivers/pn532_i2c.c +++ b/libnfc/drivers/pn532_i2c.c @@ -67,14 +67,6 @@ struct pn532_i2c_data { volatile bool abort_flag; }; -/* Delay for the loop waiting for READY frame (in ms) */ -#define PN532_RDY_LOOP_DELAY 90 - -const struct timespec rdyDelay = { - .tv_sec = 0, - .tv_nsec = PN532_RDY_LOOP_DELAY * 1000 * 1000 -}; - /* preamble and start bytes, see pn532-internal.h for details */ const uint8_t pn53x_preamble_and_start[] = { 0x00, 0x00, 0xff }; #define PN53X_PREAMBLE_AND_START_LEN (sizeof(pn53x_preamble_and_start) / sizeof(pn53x_preamble_and_start[0])) @@ -100,6 +92,70 @@ static size_t pn532_i2c_scan(const nfc_context *context, nfc_connstring connstri #define DRIVER_DATA(pnd) ((struct pn532_i2c_data*)(pnd->driver_data)) +/* + * Bus free time (in ms) between a STOP condition and START condition. See + * tBuf in the PN532 data sheet, section 12.25: Timing for the I2C interface, + * table 320. I2C timing specification, page 211, rev. 3.2 - 2007-12-07. + */ +#define PN532_BUS_FREE_TIME 1.3 +static struct timespec __transaction_stop; + +/** + * @brief Wrapper around i2c_read to ensure proper timing by respecting the + * minimal free bus time between a STOP condition and a START condition. + * + * @note This is not thread safe, but since libnfc is single threaded + * this should be okay. + * + * @param id I2C device + * @param buf pointer on buffer used to store data + * @param len length of the buffer + * @return length (in bytes) of read data, or driver error code (negative value) + */ +static ssize_t pn532_i2c_read(const i2c_device id, + uint8_t *buf, const size_t len) +{ + struct timespec transaction_start, bus_free_time = { 0 }; + ssize_t ret; + + clock_gettime(CLOCK_MONOTONIC, &transaction_start); + bus_free_time.tv_nsec = (PN532_BUS_FREE_TIME * 1000 * 1000) - + (transaction_start.tv_nsec - __transaction_stop.tv_nsec); + nanosleep(&bus_free_time, NULL); + + ret = i2c_read(id, buf, len); + clock_gettime(CLOCK_MONOTONIC, &__transaction_stop); + return ret; +} + +/** + * @brief Wrapper around i2c_write to ensure proper timing by respecting the + * minimal free bus time between a STOP condition and a START condition. + * + * @note This is not thread safe, but since libnfc is single threaded + * this should be okay. + * + * @param id I2C device + * @param buf pointer on buffer containing data + * @param len length of the buffer + * @return NFC_SUCCESS on success, otherwise driver error code + */ +static ssize_t pn532_i2c_write(const i2c_device id, + const uint8_t *buf, const size_t len) +{ + struct timespec transaction_start, bus_free_time = { 0 }; + ssize_t ret; + + clock_gettime(CLOCK_MONOTONIC, &transaction_start); + bus_free_time.tv_nsec = (PN532_BUS_FREE_TIME * 1000 * 1000) - + (transaction_start.tv_nsec - __transaction_stop.tv_nsec); + nanosleep(&bus_free_time, NULL); + + ret = i2c_write(id, buf, len); + clock_gettime(CLOCK_MONOTONIC, &__transaction_stop); + return ret; +} + /** * @brief Scan all available I2C buses to find PN532 devices. * @@ -347,7 +403,7 @@ pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int return pnd->last_error; } - res = i2c_write(DRIVER_DATA(pnd)->dev, abtFrame, szFrame); + res = pn532_i2c_write(DRIVER_DATA(pnd)->dev, abtFrame, szFrame); if (res < 0) { log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR, "%s", "Unable to transmit data. (TX)"); @@ -405,10 +461,7 @@ pn532_i2c_wait_rdyframe(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLe } do { - // Wait a little bit before reading - nanosleep(&rdyDelay, (struct timespec *) NULL); - - int recCount = i2c_read(DRIVER_DATA(pnd)->dev, i2cRx, szDataLen + 1); + int recCount = pn532_i2c_read(DRIVER_DATA(pnd)->dev, i2cRx, szDataLen + 1); if (DRIVER_DATA(pnd)->abort_flag) { // Reset abort flag @@ -575,7 +628,7 @@ error: int pn532_i2c_ack(nfc_device *pnd) { - return i2c_write(DRIVER_DATA(pnd)->dev, pn53x_ack_frame, sizeof(pn53x_ack_frame)); + return pn532_i2c_write(DRIVER_DATA(pnd)->dev, pn53x_ack_frame, sizeof(pn53x_ack_frame)); } /** From d960673681db074f99078a946c663d6d3e58c751 Mon Sep 17 00:00:00 2001 From: Olliver Schinagl Date: Mon, 24 Oct 2016 09:34:19 +0200 Subject: [PATCH 3/3] drivers: pn532_i2c: Add retry on error mechanism Currently, we very occasionally can EXNIO errors from pn532_i2c_write() -> i2c_write() -> write(). This may happen about once every 30 seconds. Based from the kernel sources, EXNIO happens if the chip no longer responds to its own address. To make sure we do not loose any sent packets, we retry to send PN532_SEND_RETRIES number of times. Since we miss 1 every 30 or so seconds, doing 1 retry should be fine. This might be considered a hack as the failure may be some other timing related issue. Signed-off-by: Olliver Schinagl --- libnfc/drivers/pn532_i2c.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/libnfc/drivers/pn532_i2c.c b/libnfc/drivers/pn532_i2c.c index 6315954..6d9e3fd 100644 --- a/libnfc/drivers/pn532_i2c.c +++ b/libnfc/drivers/pn532_i2c.c @@ -59,6 +59,13 @@ // I2C address of the PN532 chip. #define PN532_I2C_ADDR 0x24 +/* + * When sending lots of data, the pn532 occasionally fails to respond in time. + * Since it happens so rarely, lets try to fix it by re-sending the data. This + * define allows for fine tuning the number of retries. + */ +#define PN532_SEND_RETRIES 3 + // Internal data structs const struct pn53x_io pn532_i2c_io; @@ -368,6 +375,7 @@ static int pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int timeout) { int res = 0; + uint8_t retries; // Discard any existing data ? @@ -403,7 +411,13 @@ pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int return pnd->last_error; } - res = pn532_i2c_write(DRIVER_DATA(pnd)->dev, abtFrame, szFrame); + for (retries = PN532_SEND_RETRIES; retries > 0; retries--) { + res = pn532_i2c_write(DRIVER_DATA(pnd)->dev, abtFrame, szFrame); + if (res >= 0) + break; + + log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR, "Failed to transmit data. Retries left: %d.", retries - 1); + } if (res < 0) { log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR, "%s", "Unable to transmit data. (TX)");