Give the port timeout a finite default - #36
Merged
Conversation
The timeout now defaults to 2000 milliseconds and applies to every read rather than only the first, so a device that stops responding fails the command instead of blocking indefinitely. Passing -1 waits indefinitely, and values below -1 are rejected. Main now returns the exit code from the parse result, so a failed command reports non-zero instead of always reporting success.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--timeouthad no default, so omitting it awaited the read with no bound at all and a device that never replied blocked until the process was terminated by hand. Neither that nor requiring an arbitrary number on every invocation is reasonable for a simple device query, so the option now defaults to 2000 milliseconds,-1waits indefinitely, and values below-1are rejected. The option is also applied to all five reads in the root command rather than to the identity read alone.Mainnow returns whatInvokeAsyncgives it. The tool previously exited 0 for every outcome, so a timeout, a missing port and a successful read were indistinguishable to anything scripted. A bounded failure nothing can detect is not much better than a hang.Why 2000 milliseconds
The number is not justified by steady-state latency, and it is worth recording why so nobody later reduces it on that basis. Measured against a Behavior device on this branch, five sequential register reads complete inside the smallest expressible timeout:
--timeout 1succeeds, as do 2, 3, 5 and 10. Total command time is 373 to 420 milliseconds across runs, dominated by process startup and opening the port rather than by device traffic.What the default has to cover is the post-reset window instead. Hardware measurements during the firmware update work found a device enumerated and openable at roughly 130 milliseconds but not answering Harp until 300 to 800. Against that, 2000 gives about 2.5x margin where 1000 would give 1.25x.
Verified against hardware
A continuous probe at
--timeout 100across a power cycle caught four distinct device states, which between them exercise every path this change touches.Access to the path 'COM3' is denied, once in 40 attempts, exit 1.The timeout case is the one that could not be tested before and it clustered at 486 to 555 milliseconds, which is startup plus a single timeout. That confirms the exception propagates out of the first read and the remaining four never run, so the worst case for the whole command is one timeout period rather than one per read.
The other reason to fail early
Beyond not making the user wait, there is a device-side argument. The ATxmega bootloader's receive path enters a busy-wait that spins until it has taken 522 further bytes, with no timeout and no escape, so a client that keeps feeding a port whose device state it does not know can park the device in a loop that only a reset clears. Its header matcher does self-clear after about a second of silence, so a partial header does not persist, but the data state has no such protection. Short-lived commands that close the port on failure are the right posture for that reason as much as for the user's.
Closes #35