Skip to content

tests/periph_gpio: make IRQ related functionality optional#9981

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_gpio_ifdefextintfuncs
Sep 21, 2018
Merged

tests/periph_gpio: make IRQ related functionality optional#9981
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_gpio_ifdefextintfuncs

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Sep 20, 2018 •
edited by jnohlgard
Loading

Contribution description

A quick fix for the tests/periph_gpio test app, to only include the irq related shell commands in case the irq feature is selected.

Testing procedure

Build the tests/periph_gpio app as is and also without the USEMODULE += periph_gpio_irq. Both should compile fine.

Issues/PRs references

Tracker: #9977

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 20, 2018
This was referenced Sep 20, 2018
Closed
Merged
jnohlgard
jnohlgard reviewed Sep 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff display went a little nuts with the reordered functions. This review will take a little while to go through.

@cladmi cladmi self-requested a review September 20, 2018 16:37
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Sep 20, 2018

@haukepetersen please do not assign people but ask for reviews

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Sep 20, 2018

I would prefer to do the review in pieces. These changes are independent and there are no interdependencies between the CPUs. The change to the test application could be easily reviewed and merged if a separate PR. The test Makefile should change FEATURES_REQUIRED into FEATURES_OPTIONAL for periph_gpio_irq.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Sep 20, 2018

The test Makefile should change FEATURES_REQUIRED into FEATURES_OPTIONAL for periph_gpio_irq.

I was about to comment the exact same thing.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Sep 20, 2018

This is too big and spread out. It would be better to split each CPU or family into its own PR to make the review process smooth.

@haukepetersen
tests/periph_gpio: make IRQ commands optional
0b616ac 
only include irq related shell commands in case the
PERIPH_GPIO_IRQ feature is selected
@haukepetersen haukepetersen force-pushed the fix_gpio_ifdefextintfuncs branch from b4a785f to 0b616ac Compare September 21, 2018 07:03
@haukepetersen haukepetersen changed the title cpu/gpio: fix all gpio drivers to handle the periph_gpio_irq feature correctly tests/periph_gpio: make IRQ realted functionality optional Sep 21, 2018
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Sep 21, 2018

Changed to scope of this PR to only include the fixes to the tests/periph_gpio test.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Sep 21, 2018

The test Makefile should change FEATURES_REQUIRED into FEATURES_OPTIONAL for periph_gpio_irq

I was planning on this, actually already talked to @SemjonKerner yesterday and he wanted to open the PR for this today. I guess now I did his work :-)

@jnohlgard jnohlgard changed the title tests/periph_gpio: make IRQ realted functionality optional tests/periph_gpio: make IRQ related functionality optional Sep 21, 2018
jnohlgard
jnohlgard approved these changes Sep 21, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on native. tests/periph_gpio does not link in current master for BOARD=native. With this PR it works, and the init_int shell command is missing from the shell command list in that build.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Sep 21, 2018

Tested on frdm-kw41z, init_int 2 5 0 1, button SW4 makes the application print a message, as it should.

@jnohlgard jnohlgard merged commit bf7bbec into RIOT-OS:master Sep 21, 2018
@ghost ghost mentioned this pull request Sep 21, 2018
Merged
@miri64 miri64 deleted the fix_gpio_ifdefextintfuncs branch September 21, 2018 13:40
@jnohlgard jnohlgard mentioned this pull request Sep 23, 2018
Merged
@jia200x jia200x added this to the Release 2018.10 milestone Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

@cladmi cladmi Awaiting requested review from cladmi

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.10

Development

Successfully merging this pull request may close these issues.

5 participants

@haukepetersen @cladmi @jnohlgard @aabadie @jia200x