drivers/gpio_exp: map GPIO expanders onto gpio_t #9190
Conversation
|
I did a brief survey of how each system that defines gpio.c handles gpio_t, and the following systems look to be easy to add mapping support (in the same way as the atmega_common example):
The following systems can be made compatible by raising the size of gpio_t to 16bit / 32bit (or changing how gpio_t is handled):
These systems use all of a 32bit gpio_t, and would require modifications to how gpio_t is used (unless gpio_t is raised to 64bits):
|
|
Rebased to solve conflict. I have added two test routines: one for runtime and one for compile time. Neither require an actual GPIO expander (since the system will redirect to any set of functions if you tell it that it is a "driver"). I think the tests are complete enough to remove the explicit dependency on #9054 (edited my first post to remove dependency note). |
|
Travis: Uhm, no it can't? |
|
Re cppcheck: You can move the exp_entry declaration inside the if block |
|
Thanks @gebart. I had thought that variable declaration in blocks other than the outer function block was not guaranteed by the standard (other than in for loops), but a search says otherwise. |
|
Sorry for the large number of fixups. This PR is now tested and working properly on mega-xplained. There may be an issue with the method of tests/driver_gpio_exp_coll, because some CPUs have a GPIO_PIN definition that uses a cast, which is not supported in preprocessor conditions. I may be able to work around it by defining a macro for |
|
tests/driver_gpio_exp_coll now does its testing using the compiler using static_assert, rather than the preprocessor. It can understand the enum port definitions (no more hard-coding), casts, sizeof, etc. This PR now requires #9279 |
|
#9279 has been merged: the dependency has been satisfied. |
|
Rebased on trunk to bring in new CPU (fe310) and resolved conflicts. All CPUs with conventional GPIO definitions have support added, but some have collisions. None of the CPUs with odd GPIO definitions have been added yet. The compile time collision detection in driver_gpio_exp_coll is confirmed to be working properly and is detecting collisions in a couple of the CPUs. Example output when detecting a collision: |
|
This PR is ready for review: All targets with gpio support now have gpio_exp support. All collisions have been resolved with no modification to how gpios were previously used. Compile tests on all non-blacklisted targets show no errors, and a runtime test on mega-xplained passes. |
|
Thanks for your contribution! I think it can be reviewed soon. Can you squash so it's in a clean status? |
|
Squashed. |
ZetaR60
added
Type: new feature
|
Rebased on trunk and conflicts solved. |
|
I have make the intercept code in |
|
I share concerns with @kYc0o regarding the 'overhead' of this PR, but more in term of maintainability for cpu code. I also understand and see the need of this feature. I think that I think @kaspar030 opinion might also be valuable here. |
|
An alternative to the macro based intercept might be to rename all GPIO functions in This would of course involve renaming all of the GPIO implementations, but it would avoid confusion over the nature of the macro, and it would avoid the overhead of building function pointers into the GPIO interface. |
|
I'm with @kYc0o and @vincent-d, having to modify all cpu implementations is too much.
There where even multiple prototypes with different tradeoffs. For this PR, I suggest concentrating on the actual expander API first and tackle integration later. |
|
I think @kYc0o mentioned that he performed a review of the entire PR, not just the intercept code. Can you confirm kYc0o? There was an issue with |
drivers/include/gpio_exp.h
Outdated
| /** | ||
| * @brief Macro for intercepting and redirecting gpio_init calls | ||
| */ | ||
| #define GPIO_INTERCEPT_INIT(pin, mode) \ |
There was a problem hiding this comment.
Long macros are against RIOT's coding conventions. Please try another way.
There was a problem hiding this comment.
Moved to non-macro in drivers/include/periph/gpio.h.
cpu/atmega_common/periph/gpio.c
Outdated
| @@ -127,6 +129,8 @@ static inline int8_t _int_num(gpio_t pin) | |||
|
|
|||
| int gpio_init(gpio_t pin, gpio_mode_t mode) | |||
| { | |||
| GPIO_INTERCEPT_INIT(pin, mode); | |||
There was a problem hiding this comment.
Having to add code to every single implementation needs to be avoided. Please find another way.
There was a problem hiding this comment.
Fixed. The intercepts are now implemented by splitting the GPIO functions into a low-level version.
| * @{ | ||
| * | ||
| * @file | ||
| * @brief GPIO expansion default registry |
There was a problem hiding this comment.
I don't quite like that gpio_exp comes with its own registry. Is this actually necessary?
There was a problem hiding this comment.
Yes. Expanders are addressed by a number that is used with GPIO_EXP_PIN(foo_dev, bar_pin). The number foo_dev cannot be known until linking the binary, because the board may have more than one GPIO expander, so foo_dev cannot be a macro or enum set by the driver. Also, order is important, so automatically building the registry with a linked list cannot be relied on, because changes to code may change the order of device numbers. This leaves a manually defined array as the remaining satisfactory choice, which would be set up as part of the board includes by the developer porting the board.
There was a problem hiding this comment.
Note: I think I was using the word "registry" too loosely here. It is not a registry in the sense of e.g. SAUL. I will change the name of the file to gpio_exp_conf.h
There was a problem hiding this comment.
Should be clearer now.
This reverts commit 9d53f20.
|
The latest changes addresses some of the major concerns expressed so far:
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
This PR makes handling of GPIO expanders invisible to code using the GPIO pin API (periph/gpio.h). This is accomplished by reserving part of the range of values of gpio_t. When a call to the GPIO API uses a pin that falls within this range, it is parsed into a device ID that is looked up in the GPIO expansion registery and the call is redirected to the corresponding device.
Mostly done, but still a bit WIP.