Add support for OpenDeck boards #1746

paradajz · 2021-09-30T06:38:51Z

This PR adds support for OpenDeck boards.

What is OpenDeck?

OpenDeck is open-source platform for building MIDI controllers which supports many different boards (some AVR, some STM32). More info on the website. Since firmware version 6.0.0, I have added support for DMX - more specifically, through CDC interface, Enttec Widget API was implemented so that the boards are compatible with OLA. A drawback for my boards was that in default USB Pro implementation, DMX data was sent constantly to the boards, even if data hasn't changed. Since my boards do many other things other than DMX, performance issues arose (at least on AVR, STM32 can handle it). I've started discussion about it here.

What this PR does is:

Copies UltraDMX Pro device/widget files and renames them to OpenDeck device/widget files, respectively (UltraDMX Pro seemed to be the simplest one on which I could base my boards)
Implements only one output port (no input)
Changes a bit handling of serial numbers so that in OLA, they show up the same as USB serial number which I already use on my boards
Changes the handling of firmware version which gets shown in OLA Web interface (displayed in major.minor format)
Sends DMX data on board only if something has changed, and then:
- If the amount of changed channels is > 128, full DMX frame is sent
- Otherwise, send only the changed channels (two bytes for channel, one byte for value)

plugins/usbpro/OpenDeckDevice.h

peternewman

Some initial linting comments to start off with

plugins/usbpro/OpenDeckDevice.cpp

plugins/usbpro/OpenDeckDevice.h

plugins/usbpro/OpenDeckWidget.cpp

peternewman

There are four outstanding lint issues from cpplint:
https://app.travis-ci.com/github/OpenLightingProject/ola/jobs/540772260
https://app.travis-ci.com/github/OpenLightingProject/ola/jobs/540772260#L1316

Plus a few other comments.

plugins/usbpro/OpenDeckDevice.cpp

plugins/usbpro/OpenDeckWidget.cpp

plugins/usbpro/OpenDeckWidget.h

plugins/usbpro/OpenDeckDevice.h

plugins/usbpro/OpenDeckDevice.cpp

peternewman · 2021-10-23T00:53:05Z

I'm pleased to say the CI is now progressing a lot further than it was. You just need to implement your widget in the tests and I think you'll be good.

The relevant file is:
https://github.com/OpenLightingProject/ola/blob/master/plugins/usbpro/WidgetDetectorThreadTest.cpp

You can also run the tests locally with the usual make check.

You might also like to add a test for your firmware versioning (possibly pulling it out into a standalone function too).

paradajz · 2021-10-23T14:11:43Z

I'm pleased to say the CI is now progressing a lot further than it was. You just need to implement your widget in the tests and I think you'll be good.

The relevant file is: https://github.com/OpenLightingProject/ola/blob/master/plugins/usbpro/WidgetDetectorThreadTest.cpp

You can also run the tests locally with the usual make check.

You might also like to add a test for your firmware versioning (possibly pulling it out into a standalone function too).

I'm pretty lost with these tests... Not sure what the tests are doing, what's the expected data and how to interpret error I'm getting when running them, which is:

==================================
   OLA 0.10.8: ./test-suite.log
==================================

# TOTAL: 82
# PASS:  81
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: plugins/usbpro/WidgetDetectorThreadTester
===============================================

.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x6e5d80
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x7a70 (Open Lighting), device Id: 0x0001 (Unittest Device), serial: 0x12345678, f/w version: N/A
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0001730
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x6864 (JESE), device Id: 0x0002 (RDM-TRI), serial: 0x12345678, f/w version: N/A
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a00015c0
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x4744 (Goddard Design), device Id: 0x444d (DMXter4), serial: 0x12345678, f/w version: N/A
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0001af0
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x0000, device Id: 0x0000, serial: 0x12345678, f/w version: 1.4
plugins/usbpro/WidgetDetectorThread.cpp:324: Defaulting to a Usb Pro device
plugins/usbpro/WidgetDetectorThread.cpp:340: USB Pro Firmware >= 2.4 is required for RDM support, this widget is running 1.4
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0001a50
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x0000, device Id: 0x0000, serial: 0x12345678, f/w version: 1.4
plugins/usbpro/WidgetDetectorThread.cpp:324: Defaulting to a Usb Pro device
plugins/usbpro/WidgetDetectorThread.cpp:326: Found and unlocked an Enttec USB Pro Mk II
plugins/usbpro/WidgetDetectorThread.cpp:340: USB Pro Firmware >= 2.4 is required for RDM support, this widget is running 1.4
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0004140
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x0000, device Id: 0x0000, serial: 0x12345678, f/w version: 1.4
plugins/usbpro/WidgetDetectorThread.cpp:324: Defaulting to a Usb Pro device
plugins/usbpro/WidgetDetectorThread.cpp:326: Found and unlocked an Enttec USB Pro Mk II
plugins/usbpro/WidgetDetectorThread.cpp:340: USB Pro Firmware >= 2.4 is required for RDM support, this widget is running 1.4
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0004500
plugins/usbpro/UsbProWidgetDetector.cpp:323: USB Widget didn't respond to messages, esta id 0, device id 0
plugins/usbpro/UsbProWidgetDetector.cpp:326: Is device in USB Controller mode if it's a Goddard?
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 1 for 0x7f11a0004500
plugins/usbpro/RobeWidgetDetector.cpp:203: Detected Robe Device, UID : 5253:0200000a, Hardware version: 0x1, software version: 0xb, eeprom version 0x3
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0002170
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x6a6b (DMXking.com), device Id: 0x0002 (ultraDMX Pro), serial: 0x12345678, f/w version: N/A
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x7f11a0002330
plugins/usbpro/UsbProWidgetDetector.cpp:537: Detected USB Device: ESTA Id: 0x0d6b (Shantea Controls), device Id: 0x0002 (OpenDeck), serial: 0x12345678, f/w version: N/A
plugins/usbpro/WidgetDetectorThread.cpp:324: Defaulting to a Usb Pro device
Fplugins/usbpro/WidgetDetectorThread.cpp:144: 1 are still active
plugins/usbpro/WidgetDetectorThread.cpp:148: 0x7f11a0002330
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x6e3d40
plugins/usbpro/UsbProWidgetDetector.cpp:323: USB Widget didn't respond to messages, esta id 0, device id 0
plugins/usbpro/UsbProWidgetDetector.cpp:326: Is device in USB Controller mode if it's a Goddard?
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 1 for 0x6e3d40
plugins/usbpro/WidgetDetectorThread.cpp:396: no more detectors to try for  0x6e3d40
plugins/usbpro/WidgetDetectorThreadTest.cpp:142: Timeout triggered
.common/thread/Thread.cpp:200: Thread , policy SCHED_OTHER, priority 0
plugins/usbpro/WidgetDetectorThread.cpp:399: trying stage 0 for 0x6e4cf0
common/io/SelectServer.cpp:186: Removing an invalid file descriptor: 0x6e4cf0
plugins/usbpro/WidgetDetectorThreadTest.cpp:142: Timeout triggered
common/io/SelectServer.cpp:186: Removing an invalid file descriptor: 0x6e3d40


WidgetDetectorThreadTest.cpp:160:Assertion
Test name: WidgetDetectorThreadTest::testOpenDeckWidget
equality assertion failed
- Expected: 0
- Actual  : 3435

Failures !!!
Run: 11   Failure total: 1   Failures: 1   Errors: 0
FAIL plugins/usbpro/WidgetDetectorThreadTester (exit status: 1)

Also, I don't know what do \153, \15, \002 etc escape codes mean when defining manufacturer_data and device_data strings.

EDIT: Figured. Those are ESTA and device IDs. Fixed now.

peternewman

Some more tidying and comments/questions

plugins/usbpro/WidgetDetectorThreadTest.cpp

plugins/usbpro/WidgetDetectorThread.cpp

plugins/usbpro/UsbSerialPlugin.cpp

peternewman · 2021-10-24T01:47:18Z

plugins/usbpro/OpenDeckWidget.cpp

+                         reinterpret_cast<uint8_t*>(&full_frame),
+                         length + 1);
+    } else {
+      return SendMessage(DMX_LABEL, &send_buffer[0], send_buffer.size());


You currently appear to be using the same label for your special diff packets as well as normal ones! How do you tell a normal DMX packet (which happens to have fewer than 512 slots) from a special packet?

After the label byte comes data length LSB/MSB. Once I process those I know - if the size is 513 it's normal packet, otherwise it's diff mode.

Unfortunately it's not as simple as that.

Try running this on your OLA machine:
./examples/ola_set_dmx -u 0 -d 10,20,30,40,50

Or using the pre-compiled one the debs ship or whatever. That will send a DMX frame with only 5 slots in it, which will confuse your diff thing I'm sure. The standard only mandates some timing restrictions on DMX packets, they don't have to have all 512 slots plus the start code. Pedantically my 5 slot example is probably too short to be valid, but something around 30-40 slots can easily be acceptable.

Your most compliant option is probably to pick another label just for yourself (like others have done, and send that with your diff format), rather than re-using the existing diff label with a different format.

As an aside, I'd recommend making sure your code is hardened to issues like this too, e.g. if I say the length is one thing, then don't send that much data, or if with your diff format I address a slot that's greater than 512, or send the channel number (or only half of it), but not the slot value etc.

Oh! I didn't know that's possible / valid. Okay. I'll implement new label. I've put it in OpenDeckWidget.h - not sure if that's the correct place.

And sure, I'll handle the edge cases on firmware side once all the issues in this MR are solved.

Oh! I didn't know that's possible / valid. Okay. I'll implement new label. I've put it in OpenDeckWidget.h - not sure if that's the correct place.

That matches what the other devices do. We should sort you out a login for our wiki so you can add some documentation on this format:
https://wiki.openlighting.org/index.php/USB_Protocol_Extensions

plugins/usbpro/OpenDeckDevice.cpp

plugins/usbpro/OpenDeckWidget.h

paradajz · 2021-11-16T17:13:18Z

Had another look at this - not sure what to do next. Also don't know why the CI is failing, on my PC everything passes normally.

paradajz · 2022-01-18T08:43:51Z

Can I get some update on this?

peternewman · 2022-01-20T02:19:15Z

Sorry for not replying sooner @paradajz .

Had another look at this - not sure what to do next.

There are still a few outstanding comments (e.g. your own rate limiting key if you still want to do it). I'll see if the tests pass cleanly on a second run and try and re-review my outstanding comments and the code soon.

Also don't know why the CI is failing, on my PC everything passes normally.

I think that test has occasionally been a bit intermittent so it might clear with a re-run now you've fixed the broken test. I've (accidentally) updated the online version of your branch to match master, so you may need to pull on your development machine.

paradajz · 2022-01-23T22:49:20Z

Thanks for the reply - I've rebased on master to remove the merge commit.

Co-authored-by: Peter Newman <[email protected]>

paradajz · 2022-03-15T10:02:36Z

Hopefully no more changes are required. The CI is still failing but from what I see it's something completely unrelated (spell checker?).

peternewman · 2022-03-15T11:08:18Z

Hopefully no more changes are required.

🤞

The CI is still failing but from what I see it's something completely unrelated (spell checker?).

If you want to cherry-pick this commit it will shut it up:
0556472

peternewman

Really sorry, just the one niggle with the token bucket that I think will be worth testing in the long term.

peternewman · 2022-03-15T11:43:06Z

plugins/usbpro/OpenDeckDevice.h

+    if (m_bucket.GetToken(*m_wake_time)) {
+      return m_widget->SendDMX(buffer);
+    } else {
+      OLA_INFO << "Port rated limited, dropping frame";
+    }
+    return true;


While it will make it a little more complicated, you probably want to move the rate limiting check to within OpenDeckWidget.cpp, otherwise you risk wasting your tokens on unchanged packets which don't actually get sent, rather than saving them for the first diff or full packet that arrives!

Depending on your processing times, you might also choose to take another token (I think you can) if processing one (e.g. unpacking the diff) takes longer than the other one on your device.

Good point. I've added initial, maybe naive approach. Let me know what you think.

Any particular reason you haven't just moved all the TokenBucket code to OpenDeckWidget, rather than keeping it outside and passing it in?

I wanted to do that, but to create TokenBucket, I need some parameters first, namely:

max_burst, rate, max_burst, *wake_time

These aren't accessible in OpenDeckWidget. I might be missing something here.

This seems to be a bit beyond my understanding of OLA... Don't know what to do here.

Added simpler, alternative approach.

peternewman · 2022-03-15T11:51:05Z

plugins/usbpro/OpenDeckWidget.h

+
+ private:
+    static const size_t MAX_DIFF_CHANNELS = 128;
+    static const uint8_t DMX_SLOT_VALUE_DIFF_LABEL = 80;


Label 80 (aka 0x50) doesn't seem to appear in our codebase, so looks good to go! 🥳

peternewman · 2022-03-16T02:13:23Z

plugins/usbpro/OpenDeckWidget.cpp

+        return SendMessage(DMX_SLOT_VALUE_DIFF_LABEL,
                       &send_buffer[0],
                       send_buffer.size());


Fix the whitespace:

Suggested change

return SendMessage(DMX_SLOT_VALUE_DIFF_LABEL,

&send_buffer[0],

send_buffer.size());

return SendMessage(DMX_SLOT_VALUE_DIFF_LABEL,

&send_buffer[0],

send_buffer.size());

peternewman · 2022-03-16T02:14:44Z

common/utils/StringUtilsTest.cpp

-  
+


Thanks, pesky GitHub editor!

peternewman · 2022-03-16T02:17:11Z

plugins/usbpro/OpenDeckDevice.h

+    if (m_bucket.GetToken(*m_wake_time)) {
+      return m_widget->SendDMX(buffer);
+    } else {
+      OLA_INFO << "Port rated limited, dropping frame";
+    }
+    return true;


Any particular reason you haven't just moved all the TokenBucket code to OpenDeckWidget, rather than keeping it outside and passing it in?

peternewman

Minor comments

Nicked from 0556472

peternewman · 2022-03-18T11:32:29Z

plugins/usbpro/OpenDeckWidget.cpp

+bool OpenDeckWidget::IsBufferDiff(const DmxBuffer &buffer) {
+  return buffer != internal_buffer;
+}


Great minds! I literally just came here to write this after reviewing #1766 who's doing a very similar thing...

paradajz · 2022-05-06T13:20:30Z

So is this ready for merging?

paradajz commented Sep 30, 2021

View reviewed changes

plugins/usbpro/OpenDeckDevice.h Outdated Show resolved Hide resolved

peternewman linked an issue Oct 1, 2021 that may be closed by this pull request

Sending DMX data to USB only if changed? #1744

Open

peternewman requested changes Oct 1, 2021

View reviewed changes

paradajz force-pushed the support_for_opendeck branch from f9adc09 to 66b846b Compare October 1, 2021 12:07

peternewman requested changes Oct 10, 2021

View reviewed changes

paradajz force-pushed the support_for_opendeck branch from 66b846b to 7d06309 Compare October 11, 2021 06:54

peternewman reviewed Oct 11, 2021

View reviewed changes

plugins/usbpro/OpenDeckDevice.cpp Outdated Show resolved Hide resolved

paradajz force-pushed the support_for_opendeck branch from 7d06309 to b8a6513 Compare October 11, 2021 14:35

paradajz force-pushed the support_for_opendeck branch 2 times, most recently from 7a2f576 to 8073f38 Compare October 22, 2021 18:55

paradajz force-pushed the support_for_opendeck branch from 8073f38 to 2145408 Compare October 23, 2021 13:20

paradajz force-pushed the support_for_opendeck branch 3 times, most recently from 8babe5b to 771662a Compare October 23, 2021 14:51

peternewman requested changes Oct 24, 2021

View reviewed changes

paradajz force-pushed the support_for_opendeck branch from 5966a48 to 4c2f26f Compare October 24, 2021 07:45

peternewman reviewed Oct 24, 2021

View reviewed changes

plugins/usbpro/OpenDeckDevice.cpp Outdated Show resolved Hide resolved

paradajz force-pushed the support_for_opendeck branch from 4c2f26f to 1eb0c60 Compare October 24, 2021 10:14

peternewman reviewed Oct 25, 2021

View reviewed changes

plugins/usbpro/OpenDeckWidget.h Outdated Show resolved Hide resolved

paradajz force-pushed the support_for_opendeck branch 3 times, most recently from d9d2f6b to cd0633e Compare October 26, 2021 21:11

paradajz force-pushed the support_for_opendeck branch from 3a7e508 to 34d3255 Compare January 23, 2022 22:44

add support for opendeck boards

358bcf0

paradajz and others added 2 commits March 15, 2022 08:51

Update plugins/usbpro/OpenDeckWidget.cpp

3fb162f

Co-authored-by: Peter Newman <[email protected]>

Update plugins/usbpro/OpenDeckWidget.cpp

9fa3e5b

Co-authored-by: Peter Newman <[email protected]>

paradajz force-pushed the support_for_opendeck branch from 2559dd8 to ba7bdfe Compare March 15, 2022 09:07

paradajz and others added 3 commits March 15, 2022 10:40

Update plugins/usbpro/OpenDeckWidget.cpp

f80b3dc

Co-authored-by: Peter Newman <[email protected]>

format: extend ToHex with uppercase flag

3ca240c

OpenDeckDevice: use ToHex with uppercase flag

cdfbd16

paradajz force-pushed the support_for_opendeck branch from b7ccd82 to cdfbd16 Compare March 15, 2022 09:41

Test the new uppercase option for ToHex

2c0487f

peternewman added Type-DeviceSupport Component-Plugin labels Mar 15, 2022

peternewman added this to the 0.11.0 milestone Mar 15, 2022

peternewman added 3 commits March 15, 2022 11:28

Trivial whitespace tidy

3a3cd0a

Minor spelling tweak

73e64d2

Minor grammar tweak

c9682d4

peternewman reviewed Mar 15, 2022

View reviewed changes

paradajz added 4 commits March 15, 2022 14:11

OpenDeckDevice: remove unused arg from constructor doc

74c1149

OpenDeckWidget: naming consistency

75cba45

OpenDeckDevice: handle token bucket in Widget class

829adbf

misc whitespace

6013515

peternewman reviewed Mar 16, 2022

View reviewed changes

peternewman and others added 4 commits March 16, 2022 02:23

Ignore a codespell false positive due to line wrapping

4369fd8

Nicked from 0556472

whitespace

0b83176

opendeck: token bucket handling alt approach

d6142ee

fix new tests

161435b

peternewman mentioned this pull request Mar 18, 2022

Usbdmxcom #1766

Open

2 tasks

peternewman reviewed Mar 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OpenDeck boards #1746

Add support for OpenDeck boards #1746

paradajz commented Sep 30, 2021 •

edited

Loading

peternewman left a comment

peternewman left a comment

peternewman commented Oct 23, 2021

paradajz commented Oct 23, 2021 •

edited

Loading

peternewman left a comment

peternewman Oct 24, 2021

paradajz Oct 24, 2021

peternewman Oct 24, 2021

paradajz Oct 24, 2021

paradajz Oct 24, 2021

peternewman Oct 25, 2021

paradajz commented Nov 16, 2021

paradajz commented Jan 18, 2022

peternewman commented Jan 20, 2022

paradajz commented Jan 23, 2022

paradajz commented Mar 15, 2022

peternewman commented Mar 15, 2022

peternewman left a comment

peternewman Mar 15, 2022

paradajz Mar 15, 2022

peternewman Mar 16, 2022

paradajz Mar 16, 2022

paradajz Mar 16, 2022

paradajz Mar 16, 2022

peternewman Mar 15, 2022

peternewman Mar 16, 2022

paradajz Mar 16, 2022

peternewman Mar 16, 2022

peternewman Mar 16, 2022

peternewman left a comment

peternewman Mar 18, 2022

paradajz commented May 6, 2022

Add support for OpenDeck boards #1746

Are you sure you want to change the base?

Add support for OpenDeck boards #1746

Conversation

paradajz commented Sep 30, 2021 • edited Loading

peternewman left a comment

Choose a reason for hiding this comment

peternewman left a comment

Choose a reason for hiding this comment

peternewman commented Oct 23, 2021

paradajz commented Oct 23, 2021 • edited Loading

peternewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paradajz commented Nov 16, 2021

paradajz commented Jan 18, 2022

peternewman commented Jan 20, 2022

paradajz commented Jan 23, 2022

paradajz commented Mar 15, 2022

peternewman commented Mar 15, 2022

peternewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paradajz commented May 6, 2022

paradajz commented Sep 30, 2021 •

edited

Loading

paradajz commented Oct 23, 2021 •

edited

Loading