saul/gpio: Low footprint handling of active-low signals + state initialization by cladmi · Pull Request #7586 · RIOT-OS/RIOT

cladmi · 2017-09-08T14:16:49Z

This is a low footprint implementation for SAUL active-low and state init.

The basic idea, is to pass saul_gpio_params_t structure to read/write instead of only pin (which already saves code), and add a single flags entry in it.
This removes the need to duplicate drivers and only adds one enum entry.

I changed the samr21-xpro to invert led and button.

My footprint tests where done with examples/saul the samr21-xpro

Steps	`samr21-xpro`	`iotlab-m3`
Original	17364	21352
Change dev	17340	21332
Inverted support	17360	21352
Init support	17384	21380

All steps are done in different commit so I could only put some part of it.

Size testing procedure

export RIOT_VERSION='' # prevents the version to change size
make BOARD=samr21-xpro | grep 'examples/saul' | cut -f 4
make BOARD=iotlab-m3 | grep 'examples/saul' | cut -f 4

I did not put initial state setting in the output only mode for readability, but I could (it is harmless by default). Or put a DEVELHELP message when using it for an input.

This could replace #7151, #7148

cladmi · 2017-09-08T17:15:02Z

Setting dev in saul_reg_t to a pointer to const should not be a problem because both saul_read_t and saul_write_t don't modify dev: const void *dev, and so subfunctions should not modify it too.

With dev being set to a pointer to const, saul_adcs pointer array can also be removed to save space.

kaspar030 · 2017-09-15T11:50:24Z

boards/arduino-atmega-common/include/gpio_params.h

        .pin = LED0_PIN,
-        .mode = GPIO_OUT
+        .mode = GPIO_OUT,
+        .flags = 0,


(here and below) when using designated initializers, empty fields don't have to be specified. So IMO the .flags = 0 should go.

That what I was thinking too, but when talking with @haukepetersen he thought they may be required for some architecture.
I will remove them all.

kaspar030 · 2017-09-15T11:52:46Z

boards/arduino-atmega-common/include/gpio_params.h

        .name = "LED",
        .pin = LED0_PIN,
-        .mode = GPIO_OUT
+        .mode = GPIO_OUT,


Does this mode field mean that when using saul, a GPIO pin config (in/out, pullups) ist hard-coded at compile time?

I think so, then at runtime in auto_init_gpio.c the mode field is used to select in or out saul driver and is given to gpio_init.

smlng · 2017-09-18T09:37:06Z

drivers/saul/gpio_saul.c

-    gpio_t pin = *((const gpio_t *)dev);
-    res->val[0] = (gpio_read(pin)) ? 1 : 0;
+    const saul_gpio_params_t *p = (const saul_gpio_params_t *)dev;
+    int inverted = (p->flags | SAUL_GPIO_INVERTED);


should be int inverted = (p->flags & SAUL_GPIO_INVERTED);, or not?

I do this mistake all the time.

smlng · 2017-09-18T09:37:31Z

drivers/saul/gpio_saul.c

-    gpio_t pin = *((const gpio_t *)dev);
-    gpio_write(pin, state->val[0]);
+    const saul_gpio_params_t *p = (const saul_gpio_params_t *)dev;
+    int inverted = (p->flags | SAUL_GPIO_INVERTED);


dito, as above should be &?

smlng · 2017-09-18T09:39:02Z

drivers/saul/gpio_saul.c

+    const saul_gpio_params_t *p = (const saul_gpio_params_t *)dev;
+    int inverted = (p->flags | SAUL_GPIO_INVERTED);
+
+    res->val[0] = (gpio_read(p->pin)) ? !inverted : inverted;


I think you could use XOR here and below, i.e.,

res->val[0] = gpio_read(p->pin) ^ inverted;

I am doing an XOR, but not a bitwise xor.

I just checked and no it cannot be replaced, gpio_read is only expected to return a positive value on HIGH. https://github.com/RIOT-OS/RIOT/blob/2017.07/drivers/include/periph/gpio.h#L189

When looking at actual implementation, the first I found in alpha order is https://github.com/RIOT-OS/RIOT/blob/2017.07/cpu/atmega_common/periph/gpio.c#L209 which in fact does not return 1.

The previous code was also doing this (gpio_read(pin) ? 1 : 0)
The thing I have here, is that I expect SAUL_GPIO_INVERTED to be equal to 1. Maybe it would require a static assert for this.

smlng · 2017-09-18T09:40:43Z

@haukepetersen I thought there already was some similar attempt (by you) in the past to tackle this invert stuff, so could be a duplicate here?

aabadie · 2017-10-01T08:14:05Z

so could be a duplicate here?

As stated in the initial comment "This could replace #7151, #7148 ", so I don't think it's a duplicate but rather a better proposal. The #7151 version increases the firmware size by 100 bytes, here it's only 20 with other improvements included: gpio being cleared at auto_init.

I vote for this one.

aabadie

A few minor comments. Otherwise the changes here do what they are supposed to correctly (tested on samr21-xpro and nucleo-l432).

aabadie · 2017-10-01T08:36:20Z

drivers/include/saul/periph.h

 #ifdef MODULE_SAUL_GPIO
+typedef enum {
+    SAUL_GPIO_INVERTED = 1 << 0,  /**< pin is used as inverted */
+    SAUL_GPIO_CLEAR    = 1 << 1,  /**< set pin inactive after initialization */


Maybe rename to SAUL_GPIO_INIT_CLEAR to be more explicit. Same with SAUL_GPIO_INIT_SET below

aabadie · 2017-10-01T08:38:50Z

boards/samr21-xpro/include/gpio_params.h

        .pin = LED0_PIN,
-        .mode = GPIO_OUT
+        .mode = GPIO_OUT,
+        .flags = SAUL_GPIO_INVERTED,


IIUC the logic, you should add SAUL_GPIO_CLEAR (or SAUL_GPIO_INIT_CLEAR) to ensure the pin initialized cleared ? Otherwise it's set after initialization (default GPIO behavior on this board).

I think changing the initial value belongs to a different commit. So I prefer not include it in this PR.

cladmi · 2017-10-02T09:50:05Z

I can split this PR in three if some points need to be discussed specifically. Because there really are 3 parts to it:

changing to const void *dev in saul_reg_t and using it to reduce code size
Adding inverted mode support and using it for samr21
Adding state initialization support

I will wait for @haukepetersen before changing.

haukepetersen

Two minor comments, else this looks good to me.

haukepetersen · 2017-10-04T09:32:00Z

drivers/include/saul/periph.h

-    const char *name;       /**< name of the device connected to this pin */
-    gpio_t pin;             /**< GPIO pin to initialize and expose */
-    gpio_mode_t mode;       /**< pin mode to use */
+    const char *name;        /**< name of the device connected to this pin */


here and below: please align comment to 4 spaces

haukepetersen · 2017-10-04T09:32:49Z

drivers/include/saul/periph.h

+typedef enum {
+    SAUL_GPIO_INVERTED   = 1 << 0, /**< pin is used as inverted */
+    SAUL_GPIO_INIT_CLEAR = 1 << 1, /**< set pin inactive after initialization */
+    SAUL_GPIO_INIT_SET   = 1 << 2, /**< set pin active after initialization */


here and in other places of this PR: I would tend to remove the comma after the last element

Forget about it. As we don't use C89 there is no reason, so keep it as is.

haukepetersen

ACK, please squash

This provides the whole structure to read and write.

This makes LED go off when set to 0 and button reads 1 when pressed.

cladmi · 2017-10-05T15:25:17Z

Rebased and squashed

Was addressed

cladmi force-pushed the pr/saul/gpio branch from 9014689 to fa7d0a7 Compare September 8, 2017 17:20

smlng requested a review from haukepetersen September 11, 2017 08:55

smlng added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Sep 11, 2017

smlng requested review from aabadie and smlng September 11, 2017 08:55

kaspar030 previously requested changes Sep 15, 2017

View reviewed changes

cladmi force-pushed the pr/saul/gpio branch from fa7d0a7 to 15fd49d Compare September 15, 2017 13:36

smlng previously requested changes Sep 18, 2017

View reviewed changes

cladmi force-pushed the pr/saul/gpio branch from 15fd49d to f0cd167 Compare September 19, 2017 09:43

miri64 assigned kaspar030 and smlng Sep 19, 2017

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 1, 2017

aabadie previously requested changes Oct 1, 2017

View reviewed changes

haukepetersen requested changes Oct 4, 2017

View reviewed changes

This was referenced Oct 4, 2017

saul/gpio: add handling of active-low signals + state initialization #7151

Closed

saul: add PWM config for use in SAUL #7587

Closed

haukepetersen approved these changes Oct 5, 2017

View reviewed changes

haukepetersen added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 5, 2017

cladmi added 5 commits October 5, 2017 17:18

saul_reg: define saul_reg_t dev field as a pointer to const

d5ec78f

saul/gpio: use saul_gpio_params_t structure as saul device

be3029d

This provides the whole structure to read and write.

saul/gpio: add support for inverted mode gpios

c75470e

samr21-xpro/gpio: set LED/BUTTON gpios as inverted

05402de

This makes LED go off when set to 0 and button reads 1 when pressed.

saul/gpio: add support to initialized state

c39f4d8

cladmi force-pushed the pr/saul/gpio branch from 39bd47a to c39f4d8 Compare October 5, 2017 15:24

smlng mentioned this pull request Oct 6, 2017

saul: explicitly clear GPIO output pins on auto_init #7148

Closed

miri64 merged commit 72edaa3 into RIOT-OS:master Oct 10, 2017

haukepetersen mentioned this pull request Oct 11, 2017

drivers: remove accidentally added binary Makefile #7718

Merged

vincent-d mentioned this pull request Oct 11, 2017

saul_reg: const pointer for device? #7719

Closed

This was referenced Oct 12, 2017

saul_reg: make ptr to device descriptor non const #7723

Merged

boards/sodaq-explorer: add initial support #7725

Merged

cladmi deleted the pr/saul/gpio branch October 24, 2017 15:12

cladmi added this to the Release 2017.10 milestone Nov 7, 2018

Conversation

cladmi commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size testing procedure

Uh oh!

cladmi commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

smlng commented Sep 18, 2017

Uh oh!

aabadie commented Oct 1, 2017

Uh oh!

aabadie 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!

cladmi commented Oct 2, 2017

Uh oh!

haukepetersen 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!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cladmi commented Sep 8, 2017 •

edited

Loading

cladmi commented Sep 8, 2017 •

edited

Loading