[riot-notifications] [RIOT-OS/RIOT] drivers/mfrc522: add new driver (#16782)

Francisco notifications at github.com
Wed Sep 8 13:04:20 CEST 2021


@fjmolinas commented on this pull request.

Thanks, for this, sole initial comments, can you provide some testing output?

> + *
+ * @note  CRC validation can only be done if back_data and back_len are specified
+ *
+ * @param[in]     dev         Device descriptor of the MFRC522
+ * @param[in]     command     The command to execute
+ * @param[in]     wait_irq    The bits in the ComIrqReg register that signals successful completion of the command
+ * @param[in]     send_data   Data to transfer to the FIFO
+ * @param[in]     send_len    Number of bytes to transfer to the FIFO
+ * @param[in]     back_data   Buffer if data should be read back after executing the command, otherwise NULL
+ * @param[in]     back_len    Max number of bytes to write to *back_data
+ * @param[out]    back_len    The number of bytes returned
+ * @param[in/out] valid_bits  The number of valid bits in the last byte. 0 for 8 valid bits.
+ * @param[in]     rx_align    Defines the bit position in back_data[0] for the first bit received
+ * @param[in]     check_crc   True => The last two bytes of the response is assumed to be a CRC_A that must be validated
+ *
+ * @return MFRC522_STATUS_OK on success, MFRC522_STATUS_??? otherwise

Can you use ERROR codes instead according to doc.riot-os.org/driver-guide.html?

> + *
+ * @param[in]     dev         Device descriptor of the MFRC522
+ * @param[in]     command     The command to execute
+ * @param[in]     wait_irq    The bits in the ComIrqReg register that signals successful completion of the command
+ * @param[in]     send_data   Data to transfer to the FIFO
+ * @param[in]     send_len    Number of bytes to transfer to the FIFO
+ * @param[in]     back_data   Buffer if data should be read back after executing the command, otherwise NULL
+ * @param[in]     back_len    Max number of bytes to write to *back_data
+ * @param[out]    back_len    The number of bytes returned
+ * @param[in/out] valid_bits  The number of valid bits in the last byte. 0 for 8 valid bits.
+ * @param[in]     rx_align    Defines the bit position in back_data[0] for the first bit received
+ * @param[in]     check_crc   True => The last two bytes of the response is assumed to be a CRC_A that must be validated
+ *
+ * @return MFRC522_STATUS_OK on success, MFRC522_STATUS_??? otherwise
+ */
+mfrc522_status_code_t mfrc522_pcd_communicate_with_picc(mfrc522_t *dev,

Missing documentaiton

> +                                                        uint8_t *back_data,
+                                                        uint8_t *back_len,
+                                                        uint8_t *valid_bits,
+                                                        uint8_t rx_align,
+                                                        bool check_crc);
+
+/**
+ * @brief Transmits REQA, Type A. Invites PICCs in state IDLE to go to READY and prepare
+ *        for anti-collision or selection. 7 bit frame.
+ *
+ * @note  When two PICCs are in the field at the same time we often get
+ *        MFRC522_STATUS_TIMEOUT - probably due do bad antenna design.
+ *
+ * @param[in]      dev          Device descriptor of the MFRC522
+ * @param[out]     buffer_atqa  Buffer to store the ATQA in
+ * @param[in/out]  buffer_size  Buffer size, at least two bytes. Also number of bytes returned if STATUS_OK.

```suggestion
 * @param[inout]  buffer_size  Buffer size, at least two bytes. Also number of bytes returned if STATUS_OK.
```

> +
+/**
+ * @brief Transfers data to MFRC522's FIFO, executes a command, waits for
+ *        completion and transfers data back from the FIFO
+ *
+ * @note  CRC validation can only be done if back_data and back_len are specified
+ *
+ * @param[in]     dev         Device descriptor of the MFRC522
+ * @param[in]     command     The command to execute
+ * @param[in]     wait_irq    The bits in the ComIrqReg register that signals successful completion of the command
+ * @param[in]     send_data   Data to transfer to the FIFO
+ * @param[in]     send_len    Number of bytes to transfer to the FIFO
+ * @param[in]     back_data   Buffer if data should be read back after executing the command, otherwise NULL
+ * @param[in]     back_len    Max number of bytes to write to *back_data
+ * @param[out]    back_len    The number of bytes returned
+ * @param[in/out] valid_bits  The number of valid bits in the last byte. 0 for 8 valid bits.

```suggestion
 * @param[inout] valid_bits  The number of valid bits in the last byte. 0 for 8 valid bits.
```

> + * @return MFRC522_STATUS_OK on success, MFRC522_STATUS_??? otherwise
+ */
+mfrc522_status_code_t mfrc522_picc_request_a(mfrc522_t *dev,
+                                             uint8_t *buffer_atqa,
+                                             uint8_t *buffer_size);
+
+/**
+ * @brief Transmits WUPA, Type A. Invites PICCs in state IDLE and HALT to go to READY(*)
+ *        and prepare for anti-collision or selection. 7 bit frame.
+ *
+ * @note  When two PICCs are in the field at the same time we often get
+ *        MFRC522_STATUS_TIMEOUT - probably due do bad antenna design.
+ *
+ * @param[in]      dev          Device descriptor of the MFRC522
+ * @param[out]     buffer_atqa  Buffer to store the ATQA in
+ * @param[in/out]  buffer_size  Buffer size, at least two bytes. Also number of bytes returned if STATUS_OK.

```suggestion
 * @param[inout]  buffer_size  Buffer size, at least two bytes. Also number of bytes returned if STATUS_OK.
```

> @@ -0,0 +1,4 @@
+FEATURES_REQUIRED += periph_gpio
+FEATURES_REQUIRED += periph_spi
+
+USEMODULE += xtimer

Might I suggest using `ztimer` instead? 

> +        /* 5000 * 18 us sums up to 90 ms */
+        xtimer_usleep(18);
+
+        uint8_t n = mfrc522_device_read(dev, MFRC522_REG_DIV_IRQ);
+
+        /* CRCIRq bit set - calculation done */
+        if (n & MFRC522_BIT_DIV_IRQ_CRC_IRQ) {
+            /* Stop calculating CRC for new content in the FIFO */
+            mfrc522_device_write(dev, MFRC522_REG_COMMAND, MFRC522_CMD_IDLE);
+            /* Transfer the result from the registers to the result buffer */
+            result[0] = mfrc522_device_read(dev, MFRC522_REG_CRC_RESULT_LSB);
+            result[1] = mfrc522_device_read(dev, MFRC522_REG_CRC_RESULT_MSB);
+            return MFRC522_STATUS_OK;
+        }
+    }
+    

whitespace

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16782#pullrequestreview-748997321
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210908/a23c0767/attachment.htm>


More information about the notifications mailing list