Implementing WS2812b signal transmission in SPI plugin. #1382
Conversation
Update SPIOutput.h to match SPIOutput.cpp after WS2812b update
Added encoding for WS2812b.
|
We ought to get #1358 in first, which makes a few changes to the way personalities work, but fairly minimal code changes your end. |
There was a problem hiding this comment.
Thanks for doing this!
A few comments, also please can we have some tests added too (again see #1358 for an example), then we know it's always generating the right values even if people don't have hardware to test it with.
plugins/spi/SPIOutput.cpp
Outdated
| uint8_t r = buffer.Get(offset); | ||
| uint8_t g = buffer.Get(offset + 1); | ||
| uint8_t b = buffer.Get(offset + 2); | ||
| output[i * WS2812b_SPI_BYTES_PER_PIXEL] = 0x24 | ((g & 0x1) << 1) | ((g & 0x2) << 3) | ((g & 0x4) << 5); |
There was a problem hiding this comment.
Given this code is used twice, it would be good to have a function that does it, passing in the DMX data and a reference to the array we want to write it into.
There was a problem hiding this comment.
The conversion code is the same thing 6 times over, wouldn't it be better to convert that part to a function. I struggle with passing arrays around in C++ as I don't really know C++, could you give me a hint on doing this?
plugins/spi/SPIOutput.cpp
Outdated
|
|
||
| // create Pixel Data | ||
| uint8_t pixel_data[WS2812b_SPI_BYTES_PER_PIXEL]; | ||
| pixel_data[0] = 0x24 | ((g & 0x1) << 1) | ((g & 0x2) << 3) | ((g & 0x4) << 5); |
There was a problem hiding this comment.
Use the new function when you've written it.
plugins/spi/SPIOutput.cpp
Outdated
|
|
||
| // set all pixel to same value | ||
| for (uint16_t i = 0; i < m_pixel_count; i++) { | ||
| uint16_t spi_offset = (i * WS2812b_SPI_BYTES_PER_PIXEL); |
There was a problem hiding this comment.
I'd be inclined to just put this directly in the row below, rather than storing it in a variable as we're only using it once.
There was a problem hiding this comment.
Done, although I think we may be able to improve the memcpy possibly.
|
Okay @Ph0N37Ic5 the other PR is in, so do you want to resolve conflicts and update; you should see the equivalent changes to make to your PR. |
modified: .gitignore modified: .travis.yml modified: plugins/spi/SPIOutput.cpp modified: plugins/spi/SPIOutput.h modified: plugins/spi/SPIOutputTest.cpp -Syncronized upstream changes. -Applied code standards. -All caps constants. -Bug fix. -WS2812B_DMX_SLOTS_PER_PIXEL changed to WS2812B_SPI_BYTES_PER_PIXEL -Cleanup. -Removed spi_offset variable, set once, used once. -Split out conversion to separate function.
Code updated according to suggestions in OpenLightingProject#1382
|
Thanks @Ph0N37Ic5 . We're still seeing your PR offset with the recent master changes. How did you get them in? Confusingly, there don't seem to be actual conflicts, it just doesn't have your changes present as it's expecting. |
|
Sorry for being such a noob, but how do I add something as a remote? I seem to be having issues with pulling the upstream master. |
plugins/spi/SPIOutput.cpp
Outdated
|
|
||
| void SPIOutput::WS2812bByteMapper(uint8_t input, uint8_t &low, uint8_t &mid, uint8_t &high) | ||
| { | ||
| low = 0x24 | ((input & 0x1) << 1) | ((input & 0x2) << 3) | ((input & 0x4) << 5); |
There was a problem hiding this comment.
Can we have some comment or explanation of these numbers/what's going on generally with the way it works please. Or a link to a page with some detail.
|
It looks like you've managed the remote thing, but otherwise see https://help.github.com/articles/adding-a-remote/ and the other excellent articles on GitHub. |
|
There are also a few lint issues you'll need to fix please, see towards the bottom of the lint job log here: |
|
I'm working on the tests in SPIOutputTest.cpp, but it might take a little while. |
|
It's currently failing due to differing lengths: You forgot to modify SPIOutput::InternalWriteDMX so your functions are never being called yet. |
|
Now it fails in new and wonderful ways, the question is, who's calculations are wrong, its or mine? |
|
You mean who's testing the tester. 😛 Am I right in thinking this should also support WS2811, did I read that somewhere? Can you also post on the PR when you've done some hardware tests, and which flavour of LEDs it was, some other rough detail etc. |
|
It's currently compiling on my RPi. The tests will be with the WS2812b. From what I can see from the data sheets the WS2811 uses the same protocol, but I have none I can test with. In any case testing would probably have to wait until tomorrow evening. I occasionally have to sleep, and I have some engagements in the morning. |
There was a problem hiding this comment.
While you may have fixed the tests, I think you might have introduced some different issues that they're perhaps not having.
I'd suggest you look at something like the non pixel brightness version of the APA102, or another that has a different number of SPI bytes to pixel slots. Although looking at it now, I think P9813 may have a similar bug.
Although TBH, maybe the behaviour is just a bit undefined in that sort of edge case, and we need to work out what it should do and be consistent.
plugins/spi/SPIOutput.cpp
Outdated
| @@ -909,31 +909,39 @@ void SPIOutput::IndividualWS2812bControl(const DmxBuffer &buffer) { | |||
| return; | |||
| } | |||
|
|
|||
| const unsigned int length = std::min(m_pixel_count * WS2812B_SLOTS_PER_PIXEL, | |||
| buffer.Size() - first_slot); | |||
| const unsigned int length = m_pixel_count; | |||
There was a problem hiding this comment.
You need to keep the min to not over-read the buffer, otherwise if the DMX buffer is say 10 slots long in total, and you try to read data for four pixels, g and b of pixel 4 won't be in the buffer!
There was a problem hiding this comment.
No, I check for that within the loop. This is done the way it is to initialise the pixels in case the first DMX signal is not a full set of data, as is the case in the first test.
There was a problem hiding this comment.
When you say initialise, do you mean to send the o444444 out to all of them?
plugins/spi/SPIOutput.cpp
Outdated
| r = buffer.Get(offset); | ||
| g = buffer.Get(offset + 1); | ||
| b = buffer.Get(offset + 2); | ||
| } // fill further pixel data only if the pixel data is empty |
There was a problem hiding this comment.
I think this may stop it ever overwriting in future updates.
There was a problem hiding this comment.
Then test 2 should fail. This is where I prevent overreading the dmx buffer. The first time through it should fill the uninitialized pixels with 0 data, later passes it should only write data available from the dmx buffer. That is why it says else if, not just if.
The dmx standard specifies that a fixture loosing it's data input should keep doing what it was doing. I think this is the intended outcome.
|
Tomorrow or later is fine, people have been waiting a lot longer for this already! It's great you've started a PR 😄 |
plugins/spi/SPIOutputTest.cpp
Outdated
| 0x92, 0x49, 0x24, // Pixel 2 Green (0) | ||
| 0x92, 0x49, 0x24, // Pixel 2 Red (0) | ||
| 0x92, 0x49, 0x24 // Pixel 2 Blue (0) | ||
| }; |
There was a problem hiding this comment.
Can we align with the } above if you're going to leave it on it's own line please, like the one below.
|
Is there any other place there needs to be made changes in order to load the plugin from the config? It doesn't seem to want to accept any number outside 1 through 8. |
|
I'm not really sure what to suggest. Do you want to try via the RDM tab in the web UI? Are your new personalities listed there? Otherwise does olad -l 4 show anything about not loading them when it first starts up? I can at least select the APA102 flavours, and I've even pushed the info up to our website: Olad only reads the log at startup, so you need to start olad, then stop it, then edit the file (which will have been created at first run), then start it again to make it read your edits. |
|
Nope, not getting more than 2 APA102 interfaces, and no WS2812b in the list. |
|
Try a make, or make clean then a make; if you're not getting the APA pixel brightness, you're not building against master... |
|
I've ... |
|
Interesting output from ./configure checking random usability... no checking SaleaeDeviceApi.h usability... no |
|
So it compiled all right, but I seem to have gotten a different problem, just before I reset everything the webserver started complaining about not finding it's files, now after a complete rebuild, it still has the same problem. Seems that the web files got installed to /usr/local/share/olad/www but olad is looking for them in /usr/share/olad/www. Go figure, anyway, a complete reinstall does not show the last four items in the list. This might be related, as it seems most everything goes into /usr/local/... rather than /usr/... But it must be finding it, otherwise it would not show up at all, right? And there is no other instances of the libraries on the system. |
|
This is all that is mentioned about SPI in syslog: Feb 26 07:38:44 raspberrypi olad: olad/PluginManager.cpp:195: Trying to start SPI |
|
Ok, so sometimes it helps digging through the whole filesystem to find old versions. I was wondering why things didn't seem to update at all. For some reason there was a version installed in /usr/lib/arm-linux-gnueabihf/, I got things to show up now. 8/ |
…d RDM version value.
|
How are you getting on with this @Ph0N37Ic5 ? |
|
Has this effort been abandoned, or is the core functionality merged and funcional (WS2812B support via SPI plugin)? |
|
Sorry, I missed this before @Ph0N37Ic5
I think this has been fixed in a later master. Otherwise can you confirm OS version and compiler version please (or ideal share all of config.log)
That's fine, it just means you can't use the Logic sniffer to decode RDM, I don't think they do an ARM version of their libs anyway... |
Hi @petiepooo , That would be a question for @Ph0N37Ic5 . I'm not sure if they'd managed to test the code in this PR with any hardware after their initial tests. Do you have hardware? Are you interested in helping out? The code doesn't look too far away from being merge-able if it works. Also see some other options mentioned in #578 (e.g. Fadecandy support, with a few limitations) has been in for a while. |
|
Is there any update? How could I get Ws2812b leds running via OLA on my Pi 4? I have already a working C-program, with which I can control at least 1000 WS2812 leds (for help: https://github.com/jbentham/rpi). Now I wish I could build a system that takes Artnet Signal as Input and turn it into WS signal (eventually combined with my working code). As a newbie I would like to know if OLA can help me for this target. Otherwise what else could you recommend to me? |
No real change from the project side since last time this was asked:
I'm happy to help if someone wants to look at the code further, if you have some hardware on hand you could try compiling @Ph0N37Ic5 's code and see if it works, and give us some feedback if not and we can probably give some pointers as to what might be wrong.
Again, to avoid mixing and fragmenting discussion, see the more general thread here where I've responded to your other library suggestion ( #578 (comment) ):
|
There was a problem hiding this comment.
A few more comments, for @ryanriccio1 's benefit as much as anything else.
| return; | ||
| } | ||
|
|
||
| // Grab RGB data for conversion to GBR |
There was a problem hiding this comment.
I think this is a typo based on the other use and the code.
| // Grab RGB data for conversion to GBR | |
| // Grab RGB data for conversion to GRB |
| OLA_ASSERT_EQ(3u, backend.Writes(0)); | ||
|
|
||
| // test4 | ||
| // tests what happens if fewer then needed color information are received |
There was a problem hiding this comment.
SPaG
| // tests what happens if fewer then needed color information are received | |
| // tests what happens if fewer than needed slots of color information are received |
| // test7 | ||
| // test for multiple ports | ||
| // StartFrame is only allowed on first port. | ||
| SPIOutput::Options option1(1, "second SPI Device"); | ||
| // setup pixel_count to 2 (enough to test all cases) | ||
| option1.pixel_count = 2; | ||
| // setup SPIOutput | ||
| SPIOutput output1(m_uid, &backend, option1); | ||
| // set personality | ||
| output1.SetPersonality(this_test_personality); | ||
| // setup some 'DMX' data | ||
| buffer.SetFromString("1, 10, 100"); | ||
| // simulate incoming data | ||
| output1.WriteDMX(buffer); | ||
| // get fake SPI data stream | ||
| data = backend.GetData(1, &length); | ||
| // this is the expected spi data stream: | ||
| // StartFrame is missing --> port is >0 ! | ||
| const uint8_t EXPECTED7[] = { 0x92, 0x4D, 0x34, // Pixel 1 Green (10) | ||
| 0x92, 0x49, 0x26, // Pixel 1 Red (1) | ||
| 0x9B, 0x49, 0xA4, // Pixel 1 Blue (100) | ||
| 0x92, 0x49, 0x24, // Pixel 2 Green (0) | ||
| 0x92, 0x49, 0x24, // Pixel 2 Red (0) | ||
| 0x92, 0x49, 0x24 // Pixel 2 Blue (0) | ||
| }; | ||
| // check for Equality | ||
| OLA_ASSERT_DATA_EQUALS(EXPECTED7, arraysize(EXPECTED7), data, length); | ||
| // check if the output writes are 1 | ||
| OLA_ASSERT_EQ(1u, backend.Writes(1)); | ||
| } |
There was a problem hiding this comment.
This protocol doesn't seem to have a start frame, so I'm not sure this test is required.
|
|
||
| // simulate incoming dmx data with this buffer | ||
| DmxBuffer buffer; | ||
| // setup an pointer to the returned data (the fake SPI data stream) |
There was a problem hiding this comment.
SPaG
| // setup an pointer to the returned data (the fake SPI data stream) | |
| // setup a pointer to the returned data (the fake SPI data stream) |
| // test3 | ||
| // test what happens when only new data for the first leds is available. | ||
| // later data should be not modified so for pixel2 data set in test2 is valid | ||
| buffer.SetFromString("34,56,78"); | ||
| output.WriteDMX(buffer); | ||
| data = backend.GetData(0, &length); | ||
| const uint8_t EXPECTED3[] = { 0x93, 0x6D, 0x24, // Pixel 1 Green (56) | ||
| 0x93, 0x49, 0x34, // Pixel 1 Red (34) | ||
| 0x9A, 0x4D, 0xB4, // Pixel 1 Blue (78) | ||
| 0x93, 0x6D, 0x24, // Pixel 2 Green (56) | ||
| 0x93, 0x49, 0x34, // Pixel 2 Red (34) | ||
| 0x9A, 0x4D, 0xB4 // Pixel 2 Blue (78) | ||
| }; | ||
| OLA_ASSERT_DATA_EQUALS(EXPECTED3, arraysize(EXPECTED3), data, length); | ||
| OLA_ASSERT_EQ(3u, backend.Writes(0)); |
There was a problem hiding this comment.
I'm not sure this is relevant for the combined mode?
| OLA_ASSERT_EQ(3u, backend.Writes(0)); | ||
|
|
||
| // test4 | ||
| // tests what happens if fewer then needed color information are received |
There was a problem hiding this comment.
| // tests what happens if fewer then needed color information are received | |
| // tests what happens if fewer than needed slots of color information are received |
| // test7 | ||
| // test for multiple ports | ||
| // StartFrame is only allowed on first port. | ||
| SPIOutput::Options option1(1, "second SPI Device"); | ||
| // setup pixel_count to 2 (enough to test all cases) | ||
| option1.pixel_count = 2; | ||
| // setup SPIOutput | ||
| SPIOutput output1(m_uid, &backend, option1); | ||
| // set personality | ||
| output1.SetPersonality(this_test_personality); | ||
| // setup some 'DMX' data | ||
| buffer.SetFromString("1, 10, 100"); | ||
| // simulate incoming data | ||
| output1.WriteDMX(buffer); | ||
| // get fake SPI data stream | ||
| data = backend.GetData(1, &length); | ||
| // this is the expected spi data stream: | ||
| // StartFrame is missing --> port is >0 ! | ||
| const uint8_t EXPECTED7[] = { 0x92, 0x4D, 0x34, // Pixel 1 Green (10) | ||
| 0x92, 0x49, 0x26, // Pixel 1 Red (1) | ||
| 0x9B, 0x49, 0xA4, // Pixel 1 Blue (100) | ||
| 0x92, 0x4D, 0x34, // Pixel 2 Green (10) | ||
| 0x92, 0x49, 0x26, // Pixel 2 Red (1) | ||
| 0x9B, 0x49, 0xA4 // Pixel 2 Blue (100) | ||
| }; | ||
| // check for Equality | ||
| OLA_ASSERT_DATA_EQUALS(EXPECTED7, arraysize(EXPECTED7), data, length); | ||
| // check if the output writes are 1 | ||
| OLA_ASSERT_EQ(1u, backend.Writes(1)); | ||
| } |
There was a problem hiding this comment.
Again I'm not convinced this is required.
|
@peternewman I did try #1382 and although the software side of things went well, when it outputted to the WS2811, it would flicker and the first LED in the strand would not have the correct color. I assume it works well with WS2812b just fine but haven't tested it as it isn't my use case. I also don't have the know-how to debug it on that low of a level and I don't have a scope. Originally posted by @ryanriccio1 in #1740 (comment) |
|
I have the hardware to test this ( scope and lots of different led tape ), but not the expertise to get this to build. Happy to test is someone can help with the software side. |
Currently untested, need hardware, but compiles.