Skip to content

cpu/mips: remove periph_gpio_irq feature and stub#9974

Merged
cladmi merged 2 commits intoRIOT-OS:masterfrom
haukepetersen:fix_mips_gpioirqdisen
Sep 20, 2018
Merged

cpu/mips: remove periph_gpio_irq feature and stub#9974
cladmi merged 2 commits intoRIOT-OS:masterfrom
haukepetersen:fix_mips_gpioirqdisen

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

Contribution description

The gpio driver for the pic32 does not implement external interrupts (feature ´periph_gpio_irq`), it had simply a stub function for it. With #9035 the clean way to handle this is however to remove the stub and the feature declaration from the boards using that CPU, so applications that depend no this GPIO feature do not event build for those boards anymore...

Testing procedure

build #9035 with and without this PR

Issues/PRs references

Stumbled upon this while finalizing #9035
#9845 broke it :-)

@haukepetersen haukepetersen added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Sep 20, 2018
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 they were just added everywhere using sed.

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.

Ooops, just checked, the hifive1 board uses fe310 cpu that defines gpio_irq_enable.

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.

Untested ACK. Like @cladmi said, these feature tags were added categorically using sed.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

these feature tags were added categorically using sed.

Even if so, I would expect at least the reviewers to notice these kind of things! Its just very annoying to have these simple fails all over the place all the time...

@haukepetersen
Copy link
Copy Markdown
Contributor Author

removed the patch for the hifive1 board, as it uses a different cpu...

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Sep 20, 2018

The tests/periph_gpio uses gpio_init_int so detects already if the function is there. So adding gpio_irq everywhere made sense except for the boards in BOARD_INSUFFICIENT_MEMORY maybe.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

The tests/periph_gpio uses gpio_init_int so detects already if the function is there. So adding gpio_irq everywhere made sense except for the boards in BOARD_INSUFFICIENT_MEMORY.

Sorry, but that is bullshit (and very unclean).

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Sep 20, 2018

Murdock checks that gpio_init_int is declared for all boards that are compiled in tests/periph_gpio that is what I checked when reviewing the initial PR.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Sep 20, 2018

To keep a working git history, the commits should be in the inverted order with disabling the feature before removing the code.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

Yes, but this is not about making Murdock pass somehow, but about using the feature defs as they are meant to: declare, if a certain feature is available on a certain platform!

I don't mean to be rude, its just that I think we need to be more careful when reviewing/refactoring/applying API changes, as we tend to be way to sloppy when doing these things... So next time lets look at these kind of changes more carefully!

@haukepetersen
Copy link
Copy Markdown
Contributor Author

To keep a working git history

Last time I checked the convention was, that not every commit needs to be able to build... But I can adapt this.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

changed order of commits.

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 waiting for murdock.

Github is still displaying them in the old order but it is a known issue with github displaying by 'author' date, checking locally displays the correct commit order.

@jnohlgard
Copy link
Copy Markdown
Member

@haukepetersen it helps if every commit can build properly when doing bisections. It may not always be desirable due to the resulting size of a single commit in certain cases, but try to keep the build working for each commit if possible.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

yepp, makes sense.

@cladmi cladmi merged commit c9f3b6d into RIOT-OS:master Sep 20, 2018
@haukepetersen haukepetersen deleted the fix_mips_gpioirqdisen branch September 20, 2018 13:18
@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

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants