Fixed oddity in mockendpoint

[rewolff]

The "+1" in the length is probably due to when XX bytes of DMX data is sent, there is also an error/status byte. This leads to

  packet->len = (data_len+1) & 0xff;
  packet->len_hi = (data_len+1) >> 8;

Probably this code was partially copied/half-fixed from code that worked like this.

[peternewman]

I'd agree that doesn't look right, there are also a few more instances:
grep -n -iIR "data_size + 1" plugins/usbpro/
plugins/usbpro/CommonWidgetTest.cpp:62: frame[3] = (data_size + 1) >> 8; // len hi
plugins/usbpro/MockEndpoint.cpp:339: frame[3] = (data_size + 1) >> 8; // len hi
plugins/usbpro/MockEndpoint.cpp:363: frame[3] = (data_size + 1) >> 8; // len hi

TBH they would probably all be a good candidate for SplitUInt16 to avoid such issues:
ola::utils::SplitUInt16(start, &msg[2], &msg[1]);

[peternewman]

I'm not sure you're theory is correct @rewolff as it's only the high byte being modified currently, which is then shifted, so the bottom 8 bits of it (that have just been modified) will be ignored in most scenarios (unless the low byte is already 255). This looks more like a mix up with pointer access, as you may expect similar code for the left hand side of the assignment, to populate the high byte in the byte array.

[rewolff]

Just to be sure what we're talking about:
The code now reads:

 packet->len = data_size & 0xff;
 packet->len_hi = (data_size+1) >> 8;

This means that for data_size is 0xff or 0x1ff (i.e. only two possible values), the packet would be filled with 0x1ff (0xff, 0x01) and 0x2ff (0xff 0x02) where I think those would be interpreted as 256 too much. For all other values the correct and the erroneous code deliver the exact same result.

[rewolff]

Here is the actual patch:

--- a/plugins/usbpro/MockEndpoint.cpp
+++ b/plugins/usbpro/MockEndpoint.cpp
@@ -360,7 +360,7 @@ uint8_t *MockEndpoint::BuildRobeMessage(uint8_t label,
   frame[0] = 0xa5;  // som
   frame[1] = label;
   frame[2] = data_size & 0xff;  // len
-  frame[3] = (data_size + 1) >> 8;  // len hi
+  frame[3] = data_size >> 8;  // len hi
 
   // header crc
   uint8_t crc = 0;

I'm not sure you're theory is correct @rewolff as it's only the high byte being modified currently, which is then shifted, so the bottom 8 bits of it (that have just been modified) will be ignored in most scenarios (unless the low byte is already 255).
Yes, the wrong code will only give different results if the bottom byte is 0xff.

I can't fanthom what train-of-thought might transport code that is normally seen on the left of the assignment to the right. But even if it exists the code as it stands is wrong.

As far as I can tell this code is only used when testing. And from what I've seen from the tests, they are not testing with anywhere near 255 databytes. So we've also found that the testing is not anywhere near good. Proper testing would test stuff like edge cases, which for DMX would be say 0, 1, 2 byte Universes and say 510, 511, 512 and 513 byte universes (the last one, 513, should be flagged as invalid). But when you test the 510 byte universe (+1 statusbyte = 511 bytes in the packet, incorrectly assembled into the packet as 0x2ff), you'd notice that the packet is flagged as wrong/bad/too long. And you'd find the error in the testing code.

That's how good testing would find this problem. Anyway, just like the other issue I reported (differences packet length verification), this is effectively dead code. Wrong and untested.

If "a quick grep" finds more of these places, I could re-issue the pull request with those fixed as well. But I'd fix them on a "from code inspection" basis, not because I can easily test them.

I tried improving the tests for the differences packets but my C++ skills are a bit lacking. I tried a new test-packet, and apparently the results do not compare. So I need to print the buffer to see if my test is wrong or the interpretation of the test-packet that I sent.

[peternewman]

I suspect it's because it's a test. Someone will have taken some working code to parse a byte array and reversed it (to generate one to test the original code with). Obviously that reversal requires swapping the value with what it's assigned to.

Yes, you're correct, the MockEndpoint and the CommonWidgetTest will both just be used for testing.

PRs certainly welcome to improve the tests; we've got an outstanding issue open to improve to 100% coverage. We also run coverage, so we can see what we are and aren't testing. https://coveralls.io/github/OpenLightingProject/ola So you can see which bits of the function we do currently test:
https://coveralls.io/builds/12840440/source?filename=plugins%2Fusbpro%2FEnttecUsbProWidget.cpp#L495

Looking at them, I strongly suspect they're all copied and pasted from the same code, so the same fix ought to be fine. When Travis runs make check should be sufficient to ensure you've not made it worse, combined with a PR review from me and/or @nomis52 (which is why all the code gets reviewed by someone else, even mine and Simon's).

In terms of your failing test for the DMXDiff packet, if you push up what you've got so far we can give you a hand to fix/improve it. E.g. include/ola/testing/TestUtils.h has OLA_ASSERT_DATA_EQUALS which we're already using to compare the message expected matches the message received, but it sounds like we're not using it to compare the resulting DMX frames. Indeed that looks like it's just a case of tweaking EnttecUsbProWidgetTest::ValidateDMX to use the other function; the other macro makes debugging such test failures much easier, as it does the printing automatically if it fails.

[rewolff]

Do you mind if we mis-appropriate this thread here for the testing stuff too? This one is dead: I'm going to grep for other occurrences of the same mistake and resubmit.

For the testing coverage:

  m_endpoint->SendUnsolicitedUsbProData(
      CHANGE_OF_STATE_LABEL,
      change_of_state_data,
      6 + 3 /* six header bytes, 3 databytes */ );
  m_ss.Run();
  std::cerr << buffer.ToString ();
  m_endpoint->Verify();
  OLA_ASSERT(m_got_dmx);

The "buffer" is the expected buffer contents. I can fill that buffer as far as I can see. What I need to know is: How do I debug/print the contents of the expected buffer, The current cerr << doesn't work, or at least I don't know where to find that output. I've also tried OLA_WARN << ... . Next question is: what's the "acutal" buffer called that Verify() uses to compare against buffer ....

[peternewman]

Yeah that should be fine, I don't think this PR is at risk of getting too big/complicated. You can just add a new commit on this branch to fix the other bits.

At the top, the DMX callback is set as follows:

  port->SetDMXCallback(ola::NewCallback(
      this,
      &EnttecUsbProWidgetTest::ValidateDMX,
      port,
      const_cast<const ola::DmxBuffer*>(&buffer)));

I think cerr may work, it all depends how you're calling/running the tests. However like I say, switching to the correct testing macro should work around these issues and avoid you re-inventing the wheel. The validation function is here:
https://github.com/OpenLightingProject/ola/blob/master/plugins/usbpro/EnttecUsbProWidgetTest.cpp#L312

It should just be a case of changing the OLA_ASSET, or actually perhaps just add the other one I mentioned above, in case we ever add more comparison code to DMXBuffer's == operator. GetRaw() will get the raw data and Size() the length to pass in.

[peternewman]

Hi @rewolff , how did you get on with adding OLA_ASSERT_DATA_EQUALS ? Do you want me to sort out that bit while you concentrate on the issues you were actually trying to fix?

[rewolff]

How I'm getting along? I'm getting a bit further with the work I'm supposed to be doing. Nothing DMX related. Sorry.

[peternewman]

Hi @rewolff we've now merged #1326 which significantly simplifies testing if two DmxBuffer's match by adding a macro specifically for that.