[riot-notifications] [RIOT-OS/RIOT] drivers/dcf77:Inital Support DCF77 (#12259)

benpicco notifications at github.com
Tue Sep 24 14:35:38 CEST 2019


benpicco commented on this pull request.

I think blocking in `dcf77_read()` is not very useful.

How about you create a dedicated thread for dcf77 that continuously waits for interrupts to decode the signal? Possibly you don't even need a thread and can do all the work in the interrupt handler.
As all you do is reading timers and setting bits, there is no I/O and functions are short, it should be save to do this in the GPIO interrupt callback. Of course you can't do any polling then 😉 

Btw.: What kind of dcf77 module are you using? 

> +    WEEKDAY_QUAD      = 44,
+    MONTH_SINGLE      = 45,
+    MONTH_SECOND      = 46,
+    MONTH_QUAD        = 47,
+    MONTH_EIGHT       = 48,
+    MONTH_TENNER      = 49,
+    YEAR_SINGLE       = 50,
+    YEAR_SECOND       = 51,
+    YEAR_QUAD         = 52,
+    YEAR_EIGHT        = 53,
+    YEAR_TENNER       = 54,
+    YEAR_TEWENTIES    = 55,
+    YEAR_FOURTIES     = 56,
+    YEAR_EIGHTIES     = 57,
+    YEAR_PR           = 58
+};

Don't put that into the global namespace, without a prefix those constants will clash with other code.

Put that into a private file inside `dcf77/include`.

> + * @brief   Data type for storing DCF77 sensor readings
+ */
+typedef struct {
+    uint8_t minute;       /**< minutes*/
+    uint8_t hours;        /**< hours*/
+    uint8_t day;          /**< days*/
+    uint8_t month;        /**< month*/
+    uint8_t year;         /**< years*/
+    uint8_t weekday;      /**< weekday*/
+    uint8_t mesz;         /**< Status*/
+    uint8_t parity;       /**< parity*/
+    uint16_t addons;      /**< Addons Wheater, emergency measures*/
+} dcf77_data_t;
+
+
+

One new line should be enough.

> + *
+ * @param[in]  dev          device descriptor of a DCF device
+ * @param[out] minute       minute
+ * @param[out] hour         hour
+ * @param[out] weekday      weekday
+ * @param[out] calenderday  calenderday
+ * @param[out] month        month
+ * @param[out] year         year
+ * @param[out] mesz         mesz
+ *
+ * @retval `DCF_OK`         Success
+ * @retval `DCF_NOCSUM`     Checksum error
+ * @retval `DCF_TIMEOUT`    Reading data timed out (check wires!)
+ */
+int dcf77_read(dcf77_t *dev, uint8_t *minute, uint8_t *hour, uint8_t *weekday,
+  uint8_t *calenderday, uint8_t *month, uint8_t *year, uint8_t *mesz);

Just use [`struct tm`](https://pubs.opengroup.org/onlinepubs/007908799/xsh/time.h.html).

> + * @param   dev     device
+ * @param   params  device_params
+ *
+ * @retval  DCF77_OK             Success
+ * @retval  DCF77_INIT_ERROR     Error in initalization
+ */
+
+int dcf77_init(dcf77_t *dev, const dcf77_params_t *params)
+{
+    DEBUG("dcf77_init\n");
+
+    /* check parameters and configuration */
+    assert(dev && params);
+    dev->params = *params;
+    if(gpio_init(dev->params.pin, dev->params.in_mode)!=DCF77_NOCSUM){
+    xtimer_usleep(2000 * US_PER_MS);

Why sleep for 2s here?

> +int dcf77_init(dcf77_t *dev, const dcf77_params_t *params)
+{
+    DEBUG("dcf77_init\n");
+
+    /* check parameters and configuration */
+    assert(dev && params);
+    dev->params = *params;
+    if(gpio_init(dev->params.pin, dev->params.in_mode)!=DCF77_NOCSUM){
+    xtimer_usleep(2000 * US_PER_MS);
+
+    DEBUG("dcf77_init: success\n");
+    return DCF77_OK;
+  }else{
+    DEBUG("dcf77_init: failed\n");
+    return DCF77_INIT_ERROR;
+  }

The indentation here is entirely messed up.
Also see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions

`$ uncrustify -c $RIOTBASE/uncrustify-riot.cfg <your file>`

Should help.

> +
+#define READING_CYCLE               59
+
+
+
+/**
+ * @brief   Wait until the pin @p pin has level @p expect
+ *
+ * @param   pin     GPIO pin to wait for
+ * @param   expect  Wait until @p pin has this logic level
+ * @param   timeout Timeout in µs
+ *
+ * @retval  0       Success
+ * @retval  -1      Timeout occurred before level was reached
+ */
+static inline int _wait_for_level(gpio_t pin, bool expect, unsigned timeout)

Why not use an [interrupt](https://doc.riot-os.org/group__drivers__periph__gpio.html#ga48ba4c318486db320a4bf6237b49d61c) for this?

> +     return DCF77_TIMEOUT;
+  }
+  start = xtimer_now_usec();
+
+  if (_wait_for_level(dev->params.pin, 1, TIMEOUT)) {
+     DEBUG("Timeout wait for HIGH\n");
+     return DCF77_TIMEOUT;
+
+  }
+  end = xtimer_now_usec();
+  }
+
+  /* Start with reading Bit 0*/
+
+  for (int i = 0; i < READING_CYCLE; i++) {
+    if(_read(&result,dev->params.pin,1,0,PULSE_WIDTH_THRESHOLD)==DCF77_OK){

Please add spaces after each comma.

-- 
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/12259#pullrequestreview-290452545
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190924/b9ca6076/attachment-0001.htm>


More information about the notifications mailing list