[PATCH v5 2/4] bsps: New GPIO API & peripherals API framework

[oss at c-mauderer.de]

Hello Duc,

Am 02.08.22 um 09:17 schrieb Duc Doan:
[...]
>>>>> diff --git a/bsps/include/bsp/periph_api.h
>>>>> b/bsps/include/bsp/periph_api.h
>>>>> new file mode 100644
>>>>> index 0000000000..fb02b701dc
>>>>> --- /dev/null
>>>>> +++ b/bsps/include/bsp/periph_api.h
>>>>
>>>> Isn't it an API just because you add it here. Is it really
>>>> necessary
>>>> to
>>>> add the "API" in the name?
>>>>
>>>
>>> I was thinking that this periph_api is the base for other APIs, so
>>> I
>>> added "api" in the name.
>>>
>>>> I'm not entirely sure yet whether this API is really something
>>>> separate
>>>> from your GPIO API. The "gpio_start" calls the "periph_api_start"
>>>> and
>>>> the periph_api functions use gpio internal structures. I think I
>>>> would
>>>> just add these few functions to the GPIO API.
>>>>
>>>
>>> That is also an option. The reason I separated them is that I
>>> thought
>>> these additional functions should be somehow separated from the
>>> basic
>>> GPIO to avoid making GPIO API too complicated. Also, this API is
>>> mostly
>>> for adding new peripheral APIs and not targetting user application.
>>> Users only need to use one function, set_api().
>>
>> Please note that in our case a user can and often will add his own
>> drivers to an application too. Not all drivers are in the BSP. Some
>> applications need specialy tuned drivers and therefor will bring
>> their
>> own ones.
>>
>>>
>>>> I'm still not convinced that this is necessary at all. A
>>>> peripheral
>>>> has
>>>> to know it's pins. But the pin doesn't have to know anything
>>>> about
>>>> the
>>>> connected peripheral. So why do we need that? At the moment it
>>>> seems
>>>> to
>>>> add mainly some complexity and uses some memory.
>>>>
>>>
>>> Logically, a pin doesn't have to know about the connected
>>> peripheral.
>>> However, this newly added API is just a way to make things easier
>>> for
>>> users.
>>>
>>> Without this API, for each peripheral, users need to create new
>>> objects
>>> that hold information about both the GPIO pins and the handlers of
>>> that
>>> peripheral. They would have to maintain those objects all the time
>>> during the use of that peripheral. If users want to change the
>>> functionality, they have to create new objects of that peripheral
>>> type.
>>
>> If the functionallity does not change, the peripheral can just know
>> it's
>> pin and the user just has to know and handle the peripheral. Only
>> during
>> the initialization he has to init the pin first and the peripheral
>> afterwards. But that's the case with your API too, isn't it?
>>
>> If the functionality of a pin changes (which is a really rare use
>> case)
>> the user will need two APIs. So if I understand your code correctly,
>> he
>> would have to:
>>
>> - Init GPIO pin.
>> - Add API for example for ADC.
>> - Use that pin.
>> - Remove API for ADC.
>> - Add API for example for DAC.
>> - Use the pin.
>> - Remove API for DAC.
>> ...
>>
>> So he has to switch in or out the API. For this switching he
>> eitherhas
>> to provide memory for the ADC / DAC API or the memory is dynamically
>> allocated on every switch. I think I would rather keep multiple
>> pointers
>> in my application instead of allocating something every time.
>>
>>>
>>> This API avoids that by reusing the already-existing GPIO pin
>>> objects.
>>> Users only need a single GPIO object for a pin for all operations,
>>> be
>>> it basic GPIO or additional peripherals. This creates simplicity
>>> for
>>> user application at the cost of added memory (one additional
>>> pointer
>>> member, which is not much in my opinion).
>>>
>>
>> For a single use pin that doesn't have to switch function, the user
>> needs only one pointer. The one to the object of the driver with the
>> function he uses. The pin can be a part of that object.
>>
>>> By the way, this newly added API is mostly targeting peripherals
>>> that
>>> require a single pin like ADC or DAC.
>>
>> That doesn't make it better. Now you added complexity for very
>> special
>> cases ;-)
>>
> 
> Hmm your reasoning makes sense. Should I change so that ADC contains
> the pin as a member (and thus remove the peripheral API)?

 From my point of view, that would be better. You maybe could think 
about hinting on that point in the meeting tomorrow. Maybe someone else 
will add an oppinion too.

Best regards

Christian

> 
> Best,
> 
> Duc
> 
>>>
[...]