-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: add MCP23x17 I/O expander support #10688
base: master
Are you sure you want to change the base?
Conversation
@ZetaR60 Again a GPIO expander driver that is working perfect with your last version of the extension API. |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a little bit old but left some comments for a first pass, I hope it gets a rebase in the near future as the driver seems very complete.
drivers/mcp23x17/mcp23x17.c
Outdated
return -MCP23X17_ERROR_NO_DEV; | ||
} | ||
#endif | ||
if (params->reset_pin != GPIO_UNDEF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (params->reset_pin != GPIO_UNDEF) { | |
if (gpio_is_valid(params->reset_pin)) { |
drivers/mcp23x17/mcp23x17.c
Outdated
* If GPIO pin for interrupt signals INTA and INTB is defined in parameters, | ||
* we use interrupts. | ||
*/ | ||
if (params->int_pin != GPIO_UNDEF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dito
return MCP23X17_OK; | ||
} | ||
|
||
int mcp23x17_gpio_init(mcp23x17_t *dev, gpio_t pin, gpio_mode_t mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm bit unsure about using gpio_t
in here as some CPUs could define it as an structure, I think it may be more suitable to add a proper mc23x17_gpio_t
type, same could apply for mode as CPUs could define their own and probably the IO expander has it's modes defined independently of the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpio_t
and gpio_mode_t
are used here intentionally. As a result of the review of the first driver provided in PR #7652, the goal of this PR was to implement an API that is compatible with the GPIO API in order to integrate the GPIO expanders transparently.
At the moment, gpio_t must be a scalar for all GPIO implementations because there are hundreds of comparisons in RIOT that use the == operator.
In PR #12877 and later in PR #14602 I tried to introduce a GPIO API using a structured gpio_t
type more than a year ago, but I didn't succeed until now. So there is no reason not to use gpio_t
at the moment. Once, the structured GPIO is introduced, the driver could easily be changed, as the example of the low-level GPIO API implementation of an expander driver in PR #14602 shows.
b7f0347
to
ddaa3bf
Compare
@@ -0,0 +1,40 @@ | |||
# Copyright (c) 2021 Gunar Schorcht |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leandrolanzieri How can I test the config for a new driver. Everytime I use menuconfig
as make target, I only see LM75A/TMP1075
in Sensor Device Drivers
and nothing else. When I use [A] Toggle show-all mode
, I see all variables in red, but I can't change anything. Furthermore, I get the following message in red.
*** !! ERROR: There are conflicting modules active !! ***
I get this message also for other test applications in release branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gschorcht to test the dependency modelling with Kconfig you should set a variable: env TEST_KCONFIG=1 make menuconfig
.
Furthermore, I get the following message in red.
*** !! ERROR: There are conflicting modules active !! ***
Everything that is in red is not visible, so that message is actually hidden if I understood you correctly.
For this modelling I'd suggest the following change:
diff --git a/drivers/mcp23x17/Kconfig b/drivers/mcp23x17/Kconfig
index ca79fcc382..705780263b 100644
--- a/drivers/mcp23x17/Kconfig
+++ b/drivers/mcp23x17/Kconfig
@@ -36,6 +36,8 @@ config MODULE_MCP23X17_IRQ
depends on HAS_PERIPH_GPIO_IRQ
depends on MODULE_MCP23X17
select MODULE_PERIPH_GPIO_IRQ
- select MODULE_EVENT_THREAD_MEDIUM
+ depends on MODULE_EVENT_THREAD_MEDIUM
+ help
+ To use the IRQs the MODULE_EVENT_THREAD symbol should be set.
endif # MODULE_MCP23X17
diff --git a/tests/driver_mcp23x17/app.config.test b/tests/driver_mcp23x17/app.config.test
index 181cba5f04..9d5911c6bf 100644
--- a/tests/driver_mcp23x17/app.config.test
+++ b/tests/driver_mcp23x17/app.config.test
@@ -2,6 +2,10 @@
# application configuration. This is only needed during migration.
CONFIG_MODULE_MCP23X17=y
+# enable event thread to process IRQs
+CONFIG_MODULE_EVENT=y
+CONFIG_MODULE_EVENT_THREAD=y
+CONFIG_MODULE_EVENT_THREAD_MEDIUM=y
# enable interrupt support by default
CONFIG_MODULE_MCP23X17_IRQ=y
You may also want to expose which thread priority to use on the driver, similar to the at
driver (see here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leandrolanzieri Either I still don't understand how Kconfig is used correctly or there is something wrong with ESP8266 Kconfig. After making the changes according to your suggestions and running Kconfig, I get the following output for USEMODULE
variable:
TEST_KCONFIG=1 make -C tests/driver_mcp23x17/ BOARD=esp8266-esp-12x menuconfig
TEST_KCONFIG=1 make -C tests/driver_mcp23x17/ BOARD=esp8266-esp-12x info-debug-variable-USEMODULE
```suggestion
benchmark tsrb core_init periph_init cpu periph_init_uart newlib stdin mcp23x17_irq event core_panic
mcp23x17 periph_i2c periph_init_gpio periph_timer mcp23x17_i2c core periph_uart periph_init_pm
event_thread periph_common event_thread_medium stdio_uart_rx shell periph_init_timer core_msg
periph_gpio_irq newlib_syscalls_default auto_init core_thread_flags periph_gpio periph_pm div sys
periph_init_i2c auto_init_xtimer stdio_uart isrpipe board xtimer core_idle_thread periph_init_gpio_irq
mcp23x17_irq_medium
When I set USEMODULE
as environment variable, I get the following output for USEMODULE
variable:
USEMODULE='mcp23x17_i2c mcp23x17_irq' make -C tests/driver_mcp23x17 BOARD=esp8266-esp-12x info-debug-variable-USEMODULE
auto_init auto_init_random auto_init_xtimer benchmark board boards_common_esp8266 core core_idle_thread
core_init core_msg core_panic core_thread_flags cpp cpu div esp_common esp_common_periph esp_freertos
esp_freertos_common esp_i2c_sw esp_idf esp_idf_esp8266 esp_idf_nvs_flash esp_idf_spi_flash esp_idf_util
esp_idf_wpa_supplicant_crypto esp_sdk event event_thread event_thread_medium isrpipe log luid mcp23017
mcp23x17 mcp23x17_i2c mcp23x17_irq mtd newlib newlib_syscalls_default periph periph_common
periph_cpuid periph_flash periph_gpio periph_gpio_irq periph_hwrng periph_i2c periph_i2c_sw periph_init
periph_init_cpuid periph_init_flash periph_init_gpio periph_init_gpio_irq periph_init_hwrng periph_init_i2c
periph_init_i2c_sw periph_init_pm periph_init_timer periph_init_uart periph_pm periph_timer periph_uart prng
prng_tinymt32 ps random shell stdin stdio_uart stdio_uart_rx sys tinymt32 tsrb xtensa xtimer
That is, a number of specific esp_*
modules are missing in Kconfig configuration for ESP8266 boards. The same happens for ESP32 boards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still did not make it to the esp for the Kconfig modelling of dependencies, that's why it's not tested yet in the CI
Lines 10 to 18 in 9256970
: ${TEST_KCONFIG_BOARDS_AVAILABLE:=" | |
dwm1001 | |
hifive1 | |
native | |
nrf52840dk | |
nucleo-f103rb | |
remote-revb | |
samr21-xpro | |
seeedstudio-gd32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leandrolanzieri Is there a more elegant way to map a possible choice to an enum than the following?
driver.h
typedef enum {
DRIVER_MODE_1 = 0x10,
DRIVER_MODE_2 = 0x20,
DRIVER_MODE_3 = 0x30,
} driver_mode_t;
driver_params.h
#ifdef CONFIG_DRIVER_MODE_1
#define CONFIG_DRIVER_MODE (DRIVER_MODE_1)
#elif CONFIG_DRIVER_MODE_2
#define CONFIG_DRIVER_MODE (DRIVER_MODE_2)
#elif CONFIG_DRIVER_MODE_3
#define CONFIG_DRIVER_MODE (DRIVER_MODE_3)
#endif
Kconfig:
choice
bool "Select right mode
default DRIVER_MODE_1
config DRIVER_MODE_1
bool "Mode 1"
config DRIVER_MODE_2
bool "Mode 2"
config DRIVER_MODE_3
bool "Mode 3"
endchoice
When migrating default configuration parameters to Kconfig
, a further question arose. What are the rules when a define in driver_params.h
should be called CONFIG_DRIVER_PARAM_<NAME>
and when a parameter should simply be called DRIVER_PARAM_<NAME>
? I have seen a mixture which doesn't seem to be consistent.
Would be a mapping
#define DRIVER_PARAM_<NAME> (CONFIG_DRIVER_PARAM_<NAME>`
OK to keep the compatibility with existing documentation where the default parameters could be changed by overriding them by CFLAGS
at the make command line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leandrolanzieri Is there a more elegant way to map a possible choice to an enum than the following?
What about the following:
choice
bool "Select right mode"
default DRIVER_MODE_1
config DRIVER_MODE_1
bool "Mode 1"
config DRIVER_MODE_2
bool "Mode 2"
config DRIVER_MODE_3
bool "Mode 3"
endchoice
config DRIVER_MODE
hex
default DRIVER_MODE_1_VAL if DRIVER_MODE_1
default DRIVER_MODE_2_VAL if DRIVER_MODE_2
default DRIVER_MODE_3_VAL if DRIVER_MODE_3
config DRIVER_MODE_1_VAL
hex
default 0x10
config DRIVER_MODE_2_VAL
hex
default 0x20
config DRIVER_MODE_3_VAL
hex
default 0x30
Then you can also have:
typedef enum {
DRIVER_MODE_1 = CONFIG_DRIVER_MODE_1_VAL, // not actually configurable, just not to duplicate
DRIVER_MODE_2 = CONFIG_DRIVER_MODE_2_VAL,
DRIVER_MODE_3 = CONFIG_DRIVER_MODE_3_VAL,
} driver_mode_t;
Note that this last part would only work in the case Kconfig is actually run.
What are the rules when a define in
driver_params.h
should be calledCONFIG_DRIVER_PARAM_<NAME>
and when a parameter should simply be calledDRIVER_PARAM_<NAME>
? I have seen a mixture which doesn't seem to be consistent.
Personally I think many of those parameters are more a hardware configuration rather than a configuration of the driver itself, and would be better off left for other tools (e.g. the current driver_params.h
or Device Tree). Nevertheless there are some drivers that expose these default values for their first instance as Kconfig symbols.
Would be a mapping
#define DRIVER_PARAM_<NAME> (CONFIG_DRIVER_PARAM_<NAME>`
OK to keep the compatibility with existing documentation where the default parameters could be changed by overriding them by
CFLAGS
at the make command line?
When Kconfig is not used (it does not run by default) the CONFIG_DRIVER_PARAM_<NAME>
value can also be injected via CFLAGS
, so I think just a documentation update is enough, no mapping should be needed if I understood you correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the following:
... config DRIVER_MODE_1_VAL hex default 0x10 ...
Hm, that would require coding enumerations again, which I think is error prone. In my approach I can use the named constants as they are already defined by the driver.
Personally I think many of those parameters are more a hardware configuration rather than a configuration of the driver itself, and would be better off left for other tools (e.g. the current
driver_params.h
or Device Tree).
I'm only talking about selections the user has to make. A typical example would be a sensor for which the desired output data rate has to be defined. The output data rate is configured using a register and all possible values are defined as an enumeration. For efficiency, the enumeration directly encodes the register values.
19cc415
to
fc1cc09
Compare
Since the review of the PR hasn't really started yet and I've changed a lot, I felt free to restructure the PR and force-push it. |
fc1cc09
to
e7bc305
Compare
Does anyone of the active @RIOT-OS/maintainers have hardware and time to test this? Seems like a worthwhile PR to me. |
Is there any supported board on which this is present? |
I am not a maintainer, but I can test it in the near future (once the Mouser order arrives). Mikro-E makes breakout boards for both chips that are covered by this PR. Given that this is a driver, the potential impact on the rest of RIOT should be low. |
tests/driver_mcp23x17/README.md
Outdated
|
||
Used MCP23x17 I/O expander or interface variants have to be defined by | ||
the variable `DRIVER` using the corresponding pseudomodules `mcp23017`, | ||
`mcp23s17`, `mcp23017_i2c` and/or `mcp23017_spi`, for example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`mcp23s17`, `mcp23017_i2c` and/or `mcp23017_spi`, for example: | |
`mcp23s17`, `mcp23x17_i2c` and/or `mcp23x17_spi`, for example: |
The generic pseudomodules have the 'x' instead of the '0' and 'S'. Just a typo.
tests/driver_mcp23x17/Makefile
Outdated
@@ -0,0 +1,10 @@ | |||
include ../Makefile.tests_common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include ../Makefile.tests_common | |
include ../Makefile.drivers_common |
The test have been restructured in the meantime and now this is the common Makefile for all tests of drivers.
So... I'm not sure if we can reach @gschorcht, but the main thing that conflicts with a rebase to the current master branch is the location of the test in The The only change that has to be made to compile the code is in the suggestion above. So far I did some very basic tests with both chips and a NUCLEO-L452RE and they generally seem to work. I can run the benchmark tests, configure and read/write the GPIOs. I'll take a deeper look at everything and give it a more thorough test, but for now it's good that it can be rebased and compiles with the current master. |
e7bc305
to
2a83edd
Compare
Contribution description
This PR adds a driver for Microchip MCP23x17 I/O expanders which can be used to extend the number of GPIOs over I2C or SPI. It is a complete rework of the driver that was provided in PR #7652 and rewritten from scratch.
The following expander devices are supported:
mcp23017
ormcp23x17_i2c
mcp23s17
ormcp23x17_spi
The main features of the driver are:
mcp23x17_
and require an additional parameter, the pointer to the expander device of typemcp23x17_
It supports the GPIO extension API as defined in PR periph_*: Allow extensions for external periph adapters #9582 and provided in PR periph/gpio: support for extension API #9860 and periph/gpio: support for extension API (part 2) #9958.GPIO_IN
,GPIO_IN_PU
andGPIO_OUT
in hardwareGPIO_OD
andGPIO_OD_PU
are emulated.periph_gpio_irq
Benchmarks
MCP23x17 driver run-time performance benchmark over SPI using 10 MHz clock speed:
MCP23x17 driver run-time performance benchmark over SPI using 1 MHz clock speed:
MCP23x17 driver run-time performance benchmark over I2C using 400 kHz clock speed:
Reference board was an
esp8266-esp-12x
.Testing procedure
The test application
tests/driver_mcp23x17
can be used to test each MCP23x17 I/O expander pin with shell commands.Compile the application. Dependent on the MCP23x17 expander module, compile it with at least one of the pseudomodules
mcp23x17_i2c
ormcp23x17_spi
, e.g., for the I2C version MCP23017:Add the interrupt functionality by enabling module
mcp23x17_irq
and override the default GPIO pin for the interrupt signal, e.g. for the I2C version MCP23017:Connect the MCP23x17 I/O expander module using the interface and the GPIO for the interrupt signal as defined during compilation.
Connect a GPIO pin of the MCU, e.g.
GPIO_PIN(0,4)
, and a MCP23x17 I/O expander pin, e.g. Port A (port 16 in test application) and Pin 6.Execute some or all of the following commands (results in comment):
Issues/PRs references
The driver is compatible with the GPIO extension API, PRs #9582, #9860 and #9958, but it does not depend on it.