search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.83k stars 1.97k forks source link

periph/gpio: enable API usage for expanders #9690

Open kYc0o opened 5 years ago

kYc0o commented 5 years ago

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

kYc0o commented 5 years ago

@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 commented 5 years ago

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

ZetaR60 commented 5 years ago

@kYc0o Updated the TODO list with #9860.

ZetaR60 commented 5 years ago

@kYc0o Updated the TODO list with #9958.

gschorcht commented 5 years ago

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 commented 5 years ago

Thank you for your kind words @gschorcht !

gschorcht commented 5 years ago

@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 commented 5 years ago

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

kaspar030 commented 5 years ago

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

If I understand the current method correctly:

So far, so good.

I propose the following:

    typedef struct {
      gpio_ext_t *driver;
      union {
        unsigned pin;
        gpio_cpu_t cpu_pin;
    } 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(gpiot)" 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 "gpioport*", exposing whole ports even for the MCU internal gpio ports.

ZetaR60 commented 5 years ago

@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:

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):

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 commented 5 years ago

@kaspar030

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

? :wink:

gschorcht commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

? wink

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

kaspar030 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

#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 gpiocpu* to use the gpio_t field for port and pin, at some point, we can turn the two into one.

gschorcht commented 5 years ago

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

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

ZetaR60 commented 5 years ago

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:

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 commented 4 years ago

@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 commented 4 years ago

@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. https://github.com/RIOT-OS/RIOT/blob/c225636966df5e17113b0bee9bc5f306911df6e4/drivers/periph_common/spi.c#L31-L38 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 commented 4 years ago

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

gschorcht commented 4 years ago

@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 :sunglasses: 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[bot] commented 4 years ago

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.