gpio.h/cb_mux API change and interim compatibilty

[ZetaR60]

This PR adds support for cb_mux in periph/gpio.h to be used for managing interrupts.

Specific changes:

• gpio_init_int now has an additional argument: a pointer to the passed cb_mux entry
• All CPUs currently ignore entry using (void)entry;
• If MODULE_CB_MUX is enabled and gpio_init_int is called, that file will also declare a global cb_mux entry and pass it to gpio_init_int.
• If MODULE_CB_MUX is disabled and gpio_init_int is called, a NULL pointer is passed to it.

This set up permits CPUs to set up cb_mux on a case by case basis, and not have any changes to the behavior of anything that does not implement it.

Depends on: #8993

[ZetaR60]

The dependency, #8993, has now been merged.

This PR is required for @roberthartung and @TorbenPetersen to continue their work on a rewrite of #7610

[TorbenPetersen]

Hi @ZetaR60 I have implemented a first version of #7610 however I receive an error:

/misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/drivers/at86rf2xx/at86rf2xx_netdev.c:66:19: error: passing argument 1 of ‘gpio_init_int’ discards ‘volatile’ qualifier from pointer target type [-Werror]
 #define INT_ENTRY (&int_entry)
                   ^
/misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/drivers/at86rf2xx/at86rf2xx_netdev.c:90:19: note: in expansion of macro ‘INT_ENTRY’
     gpio_init_int(INT_ENTRY, dev->params.int_pin, GPIO_IN,
                   ^
In file included from /misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/boards/inga_red/include/board.h:14:0,
                 from /misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/drivers/include/at86rf2xx.h:35,
                 from /misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/drivers/at86rf2xx/at86rf2xx_netdev.c:37:
/misc/ibr/home/petersen/RIOT/PinChangeInterruptBranch/RIOT/drivers/include/periph/gpio.h:175:5: note: expected ‘struct gpio_int_t *’ but argument is of type ‘volatile struct gpio_int_t *’
 int gpio_init_int(gpio_int_t *entry, gpio_t pin, gpio_mode_t mode,
     ^

Additionally why is this PR not based on the code you used in #8951?

[ZetaR60]

Rebased on trunk, and removed use of volatile (it's probably not necessary). I also made the definitions of INT_ENTRY agnostic to the presence of cb_mux.

@TorbenPetersen I did not base the code on #8951 because this PR is not dependent on the refactor, and I anticipated that I would need to resolve conflicts anyway (there was 1 other conflict).

[TorbenPetersen]

Can we get this merged? Once this is merged, we can create a replacement PR for #7610 that implements pin change interrupts with cb_mux.

[ZetaR60]

@TorbenPetersen Can you post some test results here using the new API with your PCINT code? The interrupt commands in tests/periph_gpio would work well for this.

[TorbenPetersen]

@ZetaR60 requested that I post some test results of my PCINT code.
I used an INGA Board (see #7604 for more), which uses an atmega1284p. I activated the PCINT for PB2, which is connected to a button. Unfortunately the INGA has only one button. Here is my test:

2018-06-11 15:44:17,291 - INFO # main(): This is RIOT! (Version: 2018.07-devel-494-g015b4-pc13-tp/inga_pcint_testbranch)
2018-06-11 15:44:17,307 - INFO # GPIO peripheral driver test
2018-06-11 15:44:17,307 - INFO #
2018-06-11 15:44:17,339 - INFO # In this test, pins are specified by integer port and pin numbers.
2018-06-11 15:44:17,371 - INFO # So if your platform has a pin PA01, it will be port=0 and pin=1,
2018-06-11 15:44:17,403 - INFO # PC14 would be port=2 and pin=14 etc.
2018-06-11 15:44:17,403 - INFO #
2018-06-11 15:44:17,435 - INFO # NOTE: make sure the values you use exist on your platform! The
2018-06-11 15:44:17,467 - INFO # behavior for not existing ports/pins is not defined!`
help
2018-06-11 15:44:28,494 - INFO # help
2018-06-11 15:44:28,510 - INFO # Command Description
2018-06-11 15:44:28,526 - INFO # ---------------------------------------
2018-06-11 15:44:28,558 - INFO # init_out init as output (push-pull mode)
2018-06-11 15:44:28,574 - INFO # init_in init as input w/o pull resistor
2018-06-11 15:44:28,606 - INFO # init_in_pu init as input with pull-up
2018-06-11 15:44:28,638 - INFO # init_in_pd init as input with pull-down
2018-06-11 15:44:28,670 - INFO # init_od init as output (open-drain without pull resistor)
2018-06-11 15:44:28,702 - INFO # init_od_pu init as output (open-drain with pull-up)
2018-06-11 15:44:28,734 - INFO # init_int init as external INT w/o pull resistor
2018-06-11 15:44:28,750 - INFO # read read pin status
2018-06-11 15:44:28,766 - INFO # set set pin to HIGH
2018-06-11 15:44:28,782 - INFO # clear set pin to LOW
2018-06-11 15:44:28,798 - INFO # toggle toggle pin
2018-06-11 15:44:28,830 - INFO # bench run a set of predefined benchmarks
init_int
2018-06-11 15:44:36,053 - INFO # init_int
2018-06-11 15:44:36,069 - INFO # usage: init_int [pull_config]
2018-06-11 15:44:36,085 - INFO # flank:
2018-06-11 15:44:36,085 - INFO # 0: falling
2018-06-11 15:44:36,085 - INFO # 1: rising
2018-06-11 15:44:36,101 - INFO # 2: both
2018-06-11 15:44:36,101 - INFO # pull_config:
2018-06-11 15:44:36,117 - INFO # 0: no pull resistor (default)
2018-06-11 15:44:36,133 - INFO # 1: pull up
2018-06-11 15:44:36,133 - INFO # 2: pull down
init_int 1 2 1
2018-06-11 15:44:45,274 - INFO # init_int 1 2 1
2018-06-11 15:44:45,291 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:45,306 - INFO # GPIO_PIN(1, 2) successfully initialized as ext int
2018-06-11 15:44:49,158 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:51,172 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:51,204 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:51,683 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,003 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,306 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,482 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,498 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,546 - INFO # INT: external interrupt from pin 2
2018-06-11 15:44:52,770 - INFO # INT: external interrupt from pin 2 <

As you can see, the Interrupt is working. Is this enough @ZetaR60?

[ZetaR60]

@TorbenPetersen Thank you, that is excellent. I didn't have a way of testing this and I didn't want another bug like the issue you had with volatile sneaking into the master branch.

[TorbenPetersen]

Hi, when can we expect his to be merged? We need this Branch for the implementation of PCINT. As far as I know, there is no other way to handle the callbacks for the PCINTs.

[ZetaR60]

Rebased on trunk and resolved conflicts.

[kYc0o]

Unfortunately it needs another rebase... We should try to get this in so it doesn't rebase every time a file is touched.

[ZetaR60]

@kaspar030 I find this conversation a bit frustrating, because we discussed different possibilities in #8993. I had initially discarded the idea of an API change in favor of arrays, except that you suggested that an API change would be preferable:

Maybe if we'd introduce a gpio_int_t type, which any user of a gpio interrupt needs to provide? That struct could contain necessary per-platform fields. It would be an API change, but we could move the allocation to wherever the interrupt is used.

What exactly were you suggesting, if not what was in this PR? I tried to stick closely your suggestion to ensure that this PR would be approved.

[ZetaR60]

@kaspar030 Just to point out a couple of counterpoints to your objections:

as is, this API change would require every user of the API to always pass memory for the entry parameter, even if the platform is not using it. that is a no-go.

Platforms that do not use this feature just pass a null pointer, not actual memory.

while this might optimize memory usage, it also increases interrupt latency. up to now, we did pay the price for minimal interrupt latency (with increase memory usage). we won't change that (without a thorough discussion)

I wrote a benchmark test in #8993 to address this specific issue. It was found that the traversal time of the singly linked list in cb_mux is approximately 4us per entry on an ATmega1284P at 8MHz (mega-xplained). This CPU is actually one of the worst cases for this as well: it is 8 bit, so it must use multiple operations to do >8bit comparisons; it is also one of the slowest clocks of the various boards that RIOT supports, with many systems >5x the clock rate, and a few at 10x or more; it is also an old architecture, which may use its clock cycles less efficiently than newer ones. A typical system using only a few interrupts would use a couple microseconds or less to retrieve the information it needs. I don't think it is typical to require an interrupt response of less than a couple microseconds.

I have also been considering a simpler way of allocating the memory. A simple macro would be capable of both allocating the memory and returning a pointer to it, so it could just be put into the place of the entry pointer argument in gpio_init_int. Or it would return NULL, if the memory is not required. This would alleviate concerns about users of gpio_init_int having to deal with adding the code to decide whether to allocate the memory. Currently the code to decide looks like this (a bit longer):

/* Interrupt struct for gpio_init_int */
#if GPIO_USE_INT_ENTRY
static gpio_int_t int_entry;
#define INT_ENTRY (&int_entry)
#else
#define INT_ENTRY (NULL)
#endif

[kaspar030]

I find this conversation a bit frustrating, because we discussed different possibilities in #8993. I had initially discarded the idea of an API change in favor of arrays, except that you suggested that an API change would be preferable:

Yes, and I'm sorry. It seemed like a good idea at the time.
Now that I see it implemented, it's drawbacks come to light.

Is there an issue describing the actual issue we're trying to solve? I think we need to come up with a more solid idea of what we're trying to solve and what are the implications of possible solutions.

I'm still not unconvinced that passing memory for callbacks could not be a viable solution. But as is, that memory is wasted on platforms which do not use it (for portable code), and that is a no-go.
The actual benefits of that passed memory only come into play when linked lists are used for all gpio interrupt lines, which is a design decision (with implications on interrupt latency and determinism) which I'm not sure we should bake into the interface.

[kaspar030]

It was found that the traversal time of the singly linked list in cb_mux is approximately 4us per entry on an ATmega1284P at 8MHz (mega-xplained). This CPU is actually one of the worst cases for this as well

One problem is that the induced delay (of using a linked list) is also indeterministic. That might be acceptable in many applications, but should not be forced by the generic interface.

[ZetaR60]

I'm still not unconvinced that passing memory for callbacks could not be a viable solution. But as is, that memory is wasted on platforms which do not use it (for portable code), and that is a no-go.

There is no memory allocated on systems that do not use it. In order for memory to be allocated, a macro GPIO_USE_INT_ENTRY must be defined (which would be done in periph_cpu.h). Otherwise, no variables are declared and only NULL is passed as the argument to gpio_init_int instead of a pointer to the memory. I suppose the comments in the code about utilizing the API change may be confusing here; I will remove them.

Is there an issue describing the actual issue we're trying to solve? I think we need to come up with a more solid idea of what we're trying to solve and what are the implications of possible solutions.

The problem is to design a method of allocating memory for interrupt information that has the following qualities:

1. Uses a small amount of memory when only a small number of pin interrupts are used
2. Permits the use of many pin interrupts
3. Is determined automatically, without input from the user

Some possible solutions:

• Hard coded memory allocations (the current solution). Fails either 1 or 2 depending on how many you allocate.
• Compile time determination using a macro to define the size of an array. Fails 3, especially when the application needs pin interrupts too.
• Pass memory with the API call (the solution in this PR).

One problem is that the induced delay (of using a linked list) is also indeterministic. That might be acceptable in many applications, but should not be forced by the generic interface.

An indeterministic delay, when sufficiently small, is in practice equivalent to a deterministic delay of zero. There are various delays of this sort already. Also, a change in various conditions during compilation could introduce a variance in the deterministic delay (e.g. compiler changes, or just fixes to the code). In order for code to be stable it must handle this variance to the deterministic delay, which would also lead to resilience against small indeterministic delays.

The actual benefits of that passed memory only come into play when linked lists are used for all gpio interrupt lines, which is a design decision (with implications on interrupt latency and determinism) which I'm not sure we should bake into the interface.

Is this the primary issue with the PR after the clarification on the memory issue? Less than a couple microseconds is insufficiently small? We could benchmark some other systems using tests/cb_mux_bench, if you prefer.

[ZetaR60]

@kaspar030 Could you elaborate on the justification for your objections? I am assuming that your point about always passing memory has been resolved by my earlier clarification, which leaves the point about interrupt delays and indeterminism. Is this the sole remaining objection? Do you have a particular example or use-case that requires pin interrupt delays to be less than a couple microseconds, or requires said delays to vary less than a couple microseconds?

I apologize if I seem to be argumentative or pushy on this issue, especially since I know that you are pretty busy and this takes you away from other work. I would not be objecting so strongly if there was not already a significant amount of labor associated with this PR, and that several problems that have simple solutions with this PR will become awkward and difficult again.

[ZetaR60]

ping @kaspar030 I was hoping to finish our discussion in time for the deadline on the 16th.

[kaspar030]

I was hoping to finish our discussion in time for the deadline on the 16th.

Sorry for the delay.

I have also been considering a simpler way of allocating the memory. A simple macro would be capable of both allocating the memory and returning a pointer to it, so it could just be put into the place of the entry pointer argument in gpio_init_int.

In RIOT, we try hard to avoid macros for many reasons. You will not find many throughout the codebase. Sometimes there are exceptions, but those are usually well argued for and hidden deep in implementations. The extra parameter would require every user of the API to use a macro (in order to avoid wasting memory in implementations that don't need it).

Do you have a particular example or use-case that requires pin interrupt delays to be less than a couple microseconds, or requires said delays to vary less than a couple microseconds?

Random google hit on microsecond interrupt latency:

https://www.electronicproducts.com/Software/System/Microseconds_matter_reducing_interrupt_latency_in_industrial_control_systems.aspx

The gist of it is that in real time control applications, microseconds do matter.
"a couple of microseconds" is ages for an MCU.

Please also consider the concept behind periph/gpio (and all the other periph interfaces). They try to be as slim as possible. In this case that means, if the hardware can do an interrupt latency (from pin event to execution of ISR code) of N cycles, periph/gpio tries hard to add only a minimal amount to that. In theory, with shared EXTI lines, that should not be more than a "find first set" operation on a register, a memory read for getting callback and arg and the function call overhead. Having to traverse a linked list would multiply that latency.

Also, even if the actual gpio application might not need a very low delay, something else in the system might need the cycles. Remember, the MCU is blocked during that time.

It would be OK for e.g., the AVR implementation to use linked lists because the platform has tighter memory constraints and therefore the tradeoffs might be chosen differently. But periph/gpio is supposed to be a generic interface, thus we don't want to introduce the overhead at this level, if we don't have to.

[TorbenPetersen]

Hi,
I have been gone for a month now (Exams) and I have a question: What is the current status on PCINTs? As far as I know, we started the implementation of this approach on PCINTs because predefined arrays seemed not like the best solution. Should we go back to the implementation using arrays? Is there already a new PR for this?
Thanks in advance for any new information regarding the implementation of PCINTs.

[ZetaR60]

@TorbenPetersen I have been considering how best to proceed.

I think there are some unsolved problems with using arrays with an adjustable array size. Right now, a parameter like this would be set in the board files. This may be reasonable for many uses (specifically handling interrupts generated on the board), but some applications may require additional interrupts coming from off-board. I don't think there is currently any good way of setting a default in the board files and then overriding it in the application files. Also, even with a solution, it is much more desirable to configure it automatically.

A possible alternative to arrays is to implement sbrk(size) for the ATmega family so that oneway-malloc can be used with cb_mux. While malloc in general is undesirable, oneway-malloc does not actually require management of dynamic memory like normal malloc because it does away with freeing memory. I think sbrk just needs to keep track of a pointer to the memory pool and increment it by size when it is called, but I am unsure of possible pitfalls in writing an implementation.

Another possibility is to implement tiers of interrupts, where there is an arbitrary number of low-speed interrupts using cb_mux with passed memory, and a few high-speed interrupts using directly accessed callback information (like the current interrupt implementations). This would allow for mitigation of the latency issue we have been discussing. Having two systems in parallel would add complexity though.

Sorry if this isn't a great answer for picking a specific path forward. I know there desire for an implementation soon, but I have been having trouble coming up with a good solution that is both widely practical and fulfills the stringent requirements that we have been discussing.

[TorbenPetersen]

The be completely honest: your opinion on this would be really much appreciated @kaspar030 . It would be rather painfully to go through all of this again (find another solution, write code, scrap it at the end).

I don't think there is currently any good way of setting a default in the board files and then overriding it in the application files. Also, even with a solution, it is much more desirable to configure it automatically.

It may be desirable to configure it automatically. But I don't think this needs to be there from the start. Cant we just find a way to implement this first, so we can have a working way for PCINTs in RIOT? Such features like automatic configuration can be added later.

Another possibility is to implement tiers of interrupts, where there is an arbitrary number of low-speed interrupts using cb_mux with passed memory, and a few high-speed interrupts using directly accessed callback information (like the current interrupt implementations). This would allow for mitigation of the latency issue we have been discussing. Having two systems in parallel would add complexity though.

I agree. This approach seems memory consuming too.

I was not able to come up with any other solution either. FYI: if we want to implement the PCINTs using arrays, we could use this PR: #7610

[TorbenPetersen]

ping @ALL ?

[ZetaR60]

@kaspar030 Would a sbrk() implementation (for oneway-malloc) be acceptable for allocating memory for interrupts?

[TorbenPetersen]

Whats the advantage of a one-way malloc over the LL?

[ZetaR60]

@TorbenPetersen Oneway-malloc is for allocating memory for the linked list. With typical application programming, you would just use malloc() when you needed memory for something like this. On MCUs the memory management can cause problems for various reasons. Oneway-malloc is like normal malloc(), but doesn't do memory management, which means you can't free memory but it avoids the problems. sbrk() is a system call that normally gets more memory for malloc(), but I think in this case it just keeps track of a heap pointer.

[roberthartung]

I see, but using a LL still has some overhead in performance which is what initially caused this to not be merged?

[ZetaR60]

@roberthartung See @kaspar030 's comment above:

It would be OK for e.g., the AVR implementation to use linked lists because the platform has tighter memory constraints and therefore the tradeoffs might be chosen differently.

The issue was changing the API to support the memory allocation for the linked lists, not its use on AVR.

[ZetaR60]

@kaspar030 Could you give us some feedback on the sbrk() / oneway-malloc question?

[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.