cpu/atmega_common: external interrupt fix and refactor #8951
Conversation
| } | ||
| #if defined(EICRB) | ||
| else { | ||
| EICRB |= (flank << (pin_num * 2) % 4); | ||
| EICRB |= (flank << ((int_num % 4) * 2)); |
There was a problem hiding this comment.
Can you tell more about this change? At a first glance it looks like the same, but I know there's a difference. I also wonder how it worked in the past.
There was a problem hiding this comment.
Here is an example if int_num == 6:
- Old behavior: multiply by 2 (== 12), then modulo 4 (remainder of 12/4 = 0), ergo use ISC40 and ISC41 in EICRB (wrong).
- New behavior: modulo 4 (remainder of 6/4 = 2), then multiply by 2 (2*2 = 4), ergo use ISC60 and ISC61 in EICRB (right).
There was a problem hiding this comment.
Ok, thanks for the explanation!
|
One thing that concerns me: if GPIO_EXT_INT_NUMOF is different the size of the CPU_ATMEGA_EXT_INTS array, then non-existent members of ext_ints or the config global can be addressed. Normally this isn't a problem, since this is a low-level definition (and wouldn't be changed often). It is difficult to not use two definitions however, you can't get the sizeof CPU_ATMEGA_EXT_INTS prior to defining the config global. Thoughts? |
|
Can't believe I missed this super obvious solution to my concern posted above. |
|
Hmm. It is failing compile with the change I just made. I am not able to properly debug this at the moment (I am out of town). If you wish to undo / fix / change / squash the code, feel free. |
|
Solved the problem anyway... by fixing yet another existing bug in cpu/atmega_common/periph/gpio.c. Wow. The order of the ifdefs was reversed from what it should be. |
|
What about Pin Change Interrupts? See #7610 |
|
@roberthartung My understanding of the situation: There are multiple bugs that this PR fixes. Since RIOT is in feature freeze for the new release, this is probably going to get added (as a bug fix) for the release before #7610 gets added. So, #7610 should be tweaked to be compatible with #8951. Frustrating, I know. I had some stuff that was ready but didn't make it in as well. I also have some other suggestions for #7610, which I will post over there. |
8694075 to
8971175
Compare
|
Squashed with requested changes incorporated. EDIT: Oops; wasn't supposed to squash. Unsquashed from backup. Sorry. |
8971175 to
8694075
Compare
|
ping @kYc0o |
| #else | ||
| EIMSK |= (1 << _pin_num(pin)); | ||
| #endif | ||
| EIMSK |= (1 << _int_num(pin)); |
There was a problem hiding this comment.
One Question: If I try to enable the IRQ for a non valid PIN, _pin_num will return -1. Is shifting with a negative shift count valid? According to this: https://stackoverflow.com/questions/4945703/left-shifting-with-a-negative-shift-count the behaviour for this is not defined.
There was a problem hiding this comment.
The pin validity is checked in gpio_init_int and afterwards presumed valid.
|
@kYc0o Is there anything else this PR needs? |
|
Looks ok for me, but need to test, afterwards I can give my tested ACK. However I still wonder if this PR conflicts with #7610, I'd like to have @roberthartung 's opinion on this. |
|
ping @roberthartung |
|
@ZetaR60 @kYc0o Go ahead with this one. Afterwards @TorbenPetersen can work on #7610 once #8993 is merged. |
|
@roberthartung thanks! @ZetaR60 you might squash so the CI tests will pass. |
8694075 to
0b2d620
Compare
|
Squashed and double checked with tests/periph_gpio. Everything is looking good and ready to go. Thanks for the help with this PR! |
|
Can you rebase on master to remove the external interrupt context switching? |
On the ATmegas there are several issues currently with external interrupts in cpu/atmega_common/periph/gpio.c:
This PR:
Tested and working using tests/periph_gpio on ATmega1284p / mega-xplained, at least for INT2 (could not test INT0-1, since they are on the serial port).