USB PRO: Warnings for "normal" changed packages?

[rewolff]

I'm implementing an USB_PRO compatible device, reading the code that I'll have to work with...

https://github.com/OpenLightingProject/ola/blob/master/plugins/usbpro/GenericUsbProWidget.cpp#L273

Here a "changed" packet is handled. The format is designed to be able to compress changes by only sending the changed bytes not all bytes.

Now this implementation in OLA, that issues a warning if the packet is not the full maximum of 46 bytes, hints at that the original implementation was faulty.

Suppose DMX channel 30 and 160 have changed. I'd expect the device to send:

<start-of-packet: 7e><packet label: changed-data>  <length: bond, james: 0x007>
  <3: first change is at 3*8=24>  <0x40: the 6th byte has changes> <0x00: no changes in 32-39> <0> <0> <0> <newvalue for ch 30> <39 slots not sent> <end-of-packet: e7>

And then another

<length: 7>
  <20: first change is at 20*8=160>  <0x01: the frist byte has changes> <0x00: no more changes> <0> <0> <0> <newvalue for ch 160> <39 slots not sent>

I don't HAVE any original devices, so I can't test them. But I'd expect the olad console to be flooded with these warnings for devices doing "the right thing".

So... IMHO the warning and return should go. Wait! Actually... The check should be against the minimum packet length of 6 or 7. (minimum valid packet length is 6. But it'd be odd to send one. And "return" is a valid action. So I can live with a warning in that case. The realistic minimum is 7.

Agreed? Anybody?

Actually... I just realized, you should check agianst BIGGER than the maximum changed-data-packet as well.

if (length > sizeof(widget_data_changed)) {
    OLA_WARN << "Change of state packet was too big: " << length;
    return;
  }

[nomis52]

It's been a while, but the specification at https://dol2kh495zr52.cloudfront.net/pdf/misc/dmx_usb_pro_api_spec.pdf

It's not clear on how this is to be interpreted, one way to read it is that the data field varies from 1 to 40 bytes another is that the data field is always 40 bytes long.

Given that I've never seen this warning, I suspect that the official pro product does the latter. The check is there to avoid reading past the end of the data array if the packet is small but upper bits of the changed-bit-array are set.

[rewolff]

It says: "Size in bytes: 1 to 40". This means that the total packet size is 1+5 + 1-40. or 7 to 46. Clearly the size of this packet can be less than 46.
Then it says: "one byte is present for each set bit", i.e. NO byte is present for each cleared bit.
So I think the spec is quite clear...

Now, I don't mind some devices/implementations sending more than what is used, but the whole goal of this part of the protocol is to reduce the communication volume, so sending more bytes than necessary is nonsensical. (I suspect that some devices have a real serial link internally, so if that runs at 115200 kbps, you can't send the full 250kbps 40 fps DMX bandwidth to the host. So compressing the data by knowing a bit about the data involved is a good plan.

Let me Try to set up an experiment. (I already moved the borrowed real enttec pro from "near the door" to "near my desk".) :-)

[rewolff]

Update:
I can't get the enttec to receive data. At all. My own clone works. Not worth my time to investigate further. Sorry. Maybe the cable to the enttec is broken, something like that.

[rewolff]

OK. My bad. My transmitter wasn't transmitting. :-( Sorry about that.

I have a test where I send the current date/time as DMX data. The enttec reports:
00000000 7e 05 08 00 00 00 11 07 1f 0a 34 00 e7 |~.........4..|
0x11 = 17 from 2017. 7th mont, 31st day, 10 oclock (AM) 0x32=52 minutes and zero seconds.

After I changed it... it first reported:
00000000 7e 09 0b 00 00 3e 00 00 00 00 11 07 1f 0a 34 e7 |~....>........4.|
with an 11 byte long packet payload, consisting of the 1 startbyte, 5 bitfields and the five databytes. Surprisingly not listing the last byte that had a 1/60 chance of being zero at the time I switched. (Note the bitfield 0x3e contains five set bits).

From then on I get:
00000000 7e 09 07 00 00 40 00 00 00 00 01 e7 |~....@......|
packets with 7 databytes, only the seconds have changed.

OLA seems to "not use" the incremental reporting mode. It seems I accidentally caught the reason for this: You cannot determine the size of the received universe from the incremental data. From the first "change packet" in this case you might deduce that the universe was at a minimum 5 bytes. From then on you'd deduce the correct size of 6, because my test-universe has a nonzero ending. But if the universe had been 30 bytes, 10 RGB lamps with the last 5 lamps set to "off", you can't deduce if the universe is 30 bytes from the 15 changed bytes that you'd receive. A proper firmware would send the last byte of the universe at least once.

[rewolff]

UPDATE: This issue is now not only a "remove annoying message" but a real "it doesn't work" bug....

After doing the above tests, I was "on my way" to start working on my compatible hardware. So I stopped the hexdump and allowed ola to retake the device..... I now get messages like:

plugins/usbpro/EnttecUsbProWidget.cpp:503: Change of state packet was too small: 7

and because that results in the packet not getting processed, the universe doesn't update at all. This happened because I put the device in send changes only mode outside of ola and ola doesn't reset that. The code is there to almost make it work, just this sanity check is wrong.

[nomis52]

Please post a patch and either myself or @peternewman will look at it.

[rewolff]

In one place I've coded it like this:

  if ((length < 7) || (length > sizeof(widget_data_changed))) {
    OLA_WARN << "Change of state packet invalid: " << length;
    return;
  }

In another place I've written:

  if (length < 7) {
    OLA_WARN << "Change of state packet was too small: " << length;
    return;
  }

  if (length > sizeof (widget_data_changed)) {
    OLA_WARN << "Change of state packet was too big: " << length;
    return;
  }

The first was in a git repo, so I could extract a patch: http://prive.bitwizard.nl/len_invalid.patch

The file "plugins/usbpro/EnttecUsbProWidget.cpp" also needs to be patched.

Which version do you prefer? If you want I can create a pull-request with everything patched, if you tell me which one you like best. (as it's not supposed to happen, the user can't do much about it and an expert has enough information from "invalid", my preference would be to have the smaller code footprint)

[nomis52]

The former is fine.

[rewolff]

You want me to make a git pull? Ok.

[nomis52]

Yes please.

[peternewman]

Hi @rewolff . Can we have it in a fresh branch that's not your master one please, it's currently disappeared into the bottom of the #1161 PR.