drivers/ws281x: add VT100 backend for native by benpicco · Pull Request #12793 · RIOT-OS/RIOT

benpicco · 2019-11-23T08:34:07Z

Contribution description

To quickly iterate on animations it is handy to being able to simulate the output on native.
This adds a VT100 terminal backend to the ws281x driver that outputs the colors straight to the terminal.
It should work with any sufficiently modern terminal emulator.

Testing procedure

run tests/driver_ws281x on native.

Issues/PRs references

benpicco · 2019-11-23T09:27:15Z

Meh, it looks like there is currently no way to have two disjunctive FEATURES_REQUIREDs and that ifneq ($(BOARD),native) hack doesn't fly with Murdock.

maribu · 2019-11-23T09:34:10Z

You can use features optional and in the second recursive round of dependency resolution the optional feature becomes used, if it is available

aabadie · 2019-11-23T09:40:52Z

drivers/Makefile.dep

      USEMODULE += ws281x_atmega
    endif
+    # fall back to VT100 terminal output if no other
+    # backend is availiable.


Suggested change

# backend is availiable.

# backend is available.

aabadie · 2019-11-23T09:41:35Z

drivers/Makefile.dep

  USEMODULE += xtimer
 endif

+ifneq (,$(filter ws281x_vt100,$(USEMODULE)))


This block should be moved to drivers/Makefile.include

But it looks so out of place there. 😮

The idea of the Makefile.dep is to set/resolve the dependencies of a module in the build. Makefile.include should contain code for setting build configuration variables. For me, CFLAGS is a build configuration variable, so it should be in drivers/Makefile.include.
git grep gives an opposite result as what I just said but I think the already existing CFLAGS defined in Makefile.dep should be moved in Makefile.include.

I think the cleanest option would be to have those defines in a header file. Via CFLAGS, those defines are available for every module and compilation unit, even though they are only needed for the ws281x module.

I see to options for using a header file for this:

a) A common header, let's say ws281x_backend.h, which uses #ifdef MODULE_WS281X_<BACKEND> to provide the required defines.
b) A separated ws281x_backend.h for each backend located at drivers/ws281x/include/<BACKEND_NAME>/ws281x_backend.h. And Makefile.include is used to add drivers/ws281x/include/<BACKEND_NAME> to INCLUDES for the used backend.

The simpler the better - I prefer option a).

benpicco · 2019-11-23T09:43:22Z

You can use features optional and in the second recursive round of dependency resolution the optional feature becomes used, if it is available

But then arch_avr8 is still required.
If both features are optional it will also build (and fail) if neither is available.

How about just using the printf() version as a fallback if no other backend is available? It will work on any board, it's just not what most users would expect 😆

maribu · 2019-11-23T11:06:57Z

Like this:

ifneq (,$(filter ws281x,$(USEMODULE)))
  FEATURES_OPTIONAL += arch_avr8 arch_native
  ifeq (,$(filter ws281x_%,$(USEMODULE)))
    ifneq (,$(filter arch_avr8,$(FEATURES_USED))) 
      USEMODULE += ws281x_atmega
    endif
    ifneq (,$(filter arch_avr8,$(FEATURES_USED))) 
      USEMODULE += ws281x_vt100
    endif
  USEMODULE += xtimer
endif

maribu · 2019-11-23T11:07:59Z

You need to overwrite the ws281x_init() in order to not init the GPIO

benpicco · 2019-11-23T12:12:52Z

Like this

That's what I did in 0b661f7

The problem is in the test Makefile.

maribu · 2019-11-25T20:30:55Z

tests/driver_ws281x/Makefile

@@ -1,4 +1,4 @@
-BOARD ?= atmega328p
+BOARD ?= native


How about adding:

ifneq (native,$(BOARD)) FEATURES_REQUIRED := arch_avr8 endif

That's what I tried first.

The problem is that Murdock then parsed the file differently than make would (not sure which script is responsible for it) and ended up building it for all platforms, ignoring the conditional FEATURES_REQUIRED.

Murdock uses make info-boards-supported to get the list of supported boards. In the worst case, we would need to add FEATURES_PROVIDED += ws281x_backend to cpu/atmega_common/Makefile.features and to cpu/native/Makefile.features and require that... But I'm not really thrilled but that idea.

The problem is that Murdock then parsed the file differently than make would (not sure which script is responsible for it) and ended up building it for all platforms, ignoring the conditional FEATURES_REQUIRED.

No, the issue is that the main application Makefile always gets fully evaluated before info-boards-supported, independent of Murdock.

So the if native: require avr thing needs to be somewhere in Makefile.dep. (It seems weird to begin with, though though. 😉 )

So the if native: require avr thing needs to be somewhere in Makefile.dep. (It seems weird to begin with, though though. )

That will work for native, but what if we add that SPI backed?
Testing the contents of FEATURES_PROVIDED is not allowed and I'd rather not maintain a BOARD_PROVIDES_SPI list shiver

maribu

ACK. Code looks good and the animations work quite well on the terminal :-D

maribu · 2020-02-10T13:38:33Z

I think I finally worked up the review backlog I was carrying around :-)

To quickly iterate on animations it is handy to being able to simulate the output on native. This adds a VT100 terminal backend to the ws281x driver that outputs the colors straight to the terminal.

benpicco · 2020-02-10T13:40:56Z

Thank you for all the reviews! 😄

tests/driver_ws281x/Makefile

Also add an additional newline before each test title to improve the rendering on native console output.

This brings the backend-dependant init() function in line with `WS281X_HAVE_PREPARE_TRANSMISSION` and `WS281X_HAVE_END_TRANSMISSION`.

benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 23, 2019

benpicco requested a review from maribu November 23, 2019 08:34

aabadie reviewed Nov 23, 2019

View reviewed changes

maribu reviewed Nov 25, 2019

View reviewed changes

benpicco mentioned this pull request Nov 30, 2019

Makefile.dep: use puf_sram for random when available #12849

Closed

benpicco force-pushed the ws281x_vt100 branch from 5c2aa25 to 0657fd3 Compare November 30, 2019 01:00

maribu approved these changes Feb 10, 2020

View reviewed changes

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 10, 2020

drivers/ws281x: add VT100 backend for native

8352e4a

To quickly iterate on animations it is handy to being able to simulate the output on native. This adds a VT100 terminal backend to the ws281x driver that outputs the colors straight to the terminal.

benpicco force-pushed the ws281x_vt100 branch from f6a3583 to d80d42a Compare February 10, 2020 13:40

benpicco commented Feb 10, 2020

View reviewed changes

tests/driver_ws281x/Makefile Outdated Show resolved Hide resolved

benpicco and others added 2 commits February 10, 2020 14:45

tests/driver_ws281x: allow build on native

720eff3

Also add an additional newline before each test title to improve the rendering on native console output.

drivers/ws281x: introduce WS281X_HAVE_INIT

11d7727

This brings the backend-dependant init() function in line with `WS281X_HAVE_PREPARE_TRANSMISSION` and `WS281X_HAVE_END_TRANSMISSION`.

benpicco force-pushed the ws281x_vt100 branch from d80d42a to 11d7727 Compare February 10, 2020 13:45

benpicco merged commit 4f8114a into RIOT-OS:master Feb 10, 2020

benpicco deleted the ws281x_vt100 branch February 10, 2020 17:23

miri64 mentioned this pull request Feb 11, 2020

tests/driver_ws281x: resolve weird feature dependencies #13343

Merged

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

Conversation

benpicco commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Nov 23, 2019

Uh oh!

maribu commented Nov 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Nov 23, 2019

Uh oh!

maribu commented Nov 23, 2019

Uh oh!

benpicco commented Nov 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Feb 10, 2020

Uh oh!

benpicco commented Feb 10, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benpicco commented Nov 23, 2019 •

edited

Loading

benpicco commented Nov 23, 2019 •

edited

Loading