cpu/native: add gpio-mock #20431
cpu/native: add gpio-mock #20431
Conversation
github-actions
bot
added
Platform: native
There was a problem hiding this comment.
This looks good on a high-level and line-by-line level, setting the appropriate review flags.
Allowing everything in gpio_mock makes sense -- there is a strategy of doing a mock where things would be strict, but then they'd need to be consistently so (and right now this is more an "we drive with zero strength and the GPIOs are wired to GND" implementation without any checking whatsoever).
Some remarks inline.
cpu/native/include/periph_cpu.h
Outdated
| @@ -63,6 +59,10 @@ extern "C" { | |||
| */ | |||
| #define GPIO_PIN(port, pin) (gpio_t)((port << GPIO_PORT_SHIFT) | pin) | |||
|
|
|||
| /* GPIO configuration only if the module is available (=Linux) */ | |||
There was a problem hiding this comment.
I'm unsure this is the right approach here:
The mock implementation is distinct from the native implementation, so reusing the same settings for both is confusing.
What is wrong with GPIO_PIN(1,3) == GPIO_PIN(0, 3)? The hardware that is being mocked could just only have one port. Even with the current defines, GPIO_PIN(0, 1<<24) would be identical to GPIO_PIN(1, 0); aliasing is only avoided if the mock module either established some boundaries or chose to use a two-int sized (eg. struct) type as its gpio_t (which I'd welcome as a test to see whether any piece of the code tacitly assumes that gpio_t can be cast around).
There was a problem hiding this comment.
Each GPIO "identifier" should be unique.
If I want to mock STM32 GPIOs for instance, GPIO_PIN(PORT_A, 1) is not the same than GPIO_PIN(PORT_B, 1). If we want to mock GPIOs, it should be close to real world.
But it is hard to define some boundaries as it is hard to predict what will be mocked.
Moreover, if you want to perform a switch case, if the macro returns twice same value it leads to such kind of error:
pf_positional_actuators.cpp: In function ‘void* cogip::pf::actuators::positional_actuators::_gpio_handling_thread(void*)’:
pf_positional_actuators.cpp:218:9: error: duplicate case value
218 | case pin_limit_switch_left_arm_lift_bottom:
| ^~~~
pf_positional_actuators.cpp:202:9: note: previously used here
202 | case pin_limit_switch_central_lift_bottom:
| ^~~~
pf_positional_actuators.cpp:233:9: error: duplicate case value
233 | case pin_sensor_pump_left:
| ^~~~
pf_positional_actuators.cpp:214:9: note: previously used here
214 | case pin_limit_switch_right_arm_lift_top:
| ^~~~There was a problem hiding this comment.
The GPIO pin and port arguments, though theoretically generic, are both integer, and thus the only way to avoid equality of valid GPIOs are to either decide to subset them at check time (say, use 16 bits for pin and 16 bit for port number, and enforce that no other pins are valid), or to use a larger than default gpio_t.
To some extent, the bug here is that arbitrary pins are created in the first place -- any time input to GPIO_PIN is provided, the application needs to know that that pin exists (or rely on the verify function, which in the mock case is not accurately representing that there is really just one port). Usually the pins are configurable per board, and for native they would just all be mapped to the 0 port.
If the gpio_mock module is supposed to be used for arbitrary pin setup emulation (as is suggested by the presence of the weak attributes; a comment similar to the one on the i2c side could clarify that), I think that the best route would be to typedef struct { unsigned int port; unsigned in pin; bool is_undef; } gpio_t; and #define GPIO_PIN accordingly (possibly through a static inline function because C is odd about where struct literals can be used).
Alternatively, copy what is done in the gpio_linux case and pick some grouping of the bits what is for registers and what is for pins. But in that case, that choice (including the concrete shift) becomes part of the public API for creating the per-application gpio_… mock functions, and they need to pick their arguments apart again using the same shift.
Semi-relatedly, I'm not sure doing a switch-case on pins is futureproof -- there exists an gpio_is_equal function that may change its implementation in the future.
There was a problem hiding this comment.
@chrysn I now understand what you mean and I agree. 👍
However, it's not entirely straightforward, because if I change gpio_t to something other (struct, pointer) than the default uint32_t type, it results in a build error if I mix with a pcf875x gpio expander for example.
I think it's not as simple as I thought at first and it would take more time to get it right.
And it is always possible to define my own GPIO_PIN macro in the context of the application or my own board, in periph_conf.h for instance.
I therefore propose to abandon this commit. Are you OK with that ?
There was a problem hiding this comment.
uh pcf857x should not (ab)use gpio_t like that. It expects a plain integer (index of the GPIO), using gpio_t here will just lead to chaos.
There was a problem hiding this comment.
/** conversion of (port x : pin y) to a pin number */
#define PCF857X_GPIO_PIN(x,y) ((gpio_t)((x << 3) | y))All the pcf857x API is modeled on periph_gpio. As mainly using stm32 and native, planets were aligned but following @chrysn implementation, I confirm, it leads to chaos in my build due to pcf857x. 😆
RIOT/drivers/include/pcf857x.h
Line 487 in f048714
It seems more safe to drop this commit for now, just let me know.
There was a problem hiding this comment.
Ben fixed this already in #20435, so the option of doing the struct would be on the table again. Abandoning this is fine, but if you happen to have already some code around where you tried the struct approach, I'd love to see it here.
There was a problem hiding this comment.
Done @chrysn . You can check the last commit.
$ tests/periph/gpio/bin/native/tests_gpio.elf
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.
main(): This is RIOT! (Version: 2024.04-devel-332-g721330-native_fixes)
GPIO peripheral driver test
In this test, pins are specified by integer port and pin numbers.
So if your platform has a pin PA01, it will be port=0 and pin=1,
PC14 would be port=2 and pin=14 etc.
NOTE: make sure the values you use exist on your platform! The
behavior for not existing ports/pins is not defined!
> init_out 0 1
init_out 0 1
> set 0 1
set 0 1
> read 0 1
read 0 1
GPIO_PIN(0.01) is HIGH
> read 0 0
read 0 0
GPIO_PIN(0.00) is LOW
> toggle 0 0
toggle 0 0
> read 0 0
read 0 0
GPIO_PIN(0.00) is HIGH
> toggle 0 0
toggle 0 0
> read 0 0
read 0 0
GPIO_PIN(0.00) is LOW
> bench 0 2
bench 0 2
GPIO driver run-time performance benchmark
nop loop: 92us --- 0.000us per call --- 1086956521 calls per sec
gpio_set: 346us --- 0.003us per call --- 289017341 calls per sec
gpio_clear: 346us --- 0.003us per call --- 289017341 calls per sec
gpio_toggle: 387us --- 0.003us per call --- 258397932 calls per sec
gpio_read: 329us --- 0.003us per call --- 303951367 calls per sec
gpio_write: 383us --- 0.003us per call --- 261096605 calls per sec
--- DONE ---
> There was a problem hiding this comment.
@chrysn what do you think about that proposal for gpio mock in last commit ?
chrysn
added
Reviewed: 2-code-design
What toolchain are you using and how is this related to |
gdoffe
force-pushed
the
native_fixes
branch
4 times, most recently
from
8204b94 to
55cfe0c
Compare
it was not relative to But just for information: |
|
Just removed 2 commits already applied in #20430 |
cpu/native/include/periph_cpu.h
Outdated
| /** | ||
| * @brief Mocked GPIO array | ||
| */ | ||
| static gpio_mock_t __attribute__((unused)) gpio_mock[GPIO_PORT_MAX][GPIO_PIN_MAX]; |
There was a problem hiding this comment.
this array needs to go into the c file and this needs to be
| static gpio_mock_t __attribute__((unused)) gpio_mock[GPIO_PORT_MAX][GPIO_PIN_MAX]; | |
| extern gpio_mock_t gpio_mock[GPIO_PORT_MAX][GPIO_PIN_MAX]; |
else we have an array per compilation unit
which is probably not intended
|
please fix the pr description |
Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
If the gpio is initialized as an input or interruptable pin, the gpio_mock driver returns -1 leading to failed initialization. However that is not because nothing can change the GPIO state that it has to be an error. Return 0 in all cases. Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
This allows developpers to override gpio behavior into their applications. Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
cpu/native/periph/gpio_mock.c
Outdated
| @@ -18,60 +18,85 @@ | |||
| */ | |||
|
|
|||
| #include "periph/gpio.h" | |||
| #include <stdio.h> | |||
kfessel
changed the title
cpu/native/include/periph_cpu.h
Outdated
| @@ -96,6 +96,52 @@ typedef enum { | |||
| } gpio_flank_t; | |||
|
|
|||
| /** @} */ | |||
| #else | |||
There was a problem hiding this comment.
| #else | |
| #else /* MODULE_PERIPH_GPIO_LINUX | DOXYGEN */ |
but shouldn't there be some GPIO_MOCK module to activate this?
There was a problem hiding this comment.
done (see next comment)
This commit introduces a more robust GPIO mocking mechanism by utilizing a 2-dimensional array. Each element of the array holds a gpio_mock_t structure describing a pin's attributes such as value, mode, flank, interruption callback, and callback argument. This enhancement allows for the arbitrary simulation of GPIOs across various microcontroller architectures using the current API, while maintaining consistency through the use of the GPIO_PIN macro. Additionally, it should be noted that only the maximum number of ports and maximum number of pins can be altered according to the context. The implemented API in gpio_mock.c remains rudimentary, providing no validation but fulfilling the required functions. However, it remains customizable as all its functions are marked as weak. Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
|
@gdoffe please fill the testin procedure section of the PR-message (include test ressults if possible like #20431 (comment)) does this realy depend on #20430 ( or is it the other way around) |
Sorry, done. It depends previously of #20430 but not the case anymore. |
Contribution description
Modify gpio-mock to allow arbitrary GPIO emulation.
Testing procedure
Build:
Test: