[PATCH] Proposal for new GPIO API and example implementation for STM32F4 BSP

[oss at c-mauderer.de]

Am 21.06.22 um 16:31 schrieb Duc Doan:
>     Using examples of working APIs is a good idea. But be careful not to
>     copy any code from that framework because the license (GPL) wouldn't be
>     compatible.
> 
> 
> Sure, I don't copy their code but only see what functions should be in a 
> GPIO API (like read, write, toggle, pinmode).
> 
>     Is set and reset the only state a pin can have? Or is that field
>     thought
>     to be extended with (for example) open drain or similar states if a
>     device supports that?
> 
> 
> I think it can be extended but I have only put in the most basic states 
> for now.
> 
> 
>     Beneath that: Not a fan of SET == 0. I think RESET == 0 would be better
>     because than you can use it as a boolean without conversion.
> 
> 
> You are right, that was my mistake. I have fixed it.
> 
>     Does that mean it is (for example) a GPIO controller? In that case
>     maybe
>     call it "rtems_gpio_ctrl_t". My first guess from the name would have
>     been that it is already a pin.
> 
> I have made changes in my newest patch, and rtems_gpio_t is now 
> essentially a pin.
> 
>     Same is true for the structures below: How would I initialize one of
>     these objects in the application?
> 
> 
> The example initialization of those structures can be found in my sample 
> blink code: 
> https://github.com/dtbpkmte/GSoC-2022-RTEMS-Sample-Apps/blob/main/RTEMS_Blink_API/src/init.c 
> <https://github.com/dtbpkmte/GSoC-2022-RTEMS-Sample-Apps/blob/main/RTEMS_Blink_API/src/init.c> 

OK. So every BSP that want's to use that API will have a different 
rtems_gpio_config_t (or every other structure) that is defined in (for 
example) bsp.h? The application has to know the details of the current 
BSP and initialize the structure accordingly.

That means that if I write an application that can run on an STM32 or 
alternatively on some RISC-V based CPU the API will be different. Not 
really portable.

I know that it is difficult in this case. On the one hand an API should 
be portable. On the other hand it should be able to handle very 
different hardware. And all that with the lowest possible overhead. In 
the end it will be some compromise.

Your goal is to either offer a better compromise than the API that is 
currently used for beagle and RPi. Or as an alternative: Don't try to go 
for an universal one and focus on your current device like a lot of the 
BSPs do. I would prefer a good universal API (better than the Beagle / 
RPi one (*)) but I know the problems so I could live with a device 
specific too. But note that this is my personal opinion. Others might 
have a different one.

(*) In case someone didn't see that (I think it was on discord): The 
Beagle / RPi API has two weaknesses in my point of view:

1. It mixes GPIO with Pinmuxing which is not true for all chips. For 
example the whole Freescale / NXP devices tend to have separate 
controllers for that. Having two APIs for that would be better in my 
point of view.

2. It is written with a quite fixed structure in mind. Controllers that 
don't have that structure need bigger workarounds which makes the API slow.

> . The definition of the structures are defined by BSP. Here are 
> STM32F4's definitions:
> /********************* CODE BEGINS ***************************************/
> struct rtems_gpio_t {
>      GPIO_TypeDef *port;
>      uint16_t pin;
> };
> 
> /**
>    * This structure should be initialized to 0
>    *
>    * This structure holds configuration options for STM32F4-specific
>    * GPIO. The 2 specific modes are Event and Alternate modes. Select
>    * a mode by setting the correct field (is_event_mode or 
> is_alternate_mode)
>    * to 1.
>    */
> struct rtems_gpio_config_t {
>      uint32_t speed;             /* Speed of the pin. Must be specified */
> 
>      uint32_t interrupt_mode;    /* The type of interrupt trigger of the 
> pin
>                                     Use if the pin is in Interrupt mode */
> 
>      uint32_t is_event_mode;     /* Sets to 1 if the pin is in event mode,
>                                     0 other wise.
>                                     Use if the pin is in Event mode */
>      uint32_t event_mode;        /* The type of event trigger of the pin
>                                     Use if the pin is in Event mode */
> 
>      uint32_t is_alternate_mode; /* Sets to 1 if the pin is in Alternate 
> mode,
>                                     0 other wise.
>                                     Use if the pin is in Alternate mode */
>      uint32_t alternate_mode;    /* Open drain or Push-pull mode
>                                     Use if the pin is in Alternate mode */
>      uint32_t alternate_fn;      /* The alternate function of the pin
>                                     Use if the pin is in Alternate mode */
> };
> /********************* CODE ENDS ***************************************/
> 
>     What would be done in that function?
> 
>     Why does the application has to implement it?
> 
>     Where would it be called?
> 
> 
> The initialize function is meant to perform setup code for the GPIO. For 
> example, for the STM32, it would be enabling the clocks of the ports. 
> The application needs to implement it if they want to use GPIO because 
> the peripheral needs initialization. They don't have to, though, if they 
> want to do that initialization somewhere else. This function is called 
> in bsp_start(). It is already implemented as a weak, empty function, so 
> there is no problem if the user doesn't implement it.

If you ask me: We have SYSINIT functions for this kind of 
initializations. If something should be done at about bsp_start, the 
user can register a function at RTEMS_SYSINIT_BSP_START.

> 
>     What would be possible error cases? Is one "UNSATISFIED" enough or
>     should it be able to return other errors too (like the return value
>     from
>     an I2C transfer because the pin is connected to some SPI-GPIO extender).
> 
> 
>   I think "UNSATISFIED" is enough because these are quite low-level. I 
> think the return values from more complicated things like I2C should be 
> handled in other functions (like the I2C API), and I want to keep the 
> GPIO thing portable. I am open to more discussion though, because I am 
> not sure I can think of all use cases of this GPIO API.

I think I haven't written clearly what I meant: Let's assume a I2C chip 
like the TCA9537 from TI (can be any other GPIO expander from any other 
vendor): You connect it to a I2C bus and control 4 GPIOs with it. It's a 
GPIO so a good GPIO API should be able to handle it.

In that case the GPIO API would do some I2C transfers under the hood if 
you switch or read a GPIO. So what happens if some error happens during 
one of these transfers. If the only error option is UNSATISFIED, the 
application can't distinguish the errors. It only knows something didn't 
work. It can't (for example) retry only on certain error cases like if 
the bus is busy.

Please also note that the I2C GPIO expander is one example where a BSP 
would have two completely different GPIO controllers: A number of 
integrated ones and a number of I2C ones. A generic API should be able 
to handle that case too.

Best regards

Christian

> 
>     If the write can fail, the read can also fail. Do we need an error
>     return value too?
> 
> Yes, I think that's a good idea. I changed it as below:
> 
> /**
>    * @brief Reads the digital value of a pin.
>    *
>    * @param[in] gpiox The GPIO object that owns the pin.
>    *
>    * @param[out] value The state of the pin.
>    *
>    * @retval RTEMS_SUCCESSFUL Pin succesfully read.
>    * @retval RTEMS_UNSATISFIED Could not read pin.
>    */
> extern rtems_status_code rtems_gpio_read_pin(rtems_gpio_t *gpiox, 
> rtems_gpio_pin_state *value);
> 
> Best,
> 
> Duc Doan