LED strip refactor #3158

nwf · 2020-06-13T00:23:33Z

Addresses #2916 (mostly, anyway). Leaves #3122 to be done, and doesn't change any of the color utils libraries.

I've moved the so-called "ws2812" tests, which were really just tests of its buffer, over to the generic pixbuf and this now passes those tests. While the ws2812_effects library is ported to work with pixbuf objects and does compile, it is operationally untested (though I don't anticipate any real breakage).

I have not actually attempted to use this to drive LEDs yet (ha) but the drivers are only lightly modified to use the new data structure, so I hope it should work fine.

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/*.

app/modules/apa102.c

app/modules/pixbuf.h

app/modules/pixbuf.c

app/modules/ws2812.c

docs/modules/pixbuf.md

HHHartmann · 2020-06-21T06:31:07Z

lua_tests/mispec_pixbuf_1.lua


        buffer1:fill(10,22,54)
        buffer2:fill(10,27,55)
-        buffer1:mix(-257,buffer1:dump(),255,buffer2:dump())


Did you remove the string capability from pixbuf?

Yes, though perhaps for somewhat specious reasoning: it wasn't documented and the test was flirting with OOM.

True, Maybe we should document it rather and if necessary split the tests in even more parts. As string buffers are allowed everywhere else it would be inconsistent if it wasn't here.

"allowed everywhere else"?

like in ws2812.write(), ws2812.buffer:set(), buffer:replace()
not documented and not implemented for ws2812.buffer:fill(), __concat() mix()

Did I just write the mix des not support strings? Then why did this test ever run at all??

HHHartmann · 2020-06-21T06:32:49Z

lua_tests/mispec_pixbuf_2.lua


        initBuffer(buffer1,7,8,9,12)
        initBuffer(buffer2,0,0,7,8)
        buffer1:shift(2)
-        ok(equalsBuffer(buffer1, buffer2), "shift right")
+        ok(buffer1 == buffer2, "shift right")


If we use our own == her we would need to add tests for == somewhere.
Else we test the operations with untested code (our ==)

Well, these are tests of our ==. But yes, maybe we should test for ~= sometimes. Here and elsewhere, more tests are welcome.

But still we should only test one aspect in one test and not use two untested components.

In black-box testing, it's often not possible to "only test one aspect" at a time. Previously, this test depended on fill (via initBuffer), shift, and dump; now it depends on fill, shift, and eq. I'm not opposed to having more tests, and even some that compare == on dump output, but I don't think the lack of orthogonality is new as of this diff.

True, but opposed to == there are tests for fill and shift.

nwf

Thanks for the review. Standby for incoming changes.

app/modules/apa102.c

app/modules/pixbuf.c

app/modules/pixbuf.h

docs/modules/pixbuf.md

nwf · 2020-06-21T13:23:03Z

lua_tests/mispec_pixbuf_1.lua


        buffer1:fill(10,22,54)
        buffer2:fill(10,27,55)
-        buffer1:mix(-257,buffer1:dump(),255,buffer2:dump())


Yes, though perhaps for somewhat specious reasoning: it wasn't documented and the test was flirting with OOM.

nwf · 2020-06-21T13:26:55Z

lua_tests/mispec_pixbuf_2.lua


        initBuffer(buffer1,7,8,9,12)
        initBuffer(buffer2,0,0,7,8)
        buffer1:shift(2)
-        ok(equalsBuffer(buffer1, buffer2), "shift right")
+        ok(buffer1 == buffer2, "shift right")


Well, these are tests of our ==. But yes, maybe we should test for ~= sometimes. Here and elsewhere, more tests are welcome.

app/modules/pixbuf.c

HHHartmann · 2020-06-28T07:53:33Z

app/modules/pixbuf.c

+  LROT_FUNCENTRY( __concat, pixbuf_concat_lua )
+  LROT_FUNCENTRY( __tostring, pixbuf_tostring_lua )
+
+  LROT_FUNCENTRY( dump, pixbuf_dump_lua )


Do we need an RGB version here? Or is it good enough to treat string buffers as raw everywhere and leaving the details to the app developer.
Do we need conversion functions RGB <=> GBR
Should we allow for a format parameter whenever string buffers are used?
So many questions. Sorry. If you would prefer to postpone this to another PR that would be fine with me.
We should the include that PR into the next release though or have it without Lua code change requirements to users.

I think dump is mostly a debugging function, not something we expect people to use day-to-day.

dump may also be used for persisting patterns (in a file)

app/modules/pixbuf.h

HHHartmann · 2020-06-28T08:12:52Z

lua_tests/mispec_pixbuf_1.lua


        buffer1:fill(10,22,54)
        buffer2:fill(10,27,55)
-        buffer1:mix(-257,buffer1:dump(),255,buffer2:dump())


True, Maybe we should document it rather and if necessary split the tests in even more parts. As string buffers are allowed everywhere else it would be inconsistent if it wasn't here.

HHHartmann · 2020-06-28T08:18:09Z

lua_tests/mispec_pixbuf_2.lua


        initBuffer(buffer1,7,8,9,12)
        initBuffer(buffer2,0,0,7,8)
        buffer1:shift(2)
-        ok(equalsBuffer(buffer1, buffer2), "shift right")
+        ok(buffer1 == buffer2, "shift right")


But still we should only test one aspect in one test and not use two untested components.

HHHartmann

I just found an old open browser tab (I got tons of them) with the documentation of another led strip module: tm1829 led strips, which also should be adapted to use PixBuf as input.

nwf · 2020-06-28T13:46:26Z

I am deeply skeptical of the tm1829 code; it appears to be generating a very ws2812-style waveform in software, but it bounces interrupts per byte, rather than per packet, which suggests to me that some fraction of the time it gets the timing wrong and the strip glitches.

Also despite the documentation claiming that its input string is in RGB order, the code does not actually change the order of any bytes, so its claim of sending GRB is probably wrong. But the documentation may predate the conversion to not corrupt the input string, too, so...

Anyway, I took a stab at converting it.

nwf · 2020-09-26T12:58:57Z

Blocking on #3230 since I want to be able to test pixbufs on the host.

nwf · 2020-09-26T14:31:44Z

Eh, nevermind. I'll just carry the local bits for host testing in my tree for a bit; no reason to block this.

HHHartmann

Still would like to have C functions to adjust channel order.
Using :map will be around 8 to 10 times slower.

But will be left for another PR then

HHHartmann · 2021-01-04T20:46:29Z

tests/NTest_pixbuf.lua

+end)
+
+N.test('mix with strings correctly ', function()
+    local buffer1 = pixbuf.newBuffer(1, 3)


this test should be removed as it does the same as the one above

HHHartmann · 2021-01-04T21:01:57Z

tests/NTest_pixbuf.lua

+    ok(eq(buffer1:size(), 0), "sub empty")
+
+    buffer1 = pixbuf.newBuffer(4, 4)
+    initBuffer(buffer1,7,8,9,12)


One line missing here: test fails

Suggested change

initBuffer(buffer1,7,8,9,12)

buffer2 = ws2812.newBuffer(2, 4)

initBuffer(buffer1,7,8,9,12)

HHHartmann · 2021-01-04T21:21:20Z

docs/modules/pixbuf.md

+```
+buffer:map(function(r,g,b) return g,r,b end) -- Change channel order
+buffer:map(function(r,g,b) return g,r,b end, nil, 2, 5) -- Change channel order for a subset of pixels
+outbuf:map(function(r,g,b) return b end, inbuf, 10, 20) -- Extract one channel for a subset of pixels


wouldn't this fail because there are less return values than channels? (unless outbuf only has 1 channel)

HHHartmann · 2021-01-04T21:24:24Z

docs/modules/pixbuf.md

+#### Returns
+`buffer0`
+
+#### Example


After looking at the code I understand, but from the examples and description I won't be able to use this with more than a function as param.
Especially the fact that the channels of the second buffer are also passed and can have a different number.

nwf · 2021-01-05T00:39:12Z

Thanks. Does 74a4051 address your concerns?

The new pixbuf module has more functionality than the ws2812-specific buffer it replaces. This is work in progress towards nodemcu#2916 While here, document the ws2812 UART-based overlapping with mainline execution. Fixes nodemcu#3140 This includes some untested hackery to keep the deprecated ws2812_effects compiling. We anticipate this module going away soon, however. See nodemcu#3122

These were never really driving the actual LEDs, which would be tricky to test. Convert to NTest and update

We were heading down the path of being a generic graphics library, which is not the goal.

nwf · 2021-01-05T11:16:02Z

Still would like to have C functions to adjust channel order.
Using :map will be around 8 to 10 times slower.

A pure-C byte-order-swizzling operation is probably straightforward... what do you imagine its API should be? That is, how do you want to specify the mapping function?

HHHartmann

Still would like to have C functions to adjust channel order.
Using :map will be around 8 to 10 times slower.

A pure-C byte-order-swizzling operation is probably straightforward... what do you imagine its API should be? That is, how do you want to specify the mapping function?

Maybe the map method could take defined values instead of a function to select a C operation.
buf:map(RGBtoGRB, [buf1[, ilo[, ihi]]]) -- SwapRG or ToGRB might also be a naming. Base would always be RGB. Others have to use their brain to find the correct setting.

This would be nice if you have strips with different channels connected together. But it would require a second buffer which uses memory. It is also a rather exotic scenario, which leads me to
another solution. That would be to add the parameter to the write methods.
`ws2812.write(bug, SwapRG)

To be channel neutral we could also pass ints to determin the new position of the channels.
Either as separate values or as array.
{2,1,3} would be equivalent to the above RGBtoGRB. It would also allow for 3 and 4 channels

docs/modules/pixbuf.md

HHHartmann · 2021-01-05T19:04:47Z

app/lua53/linit.c

@@ -48,6 +48,9 @@ extern LROT_TABLE(co_funcs);
 extern LROT_TABLE(mathlib);
 extern LROT_TABLE(utf8);

+extern LROT_TABLE(pixbuf_map);
+extern int luaopen_pixbuf(lua_State *);
+


Please also see Terrys comment #3230 (comment)

Should we have this in one PR for Lua 5.1 and Lua 5.3?

See #3230

I'll back the changes to this file out and we can deal with host-side deltas later.

docs/modules/pixbuf.md

marcelstoer · 2021-02-03T21:23:00Z

Sorry for coming back to this but I'm confused.

I was expecting some "Attention! This requires the pixbuf module"-style hints in the documentation of the affected modules but found none. I see that they all (unconditionally) #include "pixbuf.h" even though we've got //#define LUA_USE_MODULES_PIXBUF in the user_modules.h. Then I realized we don't use this define anywhere.

HHHartmann · 2021-02-03T21:42:11Z

There are several modules whose define is never used. Expect four the link magic which decides what goes on the firmware. If I'm not mistaken we always build all modules. Conditional include only is used if the behavior would change. But indeed some hints might make sense.

nwf · 2021-02-03T23:24:23Z

Ideally, such intermodule dependencies would be handled by Kconfig (#3130). I'm rather buried at work right now, so may I ask someone else to update the docs in the interim?

HHHartmann · 2021-02-04T06:28:39Z

Thought about it again.
There should be #IFDEF around the usage of pixbuf.
The hardware modules can be used without as data can be presented as stirrings too.
Only the effects need pixbuf and ws2812

marcelstoer · 2021-02-04T06:57:36Z

may I ask someone else to update the docs in the interim?

That was my initial intention when I realised those hints wrt pixbuf were missing. Then I noticed that pixbuf is always compiled into the firmware as LUA_USE_MODULES_PIXBUF isn't used to steer that (my conclusion).

Now I don't know what the real issue is here: missing docs or missing define guards.

HHHartmann · 2021-02-04T08:54:21Z

The build will break if one of the other modules are enabled without LUA_USE_MODULES_PIXBUF.
So this has to be fixed.
The documentation should mention that you need to enable pixbuf of you want to use it. Except for the effects module which requires pixbuf

marcelstoer · 2021-02-04T16:59:52Z

I think we're talking past each other, sorry.

The build will break if one of the other modules are enabled without LUA_USE_MODULES_PIXBUF.

It doesn't, that's my point 😉 I disabled pixbuf (i.e. use the default) but enabled apa102, ws2801, ws2812, and ws2812_effects as they import pixbuf.h. I was stunned when the build didn't fail.

There's only a single occurrence of LUA_USE_MODULES_PIXBUF - in the user_modules.h. Doesn't that mean it's always compiled into the firmware? That would explain why my build didn't fail.

HHHartmann · 2021-02-05T18:31:47Z

Yes, also checked and was surprised that it worked. It seems that the linker just pulls in the parts it needs. So I would guess that half the pixbuf module is linked. But haven't checked yet. EDIT: it seems that the whole module is pulled in.

The trick is done in app/Makefile and the NODEMCU_MODULE macro in app/include/module.h
The macro generates a symbol which is the pulled in the firmware if enabled. Don't really understand this in detail.

I think the linker then also pulls in the required modules.

So what do we want?
The modules apa102, ws2801, ws2812 should work even when pixbuf is disabled. Just without the pixbuf interface.
Use #ifdef in the code + notes in the doc that it will work but only with strings.
The module ws2812_effects without pixbuf will not work however. So this should fail the build I think.

nwf requested a review from HHHartmann June 13, 2020 00:23

marcelstoer added this to the Next release milestone Jun 13, 2020

This was referenced Jun 16, 2020

Tidying up the Library coding styles #1028

Closed

Deprecate and remove ws2812 effects (and provide an in-Lua replacement) #3122

Open

HHHartmann requested changes Jun 21, 2020

View reviewed changes

nwf commented Jun 21, 2020

View reviewed changes

nwf force-pushed the led-strip-refactor branch from 037ddbe to fd8ef76 Compare June 21, 2020 13:32

HHHartmann reviewed Jun 28, 2020

View reviewed changes

app/modules/pixbuf.c Outdated Show resolved Hide resolved

HHHartmann requested changes Jun 28, 2020

View reviewed changes

nwf force-pushed the led-strip-refactor branch from fd8ef76 to f376a8e Compare June 28, 2020 13:47

HHHartmann mentioned this pull request Jul 11, 2020

ws2812_effects expects GRB led strip, so RGB led strip colors are off #2537

Closed

nwf force-pushed the led-strip-refactor branch from f376a8e to c80fc3f Compare July 18, 2020 11:42

nwf mentioned this pull request Jul 18, 2020

Fixing modules wthich don't use the Lua memory allocator #2303

Closed

nwf removed this from the Next release milestone Sep 1, 2020

nwf added this to the Next release milestone Sep 23, 2020

nwf force-pushed the led-strip-refactor branch 2 times, most recently from dbbd0b6 to dce58ac Compare September 23, 2020 00:36

nwf requested a review from HHHartmann September 23, 2020 00:36

nwf force-pushed the led-strip-refactor branch from dce58ac to 55a4d83 Compare September 26, 2020 11:56

nwf added blocked Waiting on another PR to land enhancement new module labels Sep 26, 2020

nwf removed the blocked Waiting on another PR to land label Sep 26, 2020

nwf force-pushed the led-strip-refactor branch 2 times, most recently from 21ff0d0 to 9890313 Compare September 26, 2020 14:34

marcelstoer removed this from the Next release milestone Nov 3, 2020

HHHartmann requested changes Jan 4, 2021

View reviewed changes

nwf force-pushed the led-strip-refactor branch from 51fa0a8 to 74a4051 Compare January 5, 2021 00:38

nwf added 7 commits January 5, 2021 11:12

ws2801: pixbuf support

4dd575d

apa102 pixbuf support

acb9ebf

tm1829 pixbuf support

1b87e95

mispec "ws2812" tests -> NTest pixbuf tests

8d7d207

These were never really driving the actual LEDs, which would be tricky to test. Convert to NTest and update

pixbuf: remove types, add some *I methods

4fe9afb

We were heading down the path of being a generic graphics library, which is not the goal.

pixbuf: map

ada272b

HHHartmann requested changes Jan 5, 2021

View reviewed changes

HHHartmann and others added 5 commits January 5, 2021 21:18

add pixbuf to luacheck_config.lua

76bd3d3

fix luacheck errors

c39903d

reapply lost changes - fix local/local mix

faace6e

Correct typo in NTest pixbuf test

3ad6f65

pixbuf: incorporate review comments

bdd14f8

nwf force-pushed the led-strip-refactor branch from 74a4051 to bdd14f8 Compare January 5, 2021 21:20

HHHartmann approved these changes Jan 6, 2021

View reviewed changes

HHHartmann added this to the Next release milestone Jan 6, 2021

nwf merged commit 85df6b5 into nodemcu:dev Jan 6, 2021

HHHartmann mentioned this pull request Feb 11, 2021

handle impact of excluding pixbuf from build #3398

Merged

4 tasks

	initBuffer(buffer1,7,8,9,12)
	buffer2 = ws2812.newBuffer(2, 4)
	initBuffer(buffer1,7,8,9,12)

LED strip refactor #3158

LED strip refactor #3158

Uh oh!

Conversation

nwf commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

nwf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HHHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

nwf commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nwf commented Sep 26, 2020

Uh oh!

nwf commented Sep 26, 2020

Uh oh!

HHHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nwf commented Jun 13, 2020 •

edited

Loading

nwf commented Jun 28, 2020 •

edited

Loading

marcelstoer commented Feb 3, 2021 •

edited

Loading