Sensor driver: PIR-based occupancy sensing by Hyungsin · Pull Request #7823 · RIOT-OS/RIOT

Hyungsin · 2017-10-24T18:22:04Z

This PR provides a driver for EKMB PIR motion sensor.

The current driver operation is:

When the sensor detects motion, the connected GPIO pin goes high. Otherwise, it goes low.
The current driver supports to detect occupancy in % (how long has the GPIO been high?).
It supports AUTO_INIT and SAUL.

PeterKietzmann · 2017-11-16T12:31:36Z

@Hyungsin pleas rebase. @MichelRottleuthner can you have a look at this PR? I've seen you playing with your PIR motion sensor so often, now please provide a review!

MichelRottleuthner

Thank you for providing this PR.
My thoughts on this: The name of the driver is to specific. There are countless PIR sensors that could be used with this driver - not only EKMB/Panasonic. They all share the same simple concept that a pin changes state when presence/movement was detected. Also, there already is a PIR driver in RIOT: https://github.com/RIOT-OS/RIOT/tree/master/drivers/pir. Your driver indeed adds useful features that the current one lacks (SAUL/occupancy %). What do you think about adding your features to the already existing driver instead providing a new driver?

MichelRottleuthner · 2017-11-17T10:49:43Z

drivers/ekmb/ekmb.c

+bool     pin_high;
+uint64_t pin_rise_time;
+uint64_t accum_up_time;
+uint64_t last_reset_time;


Global state variables like the ones above must be avoided. Move all data that is needed for a single sensor-instance to the ekmb_t type. This allows the use of the driver for more than one device.

MichelRottleuthner · 2017-11-17T10:58:40Z

drivers/ekmb/ekmb.c

+    int pin_now = gpio_read(dev->p.gpio);
+
+    /* We were busy counting */
+    if (pin_high) {


As written above: don't use global state. The pin state of a triggered sensor should be configurable for each sensor (active high vs. active low)

MichelRottleuthner · 2017-11-17T11:22:29Z

drivers/ekmb/ekmb.c

+    }
+    /* Pin is rising */
+    if (pin_now) {
+        pin_rise_time = xtimer_usec_from_ticks64(xtimer_now64());


one of the xtimer calls is always executed so the call could always be executed at the beginning of the function. It only depends on the state how you use that value afterwards.

MichelRottleuthner · 2017-11-17T11:27:46Z

drivers/ekmb/ekmb.c

+
+int ekmb_read(ekmb_t *dev, int16_t *occup) {
+    uint64_t now = xtimer_usec_from_ticks64(xtimer_now64());
+    *occup = (uint16_t)((accum_up_time * 10000) / (now - last_reset_time));


now - last_reset_time could evaluate to 0 and thus crash the system if ekmb_read is called fast enough (especially if xtimer only has ms resolution)

MichelRottleuthner · 2017-11-17T12:29:06Z

drivers/include/ekmb.h

+ * @brief   Parameters needed for device initialization
+ */
+typedef struct {
+    gpio_t    gpio;            /**< GPIO pin that sensor is connected to */


add a param for active-high/low

MichelRottleuthner · 2017-11-17T12:29:11Z

drivers/include/ekmb.h

+  * @{
+  */
+typedef struct {
+    ekmb_params_t p;


move the global stete vars here

MichelRottleuthner · 2017-11-17T12:29:40Z

drivers/include/ekmb.h

+ * @param[out] dev          device descriptor
+ * @param[in] params        GPIO bus the device is connected to
+ *
+ * The valid address range is 0x1E - 0x1F depending on the configuration of the


copy pasta?

MichelRottleuthner · 2017-11-17T12:31:29Z

drivers/ekmb/ekmb.c

+    pin_rise_time = 0;
+    last_reset_time = xtimer_usec_from_ticks64(xtimer_now64());
+    if (gpio_init_int(params->gpio, GPIO_IN_PD, GPIO_BOTH, ekmb_trigger, dev)) {
+        return -1;


please use names for return codes

MichelRottleuthner · 2017-11-17T12:32:17Z

drivers/include/ekmb.h

+ *
+ * @return                   0 on success
+ * @return                  -1 on error
+ * @return                  -2 on invalid address


copy pasta?

MichelRottleuthner · 2017-11-17T12:32:37Z

drivers/include/ekmb.h

+ */
+int ekmb_init(ekmb_t* dev, const ekmb_params_t* params);
+
+int ekmb_read(ekmb_t* dev, int16_t *light);


Hyungsin · 2017-11-18T01:56:01Z

@MichelRottleuthner, Thank you for your review :)
I first reflected your comments. Please check.

I think adding these features to the existing PIR can be a separate work.
The PIR driver seems like using an additional thread right? That may consume more memory, which I don't like.

Hyungsin · 2018-04-18T05:28:20Z

@MichelRottleuthner, @PeterKietzmann, I checked that the PIR driver does not necessarily add another thread, and revised this PR based on the existing PIR driver. I think it is good to be reviewed! :)

tcschmidt · 2018-06-14T13:05:42Z

@MichelRottleuthner changes o.k.?

MichelRottleuthner

Sorry for my poor response time here - let's get this done finally :)
Thanks for moving your improvements to the old driver instead of creating another driver. I marked quite a few things that need some cleanup but most of it should be straightforward to apply.

MichelRottleuthner · 2018-06-18T07:26:16Z

drivers/include/pir.h

 *
 * @param[out] dev      device descriptor of an PIR sensor
- * @param[in] gpio      the GPIO device the sensor is connected to
+ * @param[in] params    parameters from the PIR sensor


replace from with of?

MichelRottleuthner · 2018-06-18T07:36:11Z

drivers/include/pir.h

+ * @brief   Read OCCUPANCY value
+ *
+ * @param[in] dev       device descriptor of the PIR motion sensor to read from
+ * @param[out] occup    occupancy radio [in 100 * percentage]


typo: ratio
Maybe enhance documentation on what this value actually stands for. Also it should be made clear that the value is affected by calling this function (e.g. percentage of occupancy since the last read).

MichelRottleuthner · 2018-06-18T07:38:52Z

drivers/pir/include/pir_params.h

+ * @{
+ */
+#ifndef PIR_PARAM_GPIO
+#define PIR_PARAM_GPIO             GPIO_PIN(PA, 6)


please use GPIO_PIN(0, 6) instead to make sure it works on platforms that use other naming schemes

MichelRottleuthner · 2018-06-18T07:42:15Z

drivers/pir/include/pir_params.h

+
+#ifndef PIR_PARAMS
+#define PIR_PARAMS                 { .gpio        = PIR_PARAM_GPIO, \
+                                     .active_high = PIR_PARAM_ACTIVE_HIGH }


MichelRottleuthner · 2018-06-18T07:46:23Z

drivers/pir/pir.c

+    dev->active = false;
+    dev->accum_active_time = 0;
+    dev->start_active_time = 0;
+    dev->last_read_time = xtimer_usec_from_ticks64(xtimer_now64());


use xtimer_now_usec64 directly here?

MichelRottleuthner · 2018-06-18T09:15:53Z

drivers/pir/pir.c

+        dev->active = true;
+    /* Pin is falling */
+    } else {
+        dev->start_active_time = 0xFFFFFFFFFFFFFFFF;


is this marking actuallly needed i.e. isn't the active flag sufficient? After all this callback is only called when the pin level changes, right?

I think so too. Thanks!

MichelRottleuthner · 2018-06-18T09:18:02Z

sys/auto_init/saul/auto_init_pir.c

+static saul_reg_t saul_entries[PIR_NUM];
+
+/**
+ * @brief   Reference the occupancy driver struct


Reference to...

MichelRottleuthner · 2018-06-18T09:31:26Z

tests/driver_pir/main.c


 static char pir_handler_stack[THREAD_STACKSIZE_MAIN];
 static pir_t dev;
+static const pir_params_t pir_param = {.gpio = PIR_GPIO, .active_high = true};


don't define params here. Instead, include pir_params.h to use default parameters defined there. This also enables overwriting the active high parameter by CFLAGS commandline parameter. References to the old PIR_GPIO in the driver_pir's Makefile and main.c should then also be adapted to your newly introduced param defines (PIR_PARAM_GPIO and PIR_PARAM_ACTIVE_HIGH)

MichelRottleuthner · 2018-06-18T09:33:21Z

tests/driver_pir/main.c

    puts("PIR motion sensor test application\n");
    printf("Initializing PIR sensor at GPIO_%ld... ", (long)PIR_GPIO);
-    if (pir_init(&dev, PIR_GPIO) == 0) {
+    if (pir_init(&dev, &pir_param) == 0) {


as stated above, use params from pir_params here (change to &pir_params[0])

MichelRottleuthner · 2018-06-18T09:40:17Z

drivers/include/pir.h

+    uint64_t accum_active_time;  /**< Accumulated active time */
+    uint64_t last_read_time;     /**< Last time when PIR status is read */
+    pir_params_t p;              /**< Configuration parameters */
 } pir_t;


I think reordering the struct members could save some memory here due to allignment/padding

Hyungsin · 2018-06-19T19:52:38Z

@MichelRottleuthner. Thank you!
I reflected your comments. Please go through it!

MichelRottleuthner · 2018-06-20T07:05:23Z

drivers/pir/pir.c

 pir_event_t pir_get_status(const pir_t *dev)
 {
-    return ((gpio_read(dev->p.gpio) == 0) ? PIR_STATUS_LO : PIR_STATUS_HI);
+    return ((gpio_read(dev->p.gpio) == dev->p.active_high) ?


careful: the gpio API returns either 0 or >0. You could change gpio_read(..) to (gpio_read(..) > 0)

MichelRottleuthner

Murdock reports errors regarding bool types on some platforms. Adding #include <stdbool.h> to pir.h should help.

Hyungsin · 2018-06-20T20:19:46Z

@MichelRottleuthner, resolved again. Thanks!
Good to squash now?

MichelRottleuthner · 2018-06-21T09:24:06Z

Yes, please go ahead and squash

Hyungsin · 2018-06-21T17:27:22Z

done :)

MichelRottleuthner · 2018-06-26T08:45:11Z

I just tested the github webeditor for resolving the remaining (minor) merge conflict. Seems like this was a bad idea and it messed something up :/ can you remove my commit and just do a proper rebase?

Hyungsin · 2018-06-26T21:16:16Z

No worries, done! :)

Hyungsin changed the title ~~EKMB PIR motion sensor driver~~ Sensor driver: EKMB PIR motion sensor driver Oct 24, 2017

Hyungsin force-pushed the hamilton-ekmb branch 2 times, most recently from e56e524 to b851aa2 Compare October 27, 2017 00:24

miri64 requested review from PeterKietzmann and haukepetersen October 28, 2017 14:49

miri64 assigned haukepetersen and PeterKietzmann Oct 28, 2017

miri64 added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 28, 2017

PeterKietzmann requested review from MichelRottleuthner and removed request for haukepetersen November 16, 2017 12:31

PeterKietzmann unassigned haukepetersen Nov 16, 2017

Hyungsin force-pushed the hamilton-ekmb branch 3 times, most recently from bc1877a to 11e8d4f Compare November 16, 2017 23:04

MichelRottleuthner requested changes Nov 17, 2017

View reviewed changes

Hyungsin force-pushed the hamilton-ekmb branch from 94107b5 to 9a1d4a6 Compare November 18, 2017 01:52

Hyungsin force-pushed the hamilton-ekmb branch from 9a1d4a6 to 7c1b2a9 Compare November 22, 2017 19:40

Hyungsin force-pushed the hamilton-ekmb branch 2 times, most recently from 51da148 to 4567661 Compare April 18, 2018 05:12

Hyungsin changed the title ~~Sensor driver: EKMB PIR motion sensor driver~~ Sensor driver: PIR-based occupancy sensing Apr 18, 2018

Hyungsin force-pushed the hamilton-ekmb branch 3 times, most recently from cbe2c83 to 8c3ec9a Compare April 18, 2018 05:26

Hyungsin force-pushed the hamilton-ekmb branch 3 times, most recently from 4d690df to aebbfc4 Compare April 18, 2018 22:17

Hyungsin force-pushed the hamilton-ekmb branch 3 times, most recently from d27221e to 5f32b4b Compare April 20, 2018 00:17

Hyungsin mentioned this pull request Apr 24, 2018

board: add hamilton board support #9013

Merged

MichelRottleuthner requested changes Jun 18, 2018

View reviewed changes

Hyungsin force-pushed the hamilton-ekmb branch from 6987399 to a1a1fcb Compare June 19, 2018 19:51

Hyungsin force-pushed the hamilton-ekmb branch 3 times, most recently from 7b442ed to 69ed4d7 Compare June 19, 2018 23:27

MichelRottleuthner reviewed Jun 20, 2018

View reviewed changes

MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 20, 2018

MichelRottleuthner requested changes Jun 20, 2018

View reviewed changes

Hyungsin force-pushed the hamilton-ekmb branch from 1b0efd6 to 8d08026 Compare June 21, 2018 16:09

MichelRottleuthner approved these changes Jun 26, 2018

View reviewed changes

driver/pir: add pir-based occupancy sensing

0c5c3d9

Hyungsin force-pushed the hamilton-ekmb branch from 403fa45 to 0c5c3d9 Compare June 26, 2018 20:46

MichelRottleuthner merged commit a34f9c1 into RIOT-OS:master Jun 27, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

Hyungsin commented Oct 24, 2017

Uh oh!

PeterKietzmann commented Nov 16, 2017

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyungsin commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hyungsin commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcschmidt commented Jun 14, 2018

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyungsin Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Hyungsin commented Nov 18, 2017 •

edited

Loading

Hyungsin commented Apr 18, 2018 •

edited

Loading

Hyungsin Jun 19, 2018 •

edited

Loading

Hyungsin commented Jun 19, 2018 •

edited

Loading

Hyungsin commented Jun 20, 2018 •

edited

Loading