Change to use TeensyDMX library

[ssilverman]

Also included is a fix to disallow packets having alternate start codes (ASC).

I can't test this locally, so I'm relying on you... :)

[netmindz]

Feels a but ugly, feature request for your library to have newFrame() method ;)

[ssilverman]

Reading and seeing what's read is one step instead of two. Also, because of the way the internals work—asynchronous—testing that there's a frame available needs a buffer to put that frame, otherwise there's a chance that the frame that's indicated as being there might be overwritten by the time you read it. TeensyDmx is a synchronous API. TeensyDMX is asynchronous. To fix it, your proposed "newFrame()" would need to be given a buffer, but this is just the same thing as "read(buffer)", just with a different name. The short of it is that the asynchronicity requires a slightly different API.

[ssilverman]

I'll add: a "newFrame()" API is possible, but it would require the library to have another 513-byte buffer. The user may only want to use, say, a 10-byte buffer, so the API as it is allows the user a little more control over the memory.

[netmindz]

Yeah it's a shame good old dmx doesn't have a sequence number like e1.31 has.

If(dmxRx.newFrame(dmxRxBuf))

Would still be much easier to read. I'm never a fan of code that needs to check on magic values, especially ones related to data being managed by underlying library

[ssilverman]

DMX doesn’t need a sequence number because packets are sent over a serial connection. They’re guaranteed to be in order. E1.31 is sent over UDP, and UDP packets may arrive out of order, so a sequence number is needed.

Magic numbers in code are okay if they’re documented. In this case, it’s clearly stated that “5” is the desired number of bytes. Also, I think that “read(start, length)” is a pretty common idiom.

I hear you on the readability. This is certainly a little harder to read. How would you propose improving this without using yet another 513-byte buffer? Remember, this is asynchronous and so the state will change underfoot between any “newFrame” and “read the bytes” calls. The user code needs to supply a buffer.
Let me update the code…

[ssilverman]

I typed that on my phone, kept mistyping and hitting "Update comment" by mistake, and I also didn't read your suggestion properly. Hence the crossed-out line. I just did a force push with a new code suggestion. Tell me what you think.

The new newFrame() function needs to return the number of bytes read because it's possible that DMX packets can be smaller than 513 bytes. The code handles these edge cases: short packets and packets having a non-zero start code (ASC (Alternate Start Code)).

[ssilverman]

Some more theory of operation: If DMX packets are always 513 bytes, and if readPacket() is called with a 513-byte buffer, then the processor has to spend time copying all 513 bytes and keeping the mutex (in this case disabled interrupts) longer (because asynchronous), for the duration of the 513-byte copy. Even though most of the bytes are not needed. My original code without the "always read 513 bytes" change only locks interrupts for the time it takes to copy up to the number of bytes requested. With the new change, interrupts are locked for the time it takes to copy all 513 bytes, even though that's not needed here. I'd rather disable interrupts for the duration of a 10-byte copy than for the duration of a 513-byte copy.

The Teensy 4 is a very fast device, and so a 513-byte copy shouldn't take that long. However, it's still good practice to not hold a lock for longer than you need.

In summary:

1. Interrupts are disabled for the duration of the copy,
2. The duration of the copy is dictated by the number of bytes requested, and
3. Shorter is better.

[netmindz]

Also included is a fix to disallow packets having alternate start codes (ASC).

I can't test this locally, so I'm relying on you... :)

My teensy 4.1 turned out to be 3.5 so couldn't connect up the Ethernet port, have ordered a 4.1

[netmindz]

error: use of deleted function 'qindesign::teensydmx::Receiver::Receiver(const qindesign::teensydmx::Receiver&)'
int read = newFrame(dmxRx, dmxRxBuf);

[ssilverman]

The receiver should be passed as a reference. Looks like that was an error. newFrame should be:

static int newFrame(teensydmx::Receiver &dmxRx)

[ssilverman]

I’ll fix that up in about a day and a half or so.