[riot-notifications] [RIOT-OS/RIOT] Wiegand driver for RIOT-OS (#12217)

benpicco notifications at github.com
Wed Sep 25 09:45:34 CEST 2019


benpicco approved this pull request.

Thank you for your contribution.

I've added a quick first review. Please read https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions

You can run 
`$ uncrustify -c $RIOTBASE/uncrustify-riot.cfg <your file>`
for automated code style conversion, but beware that manual oversight is still required.

What kind of hardware do you use with this driver?

> +  typedef struct {
+    gpio_t       d0;      /**< Wiegand D0 data line */
+    gpio_t       d1;      /**< Wiegand D1 data line */
+    gpio_flank_t flank;   /**< flank detection */
+  } wiegand_params_t;
+
+  /**
+   * @brief   Device descriptor for a wiegand device
+   */
+  typedef struct {
+    uint32_t code;                         /**< Wiegand code */
+    int16_t type;                          /**< Wiegand type */
+    volatile uint32_t wg_card_temp_high;   /**< Card MSB */
+    volatile uint32_t wg_card_temp;        /**< Card LSV  */
+    volatile uint32_t wg_last_wiegand;     /**< Last card lecture */
+    volatile int16_t wg_bit_count;         /**< Number of bits read */

Why are these `volatile`?

> +    volatile uint32_t wg_card_temp;        /**< Card LSV  */
+    volatile uint32_t wg_last_wiegand;     /**< Last card lecture */
+    volatile int16_t wg_bit_count;         /**< Number of bits read */
+    const wiegand_params_t *conn;          /**< Connection parameters */
+  } wiegand_t;
+
+  /**
+   * @brief   Initialize a Wiegand device
+   *
+   * @param[out] dev          device descriptor
+   * @param[in] params        configuration parameters
+   *
+   * @return                   0 on success
+   * @return                  -1 on error
+   */
+  int16_t wg_init(wiegand_t *dev, const wiegand_params_t *params);

I'd suggest you to use `wiegand_` as a prefix instead of `wg_`.
Also just use `int` for error codes.

> + * @file
+ * @brief       Driver for the Wiegand Protocol.
+ *
+ * @author      Mario Gomez <mario.gomez at teubi.co>
+ *
+ * @}
+ */
+
+#include "xtimer.h"
+#include "wiegand_params.h"
+
+/**
+ * @brief Handler for D0 signal in Wiegand protocol
+ *
+ */
+void wg_read_d0(void *arg)

private functions should be `static`.

> +  {
+    // shift value to high bits
+    dev->wg_card_temp_high |= ((0x80000000 & dev->wg_card_temp)>>31);
+    dev->wg_card_temp_high <<= 1;
+    dev->wg_card_temp |= 1;
+    dev->wg_card_temp <<=1;
+  }
+  else
+  {
+    // D1 represent binary 1, so OR card data with 1 then
+    dev->wg_card_temp |= 1;
+    // left shift card data
+    dev->wg_card_temp <<= 1;
+  }
+  // Keep track of last wiegand bit received
+  dev->wg_last_wiegand = xtimer_now_usec64();

since `dev->wg_last_wiegand` is 32 bit anyway, `xtimer_now_usec()` is enough.

> +   * @brief   Parameters needed for device initialization
+   */
+  typedef struct {
+    gpio_t       d0;      /**< Wiegand D0 data line */
+    gpio_t       d1;      /**< Wiegand D1 data line */
+    gpio_flank_t flank;   /**< flank detection */
+  } wiegand_params_t;
+
+  /**
+   * @brief   Device descriptor for a wiegand device
+   */
+  typedef struct {
+    uint32_t code;                         /**< Wiegand code */
+    int16_t type;                          /**< Wiegand type */
+    volatile uint32_t wg_card_temp_high;   /**< Card MSB */
+    volatile uint32_t wg_card_temp;        /**< Card LSV  */

Have you considered just using `uint64_t` here?

> +  wiegand_params_t wiegand_param = {
+    .d0 = WG_IN_D0,
+    .d1 = WG_IN_D1,
+    .flank = GPIO_RISING
+  };
+
+  // Note: Default wiegand_params can be changed on the Makefile
+  wg_init(&wiegand_dev, &wiegand_param);
+
+  // Note: uncomment the following to use the defaul parameters
+  // requires to include the file "wiegand_params.h"
+  // defaults could be changed on the Makefile
+  //wg_init(&wiegand_dev, &wiegand_params);
+
+  while(1) {
+    if(wg_available(&wiegand_dev)) {

Instead of polling, you could register a callback with your driver that gets called when data is available.

> +    return *codehigh | *codelow;
+  }
+  return *codelow; // EM tag or Mifare without parity bits
+}
+
+/**
+ * @brief Process the temporary Wiegand buffer and decodes the data.
+ *
+ */
+bool wg_do_conversion(wiegand_t *dev)
+{
+  uint64_t sysTick;
+
+  sysTick = xtimer_now_usec64();
+
+  if ((sysTick - dev->wg_last_wiegand) > 25000) // if no more signal coming through after 25ms

Instead you could do `thread_sleep()` in `wg_get_code()` and call [`xtimer_set_wakeup(timer, timeout, thread_getpid())`](https://riot-os.org/api/group__sys__xtimer.html#gab46b49ec5cf112476c83ca727bb77f67) before.

Then instead of `dev->wg_last_wiegand = xtimer_now_usec64();` you can just call `xtimer_set(timer, 25000);` to set the timeout.



-- 
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/12217#pullrequestreview-292871947
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190925/071a8d3b/attachment-0001.htm>


More information about the notifications mailing list