Re: [PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
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 00..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 [...] ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
Hello Christian, On Mon, 2022-08-01 at 20:29 +0200, o...@c-mauderer.de wrote: > Hello Duc, > > Am 01.08.22 um 05:50 schrieb Duc Doan: > > Hello Christian, > > > > On Sat, 2022-07-30 at 17:50 +0200, o...@c-mauderer.de wrote: > > > Hello Duc, > > > > > > Am 24.07.22 um 14:01 schrieb Duc Doan: > > > > --- > > > > bsps/include/bsp/gpio2.h | 528 > > > > > > > > bsps/include/bsp/periph_api.h | 142 +++ > > > > bsps/shared/dev/gpio/gpio.c | 212 ++ > > > > bsps/shared/dev/periph_api/periph_api.c | 101 + > > > > spec/build/bsps/obj.yml | 4 +- > > > > 5 files changed, 986 insertions(+), 1 deletion(-) > > > > create mode 100644 bsps/include/bsp/gpio2.h > > > > create mode 100644 bsps/include/bsp/periph_api.h > > > > create mode 100644 bsps/shared/dev/gpio/gpio.c > > > > create mode 100644 bsps/shared/dev/periph_api/periph_api.c > > > > > > > > diff --git a/bsps/include/bsp/gpio2.h > > > > b/bsps/include/bsp/gpio2.h > > > > > > I'm not really happy with the "2" in the name. But at the moment > > > I > > > don't > > > have a much better idea either. But if you call it "gpio2", you > > > should > > > call the C files "gpio2" too. Maybe "dev/gpio/gpio.h" similar to > > > the > > > "dev/i2c/i2c.h"? > > > > > > > I will rename the C file to "gpio2.c" for now. > > > > > > new file mode 100644 > > > > index 00..9cb1c720ab > > > > --- /dev/null > > > > +++ b/bsps/include/bsp/gpio2.h > > > > @@ -0,0 +1,528 @@ > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > + > > > > > > Your file is missing some general doxygen group information. Take > > > a > > > look > > > at for example bsps/include/ofw/ofw.h. > > > > > > > Thanks for the reminder, I will add that. > > > > > > +/* > > > > + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com) > > > > + * > > > > + * Redistribution and use in source and binary forms, with or > > > > without > > > > + * modification, are permitted provided that the following > > > > conditions > > > > + * are met: > > > > + * 1. Redistributions of source code must retain the above > > > > copyright > > > > + * notice, this list of conditions and the following > > > > disclaimer. > > > > + * 2. Redistributions in binary form must reproduce the above > > > > copyright > > > > + * notice, this list of conditions and the following > > > > disclaimer > > > > in the > > > > + * documentation and/or other materials provided with the > > > > distribution. > > > > + * > > > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND > > > > CONTRIBUTORS "AS IS" > > > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > > > LIMITED TO, THE > > > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A > > > > PARTICULAR PURPOSE > > > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR > > > > CONTRIBUTORS BE > > > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > > > > EXEMPLARY, OR > > > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > > > > PROCUREMENT OF > > > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > > > > PROFITS; OR > > > > BUSINESS > > > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > > > > LIABILITY, > > > > WHETHER IN > > > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE > > > > OR > > > > OTHERWISE) > > > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > > > > ADVISED OF THE > > > > + * POSSIBILITY OF SUCH DAMAGE. > > > > + */ > > > > + > > > > +#ifndef LIBBSP_BSP_GPIO2_H > > > > +#define LIBBSP_BSP_GPIO2_H > > > > + > > > > +#include > > > > +#include > > > > + > > > > +/** > > > > + * Configure the maximum number of GPIO controllers used in > > > > + * a application. > > > > + * > > > > + * The macro CONFIGURE_GPIO_MAXIMUM_CONTROLLERS can be > > > > + * defined in application code. If it is not defined, > > > > + * it will default to BSP_GPIO_NUM_CONTROLLERS. If BSP's > > > > + * number of controllers is not defined, it will default > > > > + * to 1. > > > > + */ > > > > +#ifndef CONFIGURE_GPIO_MAXIMUM_CONTROLLERS > > > > > > How do you manage that the CONFIGURE_GPIO_MAXIMUM_CONTROLLERS > > > from > > > the > > > application is visible here? > > > > > > > I forgot to remove this CONFIGURE_GPIO_MAXIMUM_CONTROLLERS thing. > > At > > first I wanted to do something like confdef but couldn't, so I > > changed > > to use BSP_GPIO_NUM_CONTROLLERS, which is a build option. I will > > delete > > that code. However, if you have a document about how confdef works, > > it > > would be helpful for me and I may be able to make that > > CONFIGURE_GPIO_MAXIMUM_CONTROLLERS work. > > I haven't checked since the new build system either. The official way > is > still to just add them to confdefs.h similar like all other options. > Not > sure whether that is documented somewhere. The better mut much more > difficult way is to
Re: [PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
Hello Christian,
On Sat, 2022-07-30 at 17:50 +0200, o...@c-mauderer.de wrote:
> Hello Duc,
>
> Am 24.07.22 um 14:01 schrieb Duc Doan:
> > ---
> > bsps/include/bsp/gpio2.h | 528
> >
> > bsps/include/bsp/periph_api.h | 142 +++
> > bsps/shared/dev/gpio/gpio.c | 212 ++
> > bsps/shared/dev/periph_api/periph_api.c | 101 +
> > spec/build/bsps/obj.yml | 4 +-
> > 5 files changed, 986 insertions(+), 1 deletion(-)
> > create mode 100644 bsps/include/bsp/gpio2.h
> > create mode 100644 bsps/include/bsp/periph_api.h
> > create mode 100644 bsps/shared/dev/gpio/gpio.c
> > create mode 100644 bsps/shared/dev/periph_api/periph_api.c
> >
> > diff --git a/bsps/include/bsp/gpio2.h b/bsps/include/bsp/gpio2.h
>
> I'm not really happy with the "2" in the name. But at the moment I
> don't
> have a much better idea either. But if you call it "gpio2", you
> should
> call the C files "gpio2" too. Maybe "dev/gpio/gpio.h" similar to the
> "dev/i2c/i2c.h"?
>
I will rename the C file to "gpio2.c" for now.
> > new file mode 100644
> > index 00..9cb1c720ab
> > --- /dev/null
> > +++ b/bsps/include/bsp/gpio2.h
> > @@ -0,0 +1,528 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +
>
> Your file is missing some general doxygen group information. Take a
> look
> at for example bsps/include/ofw/ofw.h.
>
Thanks for the reminder, I will add that.
> > +/*
> > + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
> > + *
> > + * Redistribution and use in source and binary forms, with or
> > without
> > + * modification, are permitted provided that the following
> > conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above
> > copyright
> > + * notice, this list of conditions and the following
> > disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above
> > copyright
> > + * notice, this list of conditions and the following disclaimer
> > in the
> > + * documentation and/or other materials provided with the
> > distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > PARTICULAR PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> > CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> > EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> > PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> > BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#ifndef LIBBSP_BSP_GPIO2_H
> > +#define LIBBSP_BSP_GPIO2_H
> > +
> > +#include
> > +#include
> > +
> > +/**
> > + * Configure the maximum number of GPIO controllers used in
> > + * a application.
> > + *
> > + * The macro CONFIGURE_GPIO_MAXIMUM_CONTROLLERS can be
> > + * defined in application code. If it is not defined,
> > + * it will default to BSP_GPIO_NUM_CONTROLLERS. If BSP's
> > + * number of controllers is not defined, it will default
> > + * to 1.
> > + */
> > +#ifndef CONFIGURE_GPIO_MAXIMUM_CONTROLLERS
>
> How do you manage that the CONFIGURE_GPIO_MAXIMUM_CONTROLLERS from
> the
> application is visible here?
>
I forgot to remove this CONFIGURE_GPIO_MAXIMUM_CONTROLLERS thing. At
first I wanted to do something like confdef but couldn't, so I changed
to use BSP_GPIO_NUM_CONTROLLERS, which is a build option. I will delete
that code. However, if you have a document about how confdef works, it
would be helpful for me and I may be able to make that
CONFIGURE_GPIO_MAXIMUM_CONTROLLERS work.
> > +
> > +#ifndef BSP_GPIO_NUM_CONTROLLERS
> > +#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS 1
> > +#else
> > +#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS
> > BSP_GPIO_NUM_CONTROLLERS
> > +#endif /* BSP_GPIO_NUM_CONTROLLERS */
> > +
> > +#endif /* CONFIGURE_GPIO_MAXIMUM_CONTROLLERS */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> > +/**
> > + * @brief Macro to initialize rtems_gpio.
> > + *
> > + * @param gpioh pointer to GPIO handlers
> > + */
> > +#define RTEMS_GPIO_BUILD_BASE(_gpio_handlers) \
> > + (rtems_gpio) { .virtual_pin = 0, \
> > + .gpio_handlers = ( _gpio_handlers ), \
> > + .api = NULL \
> > + };
> > +
> > +/**
> > + * @name GPIO data structures
> > + *
> > + * @{
> > + */
> > +
> > +/**
> > + * @brief GPIO bit set and reset enumeration.
> >
Re: [PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
Hello Duc,
Am 24.07.22 um 14:01 schrieb Duc Doan:
---
bsps/include/bsp/gpio2.h| 528
bsps/include/bsp/periph_api.h | 142 +++
bsps/shared/dev/gpio/gpio.c | 212 ++
bsps/shared/dev/periph_api/periph_api.c | 101 +
spec/build/bsps/obj.yml | 4 +-
5 files changed, 986 insertions(+), 1 deletion(-)
create mode 100644 bsps/include/bsp/gpio2.h
create mode 100644 bsps/include/bsp/periph_api.h
create mode 100644 bsps/shared/dev/gpio/gpio.c
create mode 100644 bsps/shared/dev/periph_api/periph_api.c
diff --git a/bsps/include/bsp/gpio2.h b/bsps/include/bsp/gpio2.h
I'm not really happy with the "2" in the name. But at the moment I don't
have a much better idea either. But if you call it "gpio2", you should
call the C files "gpio2" too. Maybe "dev/gpio/gpio.h" similar to the
"dev/i2c/i2c.h"?
new file mode 100644
index 00..9cb1c720ab
--- /dev/null
+++ b/bsps/include/bsp/gpio2.h
@@ -0,0 +1,528 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+
Your file is missing some general doxygen group information. Take a look
at for example bsps/include/ofw/ofw.h.
+/*
+ * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef LIBBSP_BSP_GPIO2_H
+#define LIBBSP_BSP_GPIO2_H
+
+#include
+#include
+
+/**
+ * Configure the maximum number of GPIO controllers used in
+ * a application.
+ *
+ * The macro CONFIGURE_GPIO_MAXIMUM_CONTROLLERS can be
+ * defined in application code. If it is not defined,
+ * it will default to BSP_GPIO_NUM_CONTROLLERS. If BSP's
+ * number of controllers is not defined, it will default
+ * to 1.
+ */
+#ifndef CONFIGURE_GPIO_MAXIMUM_CONTROLLERS
How do you manage that the CONFIGURE_GPIO_MAXIMUM_CONTROLLERS from the
application is visible here?
+
+#ifndef BSP_GPIO_NUM_CONTROLLERS
+#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS 1
+#else
+#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS BSP_GPIO_NUM_CONTROLLERS
+#endif /* BSP_GPIO_NUM_CONTROLLERS */
+
+#endif /* CONFIGURE_GPIO_MAXIMUM_CONTROLLERS */
+
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
+/**
+ * @brief Macro to initialize rtems_gpio.
+ *
+ * @param gpioh pointer to GPIO handlers
+ */
+#define RTEMS_GPIO_BUILD_BASE(_gpio_handlers) \
+(rtems_gpio) { .virtual_pin = 0,\
+ .gpio_handlers = ( _gpio_handlers ), \
+ .api = NULL \
+};
+
+/**
+ * @name GPIO data structures
+ *
+ * @{
+ */
+
+/**
+ * @brief GPIO bit set and reset enumeration.
+ */
+typedef enum {
+RTEMS_GPIO_PIN_RESET = 0,
+RTEMS_GPIO_PIN_SET = 1
+} rtems_gpio_pin_state;
+
+/**
+ * @brief GPIO pin modes.
+ */
+typedef enum {
+RTEMS_GPIO_PINMODE_OUTPUT = 0,
+RTEMS_GPIO_PINMODE_OUTPUT_PP = 0,
+RTEMS_GPIO_PINMODE_OUTPUT_OD = 1,
+RTEMS_GPIO_PINMODE_INPUT = 2,
+RTEMS_GPIO_PINMODE_ANALOG = 3,
+RTEMS_GPIO_PINMODE_BSP_SPECIFIC = 4
Maybe the BSP_SPECIFIC should start at a high, fixed offest so that
adding new general values is simpler. Something like
RTEMS_GPIO_PINMODE_BSP_SPECIFIC = 100,
A BSP can than use 100, 101, 102, ... for it's specific functions. If
someone later adds a generic RTEMS_GPIO_PINMODE_FOO = 4, it won't result
in any conflicts.
+} rtems_gpio_pin_mode;
+
+/**
+ * @brief GPIO pull resistor configuration. Defines pull-up or
+ *pull-down activation.
+ */
+typedef enum {
+RTEMS_GPIO_NOPULL,
+RTEMS_GPIO_PULLUP,
+RTEMS_GPIO_PULLDOWN
Maybe add a "BSP_SPECIFIC" here too. I know of controllers that can
activate different
