Skip to content

periph/gpio: fix doc of periph_gpio_irq submodule#9978

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

periph/gpio: fix doc of periph_gpio_irq submodule#9978
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_periph_gpiofeaturescopeanddoc

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

Contribution description

The changes in #9845 seem oddly half-baked to me: it seems like the scope of the periph_gpio_irq submodule was put wrong: IMO the gpio_irq_enable/disable() functions should clearly also only be build in if the periph_gpio_irq submodule is included, right?! At least the adaption for the msp430 as included in #9845 does apply this already...

Further more, the changes in #9845 made the function documentation for gpio_init_int disappear from the generated documentation.

This PR adapts the scope for the irq submodule in the header file and re-adds the documentation.

Again: we need to be more careful when doing these kind of API changes. If not done cleanly, it always causes a lot of trouble down the line, and these things must be caught by the reviewers! So next time...

Testing procedure

Build doc to check for doxygen, buildtest should do the trick for the scope change.

Issues/PRs references

#9845

@cladmi cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Sep 20, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Sep 20, 2018

It looks like all cpu must be updated first to do a not too expensive transition.
Changing all cpu/periph/gpio.c to make this work to only after re-migrate them may be overkill.

I could merge the first commit updating the documentation directly though if you want to split.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

It looks like all cpu must be updated first to do a not too expensive transition.

yes, and that is exactly what I meant beforehand: just doing this half-done step in the API change, just because "I don't want to/don't have the time to do it right" leaves a mess for other people down the line. So either we do it right, or we might as well leave it be... I really don't like this hacky 'now a bit and the rest maybe later' approach.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

Seems like #9981 needs be merged before this one.

@kaspar030
Copy link
Copy Markdown
Contributor

Again: we need to be more careful when doing these kind of API changes. If not done cleanly, it always causes a lot of trouble down the line, and these things must be caught by the reviewers! So next time...

IMO you're exaggerating quite a bit. This was not some deep API change, just a move towards more modularization. It is perfectly acceptable to implement such a change within the build system, but only provide an implementation that makes use of it in just one (or even none) of the platforms.
Before #9845, all platforms implicitly provided the feature, though some through stubs. After #9845, the same, but one step further towards not providing it if it is not implemented. Code that was boken before was still broken after. Now it is possible to incrementally add this optimization to more platforms.

I think this was one of these small, incremental improvements that could've been stalled by premature aim for perfection. Luckily this time, it didn't.

Further more, the changes in #9845 made the function documentation for gpio_init_int disappear from the generated documentation.

That's unfortunate. Thanks for fixing. ;)

@jnohlgard
Copy link
Copy Markdown
Member

Why would #9981 be needed for a simple documentation change?

int gpio_init_int(gpio_t pin, gpio_mode_t mode, gpio_flank_t flank,
gpio_cb_t cb, void *arg);

#endif /* MODULE_PERIPH_GPIO_IRQ */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do this in a separate PR. The reason that I hid gpio_init_int only in #9845 was to let the user (and ci) to detect when the gpio IRQ module was used without being selected as not all gpio drivers are using conditional compilation.

@haukepetersen haukepetersen force-pushed the fix_periph_gpiofeaturescopeanddoc branch from 87275b7 to bd66184 Compare September 21, 2018 06:36
@haukepetersen haukepetersen changed the title periph/gpio: fix scope and doc of periph_gpio_irq submodule periph/gpio: fix doc of periph_gpio_irq submodule Sep 21, 2018
@haukepetersen
Copy link
Copy Markdown
Contributor Author

reduced the PR to the docfix, and will open another one for the scope fix.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Sep 21, 2018

Why would #9981 be needed for a simple documentation change?

Not for the doc, but for the scope fix... But not relevant for this PR anymore - yes, I got it :-)

IMO you're exaggerating quite a bit.

maybe a little bit, its just the frustration that kicks in when I see so many things only done half-baked. I definitely see the advantage in doing things in small incremental steps, and thats how it should be done. But for many tasks, people tend to leave it at that state and the adaption process stalls half way, always leading to some non-core users being utterly disappointed about the implementation state...

So I think for deep and/or complex changes this is perfectly fine. But for a simply change as targeted by
#9845, one can easily do all the adaption at once and be done with it (of course I don't mean putting it all in 1 PR...). For this particular change, the actual code changes to all 16 effected CPUs did take something around 10min... The actual work is to create all the clean commit and PRs :-)

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 missing function have been added in the generated docs. Tested ACK

@jnohlgard jnohlgard merged commit c4016fc into RIOT-OS:master Sep 21, 2018
@miri64 miri64 deleted the fix_periph_gpiofeaturescopeanddoc branch September 21, 2018 13:40
@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

Area: cpu Area: CPU/MCU ports 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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. 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.

5 participants