Feat: Automation beta #853
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements automation beta functionality for probe manipulation, introducing automated trajectory computation, visualization, and multi-stage drive operations for target insertion and exit workflows. The changes enable automated probe movement with safety margins, real-time progress tracking, and state management for complex insertion procedures.
Key Changes:
- Added comprehensive automation logic for driving probes to target entry coordinates and insertion points with multi-stage trajectory planning
- Implemented trajectory visualization with color-coded line renderers for AP, ML, and DV movement stages
- Refactored probe depth handling to bake depth directly into APMLDV coordinates instead of maintaining separate depth fields
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
Assets/UI/Views/AutomationInspector.uxml |
Standardized XML formatting by converting empty elements to self-closing tags and fixing button naming from double underscores to double dashes |
Assets/Scripts/Utils/EphysLinkConstants.cs |
Bumped minimum EphysLink version requirement from 2.2.1 to 2.2.2 |
Assets/Scripts/UI/Views/AutomationInspectorView.cs |
Connected UI button click handlers for drive, stop, and exit commands |
Assets/Scripts/UI/ViewModels/AutomationInspectorViewModel.cs |
Implemented core automation logic including trajectory computation, multi-stage drive operations, ETA calculations, and state management for insertion/exit workflows |
Assets/Scripts/Services/EphysLinkService.cs |
Refactored visualization probe positioning to bake depth into APMLDV coordinates and removed redundant position change optimization |
Assets/Scripts/Pinpoint/Probes/ProbeController.cs |
Removed deprecated VisualizationLocalDepth field as depth is now incorporated into APMLDV |
Assets/Scripts/Models/Scene/SceneReducers.cs |
Fixed logic bug to correctly reject visualization probes as target insertion probes |
Assets/Scripts/Models/Scene/ManipulatorState.cs |
Added default value of 50 µm for DrivePastDistance field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (EphysLinkService.HasError(driveToNearTargetResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to drive to near target: {driveToNearTargetResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| case AutomationProgressState.DrivingToPastTarget: | ||
| // Drive past target at reduced speed. | ||
| var driveToPastTargetResponse = await _ephysLinkService.SetDepth( | ||
| new SetDepthRequest( | ||
| manipulatorId, | ||
| targetDepth + drivePastDistance, | ||
| baseSpeed * NEAR_TARGET_SPEED_MULTIPLIER | ||
| ) | ||
| ); | ||
|
|
||
| if (EphysLinkService.HasError(driveToPastTargetResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to drive past target: {driveToPastTargetResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| break; | ||
|
|
||
| case AutomationProgressState.ReturningToTarget: | ||
| // Return to target at reduced speed. | ||
| var returnToTargetResponse = await _ephysLinkService.SetDepth( | ||
| new SetDepthRequest( | ||
| manipulatorId, | ||
| targetDepth, | ||
| baseSpeed * NEAR_TARGET_SPEED_MULTIPLIER | ||
| ) | ||
| ); | ||
|
|
||
| if (EphysLinkService.HasError(returnToTargetResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to return to target: {returnToTargetResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
The method returns early on various error conditions (lines 693, 713, 733) without completing the operation or cleaning up state. Similar to the DriveToTargetEntryCoordinate method, consider adding comprehensive error handling and cleanup logic to prevent the automation state machine from getting stuck in an intermediate state.
| if (EphysLinkService.HasError(exitToDuraResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to exit to dura: {exitToDuraResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| break; | ||
|
|
||
| case AutomationProgressState.ExitingToMargin: | ||
| // Reset dura offset. | ||
| _storeService.Store.Dispatch( | ||
| SceneActions.RESET_DURA_OFFSET, | ||
| manipulatorId | ||
| ); | ||
|
|
||
| // Exit to the safe margin above the Dura. | ||
| var exitToMarginResponse = await _ephysLinkService.SetDepth( | ||
| new SetDepthRequest( | ||
| manipulatorId, | ||
| duraDepth - DURA_MARGIN_DISTANCE, | ||
| baseSpeed * EXIT_DRIVE_SPEED_MULTIPLIER | ||
| ) | ||
| ); | ||
|
|
||
| if (EphysLinkService.HasError(exitToMarginResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to exit to margin: {exitToMarginResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| break; | ||
|
|
||
| case AutomationProgressState.ExitingToTargetEntryCoordinate: | ||
| // Drive to the target entry coordinate. | ||
| var entryPosition = ConvertInsertionAPMLDVToManipulatorPosition( | ||
| _trajectoryCoordinates.third, | ||
| manipulatorState | ||
| ); | ||
| if (entryPosition == null) | ||
| { | ||
| Debug.LogError( | ||
| "Failed to convert entry coordinate to manipulator position" | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| var exitToEntryCoordinateResponse = await _ephysLinkService.SetPosition( | ||
| new SetPositionRequest( | ||
| manipulatorId, | ||
| entryPosition.Value, | ||
| AUTOMATIC_MOVEMENT_SPEED | ||
| ) | ||
| ); | ||
|
|
||
| if (EphysLinkService.HasError(exitToEntryCoordinateResponse.Error)) | ||
| { | ||
| Debug.LogError( | ||
| $"Failed to exit to entry coordinate: {exitToEntryCoordinateResponse.Error}" | ||
| ); | ||
| return; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Similar error handling issue exists in the InsertionExit method. Early returns on errors (lines 842, 867, 882, 897) don't clean up the cancellation token or reset the automation state, potentially leaving the system in an inconsistent state.
| var baseSpeed = CustomInsertionSpeed / 1000f; | ||
| var drivePastDistance = DrivePastDistance / 1000f; | ||
|
|
||
| // Set up cancellation token. |
There was a problem hiding this comment.
Same issue as in InsertionDrive - the existing _insertionDriveCts could be overwritten without proper cleanup, leading to resource leaks.
| // Set up cancellation token. | |
| // Set up cancellation token. | |
| if (_insertionDriveCts != null) | |
| { | |
| _insertionDriveCts.Cancel(); | |
| _insertionDriveCts.Dispose(); | |
| } |
| // Store initial ETA for progress calculation. | ||
| _originalETA = ComputeEtaSeconds(targetProbeName, baseSpeed, drivePastDistance, manipulatorState); | ||
|
|
||
| try | ||
| { | ||
| while (AutomationProgressState != AutomationProgressState.AtTarget) | ||
| { |
There was a problem hiding this comment.
The original ETA value is stored but never updated during the drive operation. If the drive is paused and resumed, or if conditions change, the progress percentage calculation using this static _originalETA may become inaccurate. Consider recalculating or updating _originalETA when resuming from a paused state.
| // Store initial ETA for progress calculation. | |
| _originalETA = ComputeEtaSeconds(targetProbeName, baseSpeed, drivePastDistance, manipulatorState); | |
| try | |
| { | |
| while (AutomationProgressState != AutomationProgressState.AtTarget) | |
| { | |
| try | |
| { | |
| while (AutomationProgressState != AutomationProgressState.AtTarget) | |
| { | |
| // Recalculate ETA each iteration so progress remains accurate if conditions change. | |
| _originalETA = ComputeEtaSeconds(targetProbeName, baseSpeed, drivePastDistance, manipulatorState); |
| while (AutomationProgressState != AutomationProgressState.AtTarget) | ||
| { |
There was a problem hiding this comment.
The while loop condition checks AutomationProgressState, but this state could potentially be changed by external factors (user interactions, other async operations) during the async operations inside the loop. This could lead to race conditions or unexpected behavior. Consider capturing the state at the beginning of each iteration or using a more robust state machine pattern.
| while (AutomationProgressState != AutomationProgressState.AtTarget) | |
| { | |
| while (true) | |
| { | |
| var currentProgressState = AutomationProgressState; | |
| if (currentProgressState == AutomationProgressState.AtTarget) | |
| { | |
| break; | |
| } |
| /// <summary> | ||
| /// Cancellation token source for stopping ongoing insertion/exit operations. | ||
| /// </summary> | ||
| private CancellationTokenSource _insertionDriveCts; |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
No description provided.