Fix Dutlink timeout handling after refactor#235
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py (3)
27-27: LGTM! Consider making the timeout configurable via environment variable.The default timeout of 20 seconds is reasonable given that power commands can take 10-15 seconds. However, to improve flexibility, consider making this configurable via an environment variable.
- timeout_s: int = field(default=20) # 20 seconds, power control sequences can block USB for a long time + timeout_s: int = field(default=int(os.getenv('DUTLINK_TIMEOUT_S', '20'))) # power control sequences can block USB for a long time
256-258: Fix line formatting for better readability.The line continuation could be formatted better.
- self.children["storage"] = DutlinkStorageMux(serial=self.serial, storage_device=self.storage_device, - timeout_s=self.timeout_s) + self.children["storage"] = DutlinkStorageMux( + serial=self.serial, + storage_device=self.storage_device, + timeout_s=self.timeout_s + )
Line range hint
211-225: Move timeout_s documentation to DutlinkConfig class.The docstring describes the
timeout_sparameter, but this parameter is defined in theDutlinkConfigclass. Consider moving this documentation to the appropriate class.@dataclass(kw_only=True) class DutlinkConfig: + """ + Parameters: + ---------- + serial : str or None + The serial number of the DUTLink device. Default is None. + timeout_s : int + The timeout in seconds for USB operations. Default is set to 20 seconds. + """ serial: str | None = field(default=None) timeout_s: int = field(default=20)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (1)
packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py (1)
56-56: LGTM! Verify timeout application at runtime.The conversion from seconds to milliseconds is correct and appropriately set at the USB device level.
Run this script to verify the timeout is properly applied:
✅ Verification successful
✅ Timeout implementation verified as correct and isolated
The timeout setting in
__post_init__is the only instance of timeout configuration in the codebase, ensuring it won't be accidentally overridden elsewhere.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any other places where USB timeouts might be set or overridden # This helps ensure our timeout setting isn't being overridden elsewhere rg -l "default_timeout|set_timeout|timeout.*=|\.timeout" --type pythonLength of output: 103
Script:
#!/bin/bash # Search for timeout and USB device configuration patterns in Python files rg -l "default_timeout|set_timeout|timeout.*=|\.timeout|usb.*timeout" "*.py" # Also search specifically in the driver directory fd -e py . packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/ -x rg -l "default_timeout|set_timeout|timeout.*=|\.timeout|usb.*timeout" {}Length of output: 351
| self.children["power"] = DutlinkPower(serial=self.serial, timeout_s=self.timeout_s) | ||
| self.children["storage"] = DutlinkStorageMux(serial=self.serial, storage_device=self.storage_device, | ||
| timeout_s=self.timeout_s) |
There was a problem hiding this comment.
💡 Codebase verification
Add timeout_s parameter to DutlinkSerial instantiation
For consistency with DutlinkPower and DutlinkStorageMux, pass the timeout_s parameter to DutlinkSerial:
self.children["console"] = DutlinkSerial(serial=self.serial, baudrate=self.baudrate, timeout_s=self.timeout_s)
🔗 Analysis chain
Verify if DutlinkSerial needs the timeout parameter.
The timeout is propagated to DutlinkPower and DutlinkStorageMux, but DutlinkSerial might also need it since it inherits from DutlinkConfig.
Run this script to analyze the inheritance and timeout usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze DutlinkSerial's inheritance chain and timeout usage
# Search for DutlinkSerial class definition and its base classes
ast-grep --pattern 'class DutlinkSerial($_)'
# Search for timeout usage in PySerial
rg -A 5 "timeout" --type python | grep -C 5 "class PySerial"
Length of output: 138
Script:
#!/bin/bash
# Search for DutlinkSerial class definition and inheritance
ast-grep --pattern 'class DutlinkSerial'
# Find all references to DutlinkSerial
rg "DutlinkSerial" -A 5 --type py
# Search for DutlinkConfig class and its timeout usage
ast-grep --pattern 'class DutlinkConfig'
rg "DutlinkConfig.*timeout" --type py
# Find timeout parameter usage in constructor
ast-grep --pattern 'def __init__($$$timeout$$$)'
Length of output: 984
Script:
#!/bin/bash
# Get DutlinkConfig implementation
rg -A 10 "class DutlinkConfig" --type py
# Get implementation of similar classes
rg -A 10 "class DutlinkPower|class DutlinkStorageMux" --type py
# Find all timeout-related parameters in the file
rg "timeout" --type py packages/jumpstarter-driver-dutlink/jumpstarter_driver_dutlink/driver.py
Length of output: 4217
|
Oh thanks Nick!, that was quick! :D |
2 participants
Some DUTLink targets have very long-running power on/off commands because those are
scripted internally and can take 10-15s, the timeout_s needs to be passed down to all
objects.
Summary by CodeRabbit