Skip to content

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

Closed
maribu wants to merge 7 commits intoRIOT-OS:masterfrom
maribu:pcint
Closed

tests: Added tests for gpio_init_int() on ATmega boards#11923
maribu wants to merge 7 commits intoRIOT-OS:masterfrom
maribu:pcint

Conversation

@maribu
Copy link
Member

@maribu maribu commented Jul 26, 2019

Contribution description

This PR adds a test that tests gpio_init_int() on ATmega boards, which differs in two major points from the existing test for gpio_init_int():

  • It can be compiled for ATmega328P based boards
  • It takes the pin changed interrupts of the ATmega into account. (A single interrupt vector for 8 pins.)

Testing procedure

Build, flash and run the provided test on a couple of ATmega based boards

  • ATmega32U4 based board (arduino-leonardo)
  • [ ] ATmega256RF2 based board (jiminy-mega256rf2) (Board support dropped in RIOT)
  • ATmega328P based board (arduino-uno or arduino-nano)
  • ATmega1281 based board (waspmote-pro)
  • ATmega1284P based board (mega-xplained or the not yet mainlined INGA board)
  • ATmega2560 based board (arduino-mega2560)

Issues/PRs references

Depends on and includes #11122 (merged)
Depends on and includes #11927 (merged)

@maribu maribu added Area: tests Area: tests and testing framework Platform: AVR Platform: This PR/issue effects AVR-based platforms State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 26, 2019
maribu added 3 commits August 2, 2019 14:40
This test is written to allow testing and debugging of the interrupt facilities
of ATmega MCUs.

Some ATmegas (e.g. the ATmega328P used on the Arduino Uno / Arduino Nano)
have so limited RAM and flash, that using standard tests cannot be compiled.
This test is written with the low resources in mind and also takes the pin
changed features of the ATmega MCUs into account.
Somehow Makefile.dep was missing on the Arduino Leonardo. This commit adds it.
Currently the configuration claims that external interrupt INT4 is present on
pin PE7. However, the ATmega32U4 datasheet (section 10.3.4 page 81) contains
the following remark to pin PE7: "Not present on pin-out". This commit removes
the PE7 from the interrupt config.
maribu added 3 commits August 2, 2019 16:56
Pins PD2 and PD3 can be used as both external interrupt and as pin changed
interrupt. (PD2: INT0/PCINT18, PD3: INT1/PCINT19.) This commit removes support
for the pin changed interrupt PCINT18 and PCINT19 on the ATmega328P MCU in
order to reflect that GPIO PD2 and PD3 cannot be set up as port changed
interrupt, as RIOT will always use the external interrupt of the pin changed
interrupt if both are available.
@maribu
Copy link
Member Author

maribu commented Aug 2, 2019

OK. I tested this on the Ardunino Lenoardo, Uno, Nano, and Mega2560. I had to add board specific pin change interrupt configurations to take unconnected or unusable GPIOs into account. I also updated the ATmega328P pin change interrupt configuration, as two pins can be used as both external interrupt and as pin changed interrupt. I disabled those as pin changed interrupts to make it more obvious that RIOT will never configure them that was, as external interrupts take preference in the implementation (which makes sense).

@roberthartung: The PR is now ready to be reviewed

@maribu
Copy link
Member Author

maribu commented Aug 19, 2019

@roberthartung: Have you found time to give it a look?

@roberthartung
Copy link
Member

roberthartung commented Oct 25, 2019

Works for me so far. :) However is it good to 'randomly' attach flanks? e.g. if pull ups/downs are attached, this might cause an issue?

@maribu
Copy link
Member Author

maribu commented Oct 25, 2019

I think that the flanks should not be an issue if external pull downs are present: One would need to use a second jumper wire connected to VDD to generate the edge for those pins.

But enabling the internal pull up will be problematic with an external pull down present. I think that the combined resistance of both pull resistors should be high enough to limit the current to a safe value, as they will be in series (and not in parallel), right? But the logical value at the pin could be "jumpy", if both pull resistors pull are similar "strong".

But this should not be an issue on the boards we currently have in RIOT, as they do not use external pull downs on the relevant pins as far as I know.

A better approach for testing would be to provide a shell and let the user pick the pins to test. But using the shell is not possible on the ATmega328P based boards due to memory constrains; and new boards having external pull downs might be using the ATmega328P. So this is not viable, sadly.

I think in case new boards are added that do have external pull downs, those pins could be added conditionally (#ifdef ...) to the blacklist. Or a second list for "use GPIO_IN instead of GPIO_IN_PU" could be added. But so far this is not needed.

@maribu maribu removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 12, 2019
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 12, 2019
@roberthartung
Copy link
Member

But this should not be an issue on the boards we currently have in RIOT, as they do not use external pull downs on the relevant pins as far as I know.

Well we also have to think about the future I guess. Just wanted to make a hint about that. Maybe we should add a README.md to the test so we mention that we assign the flanks "randomly" (round robin manner) and one needs to be careful, when using the test, if external pull downs are present?

@maribu maribu closed this May 11, 2020
@maribu maribu deleted the pcint branch May 11, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants