drivers/gpio: changed periph driver interface #3095

haukepetersen · 2015-05-27T17:32:46Z

After talking a lot with @kaspar030, we decided to give the peripheral driver a little overhaul to make them more efficient and portable. In this effort, the GPIO driver is the first I touched.

The main motivation behind this is to allow for more efficient implementation through more control on a platform to platform level. The most important difference is, that CPUs have now to define the peripheral device types (i.e. gpio_t, spi_t, etc.). This allows platforms for example to use base register addresses as peripheral identifiers directly, making these very ugly (and huge) swith-case structures superfluous in many times.

Another important change for the GPIO driver: Now are all pins accessible and it is not needed anymore to define them in the periph_conf.h.

Summarized the changes tot he GPIO peripheral driver interface:

all CPU pins are accessible
gpio_t type must be defined by the CPU
gpio_init_out() and gpio_init_in() are merged into a single function
gpio_init_int() is renamed to gpio_init_exti() (I regularly read in instead of int -> this won't happy with this change...)
a new function for configuration the internal pin multiplexing, gpio_init_mux() for internal use only was added. This will make other peripheral drivers easier to implement/make their code easier to read

To make the migration to this change interface easier, I added a compatibility header, that still allows for the known gpio_t type. To make all existing code work with the new interface, these steps have to be done:

rename gpio_init_int() to gpio_init_exti()
join gpio_init_out and gpio_init_int()
add a file called periph_cpu.h to CPU/include/ and add `#include "periph/compat.h" to this file

The plan is to agree on the interface and make all CPUs work with the new interface through the compatiblity layer as first step. As second step we would need to improve all GPIO implementations, but this can then be done step-by-step.

Contents of this PR so far:

the changed gpio interface (drivers/include/periph/gpio.h)
the compatiblity header (drivers/include/periph/compat.h)
new improved, slimmer and better GPIO implementations for the samd21, nrf51822, and the stm32f4
the iot-lab_M3 (stm32f1) works already through the compat header
all device drivers are adjusted
a new GPIO test application

As you can see, the code is much much much more easy to read and way smaller (~1kB on the stm32f4) by offering more functionality...

I will adapt all effected CPUs once the new Interface is ACKed (and help with this is very welcome...).

haukepetersen · 2015-05-27T17:34:07Z

and by the way, the test application is no optimized at all, this should be done later if anyone feels the urge...

PeterKietzmann · 2015-05-27T21:25:59Z

Wouh. I see the motivation behind it, sure. But at the same time I'm wondering if we really "need" this. Anyway, improvement is never bad! But as this proposal requires a lot of work I'd suggest not to plan it for the release in June and just start to intensively work on it after all the network-stack excitement is over. I don't like to end in a situation where the compat-workaround just stays in the code. That is why I'd suggest to (i) clearly organize the progress of adaption and (ii) take care that all drivers will be adapted, ideally during a short time period. Also I'd like to hear multiple opinions here to "ensure" we are on the right track and won't see the need of such big adaption again in near-future. I am willing to help realizing the change but I guess this won't be in the next 1-2 weeks.

PeterKietzmann · 2015-05-27T21:26:42Z

PS: When writing my post I already had the potential changes for other peripheral drivers in mind

haukepetersen · 2015-05-27T21:29:39Z

Actually, the work for this PR is not so bad... Regarding the release, I would argue exactly in the different direction: This should definitely be in, as there are more and more new boards coming in and this PR gives them a good baseline. Also the part on the device drivers is not bad at all...

jnohlgard · 2015-05-27T21:39:47Z

I can imagine that me and @jfischer-phytec-iot might actually get to implement proper hardware CS support for the Kinetis SPI with this change, but I agree with @PeterKietzmann, this is going to be a lot of work and we need to ensure that we have a proper design for the new periph drivers.

We also need to set a hard deadline for deleting the compat layer to make sure we don't get stuck with two similar APIs and lots of confused users.

PeterKietzmann · 2015-05-27T21:55:52Z

Just for clarification: I didn't want to say that the work is bad or that your approach displeases me. My scepticism is more about organisation, workload and manpower. Let's see if there appear some other opinions. But in the end I definitively won't NACK this PR ;-)

kaspar030 · 2015-05-28T12:03:50Z

drivers/include/periph/gpio.h

 */
-int gpio_init_in(gpio_t dev, gpio_pp_t pullup);
+int gpio_init_exti(gpio_t pin, gpio_pp_t pullup, gpio_flank_t flank,


IMHO this name change is unrelated and with no improvement, but I don't care enough to see this as a showstopper.

true, but see argumentation above.

well, if there's no gpio_init_in the chance of mixing up is smaller. (I actually never mixed that up).

When starting to touch so many things I'd also argue pro gpio_init_exti. I think this semantic is used more often and prevents to be misread.

but "exti" is MCU specific... Most cortex use exti as the name, some don't. I just wrote a gpio implementation for a board that has 5 GPIO ports with an ISR line to MCU per port, and an additional "exti" line. So the function name feels somehow wrong when not using it for "exti".

The fact that most cortex platforms use this semantics was my pro-argument :-) . In the situation you described it I do agree with you that it feels wrong.

Does gpio_init_isr look less confusing to gpio_init_int?
edit
Maybe gpio_init_callback or _cb?

In this case I'd suggest to stay with the current name. ..._isr brings the same problem like ..._exti as you described above. .._callback appears too specific to me.

kaspar030 · 2015-05-28T12:13:15Z

I totally agree with Hauke, we need to get this in! This is already a +900 -2600 change, and it will get better as soon all platforms are integrated. Also, the use of all those #ifdef's in the implementations is horrible, we need to get rid of it before it get's more exposure.

kaspar030 · 2015-05-28T12:16:14Z

I don't like to end in a situation where the compat-workaround just stays in the code.

The "compat" workaround just wraps the completely useless enum defining GPIO_x to x. It basically moves it from an interface problem to an implementation problem. Still, all implementations have to be adapted to the new interface.

kaspar030 · 2015-05-28T12:20:34Z

drivers/include/periph/compat.h

@@ -0,0 +1,157 @@
+/*


Doesn't make gpio_compat.h more sense?

The idea was to use the same file to also include spi_t, uart_t and so on until they are optimized for a certain platform instead of introducing one file for each module...

Fine with me.

kaspar030 · 2015-06-01T12:06:54Z

@haukepetersen One thing I stumbled over when implementing a driver against this:
gpio.h defines the values for the flags, e.g., GPIO_DIR_IN or GPIO_NOPULL. When writing my driver, I had to basically write conversion tables, converting GPIO_DIR_IN (0) to the value of the specific MCU.

Maybe we can make the defines in gpio.h optional, as in, #ifndef CPU_DEFINES_GPIO_DIR_T ...?

PeterKietzmann · 2015-06-01T12:20:43Z

After looking into the implementation a bit deeper I see the benefit (not to say "need") more and more. Still I don't like the situation that some drivers are implemented in many different ways. Afaik there are still implementations with really old peripheral drivers which don't fit the current API. With this proposal there will be more-or-less old implementations that will be wrapped and new implementations that will "totally fit" the new approach. This could lead to confusion. That's why I tend to skip this for the release and spend "much" work in an organized way afterwards.
Anyway, you are in the majority :-) . I'd like to help where I can but probably not too much before the release.

jnohlgard · 2015-06-01T12:27:36Z

@PeterKietzmann I agree, let's postpone this to the next release after 2015.06

kaspar030 · 2015-06-01T12:31:05Z

@gebart Really? there will be more implementations using all those #ifdefs popping up. Using the compat header, the changes to every platform are very small (basically only merging gpio_init_in+out).

kaspar030 · 2015-06-01T12:33:44Z

With this proposal there will be more-or-less old implementations that will be wrapped and new implementations that will "totally fit" the new approach. This could lead to confusion.

Hm, not sure I agree. The beauty is that the new interface fits the old one naturally. If an implementation decides to use 0..n for the gpio pin descriptors (as all of them had to with the old interface), it still fits in, perfectly.
If we adopt this interface quickly, we won't get the Nth implementation of a lookup table...

jnohlgard · 2015-06-01T12:33:54Z

@kaspar030 I don't think we'll be able to review it in time:

Showing 32 changed files with 978 additions and 2,649 deletions.

The release date is set to Wednesday this week, two days from now, and this is quite a major overhaul.

kaspar030 · 2015-06-01T12:35:11Z

The release date is set to Wednesday this week, two days from now

You know what I like best from deadlines? The sound when they pass by: swoooooooooooooooooooshh ...

kaspar030 · 2015-06-01T12:53:25Z

The release date is set to Wednesday this week, two days from now

To be serious, that date was set by @OlegHahm months ago. As I understand the situation, we'll never release in two days. So if we aim for a release this month, with a feature freeze int the next 14 days, this should be reviewable.
I just had Hauke on the phone, he says the changes to an implementation are just a couple of lines...

jnohlgard · 2015-06-01T12:59:57Z

OK, I did not know that about the release date. I guess we should be able to finish this for release at the end of June.

haukepetersen · 2015-06-14T14:18:55Z

The xbee driver had trouble, as it was using the GPIO_NUMOF define as value for undefined GPIO pins. Now that not every platform does have this define anymore, the test failed to build. I introduced a new GPIO_UNDEF define, which makes it possible on all platforms to specify unused GPIO pins. This approach should be very much cleaner and straight forward.

kaspar030 · 2015-06-15T11:43:17Z

Travis is happy. ACK from my side. Let's merge before it get's stale. Hauke, wanna push the button?

haukepetersen · 2015-06-15T11:57:01Z

Yes!

drivers/gpio: changed periph driver interface

michz · 2015-08-14T12:22:42Z

Just to show appreciation:
Thank you VERY much for this change. This saves SO much time whenever I say "I want to use just another GPIO".

Big +1!

This is a rewrite of the Kinetis GPIO driver which follows the refactored API in [1]. Pins are specified using the GPIO_PIN(PORT_x, y) macro, e.g. GPIO_PIN(PORT_E, 25) for the PTE25 pin. The interrupt pin handling is now implemented as a linked list, this is more memory efficient, but with a minor variation in interrupt latency depending on in what order the pins were initialized at runtime. Because the linked list entries are taken from a shared pool, there is also the possibility of running out of available configuration slots, define the preprocessor macro GPIO_INT_POOL_SIZE in periph_conf.h if you need more than 16 pins configured for interrupts in the same application. [1]: RIOT-OS#3095

haukepetersen assigned jnohlgard May 27, 2015

haukepetersen added this to the Release 2015.06 milestone May 27, 2015

PeterKietzmann mentioned this pull request May 27, 2015

Arduino-mega2560 GPIO implementation #2799

Merged

kaspar030 reviewed May 28, 2015
View reviewed changes

jnohlgard mentioned this pull request May 29, 2015

Various warning fixes #3101

Merged

PeterKietzmann mentioned this pull request Jun 1, 2015

cpu/samd21: added peripheral PWM driver #3127

Merged

jnohlgard modified the milestones: Release NEXT MAJOR, Release 2015.06 Jun 1, 2015

haukepetersen added 9 commits June 14, 2015 16:12

drivers/gpio: added GPIO_UNDEF define

4a4ee44

drivers/xbee: use GPIO_UNDEF instead of GPIO_NUMOF

55e0362

tests/driver_xbee: s/GPIO_NUMOF/GPIO_UNDEF/

e0241df

cpu/atmega2560: removed empty gpio.c file

d748d4b

cpu/nrf51822: added custom GPIO_UNDEF define

c7f54c1

cpu/samd21: added custom GPIO_UNDEF define

138a6a7

cpu/saml21: added custom GPIO_UNDEF define

ac8ad54

cpu/stm32f3: added custom GPIO_UNDEF define

8c690f1

cpu/stm32f4: added custom GPIO_UNDEF define

71995e7

haukepetersen added a commit that referenced this pull request Jun 15, 2015

Merge pull request #3095 from haukepetersen/opt_gpio

350cfc7

drivers/gpio: changed periph driver interface

haukepetersen merged commit 350cfc7 into RIOT-OS:master Jun 15, 2015

haukepetersen deleted the opt_gpio branch June 15, 2015 11:57

This was referenced Jun 15, 2015

cpu: centralized startup code for cortexm CPUs #3155

Merged

board/Nucleo-f303: initial support for the Nucleo-f303 #3165

Merged

PeterKietzmann mentioned this pull request Jul 3, 2015

Investigate strange variable derefence pattern #2726

Closed

PeterKietzmann mentioned this pull request Jul 20, 2015

board: Add nrf6310 (nRF51) #2381

Merged

PeterKietzmann mentioned this pull request Jul 29, 2015

board: limifrog-v1 initial commit #3509

Merged

katezilla mentioned this pull request Aug 18, 2015

board: added board nucleo f401 #2601

Closed

jnohlgard mentioned this pull request Oct 4, 2015

kinetis_common: Refactor GPIO implementation #4042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/gpio: changed periph driver interface #3095

drivers/gpio: changed periph driver interface #3095

haukepetersen commented May 27, 2015

haukepetersen commented May 27, 2015

PeterKietzmann commented May 27, 2015

PeterKietzmann commented May 27, 2015

haukepetersen commented May 27, 2015

jnohlgard commented May 27, 2015

PeterKietzmann commented May 27, 2015

kaspar030 May 28, 2015

haukepetersen May 28, 2015

kaspar030 May 28, 2015

PeterKietzmann Jun 1, 2015

kaspar030 Jun 1, 2015

PeterKietzmann Jun 1, 2015

kaspar030 Jun 1, 2015

PeterKietzmann Jun 1, 2015

kaspar030 commented May 28, 2015

kaspar030 commented May 28, 2015

kaspar030 May 28, 2015

haukepetersen May 28, 2015

kaspar030 May 28, 2015

kaspar030 commented Jun 1, 2015

PeterKietzmann commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

haukepetersen commented Jun 14, 2015

kaspar030 commented Jun 15, 2015

haukepetersen commented Jun 15, 2015

michz commented Aug 14, 2015

drivers/gpio: changed periph driver interface #3095

drivers/gpio: changed periph driver interface #3095

Conversation

haukepetersen commented May 27, 2015

haukepetersen commented May 27, 2015

PeterKietzmann commented May 27, 2015

PeterKietzmann commented May 27, 2015

haukepetersen commented May 27, 2015

jnohlgard commented May 27, 2015

PeterKietzmann commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented May 28, 2015

kaspar030 commented May 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Jun 1, 2015

PeterKietzmann commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

kaspar030 commented Jun 1, 2015

jnohlgard commented Jun 1, 2015

haukepetersen commented Jun 14, 2015

kaspar030 commented Jun 15, 2015

haukepetersen commented Jun 15, 2015

michz commented Aug 14, 2015