[WIP] cpu/atmega_common: pin change interrupt implementation

[roberthartung]

Contribution description

This is a new proposal of a low-memory footprint implementation for pin change interrupts on atmega platforms. First, a lookup-table is used to indicate if a PCINT is configured for a port (pcint_lookup).

Each board then defines a list of GPIOs via AVR_PCINT_GPIO_LIST that should be used as pin change interrupts. These are put into a map pcint_mapping (i -> gpio_t). When initializing a port pin, and it is not an external interrupt, it is checked if the GPIO was enabled before via AVR_PCINT_GPIO_LIST. Afterwards, the interrupt is enabled on that port and the state and config are saved.
Using a pre-defined list of available pin change interrupts for a board, the memory overhead is kept low.

Issues/PRs references

See also #7610, #8993, #9159

[maribu]

OK, I think using the "any pin on port" changed interrupt to transparently provide interrupts on pins that do not support a direct implementation is the portable way to do things. Being aware that this change has been somewhat controversial before, I would to have more than one ACK.

The main reason this change has been controversial (judging after a brief look into the previous discussions), is the requirements of ROM/RAM, which is a particularly scare resource on AVR boards. How about making this a pseudo module, so that each developer can decide if she/he does not more interrupt pins or not? In the end, this is a trade off (feature vs code size) that cannot be decided correctly by us for every use case.

Btw: Without having a deep understanding of the gory details: Has the issue with the port change interrupt being used for context switches been solved in the meantime?

[roberthartung]

@maribu Well, it's a regular feature of MCUs which simply does not exists in RIOT at the moment, and it should! My solution is the third one, after old solutions weren't good enough. Additionally, this solution comes with a very small overhead and it doesn't do anything, if not explicitely chosen (you have to define the PCINTs before anyway!).

A pseudo module sounds reasonable as this point, as this would be the same as we are already doing: if the define is set, use the code.

We are back to non-interrupt based context switches since #8904

[maribu]

Is this still required, when context swaps no longer are interrupt based?

[maribu]

From the end user point of view I think a pseudo module would be more convenient to use, as a single line of code added to the Makefile would unlock pin changed interrupts. (I don't have strong feelings if this should be more fine grained, like one pseudo module per port, or one for all ports.)

Also: The pseudo modules will make it easy for the network driver of your board. You can just check in Makefile.dep if the network driver is enabled, and if so, also enable pin changed interrupt support.

[roberthartung]

@maribu I like the idea in general, but a coarse-grain pseudo module would then again give a big memory overhead, which is why the first implementation was not merged in the first place. So in conclusion, a pseudo module for both each port and all ports would be most user-friendly?

[maribu]

From my point of view: Yes :-)

[roberthartung]

@maribu But then this would only be a convenient function? Because when we have a module per port, all pins would be enabled, which could result in potential conflicts?

[roberthartung]

Hmm .. just discovered another problem with the mapping / lookup table. Looking into that first now.

[maribu]

Because when we have a module per port, all pins would be enabled, which could result in potential conflicts?

Maybe I'm missing some detail, but I do not see any conflicts. Some "uglyness" will be required to get the offset (port_num) of the port in the pcint_lookup (the first argument to pcint_handler). E.g. if only for port 3 the port changed interrupts are enabled, its offset will be 0, while it would be 1 if for port 1 and 3 interrupts are enabled.

I'm thinking about something like:

#ifdef USEMODULE_AVR_PCINT_PORT0
#define PORT0_IDX (0)
#endif
#define _COUNTER0  (0)
#endif

#ifdef USEMODULE_AVR_PCINT_PORT1
#define _COUNTER1 (_COUNTER0 + 1)
#define PORT1_IDX _COUNTER1
#else
#define _COUNTER1 _COUNTER0
#endif

#ifdef USEMODULE_AVR_PCINT_PORT2
#define _COUNTER2 (_COUNTER1 + 1)
#define PORT1_IDX _COUNTER2
#else
#define _COUNTER2 _COUNTER1
#endif

#ifdef USEMODULE_AVR_PCINT_PORT3
#define PORT1_IDX (_COUNTER2 + 1)
#endif

And then you will be able to use PORT<X>_IDX for the index for each port.

[roberthartung]

@maribu I guess you are missing the important detail ;) For example the atmega2560 has PCINT0, PCINT1, PCINT2, which I will refer to as three "banks" of pin change interrupts. My assumption was, that each bank corresponds to one ports. But surprise: NOPE! The assignment of individual pin change interrupts is arbitrary. Therefore we don't have ports, but banks of PCINTs, and we will have a submodule for each bank now.
Additionally, this will introduce a small overhead as we need to map the pin change interrupts (0-23 in the atmega2560 case) to actual ports. This is required as the ISR currently calls pcint_handler(0, ...), where 0 is the port, which in our case is not true any more. I will push an update shortly.

[roberthartung]

@maribu see #11122

[maribu]

@roberthartung: Can we close this in favor of #11122?

[roberthartung]

@roberthartung: Can we close this in favor of #11122?

yes, please!

[roberthartung]

closed in favor of #11122