-
Notifications
You must be signed in to change notification settings - Fork 39
Feature/laiyu liquid test #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/laiyu liquid test #175
Conversation
Reviewer's GuideRefactors the Laiyu liquid test stepper driver for more robust, non-throwing Modbus communication and quieter logging, and cleans up the device registry YAML (though merge conflict markers remain). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- There are unresolved merge conflict markers in unilabos/registry/devices/laiyu_liquid_test.yaml (<<<<<<<, =======, >>>>>>>); please resolve these and pick a single value for placeholder_keys and the port default before merging.
- send_request now swallows all exceptions and returns None after retries; consider logging the last error (and possibly the frame) when giving up so that communication issues remain diagnosable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are unresolved merge conflict markers in unilabos/registry/devices/laiyu_liquid_test.yaml (<<<<<<<, =======, >>>>>>>); please resolve these and pick a single value for placeholder_keys and the port default before merging.
- send_request now swallows all exceptions and returns None after retries; consider logging the last error (and possibly the frame) when giving up so that communication issues remain diagnosable.
## Individual Comments
### Comment 1
<location> `unilabos/devices/laiyu_liquid_test/xyz_stepper_driver.py:172-174` </location>
<code_context>
def read_registers(self, addr: int, start: int, count: int) -> Optional[List[int]]:
"""
读保持寄存器:
- 成功时返回寄存器列表 List[int]
- 失败时返回 None(send_request 已经做了重试)
"""
payload = struct.pack(">HH", start, count)
resp = self.send_request(
addr,
ModbusFunction.READ_HOLDING_REGISTERS.value,
payload,
)
if resp is None:
return None
byte_count = resp[2]
regs: List[int] = []
for i in range(0, byte_count, 2):
regs.append(struct.unpack(">H", resp[3 + i:5 + i])[0])
return regs
</code_context>
<issue_to_address>
**suggestion (code-quality):** Convert for loop into list comprehension ([`list-comprehension`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/list-comprehension/))
```suggestion
regs: List[int] = [
struct.unpack(">H", resp[3 + i : 5 + i])[0]
for i in range(0, byte_count, 2)
]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Improve robustness and logging behavior for the Laiyu liquid test XYZ stepper driver and adjust its device registry configuration.
Bug Fixes:
Enhancements:
Chores: