periph/gpio: add gpio_set_cb() function

[benpicco]

Contribution description

It can be desirable to change the function or behavior associated with a GPIO interrupt.
Currently this can only be achieved by calling gpio_init_int() again, which not only does lots of unnecessary and possibly time-consuming work, but also forces to expose the GPIO configuration to callers that only want to change the callback argument.

Error handling is done (if at all) following the style of the respective GPIO driver. That is, if the rest of the driver did not check if pin is valid, I didn't add such checks for gpio_set_cb(). If it did contain such checks, I copied them.

As this function only makes sense to call after gpio_init_int() had been called on pin, it could be argued to drop the checks entirely.

Testing procedure

The following program should alternate between the two messages with each press of a button.

#include <stdio.h>
#include "periph/gpio.h"
#include "board.h"

static char* messages[] = {
    "Hello IRQ!",
    "Goodby IRQ!"
};

static void _irq_handler(void *arg)
{
    puts(arg);
    gpio_set_cb(BTN0_PIN, NULL, arg == messages[0] ? messages[1] : messages[0]);
}

int main(void)
{
    gpio_init_int(BTN0_PIN, GPIO_IN, GPIO_RISING, _irq_handler, messages[0]);
    return 0;
}

(Only tested on samr21-xpro)

Issues/PRs references

none

(this will be used by the at86rf215 driver, a device which contains two basebands but only exposes one GPIO pin. As an optimization, the context points to the netdev that last called send().)

[maribu]

I like the general idea, but two general things:

1. If the argument of the callback is NULL, the current implementation keeps the argument of the previous caller. I think the argument should be set to whatever is given in gpio_set_cb(), otherwise it would not be possible to willingly set this to NULL after the some non-NULL argument has been set.
2. The new function could be called internally in gpio_init_int() to make sure the code is tested much. (I expect the compiler to inline this call, so that this would have no effect on the ROM size. But if I'm wrong with that, I'm fine with not calling it from gpio_init_int().

I'll take a more in depth look tomorrow, I'm too tired to concentrate now.

[benpicco]

As to 1., I think I should name it gpio_update_cb() instead.
If we set the callback function to NULL, we would also have to disable the interrupt - and there is already a function for that.

The idea here is that the callback function might be static and in another compilation unit, but one still wants to update the callback argument.

As to 2., I can do it, but it seems a little overkill as this only provides a setter for two internal variables.

[kaspar030]

please keep race conditions in mind.

...
config[int_num].cb = cb;
// GPIO TRIGGERS, new cb gets executed with old arg
config[int_num].arg = arg;
...

Guard with irq_disable/restore, or add a note that this should be done externally?

[maribu]

Guard with irq_disable/restore

+1

or add a note that this should be done externally?

I strongly disagree. Keep in mind that people like me want to use the API. And this is how people like me are trying to drink water out of glass:

With this image in mind: Externally disabling interrupts is certainly to much to ask for ;-) (Also keep in mind that race conditions never trigger during development and testing. They wait until someone depends on it to show up.)

[benpicco]

@kaspar030 good point!
Since I'm in the GPIO driver anyway, I can just disable the GPIO interrupt there. (And not enable it again if cb == NULL and we go with the GPIO_UPDATE_CB_NOOP route).

[benpicco]

How about so?
I've only updated two implementations, I want to see if the API makes sense before converting the rest.

[benpicco]

I can move this to a separate file not to clutter the gpio.h API.

[maribu]

One ATmega specific detail: The ATmegas only have individual interrupt vectors for about 3 pins, but shared interrupt vectors for a group of 8 pins (called pin changed interrupts). On top of the pin changed interrupts a RIOT compatible gpio_init_int() is implemented, when pseudomodule is used. _int_num() will however only work for "proper" interrupts, not for pin changed interrupts.

(But I'd say keep it like this for now. I'd even say that it is fine to merge the PR without support for this special case in order to not get lost in the implementation details of a dying platform. This can be easily added later on.)

[benpicco]

Atmega is the C64 of the MCU world, I think it will be with us for a bit.
I can just move the code from gpio_init_int() into a helper function or call the gpio_update_cb() function from there.

[maribu]

Atmega is the C64 of the MCU world

😆 👍

And there is actually development on that platform. (Not from Microchip, but there is an improved clone of the ATmega328P - seemingly with a proper on chip debug interface (instead of debugWIRE)

[maribu]

I like the new API with separate function calls for the callback function and the argument. Keep in mind that updating the argument will also need to be done in an atomic fashion, e.g. via disabling IRQ temporary.

I think that a third function to exchange both atomically would however be needed to update both. Otherwise the interrupt might be triggered right between the calls, so a handler is called with an argument it doesn't expect.

(I know, the API is pretty verbose with 3 flavors. But that will be really trivial to use :-))

[maribu]

So, summing up the API I would like to see would look somewhat like this:

int gpio_update_int(gpio_t pin, gpio_cb_t cb, void *arg);
int gpio_update_cb(gpio_t pin, gpio_cb_t cb);
int gpio_update_arg(gpio_t pin, void *arg);

(I changed the naming a bit as I personally prefer update, as to me that name indicates that the interrupt has to be already set up prior to calling the gpio_update_<foo>() functions. But I can also live very fine with the current names.)

But maybe it makes sense (in terms of ROM and/or code size) to have all three of them a static inline functions that internally calls a single function handling all three cases? Maybe the internal function has an additional flag argument to indicate if the callback, the callback-argument or both should be updated? Or gpio_update_init() is the actual implementation and gpio_update_arg() calls gpio_update_init() with cb = NULL and gpio_update_cb() with arg =(void *)-1 (and, thus, throwing everyone under the bus who wants to set the argument to (void *)-1)?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[benpicco]

So, summing up the API I would like to see would look somewhat like this:

int gpio_update_int(gpio_t pin, gpio_cb_t cb, void *arg);
int gpio_update_cb(gpio_t pin, gpio_cb_t cb);
int gpio_update_arg(gpio_t pin, void *arg);

Just wondering, do we really need to return error here?
If pin is not a valid interrupt pin, gpio_init_int() would have failed already.

This is just updating a location in memory, so technically it could never fail.

[maribu]

If pin is not a valid interrupt pin, gpio_init_int() would have failed already.

If instead an assert() is added to check that again, I would be fine with dropping the return value. (User code could set up the interrupt for one pin, and update it for a different one. I have seen bigger mistakes in code, so I would like to have at least the assert in place to help debugging.)

[maribu]

But I wonder if we should first focus on the common GPIO API (for both periph and external GPIOs). This PR needs a rebase anyway, so it would be little effort to do it on top of the new GPIO API when it is finally in.

[miri64]

@benpicco is this PR still on the table?

[benpicco]

@miri64 no, #13925 supersedes it.