periph/gpio: enable API usage for expanders

[kYc0o]

This issue aims to track the progress on the definitions for the GPIO API extensions, and its implementations on the corresponding CPUs.

• API proposal (#9582)
• periph API interception and CPU support (periph/gpio: support for extension API #9860, periph/dac: support for DAC extension API #10512, periph/adc: support for ADC extension API  #10527, periph/pwm: support for PWM extension API #10533 )
• Redirection code implementation (periph/gpio: support for extension API (part 2) #9958, periph/dac: support for DAC extension API #10512, periph/adc: support for ADC extension API  #10527, periph/pwm: support for PWM extension API #10533)
• Tests (periph/gpio: support for extension API #9860 and periph/gpio: support for extension API (part 2) #9958)
• GPIO expander support (WIP drivers/pca95xx: support for I2C GPIO expanders #9054 and WIP drivers: add PCF857X I2C I/O expander driver #10430)

[kYc0o]

@ZetaR60 I think #9190 should be splitted into the proposed steps to ease the merging. Please feel free to propose new steps or modify the current ones if you feel so.

[ZetaR60]

Okay. Lets see how the API proposal in #9582 does and then we can better populate our TODO list.

[ZetaR60]

@kYc0o Updated the TODO list with #9860.

[ZetaR60]

@kYc0o Updated the TODO list with #9958.

[gschorcht]

Just to inform, I have used the GPIO extension API with two real expanders PCF8575 and PCF8574A.
Thanks to @ZetaR60, it works like a charm. If the driver defines an interface that corresponds to the peripheral GPIO interface, it is very easy to integrate the driver as GPIO extension. I was really impressed how easy it was.

[ZetaR60]

Thank you for your kind words @gschorcht !

[gschorcht]

@kYc0o Sorry, I missed that this tracking issue is explicity for GPIO when I was extending references with the PRs for ADC, DAC and PWM extension. Should we rename it to perhiph/* to track all types of peripheral extensions or should we open tracking issues for each type of extension API?

[kYc0o]

Hi @gschorcht ! No worries, as long as it's manageable to track from here I have nothing against the renaming.

[kaspar030]

I'd like to take another look at the redirection method.

I haven't taken an in-depth look at the API changes other than for gpio, so this applies mostly to gpio.

The reason why we want to change all the API's in the first place is that we need to support multiple "providers" of the functionality (I2C gpio extenders, bitbanging soft bus implementations, UART-over-USB, external ADC/DAC, ..). Ideally we manage to do that while

• keeping the current user-facing API (so we don't need to change all drivers)
• integrate it into the current API (so drivers can seamlessly use the new "providers")
• don't break existing applications. This includes the fact that the current implementation, when compiled with LTO, optimizes "gpio_set/get/..." to a single memory write if called with constant PIN parameters
• blah blah maintainability, code size, ram usage, ...

If I understand the current method correctly:

• the current periph code is renamed to gpio_*_cpu. IMO this is good. (although I'd prefer gpio_cpu_*, but let's talk concept first).

• the interception happens in wrapper functions named like the legacy "gpio_*" API (where else? so good.)

• If "gpio_ext" is not used, the redirection API directly calls "gpio_*_cpu" (good, optimization should make sure that existing applications lead to mostly the same binary code).

So far, so good.

• if gpio_ext_* is compiled in, should the pin number be above an MCU specific threshold, instead of calling the CPU functions, "gpio_*_redir" is called.
• gpio_*_redir calls gpio_ext_pin/dev to get driver and pin, then calls the corresponding (typical C OO) driver
• gpio_ext_dev get's the driver's "ops" struct by consulting a const static list

I propose the following:

• we rename the current gpio_t struct to "gpio_cpu_t"
• if gpio_ext is not compiled in, gpio_t is typedef'ed to gpio_cpu_t, and redirection is direct as above
• else we define "gpio_t as omething like this:

    typedef struct {
      gpio_ext_t *driver;
      union {
        unsigned pin;
        gpio_cpu_t cpu_pin;
    } gpio_t;

• in the dispatch functions, if "driver" is NULL we assume gpio_cpu_* and call those functions with the "cpu_pin" parameter
• else, we directly call "driver->get/set/...(gpio_t, ...)"

I think this way it might be possible to save quite a lot of the redirection code. It would remove the necessity for (MCU specific) "GPIO_EXT_THRESH". It would allow creation of portable GPIO_PIN(GPIO_EXT_A, pin-nr) macros. It would remove the need for a pin->driver mapping.

@ZetaR60 @gschorcht what do you think?

Another slightly more complicated option would be to have an MCU specific "gpio_is_ext(gpio_t)" function and use that instead of "driver==NULL" to do the redirection. That way gpio_*_cpu could make use of the driver field.

Another idea, if we introduce "gpio_ext", would be to go all the way and introduce "gpio_port_*", exposing whole ports even for the MCU internal gpio ports.

[ZetaR60]

@kaspar030 Looks like your summary of the work and the considerations involved is pretty spot-on.

If we are going the path of passing around pointers or structs to identify devices rather than ID numbers, then there are some big advantages that a more general transition would offer if we do some thinking about forward compatibility now. The extension interfaces would offer a limited trial of this method that could gradually be integrated more widely if it works out well. Not only could multiple providers of the same API be offered, but devices that currently have a different API for each type of underlying hardware could be unified into general APIs that only care about the broad details of how data is moved using the API.

An example use case was brought up in our discussions last year about xtimer and ztimer, and how it might be better to implement stacks of timers rather than a monolithic timer system like xtimer. Currently we have periph/timer, periph/rtt, periph/rtc, xtimer, and also ztimer at some point. If the method of accessing all of these was to pass around a pointer/struct with the driver and device settings, then all of these systems could be different providers of the same API. We would have the overhead of passing around this information, but it would be a huge simplification to the current hodge-podge of individual APIs.

Some other examples that come to mind:

• block storage APIs like periph/flashpage, SD cards, and similar could share an API.
• byte storage APIs like periph/eeprom could share an API with the various kinds of external memory.
• some of the numerous analog level read APIs and write APIs might be able to share a generic API.
• probably numerous others

I don't propose any kind of decision on whether to transition, but rather I propose that if this is a potentially valuable transition and we are starting to pass around more complex identification information anyway, that we plan ahead and make the extension interface a trial of these methods in anticipation of the possibility of more general API transitions in the future.

I suggest that most APIs could pass around pointers to a struct containing the following information (with slight variations per-API):

• pointer to driver struct
• pointer to device descriptor (void pointer requiring a cast to implementation-specific struct)
• identifier for the specific pin / line / position / timer / etc
• parameters for proper access of the API (such as the number of pins / lines / positions / timers / etc.)

Passing all of this using a pointer would also have the advantage of making the API not care where the information actually came from (rather than retrieving it from an array), so it would facilitate dynamic loading of new devices.

[gschorcht]

@kaspar030

the current periph code is renamed to "gpio_cpu". IMO this is good. (although I'd prefer "gpio_cpu", ...

? 😉

[gschorcht]

Some comments from me.

• integrate it into the current API (so drivers can seamlessly use the new "providers")

I would really like a concept that integrates expanders seamlessly and completely invisible for users. From the users point of view an ideal integration would allow to access CPU as well expander pins using the GPIO_PIN(x, y) macro without the necessity to distinguish between CPU and expander pins by using either GPIO_PIN or GPIO_EXT_PIN.

I think this way it might be possible to save quite a lot of the redirection code.* It would remove the necessity for (MCU specific) "GPIO_EXT_THRESH". It would allow creation of portable GPIO_PIN(GPIO_EXT_A, pin-nr) macros. It would remove the need for a pin->driver mapping.

This would be really great.

• else we define "gpio_t as omething like this:

 typedef struct {
   gpio_ext_t *driver;
   union {
     unsigned pin;
     gpio_cpu_t cpu_pin;
 } gpio_t;

It might be difficult to initialize these data structures for all CPU and expander pins without having tables of these structures for all pins that are initialized during the initialization of the CPU/expander. Such tables would require additional memory.

[gschorcht]

@ZetaR60 The question is how do we continue. Would you start a new try or should we discuss it a bit more. Code could be a good mean for further discussion.

[kaspar030]

? wink

Hm, I didn't escape properly. The difference is gpio_*_cpu vs gpio_cpu_*. I've edited the original post.

[kaspar030]

I would really like a concept that integrates expanders seamlessly and completely invisible for users.

Yes, me too.

Let's assume that in the application's periph_conf.h (or however we're organizing configuration in the future) there's a defined expander struct, and it has been given a speaking name like #define GPIO_EXT_0. What we want to be able to do is using both GPIO_PIN(PORT_A, 0) (referencing a CPU internal pin) and GPIO_PIN(GPIO_EXT_0, 0) for specifying the expander port.

I think with the current gpio implementations that might be difficult, as the GPIO_PIN() macros turn port, pin arguments into some CPU specific single value (or multiple). But that should be easy to change so the CPU uses the pointer+pin-number. We'd have to check for code size implications, though.

It might be difficult to initialize these data structures for all CPU and expander pins without having tables of these structures for all pins that are initialized during the initialization of the CPU/expander. Such tables would require additional memory.

Do they have to be initialized?

I'd assume an expander to have:

• some general private information (how is it connected, how configured
• some per-pin configuration (pin configured, interrupt, in/out, ...)

IMO gpio_t should only serve as reference (not contain state information), as it might be copied around into e.g., a driver's param struct. It should be left to the expander's internal implementation how to save state.

[kaspar030]

Currently we have periph/timer, periph/rtt, periph/rtc, xtimer, and also ztimer at some point. If the method of accessing all of these was to pass around a pointer/struct with the driver and device settings, then all of these systems could be different providers of the same API. We would have the overhead of passing around this information, but it would be a huge simplification to the current hodge-podge of individual APIs.

Let's not try to find "one API to rule them all(tm)". It is impossible to get minimal code size, ram requirements, runtime performance and full flexibility at the same time. The timer's are a nice example. I think it is totally valid to have a slim-as-possible abstraction on the (hardware)-timers (what periph/timer is now, plus rtc/rtc) and then create something like xtimer (and soon ztimer) on top.
What I mean that tradeoffs have to be made anyways, and instead of "too bulky for real-time" or "too inflexible for having seamless time syncronization", I'd rather have "well there are two interfaces doing the same thing but with different tradeoffs".

It is not like when periph was designed, we didn't think about making the API C-object-oriented as discussed here. Mostly code size and massively reduced optimization when using pointers lead to what we have now. Now the tradeoffs are biting because it is not (easily) possible to have extenders or software implementations in addition to the hardware ones. But it is possible to write a reasonably fast and portable bit-banging I2C using the current gpio interface. With full OO and pointer-to-ops-struct, that wouldn't be.

[gschorcht]

Do they have to be initialized?

Maybe I missed something. As I understood, the GPIO_PIN macro should lead to the struct

typedef struct {
   gpio_ext_t *driver;
   union {
     unsigned pin;
     gpio_cpu_t cpu_pin;
 } gpio_t;

that is used for the API functions gpio_init a.s.o. For CPU pins, driver has to be NULL and cpu_pin has to be assigned properly. For expander pins, the driver has point to the expander device and pin has to be assign properly to.

[kaspar030]

Maybe I missed something. As I understood, the GPIO_PIN macro should lead to the struct

Ah, I see. Yes.

Maybe in the beginning sth like this could work:

• rename GPIO_PIN to GPIO_CPU_PIN

#define GPIO_EXT_PIN(port, pin) (gpio_t){ .port=port, .pin=pin}
#define GPIO_PIN(port, pin) GPIO_EXT_PIN(NULL, GPIO_CPU_PIN(port, pin))

We'd have two defines, but this should keep current code still working. If we can transition all gpio_cpu_* to use the gpio_t field for port and pin, at some point, we can turn the two into one.

[gschorcht]

As long as nothing has been changed, it should be easy as to

• rename GPIO_PIN definitions to GPIO_CPU_PIN in all cpu/*/periph/gpio drivers as you suggested
• replace gpio_t by gpio_cpu_t in all cpu/*/periph/gpio drivers
• rename gpio_* cpu implementations to gpio_cpu_*
• declare the gpio_t struct in periph/gpio.h similar to what you suggested

   #if MODULE_EXTEND_GPIO || DOXYGEN
   typedef struct {
       gpio_ext_dev_t *dev;
       union {
           unsigned ext_pin;
           gpio_cpu_t cpu_pin;
   } gpio_t;
   #else 
   typedef gpio_cpu_t gpio_t;
   #endif  
  

  This would ensure that an application which uses the gpio_t type for any reason will compile as long as it does't use expanders.
• define in periph/gpio.h a default macro

  #ifndef GPIO_EXT_0
  #define GPIO_EXT_0     0xff /* there shoul be no CPU with more than 255 ports ;-) */
  #endif
  

• define in periph/gpio.h a macro (unconditionally)

  #if MODULE_EXTEND_GPIO || DOXYGEN
  #define GPIO_PIN(x, y)      (x < GPIO_EXT_0 ? GPIO_CPU_PIN(x, y) \
                                              : GPIO_EXT_PIN(x, y))
  #else 
  #define GPIO_PIN(x, y)
  #endif      
  

• define a macro in periph/gpio.h a default macro as you suggested

  #if MODULE_EXTEND_GPIO
  #define GPIO_EXT_PIN(dev, pin)      (gpio_t) { .dev=dev, .pin=pin }
  #endif      
  

• change gpio_* function in periph/gpio.h to

   static inline int gpio_init(gpio_t pin, gpio_mode_t mode)
   {
   #ifdef MODULE_EXTEND_GPIO
       if (pin.device) {
           return pin.dev.gpio_ext_init(pin.ext_pin, mode);
       }
   #endif 
   
   #ifdef MODULE_PERIPH_GPIO
       return gpio_cpu_init(pin.cpu_pin, mode);
   #else
       return -1;
   #endif
   }
  

I would do it in the first step since it will become difficult to use sed s/.../.../g for renaming/replacing once some of changes have been added as first step.

[ZetaR60]

I like the ideas discussed so far. I am starting work on a v3 PR.

I suggest using the following definition:

typedef struct {
#if MODULE_EXTEND_GPIO || DOXYGEN
    gpio_ext_dev_t *dev;
#endif
    gpio_num_t num;
} gpio_t;

instead of the proposed:

#if MODULE_EXTEND_GPIO || DOXYGEN
typedef struct {
    gpio_ext_dev_t *dev;
    gpio_num_t num;
} gpio_t;
#else
typedef gpio_num_t gpio_t;
#endif

because it will cause trouble with the syntax for pin usage changing depending on whether the extension interface is enabled or not. It will require changing pin access to pin.num instead (when GPIO_PIN is not being used), but that would need to be changed anyway (and to something more complex).

[gschorcht]

@ZetaR60

I like the ideas discussed so far. I am starting work on a v3 PR.

Great. I'm curious about it. I like the @kaspar030's ideas too.

[gschorcht]

@ZetaR60 Is everything OK with you and how is going with v3? 😉 I'm really curious.

[ZetaR60]

There are some difficulties with changing gpio_t into a structure. The main difficulty is that C does not allow the equality operator to be used for structs. I really wish C did something like "two structs are equal if all of their members are equal", which would make the transition much easier.

I see two potential solutions for the struct issue:

• introduce an inline function to test for equality
• cast the struct to an integer for use by the public API, and then cast it back to a struct internally

I think both can probably be made to produce the same compiled binary when the extension interface is disabled as before the API change. A function based equality check has the advantage of being the more "correct" way of doing things, but there are a lot of instances of equality checks to change and there is a potential for a mistake to make a regression with each change. A cast based method would allow all of the existing code to remain the same, but would require some care to prevent padding and such from interfering with its operation. I think the cast method is more favorable as the exact mechanics can be hidden within GPIO_PIN and GPIO_EXT_PIN.

Thoughts?

[gschorcht]

@ZetaR60 Are you still working on the API? After having almost no time the last 6 months, I will have some time in next weeks. If you don't have time, I could make a try.

[gschorcht]

@kaspar030 I would like to make some progress with the extension API for two reasons. First, it would be a great improvement of RIOT to have a concept for expanders. And second, I have already written a number of expander drivers that do not depend on the extension APIs but are already ready to work with extension APIs PR #10430, #10688, #10556, #10518.

Since @ZetaR60 does not seem to have much time at the moment, I tried the concept you suggested a little bit.

I really like the idea of ​​seamless integration that always uses the macro GPIO_PIN (port, pin), independent on whether it's a CPU pin or an expander pin. he definition of a structure, as you suggested along with the tricky macros, seems indeed possible.

However, there are a number of problems:

1. As @ZetaR60 already said, comparisons like the following are no longer possible. Unfortunatly, we have a lot of such comparisons in several parts of our code.
   

   RIOT/drivers/periph_common/spi.c

   Lines 31 to 38 in c225636

   	int spi_init_cs(spi_t bus, spi_cs_t cs)
   	{
   	if (bus >= SPI_NUMOF) {
   	return SPI_NODEV;
   	}
   	if ((cs == SPI_CS_UNDEF) || (cs == GPIO_UNDEF)) {
   	return SPI_NOCS;
   	}

   
   What should we do? Should we really replace all these comparisons of scalars by a function or a macro that compares the GPIO structures? It is also a pitfall for the user. If he uses comparisons that work when extension API is not compiled in, he will not realize that it doesn't work with extension APIs.

2. For most platforms, headers are included in following order:

   • board.h includes gpio.h at the beginning
   • gpio.h includes periph_cpu.h before type definitions to be able to override default types

   Since gpio_t is required in most periph_cpu.h to define peripheral configuration structures, most platforms define their own gpio_t in periph_cpu.h.

   We would have to rename these gpio_t to gpio_cpu_t. But, also all configuration structures would then have to be defined using gpio_cpu_t instead of gpio_t. All pins in these configurations are then just of type gpio_cpu_t and the macros used there would then be GPIO_CPU_PIN and GPIO_CPU_UNDEF. Even more, all peripheral drivers like I2C, SPI, ... would have to use gpio_cpu_t and the according macros.

   So I don't see any difference to that we had before. It's just renaming and we would have to touch hundreds of files.

   [UPDATE] This problem could be reduced probably if gpio.h would include something like gpio_arch.h in which platform specific definitions could be made if needed. This would allow to override default definitions and to include complete gpio.h wherever gpio_t is needed.

Shouldn't we better follow @ZetaR60's approach of redirection and driver list #9860 and #9958?

Also with the @ZetaR60 approach, we could implement a generic macro GPIO_PIN (port, pin), which redirects to GPIO_CPU_PIN (port, pin) or GPIO_EXT_PIN (port, pin), and not on the basis of a MCU threshold, but simply on the port which starts at a number that is high enough for external devices.

[gschorcht]

@ZetaR60 May I adopt your PRs #9860 and #9958 to continue your work?

[gschorcht]

@kaspar030 After spending some time in recent weeks changing a lot of code to try your suggested approach, I am at a point where I would be very grateful for your help 😎 Maybe you have an idea how to solve the problem or get around it.

What we have are CPU specific or default GPIO definitions that usually look like:

#define GPIO_CPU_PIN(x, y)  (x << 4 | y)
#define GPIO_CPU_UNDEF      ((gpio_cpu_t)(UINT_MAX))

enum {
    PORT_A = 0,
    PORT_B = 1,
};

typedef unsigned gpio_cpu_t;

In a first step, I renamed all gpio_* and GPIO_* definitions to gpio_cpu_* and GPIO_CPU_*, respectively. To keep the existing code with a lot of binary operations working, CPU peripheral drivers in cpu/*/periph and peripheral configurations in bords/*/periph_conf*.h use only gpio_cpu_* and GPIO_CPU_*. From my point of view, it makes sense that CPU peripheral drivers and peripheral configurations use only CPU GPIOs.

According to your proposed approach and the work of @ZetaR60 ZetaR60, I then defined the GPIO extension API as follows:

#define GPIO_EXT_PORTS      (0x80)           /**< default definition, can be overriden by CPU */
#define GPIO_EXT_PIN(x, y)  ((gpio_t){ .dev = &gpio_ext[x - GPIO_EXT_PORTS], .pin.ext_pin = y })

#define GPIO_UNDEF          { .dev = 0, .pin.cpu_pin = GPIO_CPU_UNDEF }
#define GPIO_PIN(x, y)      ((x < GPIO_EXT_PORTS) ? (gpio_t){ .dev=0, .pin.cpu_pin=GPIO_CPU_PIN(x, y) } \
                                                  : GPIO_EXT_PIN(x, y))

enum {
    PORT_EXT_A = (GPIO_EXT_PORTS + 0),
    PORT_EXT_B = (GPIO_EXT_PORTS + 1),
};

typedef uint8_t gpio_ext_t;

typedef struct gpio_ext_driver {
    /** extension driver functions */
} gpio_ext_driver_t;

typedef struct gpio_ext_dev {
    const gpio_ext_driver_t *driver;    /**< pointer to extension device driver */
    void *dev;                          /**< pointer to extension device descriptor */
} gpio_ext_dev_t;

typedef struct {
    const gpio_ext_dev_t *dev;  /**< extension device, 0 in case of CPU pin */
    union {
        gpio_cpu_t cpu_pin;     /**< pin number in case of CPU GPIO pin */
        gpio_ext_t ext_pin;     /**< pin number in case of GPIO exension device */
    } pin;
} gpio_t;

where GPIO_EXT_PORTS is any port number greater than the highest CPU port number and the application defines the extender configuration, for example:

static const gpio_ext_dev_t gpio_ext[] =
{
    {
        .driver = &extend_gpio_driver1,
        .dev = (void *)0xbeef,
    },
    {
        .driver = &extend_gpio_driver2,
        .dev = (void *)0xf00,
    },
};

The goal was to use GPIOs of CPU and extenders in same way in board configurations, e.g., for LED pins:

#define LED0_PIN = GPIO_PIN(PORT_A, 1);
#define LED1_PIN = GPIO_PIN(PORT_EXT_B, 2);

or driver configurations.

The problem is the GPIO_PIN macro:

#define GPIO_PIN(x, y)  ((x < GPIO_EXT_PORTS) ? (gpio_t){ .dev=0, .pin.cpu_pin=GPIO_CPU_PIN(x, y) } \
                                              : GPIO_EXT_PIN(x, y))

While it works perfectly for constant gpio_t variables like

static const gpio_t pin1 = GPIO_PIN(PORT_A, 1);
static const gpio_t pin2 = GPIO_PIN(PORT_EXT_A, 2);

it doesn't work for constant structures with gpio_t members like

typedef struct {
    gpio_t pin1;
    gpio_t pin2;
} foo_config_t;

static const foo_config_t cfg = {
    .pin1 = GPIO_PIN(PORT_B, 3),
    .pin2 = GPIO_PIN(PORT_EXT_B, 4)
};

because the compiler complains that the initializer isn't a constant. However, we have a lot of driver configuration structures with gpio_t members.

Changing the code in all cpu/*/periph/*.c to use a structure for gpio_t with port and pin fields instead of scalar types to have only one GPIO_PIN macro, is probably unrealistic and to error-prone.

Do you have any idea?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.