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

Jean Pierre Dudey notifications at github.com
Mon May 31 21:31:32 CEST 2021


@jeandudey requested changes on this pull request.



> +    bool "HC-SR04"
+    depends on TEST_KCONFIG

```suggestion
    bool "HC-SR04 ultrasonic distance sensor"
    depends on HAS_PERIPH_GPIO
    depends on HAS_PERIPH_GPIO_IRQ
    depends on TEST_KCONFIG
    select MODULE_PERIPH_GPIO
    select MODULE_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 "assert.h"
+#include <errno.h>
+#include <math.h>
+
+#include <stdio.h>

Is it really needed? 😉 

> +        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 = p_dev->sound_speed * dt_us_one_way / 1000;
+    }
+
+    mutex_unlock(&p_dev->lock);
+}
+
+int hcsr04_init(hcsr04_t *dev, const hcsr04_params_t *params)
+{
+    assert(dev && params);
+
+    dev->params = *params;
+
+    mutex_init(&dev->lock);

```suggestion
    mutex_init_locked(&dev->lock);
```

To save one call at `hcsr04_trigger` (initially locked, try to lock again until the isr releases it)

> +int hcsr04_trigger(hcsr04_t *dev)
+{
+    uint32_t now = xtimer_now_usec();
+    uint16_t min_response_time = hcsr04_get_max_response_time(dev);
+    
+    if ((now - dev->pre_trig_t_us) < min_response_time) {
+        return -EFREQ;
+    }
+    
+    dev->was_triggered = 1;
+    dev->pre_trig_t_us = now;
+    gpio_set(dev->params.trigger_pin);
+    xtimer_usleep(TRIGGER_TIME);
+    gpio_clear(dev->params.trigger_pin);
+    
+    mutex_unlock(&dev->lock);
+    mutex_lock(&dev->lock);
+
+    return 0;
+}

I think it could be made private (e.g.: a static function) and call it from the `hcsr04_get_distance` function as you do, thus leaving only one function for getting the distance (by triggering the read), allows to remove the `was_triggered` field saving some bytes (and anyway if the read was not triggered an error is returned so, why not 😉 ). Same with the `hcsr04_read` function (make it private).

> +    // unlock it anyway
+    mutex_unlock(&dev->lock);

```suggestion
```

Don't unlock it, for the next trigger/read the mutex needs to be locked so it may just be left as is.

> +        *distance = 0;
+        return -ECON;
+    }
+    
+    if (dev->distance < HCSR04_MIN_DISTANCE_MM) {
+        *distance = HCSR04_MIN_DISTANCE_MM;
+    } else if (dev->distance > HCSR04_MAX_DISTANCE_MM) {
+        *distance = HCSR04_MAX_DISTANCE_MM;
+    } else {
+        *distance = dev->distance;
+    }    
+
+    return 0;
+}
+
+uint16_t hcsr04_temp_to_sound_speed(uint16_t temp) 

```suggestion
uint16_t hcsr04_temp_to_sound_speed(uint16_t temp) 
```

If not used outside of the module may as well make it private 😉 

> +    mutex_unlock(&dev->lock);
+    mutex_lock(&dev->lock);

Can be removed with the `mutex_init_locked` change proposed (and leaving the mutex locked after the read).

> +#ifndef AIR_TEMPERATURE                           
+#define AIR_TEMPERATURE                     (24000U)    
+#endif

```suggestion
#ifndef HCSR04_AIR_TEMPERATURE                           
#define HCSR04_AIR_TEMPERATURE                     (24000U)    
#endif
```

> +    mutex_t lock;
+    uint8_t was_triggered;
+    uint16_t distance;
+    uint16_t sound_speed;
+    uint32_t pre_trig_t_us;
+    uint32_t pre_meas_t_us;
+    hcsr04_params_t params;
+} hcsr04_t;
+
+/**
+ * @brief   Initialize an HC-SR04 sensor
+ *
+ * @param[inout] dev        Device descriptor of the driver
+ * @param[in]    params     Initialization parameters
+ *
+ * @return                  EINVAL of invalid parameters

```suggestion
 * @retval                  EINVAL of invalid parameters
```

Same for other `@return`s with multiple values (>1) here and in other functions 

> +    // necessary in order for the next trigger not to fail due to time constraint 
+    xtimer_msleep(30);

Is this still necessary, why does it fail?

> +    dev->params.temperature = 0;
+    dev->params.trigger_pin = 0;
+    dev->params.echo_pin = 0;
+
+    mutex_unlock(&dev->lock);
+    dev->distance = 0;
+    dev->was_triggered = 0;
+    dev->sound_speed = 0;
+    dev->pre_trig_t_us = 0;
+    dev->pre_meas_t_us = 0;

```suggestion
    dev->params.temperature = 0;
    dev->params.trigger_pin = 0;
    dev->params.echo_pin = 0;

    mutex_unlock(&dev->lock);
    dev->distance = 0;
    dev->was_triggered = 0;
    dev->sound_speed = 0;
    dev->pre_trig_t_us = 0;
    dev->pre_meas_t_us = 0;
```

I think only the GPIO stuff could be left as if the memory of the device descriptor gets unused/deallocated it wont matter that much if it's all zero

> +#include "phydat.h"
+#include "saul_reg.h"
+
+#include "hcsr04_params.h"
+#include "hcsr04.h"
+
+
+#define TEST_TIME                               (3e6)
+#define TEST_CONVERTION_MACRO(x)                
+
+
+int main(void) 
+{
+    puts("********** Test HC-SR04 RIOT driver! **********");
+
+    // deactivate saul auto-initialization, due to conflict when both work at the same time

May as well use the `/* */`-style of comments (to follow the existing style in RIOT's codebase) both here and in the driver code :wink:

-- 
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-672497733
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210531/8f7e36b5/attachment-0001.htm>


More information about the notifications mailing list