epaperdisplay: fix delay when two_byte_sequence_length is true

[Sola85]

When playing around with custom LUT waveforms on an epaperdisplay, I needed two_byte_sequence_length=True, in order to be able to send more than 128 bytes of start_sequence data. However, I noticed that the delays between commands were read incorrectly in this case.

When two_byte_sequence_length=True, the adress of the delay (if present) is shifted back by 1 byte as compared to when two_byte_sequence_length=False, but prior to this PR the delay was always read from the same adress.

The memory layout for one command is: "command (1 byte), data_size (1-2 bytes), data (data_size bytes), delay (0-1 bytes)".

[jepler]

@dkulinski you added support for two byte sequence lengths back in #5874 -- if you have a chance you might want to make sure this change is compatible with your displays & their control sequences.

[jepler]

Thanks -- I think I understand why the existing code is wrong, but maybe you want to have it as delay_length_ms = *(data + data_size) so that it always refers to the next byte after the end of data. I think that's clearer overall, and might even result in smaller generated code.

[Sola85]

We can do *(data+datasize), but then I think we have to move the increment of data_size to after reading delay_length_ms. I can try this out this evening and update the PR.

[jepler]

I can't comment directly on this line of code because it's not touched by the change .. anyway, the position within the init sequence is advanced by i += 2 + data_size; and then an additional byte in the case of 2-byte lengths. This doesn't seem to take into account the presence of a delay value at all. I'm probably overlooking something, but just in case it jiggles someone's brain I wanted to mention it.

[Sola85]

I can't comment directly on this line of code because it's not touched by the change .. anyway, the position within the init sequence is advanced by i += 2 + data_size; and then an additional byte in the case of 2-byte lengths. This doesn't seem to take into account the presence of a delay value at all. I'm probably overlooking something, but just in case it jiggles someone's brain I wanted to mention it.

data_size in incremented half-way through the loop to include the delay byte conditionally. It also took me a while to understand what is going on here... This entire section would be a good canditate for refactoring, but I wanted to propose the minimal change possible to fix the issue.

[jepler]

Thanks, I missed that.

If you'd prefer not to tinker further with the code and call it good, I'm OK with that as well.

[dhalbert]

I'll test on a MagTag, just to check for regression.

[Sola85]

Thanks, I missed that.

If you'd prefer not to tinker further with the code and call it good, I'm OK with that as well.

If you're ok with it and Dan's test is ok, then let's just leave it as it is for now.

[dhalbert]

Works on a MagTag