Skip to content

make: Introduce periph_gpio_irq feature#9845

Merged
kaspar030 merged 3 commits intoRIOT-OS:masterfrom
jnohlgard:pr/features-gpio-irq
Aug 29, 2018
Merged

make: Introduce periph_gpio_irq feature#9845
kaspar030 merged 3 commits intoRIOT-OS:masterfrom
jnohlgard:pr/features-gpio-irq

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Aug 26, 2018 •
edited
Loading

Contribution description

Introduce a new feature flag periph_gpio_irq for when a driver/application requires GPIO interrupts. This allows the GPIO drivers to avoid allocating space in RAM for interrupt context. See the included change to the msp430fxyz gpio driver.

Testing procedure

No code is modified, only the header and makefiles (except for the example in msp430fxyz). If the compilation is OK, then everything should be fine.

Issues/PRs references

This could fix the build failures in #8711
Based on #9844

@jnohlgard jnohlgard added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first 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 Aug 26, 2018
@jnohlgard jnohlgard added this to the Release 2018.10 milestone Aug 26, 2018
@jnohlgard jnohlgard requested a review from cladmi August 26, 2018 07:40
@jnohlgard jnohlgard mentioned this pull request Aug 26, 2018
Closed
@cladmi cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 27, 2018
cladmi
cladmi reviewed Aug 27, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

The idea looks good to me. Should also other CPUs be adapted or was it mainly necessary for wsn430?

It can now be rebased as #9844 was merged

Makefile.dep Outdated

# Enable periph_gpio when periph_gpio_irq is required
ifneq (,$(filter periph_gpio_irq,$(USEMODULE)))
USEMODULE += periph_gpio
Copy link
Copy Markdown
Contributor

@cladmi cladmi Aug 27, 2018

Choose a reason for hiding this comment

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

In that case, should it not be FEATURES_REQUIRED += ?
It has the same result of having the name put in USEMODULE but also triggers the FEATURES dependency checks.

Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Aug 27, 2018

Choose a reason for hiding this comment

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

Sounds reasonable.

Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Aug 27, 2018

Choose a reason for hiding this comment

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

addressed

cpu/kinetis/Makefile.features
FEATURES_PROVIDED += periph_cpuid
FEATURES_PROVIDED += periph_hwrng
FEATURES_PROVIDED += periph_gpio
FEATURES_PROVIDED += periph_gpio_irq
Copy link
Copy Markdown
Contributor

@cladmi cladmi Aug 27, 2018

Choose a reason for hiding this comment

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

This one is on a different line while other are put on the same one.

Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Aug 27, 2018

Choose a reason for hiding this comment

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

I was experimenting with this CPU, then updated the others via sed.

Copy link
Copy Markdown
Contributor

@cladmi cladmi Aug 28, 2018

Choose a reason for hiding this comment

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

Maybe put it on the same line to be coherent, people like to copy paste afterwards.

@jnohlgard jnohlgard force-pushed the pr/features-gpio-irq branch from b201017 to f5adb94 Compare August 27, 2018 13:22
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 27, 2018

Rebased after #9844 was merged.

@cladmi

The idea looks good to me. Should also other CPUs be adapted or was it mainly necessary for wsn430?

It was mainly to save memory on wsn430. Other CPUs may benefit as well, but I think it is better to do follow up PRs for those.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Aug 28, 2018

It was mainly to save memory on wsn430. Other CPUs may benefit as well, but I think it is better to do follow up PRs for those.

Ok, that is what I was wondering. So would be a following tracking PR/issue.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Aug 28, 2018

Any test I could run just to validate the wsn430 ?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 28, 2018

I like this a lot. tests/minimal and the coming OTA bootloader will loose RAM weight.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 28, 2018

@kaspar030 and also ROM size, the IRQ handlers for GPIO interrupts will not be linked in the binary.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 28, 2018

@cladmi do you have the board? if so you could try tests/periph_gpio and fiddle with some wires on the GPIO pins

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 28, 2018

But anyway, if the linking is OK for wsn430, then the functions are still there, so it should be fine even without on hardware testing if it was working before.

Is the name periph_gpio_irq fine or should it be periph_gpio_int? (since the init function is named gpio_init_int)

A follow up to this PR is to add FEATURES_REQUIRED += periph_gpio to all boards which use gpio_set/clear/read etc in the board.c, alternatively, conditionally compile parts of the board_init, such as gpio_init(LED0_PIN).

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Aug 28, 2018

I prefer irq than int.

I only have the board on IoT-LAB so no direct access… :/

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Aug 28, 2018

We tried with @kYc0o on z1.

We successfully ran init_int 2 5 0 which sets an interrupt for the button both on master and this PR.

cladmi
cladmi approved these changes Aug 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK please squash

@jnohlgard jnohlgard force-pushed the pr/features-gpio-irq branch from f5adb94 to efc5f2a Compare August 29, 2018 06:53
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 29, 2018

Squashed

@kaspar030 kaspar030 merged commit 8ada7a8 into RIOT-OS:master Aug 29, 2018
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 29, 2018

Now we have "periph_gpio_irq" as module, but "gpio_init_int" as function. I suggest we rename the latter to "gpio_irq_init()", but provide a define (or static inline wrapper) for the old name.

@jnohlgard jnohlgard deleted the pr/features-gpio-irq branch August 29, 2018 08:49
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Aug 29, 2018

@kaspar030

Now we have "periph_gpio_irq" as module, but "gpio_init_int" as function. I suggest we rename the latter to "gpio_irq_init()", but provide a define (or static inline wrapper) for the old name.

Good idea! 👍

This was referenced Sep 20, 2018
Merged
Closed
Merged
Merged
Merged
@haukepetersen haukepetersen mentioned this pull request Oct 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cladmi cladmi cladmi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system 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: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2018.10

Development

Successfully merging this pull request may close these issues.

3 participants

@jnohlgard @cladmi @kaspar030