Fix various problems in I2C support of PN532: - Fix a memory leak in pn532_i2c_wait_rdyframe() - Remove unused parameters and local variables - Fix all other compilation warnings

Note: a new constant (PN53x_ACK_FRAME__LEN) has been defined in
pn53x-internal.h file to avoid hard coding the ACK frame length.

libnfc/buses/i2c.c

@@ -54,8 +54,6 @@
 #include <nfc/nfc.h>
 #include "nfc-internal.h"
 
-#define LINUX_I2C_DRIVER_NAME "linux_i2c"
-
 #define LOG_GROUP    NFC_LOG_GROUP_COM
 #define LOG_CATEGORY "libnfc.bus.i2c"
 
@@ -67,16 +65,23 @@ const char *i2c_ports_device_radix[] =
 #  endif
 
 
-struct i2c_device {
+struct i2c_device_unix {
   int fd;             // I2C device file descriptor
 };
 
-#define I2C_DATA( X ) ((struct i2c_device *) X)
+#define I2C_DATA( X ) ((struct i2c_device_unix *) X)
 
+/**
+ * @brief Open an I2C device
+ *
+ * @param pcI2C_busName I2C bus device name
+ * @param devAddr address of the I2C device on the bus
+ * @return pointer to the I2C device structure, or INVALID_I2C_BUS, INVALID_I2C_ADDRESS error codes.
+ */
 i2c_device
 i2c_open(const char *pcI2C_busName, uint32_t devAddr)
 {
-  struct i2c_device *id = malloc(sizeof(struct i2c_device));
+  struct i2c_device_unix *id = malloc(sizeof(struct i2c_device_unix));
 
   if (id == 0)
     return INVALID_I2C_BUS ;
@@ -97,6 +102,11 @@ i2c_open(const char *pcI2C_busName, uint32_t devAddr)
   return id;
 }
 
+/**
+ * @brief Close the I2C device
+ *
+ * @param id I2C device to close.
+ */
 void
 i2c_close(const i2c_device id)
 {
@@ -109,22 +119,18 @@ i2c_close(const i2c_device id)
 /**
  * @brief Read a frame from the I2C device and copy data to \a pbtRx
  *
- * @param timeout Time out for data read  (in milliseconds). 0 for not timeout.
+ * @param id I2C device.
- * @return 0 on success, otherwise driver error code
+ * @param pbtRx pointer on buffer used to store data
+ * @param szRx length of the buffer
+ * @return length (in bytes) of read data, or driver error code  (negative value)
  */
-int
+ssize_t
-i2c_read(i2c_device id, uint8_t *pbtRx, const size_t szRx, void *abort_p,
+i2c_read(i2c_device id, uint8_t *pbtRx, const size_t szRx)
-         int timeout)
 {
-  int iAbortFd = abort_p ? *((int *) abort_p) : 0;
+  ssize_t res;
+  ssize_t recCount;
 
-  int res;
+  recCount = read(I2C_DATA(id) ->fd, pbtRx, szRx);
-  int done = 0;
-
-  struct timeval start_tv, cur_tv;
-  long long duration;
-
-  ssize_t recCount = read(I2C_DATA(id) ->fd, pbtRx, szRx);
 
   if (recCount < 0) {
     res = NFC_EIO;
@@ -141,11 +147,13 @@ i2c_read(i2c_device id, uint8_t *pbtRx, const size_t szRx, void *abort_p,
 /**
  * @brief Write a frame to I2C device containing \a pbtTx content
  *
- * @param timeout Time out for data read  (in milliseconds). 0 for not timeout.
+ * @param id I2C device.
- * @return 0 on success, otherwise a driver error is returned
+ * @param pbtTx pointer on buffer containing data
+ * @param szTx length of the buffer
+ * @return NFC_SUCCESS on success, otherwise driver error code
  */
 int
-i2c_write(i2c_device id, const uint8_t *pbtTx, const size_t szTx, int timeout)
+i2c_write(i2c_device id, const uint8_t *pbtTx, const size_t szTx)
 {
   LOG_HEX(LOG_GROUP, "TX", pbtTx, szTx);
 
@@ -154,15 +162,21 @@ i2c_write(i2c_device id, const uint8_t *pbtTx, const size_t szTx, int timeout)
 
   if ((const ssize_t) szTx == writeCount) {
     log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_DEBUG,
-            "wrote %d bytes successfully.", szTx);
+            "wrote %d bytes successfully.", (int)szTx);
     return NFC_SUCCESS;
   } else {
     log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR,
-            "Error: wrote only %d bytes (%d expected).", writeCount, (int) szTx);
+            "Error: wrote only %d bytes (%d expected).", (int)writeCount, (int) szTx);
     return NFC_EIO;
   }
 }
 
+/**
+ * @brief Get the path of all I2C bus devices.
+ *
+ * @return array of strings defining the names of all I2C buses available. This array, and each string, are allocated
+ *     by this function and must be freed by caller.
+ */
 char **
 i2c_list_ports(void)
 {

libnfc/buses/i2c.h

@@ -42,8 +42,6 @@
 #  include <linux/i2c-dev.h>
 #  include <nfc/nfc-types.h>
 
-// extern const struct i2c_driver linux_i2c_driver;
-
 typedef void *i2c_device;
 #  define INVALID_I2C_BUS (void*)(~1)
 #  define INVALID_I2C_ADDRESS (void*)(~2)
@@ -52,9 +50,9 @@ i2c_device i2c_open(const char *pcI2C_busName, uint32_t devAddr);
 
 void       i2c_close(const i2c_device id);
 
-int        i2c_read(i2c_device id, uint8_t *pbtRx, const size_t szRx, void *abort_p, int timeout);
+ssize_t    i2c_read(i2c_device id, uint8_t *pbtRx, const size_t szRx);
 
-int        i2c_write(i2c_device id, const uint8_t *pbtTx, const size_t szTx, int timeout);
+int        i2c_write(i2c_device id, const uint8_t *pbtTx, const size_t szTx);
 
 char     **i2c_list_ports(void);
 

libnfc/chips/pn53x-internal.h

@@ -116,6 +116,7 @@
 #  define PN53x_NORMAL_FRAME__OVERHEAD			8
 #  define PN53x_EXTENDED_FRAME__DATA_MAX_LEN		264
 #  define PN53x_EXTENDED_FRAME__OVERHEAD		11
+#  define PN53x_ACK_FRAME__LEN                          6
 
 typedef struct {
   uint8_t ui8Code;

libnfc/drivers/pn532_i2c.c

@@ -95,7 +95,15 @@ static size_t pn532_i2c_scan(const nfc_context *context, nfc_connstring connstri
 
 #define DRIVER_DATA(pnd) ((struct pn532_i2c_data*)(pnd->driver_data))
 
-
+/**
+ * @brief Scan all available I2C buses to find PN532 devices.
+ *
+ * @param context NFC context.
+ * @param connstrings array of 'nfc_connstring' buffer  (allocated by caller). It is used to store the
+ *      connection info strings of all I2C PN532 devices found.
+ * @param connstrings_len length of the connstrings array.
+ * @return number of PN532 devices found on all I2C buses.
+ */
 static size_t
 pn532_i2c_scan(const nfc_context *context, nfc_connstring connstrings[], const size_t connstrings_len)
 {
@@ -110,7 +118,6 @@ pn532_i2c_scan(const nfc_context *context, nfc_connstring connstrings[], const s
     log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_DEBUG, "Trying to find PN532 device on I2C bus %s.", i2cPort);
 
     if ((id != INVALID_I2C_ADDRESS) && (id != INVALID_I2C_BUS)) {
-
       nfc_connstring connstring;
       snprintf(connstring, sizeof(nfc_connstring), "%s:%s", PN532_I2C_DRIVER_NAME, i2cPort);
       nfc_device *pnd = nfc_device_new(context, connstring);
@@ -169,6 +176,11 @@ pn532_i2c_scan(const nfc_context *context, nfc_connstring connstrings[], const s
   return device_found;
 }
 
+/**
+ * @brief Close I2C connection to the PN532 device.
+ *
+ * @param pnd pointer on the device to close.
+ */
 static void
 pn532_i2c_close(nfc_device *pnd)
 {
@@ -179,6 +191,13 @@ pn532_i2c_close(nfc_device *pnd)
   nfc_device_free(pnd);
 }
 
+/**
+ * @brief Open an I2C connection to the PN532 device.
+ *
+ * @param context NFC context.
+ * @param connstring connection info to the device  ( pn532_i2c:<i2c_devname> ).
+ * @return pointer to the device, or NULL in case of error.
+ */
 static nfc_device *
 pn532_i2c_open(const nfc_context *context, const nfc_connstring connstring)
 {
@@ -186,7 +205,6 @@ pn532_i2c_open(const nfc_context *context, const nfc_connstring connstring)
   i2c_device i2c_dev;
   nfc_device *pnd;
 
-  //  pn532_i2c:<i2c_device>
   int connstring_decode_level = connstring_decode(connstring, PN532_I2C_DRIVER_NAME, NULL, &i2c_devname, NULL);
 
   switch (connstring_decode_level) {
@@ -260,6 +278,16 @@ pn532_i2c_wakeup(nfc_device *pnd)
 }
 
 #define PN532_BUFFER_LEN (PN53x_EXTENDED_FRAME__DATA_MAX_LEN + PN53x_EXTENDED_FRAME__OVERHEAD)
+
+/**
+ * @brief Send data to the PN532 device.
+ *
+ * @param pnd pointer on the NFC device.
+ * @param pbtData buffer containing frame data.
+ * @param szData size of the buffer.
+ * @param timeout timeout before aborting the operation (in ms).
+ * @return NFC_SUCCESS if operation is successful, or error code.
+ */
 static int
 pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int timeout)
 {
@@ -290,7 +318,7 @@ 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] = { 0x00, 0x00, 0xff };       // Every packet must start with "00 00 ff"
   size_t szFrame = 0;
 
   if ((res = pn53x_build_frame(abtFrame, &szFrame, pbtData, szData)) < 0) {
@@ -298,7 +326,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, timeout);
+  res = 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)");
@@ -306,12 +334,15 @@ pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int
     return pnd->last_error;
   }
 
-  uint8_t abtRxBuf[6];
+  uint8_t abtRxBuf[PN53x_ACK_FRAME__LEN];
 
   // Wait for the ACK frame
-  res = pn532_i2c_wait_rdyframe(pnd, abtRxBuf, 6, timeout);
+  res = pn532_i2c_wait_rdyframe(pnd, abtRxBuf, PN53x_ACK_FRAME__LEN, timeout);
   if (res < 0) {
-    log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_DEBUG, "%s", "Unable to read ACK");
+    if (res == NFC_EOPABORTED) {
+      // Send an ACK frame from host to abort the command.
+      pn532_i2c_ack(pnd);
+    }
     pnd->last_error = res;
     return pnd->last_error;
   }
@@ -324,10 +355,16 @@ pn532_i2c_send(nfc_device *pnd, const uint8_t *pbtData, const size_t szData, int
   return NFC_SUCCESS;
 }
 
-// pbtData pointer on buffer used to store data
+/**
-// szDataLen Length of buffer
+ * @brief Read data from the PN532 device until getting a frame with RDY bit set
-//
+ *
-// Return: length (in bytes) of the actual data
+ * @param pnd pointer on the NFC device.
+ * @param pbtData buffer used to store the received frame data.
+ * @param szDataLen allocated size of buffer.
+ * @param timeout timeout delay before aborting the operation (in ms). Use 0 for no timeout.
+ * @return length (in bytes) of the received frame, or NFC_ETIMEOUT if timeout delay has expired,
+ *         NFC_EOPABORTED if operation has been aborted, NFC_EIO in case of IO failure
+ */
 static int
 pn532_i2c_wait_rdyframe(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLen, int timeout)
 {
@@ -337,29 +374,27 @@ pn532_i2c_wait_rdyframe(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLe
   struct timeval start_tv, cur_tv;
   long long duration;
 
-  bool *abort_p = NULL;
+  // Actual I2C response frame includes an additional status byte,
-
+  // so we use a temporary buffer to read the I2C frame
-  /* Actual response frame includes a RDY byte */
+  uint8_t i2cRx[PN53x_EXTENDED_FRAME__DATA_MAX_LEN + 1];
-  uint8_t *i2cRx = malloc(szDataLen + 1);
-
-  if (NULL == i2cRx) {
-    perror("malloc");
-    return NFC_ESOFT;
-  }
 
   if (timeout > 0) {
+    // If a timeout is specified, get current timestamp
     gettimeofday(&start_tv, NULL);
   }
 
   do {
-    /* Wait a little bit before reading */
+    // Wait a little bit before reading
     nanosleep(&rdyDelay, (struct timespec *) NULL);
 
-    int recCount = i2c_read(DRIVER_DATA(pnd)->dev, i2cRx, szDataLen + 1, abort_p, timeout);
+    int recCount = i2c_read(DRIVER_DATA(pnd)->dev, i2cRx, szDataLen + 1);
 
     if (DRIVER_DATA(pnd)->abort_flag) {
-      /* Reset abort flag */
+      // Reset abort flag
       DRIVER_DATA(pnd)->abort_flag = false;
+
+      log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_DEBUG,
+              "Wait for a READY frame has been aborted.");
       return NFC_EOPABORTED;
     }
 
@@ -385,10 +420,10 @@ pn532_i2c_wait_rdyframe(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLe
 
           if (duration / 1000 > timeout) {
             res = NFC_ETIMEOUT;
-            done = 1;
+            done = true;
 
             log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_DEBUG,
-                    "timeout reached with no RDY frame.");
+                    "timeout reached with no READY frame.");
           }
         }
       }
@@ -398,15 +433,30 @@ pn532_i2c_wait_rdyframe(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLe
   return res;
 }
 
+/**
+ * @brief Read a response frame from the PN532 device.
+ *
+ * @param pnd pointer on the NFC device.
+ * @param pbtData buffer used to store the response frame data.
+ * @param szDataLen allocated size of buffer.
+ * @param timeout timeout delay before aborting the operation (in ms). Use 0 for no timeout.
+ * @return length (in bytes) of the response, or NFC_ETIMEOUT if timeout delay has expired,
+ *         NFC_EOPABORTED if operation has been aborted, NFC_EIO in case of IO failure
+ */
 static int
 pn532_i2c_receive(nfc_device *pnd, uint8_t *pbtData, const size_t szDataLen, int timeout)
 {
-  uint8_t frameBuf[PN53X_MAX_FRAME_LENGTH];
+  uint8_t frameBuf[PN53x_EXTENDED_FRAME__DATA_MAX_LEN];
   int frameLength;
   int TFI_idx;
   size_t len;
 
   frameLength = pn532_i2c_wait_rdyframe(pnd, frameBuf, sizeof(frameBuf), timeout);
+
+  if (NFC_EOPABORTED == pnd->last_error) {
+    return pn532_i2c_ack(pnd);
+  }
+
   if (frameLength < 0) {
     goto error;
   }
@@ -497,18 +547,24 @@ error:
   return pnd->last_error;
 }
 
+/**
+ * @brief Send an ACK frame to the PN532 device.
+ *
+ * @param pnd pointer on the NFC device.
+ * @return NFC_SUCCESS on success, otherwise an error code
+ */
 int
 pn532_i2c_ack(nfc_device *pnd)
 {
-  if (POWERDOWN == CHIP_DATA(pnd)->power_mode) {
+  return i2c_write(DRIVER_DATA(pnd)->dev, pn53x_ack_frame, sizeof(pn53x_ack_frame));
-    int res = 0;
-    if ((res = pn532_i2c_wakeup(pnd)) < 0) {
-      return res;
-    }
-  }
-  return i2c_write(DRIVER_DATA(pnd)->dev, pn53x_ack_frame, sizeof(pn53x_ack_frame),  0);
 }
 
+/**
+ * @brief Abort any pending operation
+ *
+ * @param pnd pointer on the NFC device.
+ * @return NFC_SUCCESS
+ */
 static int
 pn532_i2c_abort_command(nfc_device *pnd)
 {

libnfc/drivers/pn532_i2c.h

@@ -34,8 +34,6 @@
 
 #include <nfc/nfc-types.h>
 
-#  define PN53X_MAX_FRAME_LENGTH  265
-
 /* Reference to the I2C driver structure */
 extern const struct nfc_driver pn532_i2c_driver;