Pin change interrupts do not work on the PCINT for the context swap!

But they were working before. Why it doesn't work with this PR?

But they were working before. Why it doesn't work with this PR?

@kYc0o Probably this wasn't explained very well. The current state is there are no Pin Change Interrupts available at all in RIOT. However, the threading implementation uses a pin change interrupt for context swaps. The ISR however, is only triggering a context swap here! See https://github.com/RIOT-OS/RIOT/blob/master/cpu/atmega_common/thread_arch.c#L254, ideally with some overhead we would have to check if a PCINT is enabled for the corresponding port and call pcint_handler from gpio.c (see my implementation). Otherwise, we'd make a context swap. However, this has a little overhead (check if other pins are enabled).

Comments and thoughts much appreciated. I would be ok with a little overhead (2-3 instructions) for context swapping.

I still don't get why you don't just implement the remaining PCINT lines and leave the ones used for context switching as is? This may avoid the use of some PCINT but context switching is much more important.

@kYc0o Have you even looked at my code? That's exactly what it does....

The problem is every simple: The Context Swap interrupt works great, but blocks all other pins on that port from being used as a pin change interrupt, which in our case is a problem because we only have very few pins that are not used on our sensor node. I just would like to start a discussion about this and document it in this pull request / issue that this still is a big problem.
Therefore I'd like to discuss quickly if we should re-design the context swap to be able to use pin change interrupts on other pins too, but be able to have a context switch here!

Yes, I have looked at your code and I understand the issue, I have no problem with that. What I didn't know (and I had no means no know) is that you needed such PCINT pins for your specific board. If the implementation of context switching (including the PCINT implementation) belongs to the board configuration is because we wanted to take advantage of "unused" pins on a given board (e.g. Arduino, super common, easy to get, cheap...). However #7594 shows that at the beginning we arbitrarily chose a PCINT line which conflicts with I2C, thus the change on that PR.

In this PR you're proposing to add support of PCINT at the cpu level, which I find great. However, as this PR makes use of all PCINT pins available on a given CPU, interrupts will be triggered at each context switch, which if I understand correctly is the problem you're stating too. Thus, I think this is a no-go for now, as we had a lot of problems before on context switching (especially for multi threading and modules like xtimer).

I agree we should find a solution to trigger interrupts for context switching without taking a whole port for that use, but for now is the only thing we have. I'd also like to have a different/cleaner/ideal solution, but this CPU's are very limited (old) and were not designed having this kind of issues in mind.

Another option (which is what is done in the STM32 IRQ lines) is tho allow only 1/vector. This means 4 (not anymore 32, in the worst case). Actually 3, considering that one vector is used for the context switch.

I would prefer the laster (I've it working locally but never got the time to cleanup and publish it). What do you think?

So what restrictions will this result in? I am not able to use all my PCINTs at the same time, or worst case only one per port? This would not be an option for me.

@kYc0o Sorry, should've made it clear that this is a problem on boards where you need almost all pins, because the context switch interrupt occupies the whole port.

However, as this PR makes use of all PCINT pins available on a given CPU, interrupts will be triggered at each context switch, which if I understand correctly is the problem you're stating too.

The only conflict will happen if you're using both at the same time (pin change + context swap on one port). Context switching already has some overhead anyway, thus checking the last state of the port against the current one, and check if only the context swap pin changed seems to be the best solution to me by now. Of course this has to be done for every single context switch :/

#define AVR_CONTEXT_SWITCH_MASK (1 << PIN4)
uint8_t gpio_state [NUM_PORTS];
ISR(...) {
  if(AVR_CONTEXT_SWITCH_PORT ^ gpio_state[AVR_CONTEXT_SWITCH_PORT_NUM] == AVR_CONTEXT_SWITCH_MASK) {
    do_context_switch();
  } else {
    handle_pin_change_interrupts(AVR_CONTEXT_SWITCH_PORT_NUM);
  }
}

Maybe we need to use #ifdef here to enable this, or as you already suggest, we can just leave it as is and ignore the PCINT for the context swap for now and make it not usable for pin change interrupts with a big hint in the documentation?

Added saving the state and some other fixes. Open points:

• Memory comsumption?
• I'd vote for leaving the PCINT out for now
• Separate file for gpio_pcint.c to remove code if it is not needed. @lebrush can you give an example on how it should be done?

Just rebased on the latest master.

ping Added a bugfix for some other atmega boards. Any more comments? We still should find a solution that does not use any memory, if no pin change interrupts are required. Ideally the board defines the number of pcints it wishes to use?

I propose to add a guard for this change. Until now, it will not introduce any overhead to existing boards, but allows our INGA platform to use all interrupts. @kYc0o @lebrush any comments?

Some thoughts:

• I think there are better ways to accomplish low-overhead than adding a guard. If an application needs one PCINT, it will pull the memory requirements for using all of the PCINTs. I suggest using a linked list to handle the addition of new callbacks as needed. See sys/cb_mux: Callback multiplexer #8993 for a rough sketch of my suggestion.
• If cpu/atmega_common: return to non-interrupt context swaps #8904 is merged, then the context swap interrupt code can be removed.
• If cpu/atmega_common: external interrupt fix and refactor #8951 is merged, then some alterations would be needed to remain compatible.

#8904 was merged! 😃 can we take a look again on this? @ZetaR60 can we merge your approach to this one?

@ZetaR60 @kYc0o I have a PR ready for this today or tomorrow.

@roberthartung Please take a look at #8993. It was intended to help with the memory overhead of adding many callbacks.

@ZetaR60 I am not sure how this would be used for the PCINTs - is there an example?

Additionally, where do I locate memory for the linked list (in general)? If I make a xyz_t a; and an LL_APPEND(...,a, ...); this will be a local variable on the stack and will be overriden once I exit the function. So where do I correctly allocate space for the xyz_t struct that I will append to the linked list?
Do I have to know the number of interrupts before hand like this:

static xyz_t xyz[ ATMEGA_PCINT_USER ] = {0};

and then the user would define how many interrupts he required? Additionally, this can be extended, if any other driver needs some pcints.

Looking forward to your feedback.

@roberthartung I had intended for cb_mux to work for this use case, but it does require declaring a global struct in the calling function (since we don't have malloc), which sort of defeats the point here. You can declare the struct in local memory, but only if you don't leave the scope before removing the entry (i.e. creating an entry, sleeping the thread, and then removing it when awoke). Sorry for my oversight; I will see if I can think of a way to salvage cb_mux for use in PCINTs.

@roberthartung New developments with cb_mux relevant to this PR. See #9159

Closed in favor of #11122