Embedded sender optimization #32
Conversation
It makes a huge difference when reading from an embedded system if you do things this way. That changes things from: Read a character <twiddle thumbs> Read the rest of the packet after possible buffer overrun has occurred to: Read everything upfront <decide on what to do with that data> This is good practice in general and, additionally, was required in order to get this code to work in my case.
I think this is a good idea but am less certain that it needs to be included. Are there acceptable cases where the length doesn’t match what was requested that should be allowed?
I copied my changes to this branch wrong. Missed a few important character.
Trying to support python 2.7 in a reasonable way. Not sure if there is a better way of accomplishing this.
|
Thank you @dcliftreaves for the contribution, it's very appreciated. I think I get the idea here, previously:
You propose to
Which helps with timing issues for some systems, presumably where OS read calls are demanded to be fast enough to keep up with the endpoint (perhaps some very fast baudrate transfer on a serial line without flow control?). I'd only be certain about merging if there are test cases to cover at least some of the edge cases here -- bad sequence numbers, badly sized blocks, timeouts, etc. that this code change makes adjustments to, I'd be worried about introducing new errors for existing users in this case, so test cases would make me much more comfortable. I'll hope to try to take a look at this / write test cases soon but no promises, anyone else please join review! |
|
This PR pretty much mirrors the issue for #19, which was for send() |
|
Thank you for your kind response! Your comment: "perhaps some very fast baudrate transfer on a serial line without flow control?" Is right on the money. Most modern UARTs on embedded systems only use two IO lines which mean that they can only use software flow control which isn't normally used during xmodem transfers. Also, yes to this: This PR pretty much mirrors the issue for #19 I am not sure I can help too much with test cases as they are fairly complex. I will try to look at adding one or two test cases. |
|
Hope to get to this over the holiday! |
When an incorrect header is received it doesn't try to resync properly. It just quickly infinite loops and then gives up.
1 similar comment
The "read sequence" section of the recv method uses a method for reading serial that makes it more likely when talking to embedded systems to miss characters or to cause timeout issues.
Generally, if you can try to read as much as possible as early as possible, you should. Also, not doing that makes it hard to reason about the delay times in the system. The getc function is called with the same timeout numerous times allowing those timeouts to stack up. Also, it can lead to bugs like in the failure case of the sequence check where no timeout is given.
Instead, the code now tries to consume all the data that the producer is expected to send without response. Then, the code conditionally strips out the subset of that data to interpret.
There is a comment that I leave up to you to remove which is to cancel the processing of data if the lengths don’t match up. I am less certain about this specific change.