tests/driver_ws281x: resolve weird feature dependencies #13343
Conversation
06cd771 to
8a53e9b
Compare
| FEATURES_BLACKLIST += arch_esp8266 | ||
| FEATURES_BLACKLIST += arch_mips32r2 | ||
| FEATURES_BLACKLIST += arch_msp430 | ||
| FEATURES_BLACKLIST += arch_riscv |
There was a problem hiding this comment.
This is not going to work. E.g. the GPIO ABC backend will add support for this driver on some ARM boards, but only those with GPIO ABC support.
This cannot be reasonably be solved with the current way of dependency tracking. Maybe KConfig allows to model "depends on: foo OR bar OR baz"
There was a problem hiding this comment.
Can't you just in this case do something like removing FEATURES_BLACKLIST += arch_arm and add FEATURES_REQUIRES += periph_gpio_abc for ARM platforms in Makefile.board.dep? And how does the current hack help with the situation?
There was a problem hiding this comment.
The current approach couldbe extended by testing for the backends one by one and adding FEATURES_REQUIRED := arch_avr8 if none of the other backend matched.
But better would be to teach the dependency system the logical or.
There was a problem hiding this comment.
You did not answer my first question. Isn't this already solvable without hacks and the current build system approach?
There was a problem hiding this comment.
This is how the appropriate patch could look like (untested):
diff --git a/tests/driver_ws281x/Makefile b/tests/driver_ws281x/Makefile
index 43323538e9..94aaac3a38 100644
--- a/tests/driver_ws281x/Makefile
+++ b/tests/driver_ws281x/Makefile
@@ -10,7 +10,6 @@ USEMODULE += ws281x
# Currently the ws281x only supports AVR-based platforms and native
# (via VT100 terminals).
# See https://doc.riot-os.org/group__drivers__ws281x.html
-FEATURES_BLACKLIST += arch_arm
FEATURES_BLACKLIST += arch_esp32
FEATURES_BLACKLIST += arch_esp8266
FEATURES_BLACKLIST += arch_mips32r2
diff --git a/tests/driver_ws281x/Makefile.board.dep b/tests/driver_ws281x/Makefile.board.dep
new file mode 100644
index 0000000000..dbc03d05fd
--- /dev/null
+++ b/tests/driver_ws281x/Makefile.board.dep
@@ -0,0 +1,3 @@
+ifneq (,$(filter arch_arm,$(FEATURES_PROVIDED)))
+ FEATURES_REQUIRED += periph_gpio_abc
+endifThere was a problem hiding this comment.
There might be ARM boards that don't have GPIO ABC, but SPI or I2S. What really is needed is a logical or. I think that this is actually quite feasible to implement.
It would be used by e.g :
FEEATURES_REQUIRED += arch_native|arch_armv8|periph_spi|periph_i2s|periph_gpio_abcThere was a problem hiding this comment.
Yes we currently can't do this in out build-system. Maybe some kind of construct similar to the one done for FEATURES_CONFLICT could be done. It could declare lists of options where at least one has to be provided.
FEATURES_REQUIRED_OPTIONS += arch_native:arch_armv8:periph_spi:periph_i2s:periph_gpio_abc
But this would require more reflection, and I'm not sure o the impact.
|
I have nothing against this PR. But on long term, a different approach will be needed. |
Ok, then let's set the label for now. Maybe @aabadie and @fjmolinas have some idea about your OR proposal in #13343 (comment) |
|
Me too, but I saw that we somehow forgot merging this, so I hit the green button. |
Contribution description
While browsing through some features I'm interested in, I stumbled upon this weird construct in the
Makefileoftests/ws281x. It was commented on in the PR that introduced it but to me no proper reason was given why not justFEATURES_BLACKLISTwas used.Testing procedure
has the same output as
(in master, all boards are outputted for the first).
Issues/PRs references
None