Closed rohanpurohit closed 3 years ago
@ethanjli initial changes for updating the cobs interface.
@ethanjli This PR is ready for review. the failing test case is now resolved, we can discuss that in the meeting, both encode and decode work fine.
Changes from last review:
I'll test this on the microcontroller prior to merging. For records-keeping:
I tested this on the microcontroller and found a COBS-related regression: now many of the frames being sent by the microcontroller have invalid COBS encodings. For example, the first COBS encoder result is valid (7-byte input of
0x92 0xfd 0x4b 0xfa 0x00 0x01 0x02
, 8-byte encoded output of0x05 0x92 0xfd 0x4b 0xfa 0x03 0x01 0x02
), but the second COBS body sent has an input of 7 bytes (0x11 0x1d 0x3e 0x6c 0x01 0x01 0x04
) and an encoded output of 9 bytes (0x08 0x11 0x1d 0x3e 0x6c 0x01 0x01 0x04 0x00
), which is clearly an incorrect length. According to the debugger, what happens in the second COBS encoder function call is that we pass it the input buffer as listed above and a 9-element output buffer which was already dirty before the function call (with data0x05 0x92 0xfd 0x4b 0xfa 0x03 0x01 0x02 0x00
). The output buffer should be getting resized down to 8 elements, but it doesn't seem to be getting resized down.Can you see if you can reproduce this behavior in unit tests? e.g. if calling
COBS::encode('0x11 0x29 0x3e 0x6c 0x01 0x01 0x04')
, where theoutput_buffer
before the function call is empty or has 9 bytes of arbitrary data inside, results in a 9-byte buffer after the function call, this suggests that there is an issue with the algorithm; otherwise, we've probably revealed or introduced an issue in howBackendSender
orFrameSender
is either resetting/clearing or not resetting/clearinginput_buffer
andoutput_buffer
.Since this issue escaped both unit tests on
Util/COBS.h
and code review, I suspect that our change to the COBS functions and calling code has broken the contract around theoutput_buffer
parameter - for example, if the new version of the COBS encoder function isn't correctly resizingoutput_buffer
s for whatever reason, now that we've stopped resizing buffers in the calling code. But if there's any relationship to the external interface of the COBS functions, we should add a unit test to reproduce and assert the correct behavior or point out an incorrect way to use the functions. We should also make sure that, for functions with buffers as output parameters, we have unit tests checking whether the value of the output parameter after the function call depends on the value of the output parameter before the function call.
I will try to reproduce that.
I tested this on the microcontroller and it works fine. I deleted the previous comment which you've quoted, and I realized that issue was because my git GUI client lied to me that it had the most recent commits on this branch when it was actually stuck on 8ec1a68, and as a result I found that the COBS encoder function was behaving incorrectly because it wasn't resizing the output buffer (passed in as 9 bytes) down to 8 bytes (the appropriate size for a 7-byte input buffer), which is the exact problem pointed out in https://github.com/pez-globo/pufferfish-software/pull/300#discussion_r601961080 and resolved in 44f6e96.
Since this resizing behavior is an important part of the contract of the COBS util functions, we should add a note in the code comment documentation for the functions about how output_buffer
is supposed to be modified - it should be resized to the appropriate size for the input, and if it can't be properly resized then it returns a bounds error. And we should make sure we have unit test cases to check that the results we get from the encode and decode functions are independent of the data (and buffer length) which had been in output_buffer
before the encode/decode functions were called. This is implied in the specification for Firmware Test Pattern 1's WHEN: WHEN: the function is called with ... as input parameters and ... as output parameters, listed from left to right in order of parameters
- so we need to specify what the values given in the output parameters, which lets us determine in review if we have sufficient coverage of the possible values which can be given in the output parameters. Should we add some sort of clarification to the test pattern's specification about coverage of the input values of output parameters?
This PR: Updates the COBS interface:
Updates to COBSDecoder and COBSencoder classes:
Add test cases for Util encode_cobs and decode_cobs