cpu/atmega_common: external interrupt fix and refactor by ZetaR60 · Pull Request #8951 · RIOT-OS/RIOT

ZetaR60 · 2018-04-16T02:53:09Z

On the ATmegas there are several issues currently with external interrupts in cpu/atmega_common/periph/gpio.c:

GPIO_EXT_INT_NUMOF is incorrectly defined for higher interrupt vectors, preventing their use.
Order of operations with EICRB write was incorrect for gpio_init_int, preventing the use of some interrupts.
Condition checking in gpio_init_int for ATmega1284p is wrong, preventing the use of one interrupt (this one is my fault).
There is this mess of conditionals with lots of CPU ifdefs.

This PR:

Removes the messy conditionals, and removes all CPU ifdefs.
Finds if a pin is an interrupt and the interrupt number by looping over an array of pins defined by CPU_ATMEGA_EXT_INTS. This is placed in periph_cpu.h of each cpu.
Corrects the bugs listed above.

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

kYc0o

Just a small question. I consider this a very good cleanup and actually a bug fix. Thus I'd like to get it into the release. I'll make more testing tomorrow.

Thanks @ZetaR60 to tackle this!

kYc0o · 2018-04-16T21:26:59Z

cpu/atmega_common/periph/gpio.c

 #if defined(EICRB)
    else {
-        EICRB |= (flank << (pin_num * 2) % 4);
+        EICRB |= (flank << ((int_num % 4) * 2));


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.

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

Ok, thanks for the explanation!

ZetaR60 · 2018-04-16T22:44:12Z

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?
[edited for clarity]

ZetaR60 · 2018-04-18T01:46:21Z

Can't believe I missed this super obvious solution to my concern posted above.

ZetaR60 · 2018-04-18T02:00:42Z

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.

ZetaR60 · 2018-04-18T02:53:41Z

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.

roberthartung · 2018-04-18T12:46:41Z

What about Pin Change Interrupts? See #7610

ZetaR60 · 2018-04-20T03:09:41Z

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

ZetaR60 · 2018-04-27T05:46:04Z

Squashed with requested changes incorporated.

EDIT: Oops; wasn't supposed to squash. Unsquashed from backup. Sorry.

ZetaR60 · 2018-05-02T19:49:42Z

ping @kYc0o

TorbenPetersen · 2018-05-09T07:21:25Z

cpu/atmega_common/periph/gpio.c

-#else
-    EIMSK |= (1 << _pin_num(pin));
-#endif
+    EIMSK |= (1 << _int_num(pin));


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.

The pin validity is checked in gpio_init_int and afterwards presumed valid.

ZetaR60 · 2018-05-19T06:12:55Z

@kYc0o Is there anything else this PR needs?

kYc0o · 2018-05-21T16:03:21Z

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.

ZetaR60 · 2018-05-28T17:32:41Z

ping @roberthartung

roberthartung · 2018-05-29T06:44:46Z

@ZetaR60 @kYc0o Go ahead with this one. Afterwards @TorbenPetersen can work on #7610 once #8993 is merged.

kYc0o · 2018-05-29T14:01:52Z

@roberthartung thanks! @ZetaR60 you might squash so the CI tests will pass.

ZetaR60 · 2018-05-29T16:04:39Z

Squashed and double checked with tests/periph_gpio. Everything is looking good and ready to go. Thanks for the help with this PR!

kYc0o · 2018-05-30T14:19:50Z

Can you rebase on master to remove the external interrupt context switching?

kYc0o

Tested on atmega2560. ACK.

aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Apr 16, 2018

kYc0o added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Apr 16, 2018

kYc0o added this to the Release 2018.04 milestone Apr 16, 2018

kYc0o self-assigned this Apr 16, 2018

kYc0o reviewed Apr 16, 2018

View reviewed changes

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 16, 2018

jnohlgard mentioned this pull request Apr 19, 2018

Received packets processing and thread change Timing #8979

Closed

ZetaR60 mentioned this pull request Apr 20, 2018

cpu/atmega: WIP implementation of pin change interrupts #7610

Closed

kaspar030 removed this from the Release 2018.04 milestone Apr 22, 2018

ZetaR60 force-pushed the RIOT_atmega_ext_int_clarity branch from 8694075 to 8971175 Compare April 27, 2018 05:42

ZetaR60 force-pushed the RIOT_atmega_ext_int_clarity branch from 8971175 to 8694075 Compare April 27, 2018 15:44

ZetaR60 mentioned this pull request May 1, 2018

drivers/pca95xx: support for I2C GPIO expanders #9054

Closed

ZetaR60 mentioned this pull request May 7, 2018

cpu/atmega_common: external interrupts not usable #9094

Closed

TorbenPetersen reviewed May 9, 2018

View reviewed changes

ZetaR60 mentioned this pull request May 16, 2018

ACU-AMNx board and MVP checklist #9143

Closed

24 tasks

cpu/atmega_common: external interrupt fix and refactor

533388d

Matthew Blue added 5 commits May 29, 2018 11:24

cpu/atmega1281: external interrupt refactor

a61aff6

cpu/atmega1284p: external interrupt refactor

327bf09

cpu/atmega2560: external interrupt refactor

737c463

cpu/atmega256rfr2: external interrupt refactor

6ea3261

cpu/atmega328p: external interrupt refactor

0b2d620

ZetaR60 force-pushed the RIOT_atmega_ext_int_clarity branch from 8694075 to 0b2d620 Compare May 29, 2018 15:29

TorbenPetersen mentioned this pull request May 30, 2018

gpio.h/cb_mux API change and interim compatibilty #9159

Closed

kYc0o approved these changes May 30, 2018

View reviewed changes

kYc0o merged commit 1aed925 into RIOT-OS:master May 30, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

ZetaR60 commented Apr 16, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZetaR60 Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

kYc0o Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

ZetaR60 commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZetaR60 commented Apr 18, 2018

Uh oh!

ZetaR60 commented Apr 18, 2018

Uh oh!

ZetaR60 commented Apr 18, 2018

Uh oh!

roberthartung commented Apr 18, 2018

Uh oh!

ZetaR60 commented Apr 20, 2018

Uh oh!

ZetaR60 commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZetaR60 commented May 2, 2018

Uh oh!

TorbenPetersen May 9, 2018

Choose a reason for hiding this comment

Uh oh!

ZetaR60 May 9, 2018

Choose a reason for hiding this comment

Uh oh!

ZetaR60 commented May 19, 2018

Uh oh!

kYc0o commented May 21, 2018

Uh oh!

ZetaR60 commented May 28, 2018

Uh oh!

roberthartung commented May 29, 2018

Uh oh!

kYc0o commented May 29, 2018

Uh oh!

ZetaR60 commented May 29, 2018

Uh oh!

kYc0o commented May 30, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kYc0o Apr 16, 2018 •

edited

Loading

ZetaR60 commented Apr 16, 2018 •

edited

Loading

ZetaR60 commented Apr 27, 2018 •

edited

Loading