[riot-notifications] [RIOT-OS/RIOT] Hcsr04 module (#16499)

Jean Pierre Dudey notifications at github.com
Fri May 28 11:42:48 CEST 2021


@jeandudey requested changes on this pull request.

Hello @eurichon , thanks for your contribution!

I've done a first pass review on the code, if you have any questions let me know 😉

> +  FEATURES_REQUIRED += periph_gpio
+  USEMODULE += periph_gpio_irq

```suggestion
  FEATURES_REQUIRED += periph_gpio periph_gpio_irq
```

> + * @}
+ */
+
+#include "hcsr04.h"
+#include "hcsr04_constants.h"
+#include "hcsr04_params.h"
+
+#include "periph/gpio.h"
+#include "xtimer.h"
+
+#include <stdint.h>
+#include <errno.h>
+#include <math.h>
+
+
+void hcsr04_int_callback(void *arg){

```suggestion
void hcsr04_int_callback(void *arg)
{
```

Same for the other function blocks 😉 

> +#include "xtimer.h"
+
+#include <stdint.h>
+#include <errno.h>
+#include <math.h>
+
+
+void hcsr04_int_callback(void *arg){
+    uint32_t now = xtimer_now_usec();
+    hcsr04_t *p_dev = (hcsr04_t *)arg;
+
+    if (gpio_read(p_dev->params.echo_pin)) {
+        p_dev->pre_meas_t_us = now;
+    } else {
+        uint32_t dt_us_one_way = (now - p_dev->pre_meas_t_us) / 2;
+        p_dev->distance = 1e-3 * p_dev->sound_speed * dt_us_one_way;

The `1e-3` is a floating point literal, this limits the hardware that could use this code, instead divide it by `1e3`. e.g:

```suggestion
        p_dev->distance = (p_dev->sound_speed * dt_us_one_way) / 1000;
```

> +    if (!dev || ! params) {
+        return -EINVAL;
+    }

```suggestion
    assert(dev && params);
```

Commonly most RIOT drivers use assertions to check for parameters 😉 

> +        p_dev->distance = 1e-3 * p_dev->sound_speed * dt_us_one_way;
+    }
+}
+
+int hcsr04_init(hcsr04_t *dev, const hcsr04_params_t *params)
+{
+    if (!dev || ! params) {
+        return -EINVAL;
+    }
+
+    dev->params = *params;
+
+    dev->distance = 0;
+    dev->pre_trig_t_us = xtimer_now_usec();
+    dev->pre_meas_t_us = xtimer_now_usec();
+    hcsr04_set_temp(dev, (*params).temperature);

```suggestion
    hcsr04_set_temp(dev, params->temperature);
```

> +    if (!dev || ! params) {
+        return -EINVAL;
+    }
+
+    dev->params = *params;
+
+    dev->distance = 0;
+    dev->pre_trig_t_us = xtimer_now_usec();
+    dev->pre_meas_t_us = xtimer_now_usec();
+    hcsr04_set_temp(dev, (*params).temperature);
+
+    if (gpio_init(dev->params.trigger_pin, GPIO_OUT)) {
+        return -EIO;
+    }
+
+    if (gpio_init_int(dev->params.echo_pin, GPIO_IN, GPIO_BOTH, hcsr04_int_callback, (void *)(dev))) {

```suggestion
    if (gpio_init_int(dev->params.echo_pin, GPIO_IN, GPIO_BOTH, hcsr04_int_callback, dev)) {
```

The conversion is automatic when converting pointers to a `void *` pointer 😉 

> +    uint16_t distance;
+    int status = hcsr04_get_distance(dev, &distance);
+
+    if (dev->distance == 0) {
+        return -ECON;
+    }

When a read is triggered the driver waits a certain time before the interrupt gets called and the `dev->distance` value gets updated right? I think it could be better to use a `mutex_t` on the device descriptor `hcsr04_t` for this.

Initially you lock the mutex before triggering the read, e.g.:
1. Lock the mutex (`mutex_lock(&dev->lock)`)
2. Then trigger the read.
3. Try to lock it again (`mutex_lock(&dev->lock)`) or with (`xtimer_mutex_lock_timeout(&dev->lock, min_wait)`) if the device may not respond and check for `distance == 0`
4. On the IRQ callback unlock the mutex to continue the execution on point 3.

This way the value should be always synchronized.

> +
+    xtimer_msleep(30);
+
+    uint16_t distance;
+    int status = hcsr04_get_distance(dev, &distance);
+
+    if (dev->distance == 0) {
+        return -ECON;
+    }
+
+    return status;
+}
+
+int hcsr04_trigger(hcsr04_t *dev) {
+    uint32_t now = xtimer_now_usec();
+    uint16_t min_delay = 1e3 * HCSR04_MAX_DISTANCE_MM / dev->sound_speed;

```suggestion
    uint16_t min_delay = 1000 * HCSR04_MAX_DISTANCE_MM / dev->sound_speed;
```

Same as above 😉 

> +# name of your application
+APPLICATION = hcsr04
+
+# If no BOARD is found in the environment, use this default:
+BOARD ?= native
+
+# This has to be the absolute path to the RIOT base directory:
+RIOTBASE ?= $(RIOTBASE)
+
+USEMODULE += hcsr04
+USEMODULE += saul_default
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+DEVELHELP ?= 1
+
+CFLAGS += -DHCSR04_TRIGGER_PIN=2
+CFLAGS += -DHCSR04_ECHO_PIN=4
+
+# Change this to 0 show compiler invocation lines by default:
+QUIET ?= 1

```suggestion
include ../Makefile.tests_common

DRIVER ?= hcsr04

USEMODULE += saul_default

CFLAGS += -DHCSR04_TRIGGER_PIN=2
CFLAGS += -DHCSR04_ECHO_PIN=4            
```

You may also specify the `HCSR04_` parameter values on the command line and remove them from the Makefile since it will prevent boards that contain the hcsr04 that define their parameters for the driver in the `board.h` file to compile this example. Like so:

`CFLAGS="-DHCSR04_TRIGGER_PIN=2 -DHCSR04_ECHO_PIN=4" make -C tests/driver_hcsr04 BOARD=<your board>`

Alternatively if your board has the given sensor you can add it to it's `board.h` file.

-- 
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/16499#pullrequestreview-671092414
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210528/1b3213cf/attachment-0001.htm>


More information about the notifications mailing list