[En-Nut-Discussion] RFA_2: Nut/OS GPIO API

[Ulrich Prinz]

Fine that we come to a solution! Everything I do not comment, I do 
silently accept :)

Am 16.10.2012 22:54, schrieb Uwe Bonnes:
>
> For the reader's ease: I mark all line from the original test to be removed
> with '---' and my own replacement with '+++'
>
> The following rules apply to the Gpio API.
> 1 The main purpose of the Gpio API is to offer a common interface for
> target-independent bit-banged drivers, typically located in nut/dev.
> +++ Implementation can be as functions or macros
>
> 2 The Gpio API may be used by portable application samples, typically
> located in nut/app.
The GPIO API _must_ be used if an application shall be portable. The 
GPIO API _must_ be used if the program is intended for Nut/OS 
application examples in nut/app.
>
> 3 The Gpio API may be used by target-specific drivers, typically located
> --- in nut/app.
> +++ in nut/arch
> 3.1 If the driver requires special function, which are not available in
> the Gpio API, then this has to be implemented somewhere else. It is not
> --- acceptable to extend the Gpio API in a target-specific way
> +++ acceptable to extend the Gpio API in a target-specific way beside using more
> +++ flags in the function to set up the pin/port.
Applications using drivers that are using these extended flags must be 
located in nut/arch/app as they are not portable anymore.
(BTW we should think about an automatism to detect this situation)
>
> 4 The parameter bank specifies the GPIO group of bits (aka. port) and
> must be specified by index of type int. This fixed type allows callers
> to handle them in a common way during runtime.
> 4.1 The bank index base is target dependent.
> 4.2 Bank indexes may not be consecutively numbered.
> 4.3 If the implementation requires a different type, this may be done by
> typecasting the integer index type.
>
> 5 The parameter bit specifies the bit within a bank and must be
> specified by an index of type int.
> 5.1 The bit index base is 0.
> 5.2 Bit indexes are consecutively numbered.
>
> +++ 6 For port-wide operations, the parameter mask specifies the sum
> +++  of the bitvalues (_BV()) from the bit indexes involved.
>
> 7 Functions, which set GPIO output values, are of type void.
> 7.1 The only way to determine the validity of a bank or related bit is
> by configuring it.
>
> --- 6 Functions, which query pin input values, must return a value of type
> ----int.
> +++ 6 The functionality to query a pin or port is often implemented as macros
> +++ and returns a value of the same type as a port register access. If
> +++ implemented as function it must return a value of type int
Couldn't we set a return value of size_t? This would fit all 
architectures and even respect 16/32 bit switching if ARM is compiled in 
ARM or thumb mode.
>
> 8.1 A value of 0 is returned, if the pin state is low. Any other value
> indicates a high state.
> 8.1 If the requested bank or bit is not available on the target, the
> function result is undefined.
> +++ 8.2. With NUTDEBUG defined, is is usefull if a possible much slower
> +++ codepath is taken that checks for parameter validity.
>
> --- 7 Functions, which query pin group (aka port) values, must return a
> --- value of type unsigned int.
> --- 7.1 This is bad for 8-bitters, but done to preserve compatibility with
> --- the existing API.
Think about size_t again?
>
> 9 The Gpio API will guarantee the following pin configuration
> capabilities for all supported targets:
> 9.1 GPIO_CFG_INPUT to switch the pin to input mode
> 9.2 GPIO_CFG_OUTPUT to switch the pin to output mode
> +++ 9.3 GPIO_CFG_INIT_HIGH|LOW to switch on the pin in the requested state
> +++ when configured as output
> --- 8.3 If the requested capability is not available for the specified
> --- pin/port, the API function must return -1 to indicate an error.
> --- Otherwise 0 is returned.
>
> 10 The Gpio API may provide the following pin configuration capabilities:
> 10.1 GPIO_CFG_PULLUP to enable the internal pull-up
> 10.2 GPIO_CFG_MULTIDRIVE to enable open drain mode of an output,
> otherwise push-pull is assumed
> --- 9.3 If the requested capability is not available for the specified
> --- pin/port, the API function must return -1 to indicate an error.
> --- Otherwise 0 is returned.
> +++ 10.3. The return values is the flags of the requested capabilities with the not
> +++   implemented capabilities masked to 0

Sorry, but this an absolute no-go! A Pin may never be configured as 
push-pull output without extra user intervention. So _OUTPUT ever only 
sets open-collector mode!
The only way to set Push-Pull ist to set _OUTPUT | _PUSHPULL (or_PP if 
you like)
Again: The risk of killing something if outputs are wired against 
outputs and then setting one of them as pushpull is high. People should 
think twice if pushpull is needed, so they should write two words for 
the configuration too.

Next topic is the naming:
MULTIDRIVE is not an exact name. Any open-source/open-collector bus is 
capable of multiple masters driving the bus. What we try to tell is, 
that the pin is of high impedance in that case, so not driving high, nor 
low. This is _HIGHZ (high impedance)
Please delete _MULTIDRIVE it is misleading.
>
> 11 The Gpio API may offer to register an interrupt handler for a
> specific bit and to enable and disable this interrupt.
> 11.1 After registration, interrupts are disabled.
> 11.2 It is expected, that interrupts are triggered on any pin change.
> Level triggering or triggering on specific edges is not supported.
> 11.3 If the requested interrupt is not available on that target or for
> the specified pin/port, the API functions must return -1 to indicate an
> error. Otherwise 0 is returned.
>
12 Architecture specific gpio handlers must be located in 
nut/arch/[architecture] and have a special #define (TBD) in their 
exported header file.
12.1 architecture specific gpio.h may be included instead of the general 
gpio.h in any application that is not passed as an example to nut/app.
12.2 the above mentioned #define is automatically detected and may abort 
compilation if the wrong platform is selected or the code was passed to 
nut/app

> 13 In order to use this API, dev/gpio.h needs to be included only.
> Depending on the target, this header may include others.
This header may not include other headers that add non general, but 
platform specific functions or definitions.
( Come on, it is not too much work to add
#include <gpio.h>
#include <arch/my_very_special_gpio.h>
)
And either you write code, using the possibly slow but general API or 
you write code using the fast and specialized functions. So no need to 
include both of them in one code file.

> 13.1 The prototypes of all public API functions are:
>
>   extern uint32_t GpioPinConfigGet(int bank, int bit);
>   extern int GpioPinConfigSet(int bank, int bit, uint32_t flags);
>   extern int GpioPortConfigSet(int bank, unsigned int mask, uint32_t flags);
>
>   extern int GpioPinGet(int bank, int bit);
>   extern void GpioPinSet(int bank, int bit, int value);
>   extern void GpioPinSetLow(int bank, int bit);
>   extern void GpioPinSetHigh(int bank, int bit);
> +++ extern void GpioPinDrive(int bank, int bit);
> +++ extern void GpioPinRelease(int bank, int bit);
> +++ void GpioPinToggle(int bank, int bit);
>
>   extern unsigned int GpioPortGet(int bank);
> --- extern void GpioPortSet(int bank, unsigned int value);
> +++ extern void GpioPortSet(int bank, unsigned int mask, unsigned int value);
>   extern void GpioPortSetLow(int bank, unsigned int mask);
>   extern void GpioPortSetHigh(int bank, unsigned int mask);
> +++  extern void GpioPortDrive(int bank, unsigned int mask);
> +++  extern void GpioPortRelease(int bank, unsigned int mask);
>
>   extern int GpioRegisterIrqHandler(GPIO_SIGNAL * sig, int bit, void
> (*handler) (void *), void *arg);
>   extern int GpioIrqEnable(GPIO_SIGNAL * sig, int bit);
>   extern int GpioIrqDisable(GPIO_SIGNAL * sig, int bit);
>
No comment but only if you like to check if some results or parameters 
are better defined as size_t?

> +++ 13.2. If not provided in the platform code, dev/gpio.h provides
> +++   GpioPinDrive(), GpioPinRelease(), GpioPortDrive() and Gpio PortRelease()
> +++   as empty macros. Platform code can have its own implementation to
> +++   allow fast switching between push/pull and tristate without
> +++   calling Gpio{Pin|Port}ConfigSet()
>
> +++ 13.2. If not provided in the platform code, dev/gpio.h provides
> +++  void GpioPinToggle(int bank, int bit);
> +++  as a inlined combination of GpioPinGet() and GpioPinSet().
>
> 14 Currently there are several features, mainly pin configurations, used
> by code in the trunk, which is temporarily accepted.
> 14.1 These additional feature can be removed from the API only, if the
> same patch (revision) includes alternatives for those parts using these
> features. In no way it is acceptable to break any existing code in the
> trunk, just to please this RFA.
>
Thanks for the work, Harald. Hope my comments help.

Ulrich