periph/gpio: fix doc of periph_gpio_irq submodule#9978
periph/gpio: fix doc of periph_gpio_irq submodule#9978jnohlgard merged 1 commit intoRIOT-OS:masterfrom
periph_gpio_irq submodule#9978Conversation
|
It looks like all cpu must be updated first to do a not too expensive transition. I could merge the first commit updating the documentation directly though if you want to split. |
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. |
|
Seems like #9981 needs be merged before this one. |
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. 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.
That's unfortunate. Thanks for fixing. ;) |
|
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 */ |
There was a problem hiding this comment.
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.
87275b7 to
bd66184
Compare
periph_gpio_irq submoduleperiph_gpio_irq submodule
|
reduced the PR to the docfix, and will open another one for the scope fix. |
Not for the doc, but for the scope fix... But not relevant for this PR anymore - yes, I got it :-)
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 |
jnohlgard
left a comment
There was a problem hiding this comment.
The missing function have been added in the generated docs. Tested ACK
Contribution description
The changes in #9845 seem oddly half-baked to me: it seems like the scope of the
periph_gpio_irqsubmodule was put wrong: IMO thegpio_irq_enable/disable()functions should clearly also only be build in if theperiph_gpio_irqsubmodule is included, right?! At least the adaption for themsp430as included in #9845 does apply this already...Further more, the changes in #9845 made the function documentation for
gpio_init_intdisappear from the generated documentation.This PR adapts the scope for the
irqsubmodule 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