[riot-notifications] [RIOT-OS/RIOT] sys/ztimer: initial import (#11874)

Marian Buschsieweke notifications at github.com
Fri Sep 6 11:22:40 CEST 2019


maribu requested changes on this pull request.

The code is disabling type checking by force casting at many instances. Most of the casts are not needed and can be replaced by code that does not disable type checking. Converting from a generic to a specific (e.g. `ztimer_dev_t *` to `ztimer_periph_t`) could be moved to a static inline function. That will make refactoring easier, e.g. if the structure layout of `ztimer_periph_t` changed that `ztimer_dev_t` no longer is the first member.

Is there any case where a `ztimer_base_t *` does not point to a `ztimer_t`? If not, what is the reason to have `ztimer_base_t`?

> +/**
+ * @ingroup     sys_ztimer_periph
+ * @{
+ *
+ * @file
+ * @brief       ztimer periph/timer backend implementation
+ *
+ * @author      Kaspar Schleiser <kaspar at schleiser.de>
+ *
+ * @}
+ */
+#include "ztimer/periph.h"
+
+static void _ztimer_periph_set(ztimer_dev_t *ztimer, uint32_t val)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;

Please add

```C
static line ztimer_periph_t * ztimer_periph_from_dev (ztimer_dev_t *dev) {
    return (ztimer_periph_t *)dev;
}
```

And use it instead of the forced cast everywhere.

> +static void _ztimer_periph_set(ztimer_dev_t *ztimer, uint32_t val)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;
+
+    unsigned adjust = ztimer_periph->adjust;
+    if (val > adjust) {
+        val -= adjust;
+    }
+
+    /* TODO: ensure this is done atomically */
+    timer_set(ztimer_periph->dev, 0, val);
+}
+
+static uint32_t _ztimer_periph_now(ztimer_dev_t *ztimer)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;

See above

> +        val -= adjust;
+    }
+
+    /* TODO: ensure this is done atomically */
+    timer_set(ztimer_periph->dev, 0, val);
+}
+
+static uint32_t _ztimer_periph_now(ztimer_dev_t *ztimer)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;
+    return timer_read(ztimer_periph->dev);
+}
+
+static void _ztimer_periph_cancel(ztimer_dev_t *ztimer)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;

See above

> +static uint32_t _ztimer_periph_now(ztimer_dev_t *ztimer)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;
+    return timer_read(ztimer_periph->dev);
+}
+
+static void _ztimer_periph_cancel(ztimer_dev_t *ztimer)
+{
+    ztimer_periph_t *ztimer_periph = (ztimer_periph_t*) ztimer;
+    timer_clear(ztimer_periph->dev, 0);
+}
+
+static void _ztimer_periph_callback(void *arg, int channel)
+{
+    (void)channel;
+    ztimer_handler((ztimer_dev_t*) arg);

No explicit cast required. (`void *` can be casted implicitly.)

> +        { .tm_year = c - 4716 - 1900, .tm_mon = e - 1, .tm_mday = f,
+          .tm_hour = h, .tm_min = m, .tm_sec = s };
+        *_tm = tmp;
+    }
+    else {
+        struct tm tmp =
+        { .tm_year = c - 4715 - 1900, .tm_mon = e - 13, .tm_mday = f,
+          .tm_hour = h, .tm_min = m, .tm_sec = s };
+        *_tm = tmp;
+    }
+}
+
+static void _ztimer_rtc_callback(void *arg)
+{
+    puts(".");
+    ztimer_handler((ztimer_dev_t *)arg);

No explicit cast required

> + * @file
+ * @brief       ztimer periph/rtt implementation
+ *
+ * @author      Kaspar Schleiser <kaspar at schleiser.de>
+ *
+ * @}
+ */
+#include "periph/rtt.h"
+#include "ztimer/rtt.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+static void _ztimer_rtt_callback(void *arg)
+{
+    ztimer_handler((ztimer_dev_t*) arg);

No explicit cast required

> + */
+#include <assert.h>
+
+#include "mutex.h"
+#include "thread.h"
+#include "ztimer.h"
+
+typedef struct {
+    mutex_t *mutex;
+    thread_t *thread;
+    int timeout;
+} mutex_thread_t;
+
+static void _callback_unlock_mutex(void* arg)
+{
+    mutex_t *mutex = (mutex_t *) arg;

No explicit cat required

> +    mutex_t *mutex = (mutex_t *) arg;
+    mutex_unlock(mutex);
+}
+
+void ztimer_sleep(ztimer_dev_t *ztimer, uint32_t duration)
+{
+/*    if (irq_is_in()) {
+        _ztimer_spin(duration);
+        return;
+    }
+*/
+    ztimer_t timer;
+    mutex_t mutex = MUTEX_INIT_LOCKED;
+
+    timer.callback = _callback_unlock_mutex;
+    timer.arg = (void*) &mutex;

Replace by
```C
timer.arg = (void *)&mutex;
```
(Space position)

> +    uint32_t now = ztimer_now(ztimer);
+    uint32_t target = *last_wakeup + period;
+    uint32_t offset = target - now;
+
+    if (offset <= period) {
+        ztimer_sleep(ztimer, offset);
+        *last_wakeup = target;
+    }
+    else {
+        *last_wakeup = now;
+    }
+}
+
+static void _callback_msg(void* arg)
+{
+    msg_t *msg = (msg_t*)arg;

No explicit cast required

> +    }
+    else {
+        *last_wakeup = now;
+    }
+}
+
+static void _callback_msg(void* arg)
+{
+    msg_t *msg = (msg_t*)arg;
+    msg_send_int(msg, msg->sender_pid);
+}
+
+static inline void _setup_msg(ztimer_t *timer, msg_t *msg, kernel_pid_t target_pid)
+{
+    timer->callback = _callback_msg;
+    timer->arg = (void*) msg;

Replace by

```C
timer->arg = (void *)msg;
```

> +#define SAMPLES 1024
+
+static ztimer_periph_t _ztimer_periph;
+
+int main(void)
+{
+    ztimer_periph_init(&_ztimer_periph, 0, 1000000LU);
+
+    uint32_t total = 0;
+
+    uint16_t min = 0xFFFF;
+    uint16_t max = 0;
+
+    unsigned n = SAMPLES;
+    while (n--) {
+        unsigned diff = ztimer_diff((ztimer_dev_t *)&_ztimer_periph, 1000);

Replace by `&_ztimer_periph.dev`

> +}
+
+static uint32_t _convert_now(ztimer_convert_t *ztimer_convert, uint32_t val)
+{
+    if (ztimer_convert->div > 1) {
+        val *= ztimer_convert->div;
+    }
+    if (ztimer_convert->mul > 1) {
+        val /= ztimer_convert->mul;
+    }
+    return val;
+}
+
+static void _ztimer_convert_cancel(ztimer_dev_t *ztimer)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;

Add static inline function for conversion

> +    }
+    if (ztimer_convert->mul > 1) {
+        val /= ztimer_convert->mul;
+    }
+    return val;
+}
+
+static void _ztimer_convert_cancel(ztimer_dev_t *ztimer)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;
+    ztimer_remove(ztimer_convert->parent, &ztimer_convert->parent_entry);
+}
+
+static void _ztimer_convert_set(ztimer_dev_t *ztimer, uint32_t val)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;

Use static inline function for conversion

> +
+static void _ztimer_convert_cancel(ztimer_dev_t *ztimer)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;
+    ztimer_remove(ztimer_convert->parent, &ztimer_convert->parent_entry);
+}
+
+static void _ztimer_convert_set(ztimer_dev_t *ztimer, uint32_t val)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;
+    ztimer_set(ztimer_convert->parent, &ztimer_convert->parent_entry, _convert_set(ztimer_convert, val));
+}
+
+static uint32_t _ztimer_convert_now(ztimer_dev_t *ztimer)
+{
+    ztimer_convert_t *ztimer_convert = (ztimer_convert_t*) ztimer;

Use static inline function for conversion

> + * @brief       ztimer overhead measurement functions
+ *
+ * @author      Kaspar Schleiser <kaspar at schleiser.de>
+ *
+ * @}
+ */
+#include "ztimer.h"
+
+typedef struct {
+    ztimer_dev_t *ztimer;
+    volatile uint32_t *val;
+} callback_arg_t;
+
+static void _callback(void* arg)
+{
+    callback_arg_t *callback_arg = (callback_arg_t*) arg;

explicit cast not required

> + * @return  full width extended counter value
+ */
+static uint32_t ztimer_extend_now32(ztimer_extend_t *self, uint32_t lower_now, uint32_t origin);
+
+/**
+ * @brief   Update the ztimer queue for the lower clock
+ *
+ * @pre Interrupts masked
+ *
+ * @param[in]   self        instance to operate on
+ */
+static void ztimer_extend_update(ztimer_extend_t *self);
+
+static void ztimer_extend_alarm_callback(void* arg)
+{
+    ztimer_extend_t *self = (ztimer_extend_t *)arg;

Explicit cast not required

> + * @pre Interrupts masked
+ *
+ * @param[in]   self        instance to operate on
+ */
+static void ztimer_extend_update(ztimer_extend_t *self);
+
+static void ztimer_extend_alarm_callback(void* arg)
+{
+    ztimer_extend_t *self = (ztimer_extend_t *)arg;
+    DEBUG("ztimer_extend_alarm_callback()\n");
+    ztimer_handler(&self->super);
+}
+
+static void ztimer_extend_overflow_callback(void* arg)
+{
+    ztimer_extend_t *self = (ztimer_extend_t *)arg;

Explicit cast not required

> +    uint32_t now32 = ztimer_extend_now32(self, lower_now, self->origin);
+    uint32_t target = self->super.list.offset + self->super.list.next->offset;
+    target -= now32;
+    if ((lower_now + target) > self->lower_max) {
+        /* Await counter rollover first */
+        return;
+    }
+    DEBUG("zx: set lower_alarm %p, target=%" PRIu32 "\n",
+        (void *)&self->lower_alarm_entry, target);
+    ztimer_set(self->lower, &self->lower_alarm_entry, target);
+}
+
+static void ztimer_extend_op_set(ztimer_dev_t *z, uint32_t val)
+{
+    (void)val;
+    ztimer_extend_t *self = (ztimer_extend_t *)z;

static inline wrapper for the cast

> +    DEBUG("zx: set lower_alarm %p, target=%" PRIu32 "\n",
+        (void *)&self->lower_alarm_entry, target);
+    ztimer_set(self->lower, &self->lower_alarm_entry, target);
+}
+
+static void ztimer_extend_op_set(ztimer_dev_t *z, uint32_t val)
+{
+    (void)val;
+    ztimer_extend_t *self = (ztimer_extend_t *)z;
+
+    ztimer_extend_update(self);
+}
+
+static void ztimer_extend_op_cancel(ztimer_dev_t *z)
+{
+    ztimer_extend_t *self = (ztimer_extend_t *) z;

See above

> +#    define ZTIMER_USEC_INIT_PERIPH() \
+            ztimer_periph_init(&_ztimer_usec_periph, CONFIG_ZTIMER_USEC_DEV, \
+                             CONFIG_ZTIMER_USEC_FREQ)
+#    define _ZTIMER_USEC_DEV _ztimer_usec_periph
+#  else
+#    error unknown CONFIG_ZTIMER_USEC_TYPE!
+#  endif /* CONFIG_ZTIMER_USEC_TYPE == ZTIMER_TYPE_PERIPH */
+
+# if (CONFIG_ZTIMER_USEC_WIDTH == 32) && (ZTIMER_USEC_CONVERT_BITS == 0)
+    ztimer_dev_t *const ZTIMER_USEC = (ztimer_dev_t *) &_ZTIMER_USEC_DEV;
+# else
+#   if (ZTIMER_USEC_DIV != 0) || (ZTIMER_USEC_MUL != 0)
+      static ztimer_convert_t _ztimer_usec_convert;
+#     define ZTIMER_USEC_INIT_CONVERT() \
+          ztimer_convert_init(&_ztimer_usec_convert, \
+                              (ztimer_dev_t *)&_ZTIMER_USEC_DEV, \

Shouldn't `&_ZTIMER_USEC_DEV->dev` work regardless of what kind of backend it is, as the dev has to be the first member in any case?

> +       define ZTIMER_USEC_CONVERT_BITS  9
+#    else
+#      error unhandled CONFIG_ZTIMER_USEC_FREQ!
+#    endif
+
+     static ztimer_periph_t _ztimer_usec_periph;
+#    define ZTIMER_USEC_INIT_PERIPH() \
+            ztimer_periph_init(&_ztimer_usec_periph, CONFIG_ZTIMER_USEC_DEV, \
+                             CONFIG_ZTIMER_USEC_FREQ)
+#    define _ZTIMER_USEC_DEV _ztimer_usec_periph
+#  else
+#    error unknown CONFIG_ZTIMER_USEC_TYPE!
+#  endif /* CONFIG_ZTIMER_USEC_TYPE == ZTIMER_TYPE_PERIPH */
+
+# if (CONFIG_ZTIMER_USEC_WIDTH == 32) && (ZTIMER_USEC_CONVERT_BITS == 0)
+    ztimer_dev_t *const ZTIMER_USEC = (ztimer_dev_t *) &_ZTIMER_USEC_DEV;

Shouldn't `&_ZTIMER_USEC_DEV->dev` work regardless of what kind of backend it is, as the dev has to be the first member in any case?

-- 
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/11874#pullrequestreview-284739621
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190906/23f45631/attachment-0001.htm>


More information about the notifications mailing list