drivers/soft_spi_as_periph_spi: new module#16196
drivers/soft_spi_as_periph_spi: new module#16196maribu wants to merge 6 commits intoRIOT-OS:masterfrom
Conversation
maribu
added
Type: enhancement
|
This PR also needs to add some documentation and it wouldn't hurt to split out the addition of the register access and the auto into their own commit, each. I will do so when #16164 is merged. |
maribu
removed
the
State: waiting for other PR
|
Rebased, split into multiple commits, and added missing doc. The test as described above still works:
|
maribu
added
the
CI: ready for build
|
I guess then, something like what I tried here (but for SPI) can be added now? #15717 (comment) |
We could also adopt the approach here for UART. As long as the signature is identical, it is no problem to provide symbol aliases for the "real" API as drop in replacement. |
drivers/include/soft_spi.h
Outdated
| * Currently only the use of MOSI in master mode is implemented. Therefore receiving | ||
| * data from a slave is currently not possible. | ||
| * The signatures of the functions are identical to the functions declared in | ||
| * `spi.h`. The clock speed is approximated by using `xtimer_nanosleep()`. |
There was a problem hiding this comment.
@maribu is it worth using nanosleep here? AFAIK most xtimer backends are using a < 1MHz timer anyway, so best result achievable would be 1us no? ztimer could use ptp but that is another story.
There was a problem hiding this comment.
I very much agree.
Note that I only changed the wording of the doc in this PR, the use of xtimer_nanosleep() is a preexisting design decision. Maybe it is best to tackle this in a separate PR?
There was a problem hiding this comment.
I agree, can you rebase this one?
There was a problem hiding this comment.
I rebased it on master. I will later retest again, the rebase was a bit involved and I hope I didn't mess anything up.
|
@maribu seems like Kconfig needs an update, otherwise there are some static check issues and this PR unconvered a bug in diff --git a/cpu/lm4f120/include/periph_cpu.h b/cpu/lm4f120/include/periph_cpu.h
index 9d39286d8b..902ce052de 100644
--- a/cpu/lm4f120/include/periph_cpu.h
+++ b/cpu/lm4f120/include/periph_cpu.h
@@ -178,7 +178,7 @@ typedef enum {
SPI_MODE_0 = SSI_FRF_MOTO_MODE_0, /**< CPOL=0, CPHA=0 */
SPI_MODE_1 = SSI_FRF_MOTO_MODE_1, /**< CPOL=0, CPHA=1 */
SPI_MODE_2 = SSI_FRF_MOTO_MODE_2, /**< CPOL=1, CPHA=0 */
- SPI_MODE_3 = SSI_FRF_MOTO_MODE_0, /**< CPOL=1, CPHA=1 */
+ SPI_MODE_3 = SSI_FRF_MOTO_MODE_3, /**< CPOL=1, CPHA=1 */
} spi_mode_t;
/** @} */
#endif /* ndef DOXYGEN */ |
github-actions
bot
added
Area: cpu
maribu
removed
the
CI: ready for build
|
Did something pop up in the ci run @maribu ? |
|
The KConfig TODO is still open. I wanted to tackle this prior to letting the CI burn throw 3 days of CPU time :-) |
OlegHahm
added
the
State: waiting for author
|
@maribu, ping! |
This should result in SPI_MODE_3 properly working on the lm4f120.
soft_spi_transfer_reg() and soft_spi_transfer_regs() never have been implemented. This fixes the issue.
- Use spi_clk_t and spi_mode_t instead of soft_spi_clk_t and
soft_spi_mode_t
- For backward compatibility, the old types are defined as aliases
- Using identical function signatures will allow using soft_spi as
drop in replacement for periph_spi
- Use negative errno codes for error reporting
- Again, backward compatibility with the enum is provided
- This brings the API in alignment with the current style
requirements
- No custom clock speeds
- Instead reuse the clock speeds of SPI and do best effort for
frequencies higher than 400 kHz
Allow using soft_spi in place if periph_api by adding
USEMODULE += soft_spi_as_periph_spi
to an application's Makefile.
maribu
requested review from
cgundogan,
jia200x and
leandrolanzieri
as code owners
|
Testing can be done with which should yield: while Should build and work (provided decent values for |
| # Add features provided by modules | ||
| # | ||
| # This is done *before* the regular dependency resolution in Makefile.dep. | ||
| # Hence, an application requiring to provide a feature via a module must do | ||
| # so early on | ||
|
|
||
| ifneq (,$(filter soft_spi_as_periph_spi,$(USEMODULE))) | ||
| FEATURES_PROVIDED += periph_spi | ||
| endif |
There was a problem hiding this comment.
This file is new and will need some discussion. I'd like to summon @fjmolinas regarding this.
As a data point: KConfig modelling was just find without crude hacks like this.
There was a problem hiding this comment.
So far all FEATURES are declarative, and this involves having them not be declarative (which is for me the major change), MODULE having FEATURES I don't see a problem with at a first glance.
What I wonder is if this couldn't be modeled differently to avoid this change. Technically all you need to provide a soft_spi is periph_timer, so this could be kept declarative by changing the model to:
FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_spi_sw
All platforms providing a periph_timer would provide the softspi feature. And then all modules requiring it would instead do:
FEATURES_REQUIRED_ANY += periph_spi|periph_spi_sw
FEATURES_CONFLICT += periph_spi|periph_spi_sw
Selecting the right modules to implement that feature becomes a dependency resolution issue.
You would still be able to have FEATURES_REQUIRED += periph_spi and USEMODULE += soft_spi. Your soft_spi_as_periph_spi is selected by the periph_spi_sw feature.
What I like of this is that it keeps it declarative. But I'm not sure how to do it in Kconfig. The FEATURES_REQUIRED_ANY and FEATURES_CONFLICT are not really modelled. But it seems that a choice with some indirection would work (summon @leandrolanzieri and @MrKevinWeiss as well)
choice PERIPH_SPI_API
bool "periph_spi api backend"
config MODULE_PERIPH_SPI
depends on HAS_PERIPH_SPI
config MODULE_SOFT_SPI_AS_PERIPH_SPI
depends on HAS_PERIPH_SPI_SW
select MODULE_SOFT_SPI
endchoice
config MODULE_SOFT_SPI
bool "Software-implemented SPI"
depends on HAS_PERIPH_GPIO
depends on TEST_KCONFIG
select MODULE_PERIPH_GPIO
select MODULE_ZTIMER
select ZTIMER_USEC
Note: all used names are place holders.
There was a problem hiding this comment.
So far all FEATURES are declarative, and this involves having them not be declarative (which is for me the major change), MODULE having FEATURES I don't see a problem with at a first glance.
What I wonder is if this couldn't be modeled differently to avoid this change. Technically all you need to provide a soft_spi is periph_timer, so this could be kept declarative by changing the model to:
FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_spi_sw
All platforms providing a periph_timer would provide the softspi feature. And then all modules requiring it would instead do:
FEATURES_REQUIRED_ANY += periph_spi|periph_spi_sw
FEATURES_CONFLICT += periph_spi|periph_spi_sw
Selecting the right modules to implement that feature becomes a dependency resolution issue.
You would still be able to have FEATURES_REQUIRED += periph_spi and USEMODULE += soft_spi. Your soft_spi_as_periph_spi is selected by the periph_spi_sw feature.
What I like of this is that it keeps it declarative. But I'm not sure how to do it in Kconfig. The FEATURES_REQUIRED_ANY and FEATURES_CONFLICT are not really modelled. But it seems that a choice with some indirection would work (summon @leandrolanzieri and @MrKevinWeiss as well)
choice PERIPH_SPI_API
bool "periph_spi api backend"
config MODULE_PERIPH_SPI
depends on HAS_PERIPH_SPI
config MODULE_SOFT_SPI_AS_PERIPH_SPI
depends on HAS_PERIPH_SPI_SW
select MODULE_SOFT_SPI
endchoice
config MODULE_SOFT_SPI
bool "Software-implemented SPI"
depends on HAS_PERIPH_GPIO
depends on TEST_KCONFIG
select MODULE_PERIPH_GPIO
select MODULE_ZTIMER
select ZTIMER_USEC
Note: all used names are place holders.
There was a problem hiding this comment.
Having different providers would not be an issue if we used strictly depends on instead of selecting some modules (although it comes with the known cumbersome configuration). Based on the @fjmolinas proposal, I think a choice is the best option. I'd add a switch symbol and a feature to be replaced across all the current users of HAS_PERIPH_SPI and MODULE_PERIPH_SPI.
# Kconfig.spi
menuconfig SPI_SUPORT
bool "SPI support"
depends on TEST_KCONFIG
depends on HAS_SPI
choice SPI_IMPLEMENTATION
bool "SPI implementation"
depends on SPI_SUPORT
config MODULE_PERIPH_SPI
bool "Peripheral"
depends on HAS_SPI
endchoice
# related features
config HAS_SPI
bool
config HAS_PERIPH_SPI
bool
select HAS_SPI
# drivers/soft_spi/Kconfig
# extend the choice options
choice SPI_IMPLEMENTATION
depends on SPI_SUPPORT
config MODULE_SOFT_SPI
bool "Software-implemented SPI"
depends on HAS_SPI_SW
select MODULE_PERIPH_GPIO
select MODULE_ZTIMER
select ZTIMER_USEC
endchoice
config HAS_SPI_SW
bool
depends on HAS_PERIPH_TIMER
depends on HAS_PERIPH_GPIO
select HAS_SPI
default y
There was a problem hiding this comment.
MODULEhavingFEATURESI don't see a problem with at a first glance.
I'm guessing this at least would change the way we produce info-features-provided, right? As a dependency resolution would be required now to actually get all the features. Moreover, order of inclusion comes into play now, as we always consider that features are set in stone before requiring/checking them in Makefile.dep.
There was a problem hiding this comment.
I think the right approach would be to let the drivers depend on an spi module, rather than on a periph_spi feature. The driver likely cares little how the API is provided, as long as the API is there and actually drives an SPI bus. And will also be easier to maintain the list of optional feature dependencies in the single place of the spi module, rather than for each driver.
In fact, making this spi module a dedicated layer would allow mixing implementations. If an application can keep all hw buses busy via DMA, there could be value in connecting another device via bit-banged SPI. Or, maybe more realistically, some MCUs like the RP2040 have hardware that can be turned into an additional SPI bus (such as the PIO state machines) but doesn't naturally map to the same driver. A high level spi module could multiplex all used SPI providers. @benpicco also on this band wagon, as far as I know.
This PR is IMO just a poor waiting to be replaced by a proper solution (: But it might still be useful when porting RIOT to a new MCU until a a hardware SPI driver materialises.
|
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
8 participants
Contribution description
Allow using soft_spi in place if periph_api by adding
to an applications Makefile.
Testing procedure
I tested this using
tests/driver_bmx280using an BME280 sensor conntected to a Nucleo-F767 with the following patch applied:The test output was:
Issues/PRs references
Depends on and includes