periph/gpio_ll: New Peripheral GPIO API #16787

maribu · 2021-08-31T08:19:56Z

Contribution description

This PR proposes a new peripheral GPIO API. It is orthogonal to #14602, which tries to expose GPIO-expanders under the same API. It doesn't intend to replace the pin-based periph/gpio API (but on the long term the intention is that this API is implemented on top of this API, to avoid having to maintain two implementations for each MCU).

Main differences:

low-level port-based GPIO access functions to allow fast and parallel access to GPIO pins
- this enables bit-banging when high throughput, parallel interfaces, strict timing, or any combination of these are required
higher-level configuration API using structs
- this allows adding new members to the structs without breaking the API (when choosing the default value to be zero, so that zero initialized structs will still show the same behavior)
- the also allows implementation specific members for internal use (e.g. to be used by peripheral drivers to set up GPIO pins and connect them to peripherals)
expose more features
- allow configuration of slew rates, Schmitt trigger, strength of pull resistors
- add pull to bus level (choosing pull up or pull down resistor depending on current bus level)
- add configuration of initial value of outputs
- add level triggered IRQs

Testing procedure

With tests/periph_gpio_ll a test application including a benchmark is provided. However, so for only for STM32 the API is implemented. It likely makes sense to not merge this prior to having at least three implementations ready.

Output of the benchmark part of the test on a Nucleo-F767ZI:

2021-08-31 09:56:23,161 # Benchmarking GPIO APIs
2021-08-31 09:56:23,163 # ============================
2021-08-31 09:56:23,164 # 
2021-08-31 09:56:23,168 # periph/gpio: Using gpio_set() and gpio_clear()
2021-08-31 09:56:23,172 # ----------------------------------------------
2021-08-31 09:56:23,264 # 100000 iterations took 89353 us
2021-08-31 09:56:23,268 # Two square waves pins at      1119156 Hz
2021-08-31 09:56:23,271 # ~193 CPU cycles per square wave period
2021-08-31 09:56:23,272 # :'-(
2021-08-31 09:56:23,272 # 
2021-08-31 09:56:23,277 # periph/gpio_ng: Using gpio_ng_set() and gpio_ng_clear()
2021-08-31 09:56:23,281 # -------------------------------------------------------
2021-08-31 09:56:23,287 # periph/gpio_ng: 100000 iterations took 928 us
2021-08-31 09:56:23,291 # Two square waves pins at    107758620 Hz
2021-08-31 09:56:23,293 # ~2 CPU cycles per square wave period
2021-08-31 09:56:23,294 # :-D
2021-08-31 09:56:23,294 # 
2021-08-31 09:56:23,297 # periph/gpio: Using 2x gpio_toggle()
2021-08-31 09:56:23,300 # -----------------------------------
2021-08-31 09:56:23,570 # 100000 iterations took 266668 us
2021-08-31 09:56:23,573 # Two square waves pins at       374998 Hz
2021-08-31 09:56:23,576 # ~577 CPU cycles per square wave period
2021-08-31 09:56:23,577 # :'-(
2021-08-31 09:56:23,577 # 
2021-08-31 09:56:23,581 # periph/gpio_ng: Using 2x gpio_ng_toggle()
2021-08-31 09:56:23,584 # -----------------------------------------
2021-08-31 09:56:23,597 # periph/gpio_ng: 100000 iterations took 7872 us
2021-08-31 09:56:23,600 # Two square waves pins at     12703252 Hz
2021-08-31 09:56:23,603 # ~17 CPU cycles per square wave period
2021-08-31 09:56:23,604 # :'-(
2021-08-31 09:56:23,604 # 
2021-08-31 09:56:23,607 # periph/gpio: Using 2x gpio_write()
2021-08-31 09:56:23,610 # ----------------------------------
2021-08-31 09:56:23,785 # 100000 iterations took 172224 us
2021-08-31 09:56:23,789 # Two square waves pins at       580639 Hz
2021-08-31 09:56:23,792 # ~373 CPU cycles per square wave period
2021-08-31 09:56:23,792 # :'-(
2021-08-31 09:56:23,793 # 
2021-08-31 09:56:23,796 # periph/gpio_ng: Using 2x gpio_ng_write()
2021-08-31 09:56:23,800 # ----------------------------------------
2021-08-31 09:56:23,805 # periph/gpio_ng: 100000 iterations took 927 us
2021-08-31 09:56:23,808 # Two square waves pins at    107874865 Hz
2021-08-31 09:56:23,811 # ~2 CPU cycles per square wave period
2021-08-31 09:56:23,812 # :-D
2021-08-31 09:56:23,812 # 
2021-08-31 09:56:23,813 # 
2021-08-31 09:56:23,814 # TEST SUCCEEDED

As seen, a square wave can be generated at have the clock rate (one CPU cycle for the high part, one CPU cycle for the low part), which is the theoretical option. The current API requires 373 CPU cycles / 193 CPU cycles instead of the 2 CPU cycles of the new API.

Issues/PRs references

Orthogonal to #14602 except for the port-wise interface, which #14602 also introduces.

Suggested Road-Map

Agree on the API
Add at least 2 more implementations, one non-Cortex-M one
Add a periph/gpio implementation in drivers/periph_common on top of periph/gpio_ll, so that on long term we don't need to maintain two APIs
Use the common periph/gpio implementation to hook in GPIO extenders periph/gpio: new expandable GPIO API #14602
Eventually, drop platform specific perpih/gpio implementations (and only keep the one implemented on top of periph/gpio_ll

cpu/stm32/Makefile.features

maribu · 2021-08-31T13:10:11Z

Test when run with LTO=1:

2021-08-31 15:06:53,958 # Benchmarking GPIO APIs
2021-08-31 15:06:53,961 # ============================
2021-08-31 15:06:53,961 # 
2021-08-31 15:06:53,965 # periph/gpio: Using gpio_set() and gpio_clear()
2021-08-31 15:06:53,969 # ----------------------------------------------
2021-08-31 15:06:53,973 # 100000 iterations took 1853 us
2021-08-31 15:06:53,977 # Two square waves pins at     53966540 Hz
2021-08-31 15:06:53,980 # ~4 CPU cycles per square wave period
2021-08-31 15:06:53,981 # :-)
2021-08-31 15:06:53,981 # 
2021-08-31 15:06:53,986 # periph/gpio_ng: Using gpio_ng_set() and gpio_ng_clear()
2021-08-31 15:06:53,990 # -------------------------------------------------------
2021-08-31 15:06:53,995 # periph/gpio_ng: 100000 iterations took 927 us
2021-08-31 15:06:53,999 # Two square waves pins at    107874865 Hz
2021-08-31 15:06:54,002 # ~2 CPU cycles per square wave period
2021-08-31 15:06:54,003 # :-D
2021-08-31 15:06:54,003 # 
2021-08-31 15:06:54,006 # periph/gpio: Using 2x gpio_toggle()
2021-08-31 15:06:54,009 # -----------------------------------
2021-08-31 15:06:54,156 # 100000 iterations took 143983 us
2021-08-31 15:06:54,159 # Two square waves pins at       694526 Hz
2021-08-31 15:06:54,163 # ~312 CPU cycles per square wave period
2021-08-31 15:06:54,164 # :'-(
2021-08-31 15:06:54,164 # 
2021-08-31 15:06:54,167 # periph/gpio_ng: Using 2x gpio_ng_toggle()
2021-08-31 15:06:54,171 # -----------------------------------------
2021-08-31 15:06:54,189 # periph/gpio_ng: 100000 iterations took 14353 us
2021-08-31 15:06:54,193 # Two square waves pins at      6967184 Hz
2021-08-31 15:06:54,196 # ~31 CPU cycles per square wave period
2021-08-31 15:06:54,197 # :'-(
2021-08-31 15:06:54,197 # 
2021-08-31 15:06:54,200 # periph/gpio: Using 2x gpio_write()
2021-08-31 15:06:54,203 # ----------------------------------
2021-08-31 15:06:54,219 # 100000 iterations took 13426 us
2021-08-31 15:06:54,223 # Two square waves pins at      7448234 Hz
2021-08-31 15:06:54,226 # ~29 CPU cycles per square wave period
2021-08-31 15:06:54,226 # :'-(
2021-08-31 15:06:54,226 # 
2021-08-31 15:06:54,230 # periph/gpio_ng: Using 2x gpio_ng_write()
2021-08-31 15:06:54,234 # ----------------------------------------
2021-08-31 15:06:54,239 # periph/gpio_ng: 100000 iterations took 927 us
2021-08-31 15:06:54,242 # Two square waves pins at    107874865 Hz
2021-08-31 15:06:54,245 # ~2 CPU cycles per square wave period
2021-08-31 15:06:54,246 # :-D

In two out of three cases, there still is a speed by one order of magnitude. In the remaining case, the speedup is due the use of a port oriented API (thus being able to set/clear both GPIOs in one function call instead of two).

chrysn · 2021-08-31T16:16:33Z

On the roadmap: Is the point where you'd like to merge this between 2 and 3, 3 and 4 or 4 and 5?

Personally, I'd be happy with this 2-to-3 or even after 1, if it's marked as an experimental API -- provided there's at least a rough demo on 2-4, but none of those need to be in mergable shape.

maribu · 2021-08-31T17:07:01Z

I'd say if there are PRs for a common periph/gpio implementation and PRs for two more implementations of periph/gpio_ng which at least pass all the tests this is ready to merge.

It might make sense to also see how the expendable API maps to this. I believe there might be some renaming needed to avoid name clashes. But this wouldn't too bad to do after it is in, since the API marked as unstable. But we could just as well wait for this before pushing this forward.

maribu · 2021-09-03T11:08:53Z

Rebased, squashed, and nRF5x backend implemented. It turned out that level triggered IRQs cannot be implemented for the nRF5x, since the approach used for the STM32 (which also don't natively support level triggered IRQs) relies on being able to trigger the GPIO IRQ from software.

gschorcht · 2021-10-25T16:44:35Z

@maribu Would it make sense to split this PR into a PR defining the API and PRs for each MCU implementation?

maribu · 2021-10-25T17:13:17Z

Absolutely! When it is ready for review (I still want at least a non-ARM implementation to make sure I don't overlook any portability issues) I will do so.

gschorcht · 2021-10-26T09:18:14Z

Absolutely! When it is ready for review (I still want at least a non-ARM implementation to make sure I don't overlook any portability issues) I will do so.

I guess that you want to implement it for AVR by yourself. I could offer to try an implementation for ESP32, although the current GPIO handling is not port based yet.

maribu · 2021-10-26T09:45:22Z

I could offer to try an implementation for ESP32

That would be nice :-)

gschorcht · 2021-10-26T10:03:47Z

I had a first short look. Following thoughts for short:

The gpio_ng API provides low-level access to GPIO pins. In step 3, you plan to implement a common gpio in drivers/periph_common based on gpio_ng. Then it will be a bit confusing to have an API gpio and an API gpio_ng. It might suggest that gpio_ng is a new API that might replace gpio in the future. In fact, gpio_ng will be the low-level API, while gpio will be the high-level API implemented on top of the low-level API. Wouldn't it be better to call the new API somethink like gpio_ll (GPIO low level)?
Even though the new API uses a port and a pin number or a pin mask, the platform implementations still rely on the scalar gpio_t type definition. This was the biggest challenge when we tried to integrate the access to expander ports seemlessly in the API. In step 4 you plan to use the common periph/gpio implementation to hook in GPIO expanders. Do you have any idea how to use it to distinguish between MCU ports and GPIO expander ports? In PR periph/gpio: new expandable GPIO API #14602 we therefore introduced a structured gpio_t type where the port member was either a register address in the case of an MCU port, a device pointer in the case of a GPIO expander, or simply a port number.
There is a new directory gpio_ng in drivers/include/periph which contains only one file irq.h. If IRQ functions have to be in a separate fiel, wouldn't it better just to call it gpio_ng_irq.h located in drivers/include/periph as other header files. A special directory containing only one file seems a bit strange.
The comments mostly have a line length of 100 characters. Does the coding convention with 80 characters line length not apply for the documentation? I thought the usual length should be 80 characters, the use of 100 characters should be an exception, even for documentation.

I know some of my questions are always with an eye towards making the integration of expanders as seamless as possible, which of course is always in my mind due to the history of our work on this topic 😉

maribu · 2021-10-26T12:38:56Z

Wouldn't it be better to call the new API gpio_ll (GPIO low level)?

Indeed, I'll rename it.

Even though the new API uses a port and a pin number or a pin mask, the platform implementations still rely on the scalar gpio_t type definition.

There only is a scalar gpio_port_t, no gpio_t in the API. (I do provide some functions to convert between gpio_t and gpio_port_t + pin number in both directions, though. Those will needed to be adapted, but that should be feasible.)

For a GPIO API on top that would provide again a gpio_t, I would do something like this:

typedef union {
    uint16_t bits;
    struct fields {
        uint16_t pin : 5;
        uint16_t port : 5;
        uint16_t device : 6;
    }
} gpio_t;

And a value of 0 for gpio_t::filed::device would identify the peripheral GPIOs, the remaining 63 possible values would be available to address port extenders.

I am aware that using this 16 bit bitmask would be less efficient (in terms of latency) than the current encoding for most implementations, but it often will be a more compact representation. IMO the use case of low latency cannot be reasonable addressed with a high level GPIO API (that does allow uniform access to both GPIO extenders and peripheral GPIOs) anyway, as the dispatching code adds precious CPU cycles. So the (relatively few) applications with that requirement will use gpio_ll to avoid this overhead anyway. So only the non-latency critical applications (which I believe is the vast majority) will use the high level GPIO API. As a result, it makes IMO sense to optimize for a compact and consistent representation by trading in a few CPU cycles.

If IRQ functions have to be in a separate fiel, wouldn't it better just to call it gpio_ng_irq.h located in drivers/include/periph as other header files.

There is no technical reason to have it a separate file. I occasionally see that students are confused by not being able to use the IRQ functions of periph/gpio.h despite having the periph_gpio used. They seem to assume 1:1 relation of features and headers. It is also easier to document that "all function header need feature " than adding a note to extra functions.

I will relocate and rename the header as suggested.

The comments mostly have a line length of 100 characters. Does the coding convention with 80 characters line length not apply for the documentation?

Apparently the coding convention was forgotten be be updated in #15793 and #16149 as it still says aim for 80 chars.

maribu · 2022-04-14T11:00:53Z

@gschorcht Could you confirm that I addressed all your requested changes? I think otherwise this PR is good to get in.

gschorcht · 2022-04-16T16:17:26Z

@gschorcht Could you confirm that I addressed all your requested changes? I think otherwise this PR is good to get in.

Since the PR for the API was merged with the PRs for the implementations (I would have preferred to keep them separate), there were a lot of files added for the implementation that I didn't check. Considering the API on its own, the PR seems fine to me.

gschorcht · 2022-04-16T16:19:49Z

Since it is a basic API change, I would prefere a double ACK.

chrysn · 2022-04-16T16:45:45Z

The API change has its two ACKs with yours and mine. I focused my review on the API and nRF parts.

Shall we try to raise someone familiar with STM and AVR for an extra review there? Or merge what had the best review?

Personally, I'd be OK with merging with the reviews we have as well -- it is an experimental API, setup mismatches should be caught rather easily (esp. in the tests), and much of the code is being taken over from the original GPIO code.

Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: chrysn <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]>

Co-authored-by: Gunar Schorcht <[email protected]>

maribu · 2022-04-22T06:39:50Z

Sorry for the delay. I split out the implementations into separate PRs as requested. Should be ready for merge now.

maribu · 2022-04-22T12:38:35Z

Thanks for the reviews!

chrysn · 2022-04-22T13:31:28Z

Thanks for all your work on this!

I'm very much looking forward to implementing (EFM32 probably, and maybe hifive1), wrapping (you know for what) and using (for WS2812B and similar LEDs) this.

gschorcht · 2022-08-31T11:07:10Z

@maribu Is file 22a1773#diff-98d165c8005ed5bc1a8eb27fb952e12a1707f5e5dd0a1aeb834eeef51da74635 what it seems to be, a core dump which was provided and merged by mistake?

maribu · 2022-08-31T11:13:52Z

Oh shit :/

chrysn · 2022-08-31T12:28:20Z

Sorry, this slipped by my review as well.

Oh shit :/

As far as these moments go, I'd put it in the "not great, not terrible" category: The 21MiB do contribute to the 250MiB checkout visibly, but once removed will only contribute around 2.5MiB compressed to the 150MiB git objects we have. (And those very concerned with data have, in recent git versions, good options around shallow checkouts). What remains is not having this too often. That means better care by revieers like me, but in terms of automation: Maybe have `core` added to .gitignore?

See-Also: RIOT-OS#16787 (comment)

maribu requested review from aabadie, cgundogan, DipSwitch, fjmolinas, jia200x, leandrolanzieri, MichelRottleuthner, MrKevinWeiss, smlng and vincent-d as code owners August 31, 2021 08:19

github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Aug 31, 2021

benpicco reviewed Aug 31, 2021

View reviewed changes

cpu/stm32/Makefile.features Outdated Show resolved Hide resolved

maribu force-pushed the periph/gpio_ng branch from 9663d1b to d323595 Compare September 3, 2021 11:07

maribu requested a review from haukepetersen as a code owner September 3, 2021 11:07

maribu force-pushed the periph/gpio_ng branch from d323595 to fed2a24 Compare October 14, 2021 10:46

maribu force-pushed the periph/gpio_ng branch from fed2a24 to 82f3869 Compare October 26, 2021 13:55

maribu force-pushed the periph/gpio_ng branch from 8380d28 to d199a78 Compare April 13, 2022 19:25

chrysn requested a review from gschorcht April 16, 2022 15:45

gschorcht approved these changes Apr 16, 2022

View reviewed changes

gschorcht added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Apr 16, 2022

This was referenced Apr 22, 2022

cpu/nrf5x_common: implement periph/gpio_ll{,_irq} #17980

Merged

cpu/stm32: Implement periph/gpio_ll{,_irq} except for STM32F1 #17981

Merged

cpu/atmega_common: implement periph/gpio_ll{,_irq} #17982

Merged

maribu and others added 3 commits April 22, 2022 08:39

drivers: Add periph/gpio_ll API

23855e1

Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: chrysn <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]>

tests: Add tests for periph/gpio_ll

22a1773

Co-authored-by: Gunar Schorcht <[email protected]>

tests/bench_periph_gpio_ll: add benchmark for periph/gpio_ll

28791c4

maribu force-pushed the periph/gpio_ng branch from d199a78 to 28791c4 Compare April 22, 2022 06:39

github-actions bot removed Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports labels Apr 22, 2022

benpicco merged commit a427d63 into RIOT-OS:master Apr 22, 2022

maribu deleted the periph/gpio_ng branch April 22, 2022 12:38

MrKevinWeiss mentioned this pull request Aug 11, 2022

.murdock: disable hash checks of kconfig/make #18423

Merged

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

maribu mentioned this pull request Aug 31, 2022

tests/periph_gpio_ll: drop core dump merged by accident #18541

Merged

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Aug 31, 2022

tests: Remove accidentally committed core dump

e76ec06

See-Also: RIOT-OS#16787 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

periph/gpio_ll: New Peripheral GPIO API #16787

periph/gpio_ll: New Peripheral GPIO API #16787

maribu commented Aug 31, 2021 •

edited

Loading

maribu commented Aug 31, 2021

chrysn commented Aug 31, 2021

maribu commented Aug 31, 2021

maribu commented Sep 3, 2021

gschorcht commented Oct 25, 2021

maribu commented Oct 25, 2021

gschorcht commented Oct 26, 2021

maribu commented Oct 26, 2021

gschorcht commented Oct 26, 2021 •

edited

Loading

maribu commented Oct 26, 2021

maribu commented Apr 14, 2022

gschorcht commented Apr 16, 2022

gschorcht commented Apr 16, 2022

chrysn commented Apr 16, 2022

maribu commented Apr 22, 2022

maribu commented Apr 22, 2022

chrysn commented Apr 22, 2022

gschorcht commented Aug 31, 2022

maribu commented Aug 31, 2022

chrysn commented Aug 31, 2022 via email

periph/gpio_ll: New Peripheral GPIO API #16787

periph/gpio_ll: New Peripheral GPIO API #16787

Conversation

maribu commented Aug 31, 2021 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

Suggested Road-Map

maribu commented Aug 31, 2021

chrysn commented Aug 31, 2021

maribu commented Aug 31, 2021

maribu commented Sep 3, 2021

gschorcht commented Oct 25, 2021

maribu commented Oct 25, 2021

gschorcht commented Oct 26, 2021

maribu commented Oct 26, 2021

gschorcht commented Oct 26, 2021 • edited Loading

maribu commented Oct 26, 2021

maribu commented Apr 14, 2022

gschorcht commented Apr 16, 2022

gschorcht commented Apr 16, 2022

chrysn commented Apr 16, 2022

maribu commented Apr 22, 2022

maribu commented Apr 22, 2022

chrysn commented Apr 22, 2022

gschorcht commented Aug 31, 2022

maribu commented Aug 31, 2022

chrysn commented Aug 31, 2022 via email

maribu commented Aug 31, 2021 •

edited

Loading

gschorcht commented Oct 26, 2021 •

edited

Loading