Conversation
|
@tginsbu1 Can you add a detailed PR description on what the proposed changes are? |
| self.gripper_place(home, target, target_approach_axis, target_approach_distance) | ||
|
|
||
| @action() | ||
| @action |
| self.logger.log_error(f"FAILED EMERGENCY STOP: {err}") | ||
| return ActionFailed(errors=str(err)) | ||
|
|
||
| def safety_stop(self) -> AdminCommandResponse: |
There was a problem hiding this comment.
safety_stop lost its error handling
| description="Execute a transfer in between source and target locations using Robotiq grippers", | ||
| ) | ||
| @action | ||
| def gripper_transfer( |
There was a problem hiding this comment.
gripper_transfer now calls node-level actions instead of the interface. This is calling back into the node's own action methods rather than delegating to the interface layer. This is a design problem — the node level should orchestrate through the interface, not chain its own actions. It also bypasses any action lifecycle management (status tracking, error handling) the framework might apply to individual action calls
| description="Execute a transfer in between source and target locations using Robotiq grippers", | ||
| ) | ||
| @action | ||
| def gripper_transfer( |
There was a problem hiding this comment.
gripper_transfer, gripper_pick and gripper_place actions strip gripper_open/gripper_close parameters. The old versions of these actions accepted gripper_open and gripper_close as parameters. The new versions don't. This means callers lose the ability to configure grip positions per-call, which is important for handling different labware.
| try: | ||
| self.logger.log("Shutting down") | ||
| self.ur_interface.disconnect() | ||
| self.integrated_controller.ur_controller.disconnect() |
There was a problem hiding this comment.
shutdown_handler accesses internals of IntegratedController directly. This reaches two levels deep into the controller's internals. The IntegratedController should expose its own disconnect() method — the node shouldn't need to know about ur_controller inside it. Notably reset() calls self.integrated_controller.disconnect() which suggests that method does exist, making the shutdown call inconsistent.
| description="Execute a transfer in between source and target locations using Robotiq grippers", | ||
| ) | ||
| @action | ||
| def gripper_transfer( |
There was a problem hiding this comment.
Resource validation is missing from gripper_transfer, gripper_pick, and gripper_place. When resources are enabled, the source location should be validated to confirm it contains an item before a pick, and the target location should be validated to confirm it is empty before a place. This check should happen at the node level before the call is passed down to the interface layer.
| ur_model: str = "UR5e" | ||
| use_resources: bool = True | ||
| end_effector: Optional[UREndEffector] = None | ||
| resource_manager_url: Optional[AnyUrl] = None |
There was a problem hiding this comment.
You should not need to specify this as RestNodeConfig pulls it from the .env file. However you can add a flag for resource usage
There was a problem hiding this comment.
This version drops a large number of actions that existed in the previous version with no mention in the PR description — getj, getl, set_freedrive, set_movement_params, movej, movel, toggle_gripper, pick_tool, place_tool, gripper_screw_transfer, pipette_transfer, pipette_pick_and_move_sample, pipette_dispense_and_retrieve, pick_and_flip_object, remove_cap, place_cap, run_urp_program, set_digital_io, get_location, and e_stop. PR description should discuss which of these are being retired, which are planned to be re-implemented.
The kinematics conversion logic that was used to convert joint angle locations to linear poses via get_pose_from_joint_angles is no longer present in any of the actions. The ur_model field remains in URNodeConfig suggesting this was not intentionally deprecated, but it is never used anywhere in the new version. It is unclear whether this is an intentional design decision that needs to be documented. If the IntegratedController is expected to handle this conversion internally, that should be explicitly stated.
Dozgulbas
left a comment
Dozgulbas
left a comment
There was a problem hiding this comment.
Thank you for the effort put into this. The introduction of a UREndEffector enum for end effector configuration and the attempt to establish a cleaner abstract interface layer are steps in a reasonable direction. However, there are several significant concerns that need to be addressed before this can move forward.
PR Readiness
This PR is not in a reviewable state. As noted in the description itself, multiple components are still unimplemented — Pipette, Tool Changer, Screwdriver, Gripper speed and force control, and Freedive. A PR that introduces a structural interface change while leaving a significant portion of the functionality unimplemented cannot be meaningfully reviewed or merged. This should be converted to a Draft PR until all listed items are re-implemented.
Scope and approach
This PR represents a full module rewrite rather than an incremental enhancement. The existing codebase had an established layered architecture — REST Node → UR Interface → Tool Controllers → End Effector Drivers — that has been working in production. Replacing it in a single PR introduces significant risk, makes the review process very difficult, and leaves the codebase in a broken state during the transition. My recommended approach would be to introduce the new design incrementally alongside the existing code through a series of smaller focused PRs, validating each layer before moving to the next, rather than removing the existing implementation before the new one is complete.
Architectural concern
The current implementation moves tool-specific motion logic — pick, place, approach sequences, resource handling — directly into IntegratedController. This works for the gripper alone but will not scale as the other end effectors are re-implemented. Each end effector has its own connection lifecycle, motion sequences, and error handling that belongs in dedicated tool controller classes. My recommended approach is to retain the tool controller layer and pass URController into the tool controllers instead of the raw URx connection, which addresses the coupling concern while keeping each layer focused and maintainable. Collapsing all tool logic into IntegratedController will result in an increasingly large and difficult to maintain class as more end effectors are added.
|
|
||
| def create_end_effector_controller(self, end_effector: Optional[UREndEffector]) -> None: | ||
| """Create appropriate end-effector controller.""" | ||
| self.logger.log_info("Creating Robotiq 2-finger gripper controller") |
There was a problem hiding this comment.
This log line runs unconditionally regardless of what end effector is being created, or even if end_effector is None. It should either be removed or placed inside the if block with the correct end effector name
| WMTOOLCHANGER = "WMTOOLCHANGER" | ||
|
|
||
|
|
||
| end_effectors = {UREndEffector.ROBOTIQ2FINGERGRIPPER: Robotiq2FingerGripper} |
There was a problem hiding this comment.
end_effectors dict only maps ROBOTIQ2FINGERGRIPPER, other enum values will crash at runtime.
| above_goal = deepcopy(target_location) | ||
| above_goal[axis] += target_approach_distance | ||
|
|
||
| self.logger.info(f"Starting pick operation from source: {target_location}") |
There was a problem hiding this comment.
log message copy pasted from gripper pick
|
|
||
| self.end_effector.open() | ||
| if self.resource_client is not None: | ||
| object, _ = self.resource_client.pop(target_location.resource_id) |
There was a problem hiding this comment.
Bug in the resource update logic. This is calling pop on the target_location during a place operation, which is the wrong direction. It should be popping from the end effector resource and pushing to the target, mirroring what gripper_pick does in reverse.
There was a problem hiding this comment.
-
The name IntegratedController does not clearly indicate that this is the highest-level interface to the UR robot. Anyone looking to use the interface directly without going through the REST node may not find this an intuitive entry point. A more descriptive name that reflects its role in the stack, such as URInterface, would make the module's purpose clearer for both direct use and future maintenance.
-
The resource client calls in
gripper_pickandgripper_placeare placed at the end of each method, after all motion has completed. This breaks the logical order of operations — ingripper_pickthe resource should be transferred from the source to the end effector immediately after the gripper closes, not after the robot returns to the above-goal position. Similarly ingripper_placethe resource should transfer from the end effector to the target immediately after the gripper opens. As it stands, if any subsequent movement step fails after the physical pick or place has already happened, the resource manager will never be updated and will be out of sync with the actual robot state. The same problem exists in the other direction — if the resource client call itself fails, the movement steps will still have executed against a resource state that was never validated, leaving the physical and tracked states inconsistent. Resource state transitions should happen at the exact point in the sequence where the physical state changes. Additionally resource client calls don't have any error handling. If a pop or push fails there is no exception caught.
| if not isinstance(self.end_effector, Gripper): | ||
| raise Exception("End-effector is not a gripper, cannot perform pick operation") | ||
| if isinstance(source, LocationArgument): | ||
| source_location = source.representation["linear_coordinates"] |
There was a problem hiding this comment.
Both gripper_pick and gripper_place expect LocationArgument.representation to be a dict with linear_coordinates and joint_angles keys. I can see this would be useful but has this been validated against to LocationManager to confirm this dict structure is what it actually returns? Also we need to support using list type locations at the same time as EPICS Interfaces do not utilize MADSci LocationArguments and instead send an action requests with List type locations
| self.ur_controller.move_to_location(above_goal, linear_motion=True) | ||
| self.logger.info("Pick operation completed successfully") | ||
|
|
||
| def disconnect(self) -> None: |
There was a problem hiding this comment.
Missing ur_dashboard.clear_operational_mode() and ur_dashboard.disconnect() as the dashboard server tent to crash when connections left open.
| logger.addHandler(handler) | ||
| return logger | ||
|
|
||
| def disconnect(self): |
There was a problem hiding this comment.
URController has neither self.ur nor self.ur_dashboard — it only has self.ur_connection. The correct call here should be self.disconnect_ur() but integrated controller's disconnect method makes a call to disconnect method in ur_controller so that needs an update too.
| self.acceleration = 0.5 | ||
| self.velocity = 0.5 | ||
| self.robot_current_joint_angles = None | ||
| self.gripper_speed: int = None |
There was a problem hiding this comment.
gripper_speed and gripper_force are initialized but never used. These attributes are carried over from the old UR class but URController no longer manages any end effectors
| raise TypeError("Hostname cannot be None Type!") | ||
|
|
||
| self.hostname = hostname | ||
| self.resource_client = resource_client |
There was a problem hiding this comment.
resource_client, resource_owner, and tool_resource_id are accepted as constructor parameters and stored but never used/ Same situation — these were relevant in the UR class where the interface managed resources directly. In this design version resources are handled at the IntegratedController level
and merge
and merge
2 participants
Reworks the end effector logic using abstract base classes.
Still to re-implement:
Pipette-abstract and specific
Tool Changer - abstract and specific
Screwdriver - abstract and specific
gripper speed and force control
Freedive