cpu/atmega_common: pseudomodule-based pin change interrupt implementation by roberthartung · Pull Request #11122 · RIOT-OS/RIOT

roberthartung · 2019-03-07T09:57:47Z

Contribution description

This is pseudomodule-based version of pin change interrupts for the atmega common platform. Challenge was, that each bank of pin change interrupts (PCINT0, ..., PCINT3) mixes ports. Therefore some magic has to happen.

The memory consumption is: (1 byte (state) + 8 byte (mapping) + 8*6 (config)) per used bank. This means if a single PCINT is used on a pin we need 57 bytes of additional memory.

Additionally, each cpu needs an atmega_pcint.h for the specific PCINT mapping.

Issues/PRs references

See also #7610, #8993, #9159. This is an follow up PR for #11114

Testing

ATmega1281 (board(s): waspmote-pro) @maribu double checked config against the data sheet (no one has the board)
ATmega1284p (board(s): mega-xplained) @TorbenPetersen tested with the INGA board
ATmega2560 (boards(s): arduino-mega2560) PCINT9 and PCINT10 are not working yet
ATmega256RFR2 (boards(s): jiminy-mega256rfr2) @maribu double checked config against the data sheet
ATmega328P (boards(s): arduino-uno) @maribu tested, works as expected

Testing for non-Arduino boards

Flash tests/periph_gpio on your avr board
Enable a pcint bank with USEMODULE += atmega_pcint0, or USEMODULE += atmega_pcint.
Using the shell commands init_int and enable_int test if interrupts are correctly fired for a couple of pins.
- Please test both pins with their own interrupt vector (to rule out regressions) and pins that rely on the "Pin Change Interrupt" vector shared by multiple GPIOs
- Please test with at least one pin per interrupt bank

Testing for Arduino Uno and Arduino Mega2560

Flash and run the test added in this PR.

Update 1: Added TODO for testing
Update 2: Elaborated on testing
Update 3: Added instructions for testing on Arduino boards, added test results

maribu

Some preliminary comments, I now need to attend some other stuff and will resume the review later

cpu/atmega_common/periph/gpio.c

maribu

Here the remaining remarks

cpu/atmega_common/Makefile.include

cpu/atmega_common/periph/gpio.c

maribu

Some more comments. (Sorry for spotting that not the first time.)

cpu/atmega_common/periph/gpio.c

cpu/atmega1284p/include/atmega_pcint.h

cpu/atmega_common/Makefile.dep

cpu/atmega1284p/Makefile.dep

maribu · 2019-03-07T15:58:01Z

OK, code-wise it almost reached the point I'd ACK (@ZetaR60 and @kYc0o: I'd feel more comfortable if you also could have a brief look at this PR).

The next step would be testing. I added a list of checkmarks above to track the state. I can test on ATmega328p and ATmega2560.

Maybe @Josar can test for ATmega256rfr2?
Maybe @ZetaR60 can test for ATmega1284P?

Regarding the ATmega1281 it will likely go untested, as no one seems to have a waspmote pro any more. (But this will not prevent this PR from merging.)

roberthartung · 2019-03-08T09:32:13Z

OK, code-wise it almost reached the point I'd ACK (@ZetaR60 and @kYc0o: I'd feel more comfortable if you also could have a brief look at this PR).

The next step would be testing. I added a list of checkmarks above to track the state. I can test on ATmega328p and ATmega2560.

Maybe @Josar can test for ATmega256rfr2?
Maybe @ZetaR60 can test for ATmega1284P?

Regarding the ATmega1281 it will likely go untested, as no one seems to have a waspmote pro any more. (But this will not prevent this PR from merging.)

Looks like I have a bug with the state, but should be easy to fix and we should be good to go for testing then.

maribu

Two more things (sorry for the seamlessly endless string of nit-picking):

Some files do not have a newline at the end, the CI will complain about that. (I think it should be exactly one new line to keep them happy, as they will complain about two and more es well.)
See inline comment

boards/common/arduino-atmega/Makefile.dep

maribu · 2019-03-08T10:06:12Z

I added some remarks how testing can be done in the (a.f.a.i.k.) most straightforward way.

roberthartung · 2019-03-08T10:35:07Z

Fun fact: atmega328p does not have all banks saturated. Will update shortly. Currently testing on an Arduino Uno!

roberthartung · 2019-03-08T10:40:32Z

Some bad things are happening for me:

make BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=arduino-uno all

Launching build container using image "riot/riotbuild:latest".
sudo docker run --rm -t -u "$(id -u)" \
    -v '/home/hartung/git/RIOT-ibr:/data/riotbuild/riotbase' \
    -v '/home/hartung/git/RIOT-ibr/build:/data/riotbuild/build' \
    -v '/home/hartung/git/RIOT-ibr/cpu:/data/riotbuild/riotcpu' \
    -v '/home/hartung/git/RIOT-ibr/boards:/data/riotbuild/riotboard' \
    -v '/home/hartung/git/RIOT-ibr/makefiles:/data/riotbuild/riotmake' \
    -v '/home/hartung/git/RIOT-ibr:/data/riotbuild/riotproject' \
    -v '/usr/share/zoneinfo/Europe/Berlin:/etc/localtime:ro' \
    -e 'RIOTBASE=/data/riotbuild/riotbase' \
    -e 'BUILD_DIR=/data/riotbuild/build' \
    -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' \
    -e 'RIOTCPU=/data/riotbuild/riotcpu' \
    -e 'RIOTBOARD=/data/riotbuild/riotboard' \
    -e 'RIOTMAKE=/data/riotbuild/riotmake' \
    -e 'RIOTPROJECT=/data/riotbuild/riotproject' \
       \
    -e 'BOARD=arduino-uno' \
    -w '/data/riotbuild/riotproject/tests/periph_gpio/' \
    'riot/riotbuild:latest' make all 'BOARD=arduino-uno'
Building application "tests_periph_gpio" for "arduino-uno" with MCU "atmega328p".

"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/auto_init
"make" -C /data/riotbuild/riotbase/sys/benchmark
"make" -C /data/riotbuild/riotbase/sys/div
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/sys/shell
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/sys/tsrb
"make" -C /data/riotbuild/riotbase/sys/xtimer
"make" -C /data/riotbuild/riotboard/arduino-uno
"make" -C /data/riotbuild/riotboard/common/arduino-atmega
"make" -C /data/riotbuild/riotboard/common/atmega
"make" -C /data/riotbuild/riotcpu/atmega328p
"make" -C /data/riotbuild/riotcpu/atmega_common
"make" -C /data/riotbuild/riotcpu/atmega_common/avr_libc_extra
"make" -C /data/riotbuild/riotcpu/atmega_common/periph
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800950 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.data' is not within region `data'
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800db6 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800db8 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.noinit' is not within region `data'
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800950 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.data' is not within region `data'
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800db6 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/4.9.2/../../../avr/bin/ld: address 0x800db8 of /data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.noinit' is not within region `data'
collect2: error: ld returned 1 exit status
/data/riotbuild/riotbase/Makefile.include:460: recipe for target '/data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf' failed
make: *** [/data/riotbuild/riotproject/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf] Error 1
make: *** [/home/hartung/git/RIOT-ibr/makefiles/docker.inc.mk:114: ..in-docker-container] Error 2

make BOARD=arduino-uno all

Building application "tests_periph_gpio" for "arduino-uno" with MCU "atmega328p".

"make" -C /home/hartung/git/RIOT-ibr/boards/arduino-uno
"make" -C /home/hartung/git/RIOT-ibr/boards/common/arduino-atmega
"make" -C /home/hartung/git/RIOT-ibr/boards/common/atmega
"make" -C /home/hartung/git/RIOT-ibr/core
"make" -C /home/hartung/git/RIOT-ibr/cpu/atmega328p
"make" -C /home/hartung/git/RIOT-ibr/cpu/atmega_common
"make" -C /home/hartung/git/RIOT-ibr/cpu/atmega_common/avr_libc_extra
"make" -C /home/hartung/git/RIOT-ibr/cpu/atmega_common/periph
"make" -C /home/hartung/git/RIOT-ibr/drivers
"make" -C /home/hartung/git/RIOT-ibr/drivers/periph_common
"make" -C /home/hartung/git/RIOT-ibr/sys
"make" -C /home/hartung/git/RIOT-ibr/sys/auto_init
"make" -C /home/hartung/git/RIOT-ibr/sys/benchmark
"make" -C /home/hartung/git/RIOT-ibr/sys/div
"make" -C /home/hartung/git/RIOT-ibr/sys/isrpipe
"make" -C /home/hartung/git/RIOT-ibr/sys/shell
"make" -C /home/hartung/git/RIOT-ibr/sys/stdio_uart
"make" -C /home/hartung/git/RIOT-ibr/sys/tsrb
"make" -C /home/hartung/git/RIOT-ibr/sys/xtimer
/usr/bin/avr-ld: address 0x800956 of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.data' is not within region `data'
/usr/bin/avr-ld: address 0x800dbc of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.bss' is not within region `data'
/usr/bin/avr-ld: address 0x800dbe of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.noinit' is not within region `data'
/usr/bin/avr-ld: address 0x800956 of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.data' is not within region `data'
/usr/bin/avr-ld: address 0x800dbc of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.bss' is not within region `data'
/usr/bin/avr-ld: address 0x800dbe of /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf section `.noinit' is not within region `data'
collect2: error: ld returned 1 exit status
make: *** [/home/hartung/git/RIOT-ibr/tests/periph_gpio/../../Makefile.include:460: /home/hartung/git/RIOT-ibr/tests/periph_gpio/bin/arduino-uno/tests_periph_gpio.elf] Error 1

maribu · 2019-03-08T10:57:05Z

Ah, that is the linker test failing because ROM/RAM requirements exceed the provided falsh/RAM.

(On ARM the message is human readable and also says how much bytes need to be squeezed out of the build to fit the board.) This just means other means of testing are required for ATmega328P based boards

maribu · 2019-03-08T10:59:04Z

(Btw: The test was to huge before this PR for the ATmega328p on the docker and my toolchain.)

maribu · 2019-03-08T11:03:32Z

The jiminiy-mega256rfr2, the waspmote-pro and the mega-xplained are the currently supported non-Arduino AVR boards and need the include $(RIOTBOARD)/common/atmega/Makefile.dep also added to their Makefile.dep.

maribu · 2019-03-08T11:21:42Z

Perfect. With all my remarks addressed please squash, so that others do not need to dig trough the long commit history.

maribu

OK, this PR adds 147 B RAM with 3 pcint banks enabled. The minimum required RAM seems to be:

Per bank: 1 byte to store the banks previous state
Per pin:
- 2 byte to store the callback function
- 2 byte to store the user argument (void *arg)
- 1 byte for the flank

In total this is 3 + 3 * 8 * 5 = 123 bytes. With the changes suggest above this PR reaches this ~~requirements~~ minimal required RAM overhead.

Also currently it adds 908B to the ROM size. I think by using a 1d array for pcint_mapping this will become a bit less, but I didn't try.

Update: Fixed confusing language

cpu/atmega_common/periph/gpio.c

maribu · 2019-03-08T15:03:10Z

Sorry for asking for more changes, but RIOT's internal code should be as efficient as possible to let the application developer have as much resources as possible left. (Also: I just wrote a simple test application that still doesn't fit the ATmega328p. I will really have trouble testing this :-()

maribu · 2019-03-08T15:42:09Z

@roberthartung: Do you have an Arduino Uno? With the changes suggest above this test for the Arduino Uno can be build and flashed (at least with my toolchain). The test works without module atmega_pcint used (for the Arduino pins 2 and 3, as they are the only with their own interrupt vector), but strange crashes occur when the module is added.

maribu · 2019-03-08T15:43:50Z

(Btw: Feel free to just cherry-pick the commit adding the test into your PR. It might be useful to have the test around in RIOT.)

roberthartung · 2019-03-08T16:01:02Z

(Btw: Feel free to just cherry-pick the commit adding the test into your PR. It might be useful to have the test around in RIOT.)

How is it a test for Aduino Uno if your makefile says BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-uno?

roberthartung · 2019-03-08T16:04:45Z

@roberthartung: Do you have an Arduino Uno? With the changes suggest above this test for the Arduino Uno can be build and flashed (at least with my toolchain). The test works without module atmega_pcint used (for the Arduino pins 2 and 3, as they are the only with their own interrupt vector), but strange crashes occur when the module is added.

Output for melooks fine so far.

maribu · 2019-07-26T06:00:00Z

OK, apart from the the test (which should expect gpio_init_int() to fail on pins not supported by a board) I'd say ACK. How about splitting the test off in a separate PR, squash this PR and merge?

To me it also is questionable if I am actually allowed to ACK the code of the test, as I authored most of it. Maybe I can open a separate PR for that and you (@roberthartung) could review the test?

maribu · 2019-07-26T08:27:14Z

Oh, I forgot that ATmega32U4 has been merged in the meantime. Can you please rebase and add the pcint mapping for that one? I will test on the Arduino Leonardo.

maribu

OK, found one more bug (see above).

So this is still needed in order to ACK:

Fix the bug above
Squash and rebase on top of current master
Add PCINT mapping for the ATmega32U4

I'm extremely sorry that this PR was so exhausting. But I'm confident it now is about to get merged :-)

cpu/atmega_common/periph/gpio.c

cpu/atmega_common/Makefile.features

cpu/atmega_common/Makefile.include

roberthartung · 2019-08-01T06:39:19Z

OK, apart from the the test (which should expect gpio_init_int() to fail on pins not supported by a board) I'd say ACK. How about splitting the test off in a separate PR, squash this PR and merge?

To me it also is questionable if I am actually allowed to ACK the code of the test, as I authored most of it. Maybe I can open a separate PR for that and you (@roberthartung) could review the test?

Sounds good for me, just removed the test from my code.

roberthartung · 2019-08-01T07:37:29Z

rebased on latest master, one commit for basic implementation, one commit for each CPU. I think having more commits makes it clearer! Additionally I fixed a bug for atmega1284p (missing .dep file) and I added the atmega256rfr2 CPU.

maribu

ACK

maribu · 2019-08-02T12:39:02Z

@roberthartung: Thanks for your patience!

roberthartung changed the title ~~Hartung/atmega pin change interrupt pseudomodules~~ [WIP] cpu/atmega_common: pseudomodule-based pin change interrupt implementation Mar 7, 2019

roberthartung mentioned this pull request Mar 7, 2019

[WIP] cpu/atmega_common: pin change interrupt implementation #11114

Closed

maribu reviewed Mar 7, 2019

View reviewed changes

cpu/atmega_common/Makefile.include Show resolved Hide resolved

cpu/atmega_common/periph/gpio.c Outdated Show resolved Hide resolved

cpu/atmega_common/periph/gpio.c Outdated Show resolved Hide resolved

maribu reviewed Mar 7, 2019

View reviewed changes

cpu/atmega_common/periph/gpio.c Outdated Show resolved Hide resolved

cpu/atmega1284p/include/atmega_pcint.h Outdated Show resolved Hide resolved

cpu/atmega1284p/include/atmega_pcint.h Outdated Show resolved Hide resolved

maribu reviewed Mar 7, 2019

View reviewed changes

cpu/atmega_common/Makefile.dep Outdated Show resolved Hide resolved

maribu reviewed Mar 7, 2019

View reviewed changes

cpu/atmega1284p/Makefile.dep Outdated Show resolved Hide resolved

maribu reviewed Mar 8, 2019

View reviewed changes

boards/common/arduino-atmega/Makefile.dep Outdated Show resolved Hide resolved

maribu added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Mar 8, 2019

roberthartung force-pushed the hartung/atmega-pin-change-interrupt-pseudomodules branch from f9b5f0a to eddfdbc Compare March 8, 2019 14:50

maribu reviewed Mar 8, 2019

View reviewed changes

maribu mentioned this pull request Jul 26, 2019

tests: Added tests for gpio_init_int() on ATmega boards #11923

Closed

5 tasks

maribu requested changes Jul 26, 2019

View reviewed changes

cpu/atmega_common/periph/gpio.c Outdated Show resolved Hide resolved

maribu reviewed Jul 26, 2019

View reviewed changes

cpu/atmega_common/Makefile.features Outdated Show resolved Hide resolved

cpu/atmega_common/Makefile.include Show resolved Hide resolved

roberthartung force-pushed the hartung/atmega-pin-change-interrupt-pseudomodules branch 2 times, most recently from 82d7ff7 to 0adcf14 Compare August 1, 2019 06:38

roberthartung force-pushed the hartung/atmega-pin-change-interrupt-pseudomodules branch from 0adcf14 to 9f976b8 Compare August 1, 2019 06:41

roberthartung added 3 commits August 1, 2019 09:23

cpu/atmega_common: implements pseudomodule-based pin change interrupts

7bca1cc

cpu/atmega1281: pin change interrupts

0fc55cd

cpu/atmega32u4: pin change interrupts

dcd9177

roberthartung force-pushed the hartung/atmega-pin-change-interrupt-pseudomodules branch 2 times, most recently from ee33a32 to 9c8e2d3 Compare August 1, 2019 07:32

roberthartung added 5 commits August 1, 2019 09:35

cpu/atmega1284p: pin change interrupts

eeb895f

cpu/atmega2560: pin change interrupts

57705d4

boards: adds atmega pin change interrupt to doc

09b2612

boards/arduino-mega2560: restrics usable pin change interrupts

18117b4

cpu/atmega256rfr2: pin change interrupts

c8d460e

roberthartung force-pushed the hartung/atmega-pin-change-interrupt-pseudomodules branch from 9c8e2d3 to c8d460e Compare August 1, 2019 07:35

maribu approved these changes Aug 2, 2019

View reviewed changes

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 2, 2019

maribu merged commit e612d3d into RIOT-OS:master Aug 2, 2019

roberthartung mentioned this pull request Aug 19, 2019

WIP: pm_layered for cpu/atmega_common #8207

Closed

maribu mentioned this pull request Sep 9, 2019

boards: Remove support for the Jiminy-Mega256RFR2 #12182

Merged

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

wosym mentioned this pull request Dec 30, 2019

cpu/atmega328p: linker/memory issue? #13016

Closed

Conversation

roberthartung commented Mar 7, 2019 • edited by maribu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Testing

Testing for non-Arduino boards

Testing for Arduino Uno and Arduino Mega2560

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Mar 7, 2019

Uh oh!

roberthartung commented Mar 8, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

roberthartung commented Mar 8, 2019

Uh oh!

roberthartung commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

maribu commented Mar 8, 2019

Uh oh!

roberthartung commented Mar 8, 2019

Uh oh!

roberthartung commented Mar 8, 2019

Uh oh!

maribu commented Jul 26, 2019

Uh oh!

roberthartung commented Mar 7, 2019 •

edited by maribu

Loading

maribu left a comment •

edited

Loading