feat(api): block robot movement when Estop is engaged or missing #13147
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## edge #13147 +/- ##
==========================================
- Coverage 72.25% 71.74% -0.51%
==========================================
Files 2399 1578 -821
Lines 66420 52242 -14178
Branches 7385 3309 -4076
==========================================
- Hits 47989 37482 -10507
+ Misses 16661 14239 -2422
+ Partials 1770 521 -1249
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sfoster1
approved these changes
Jul 21, 2023
…en, and set up Robot Server to actually hide settings that should be hidden
Added the feature flag and tested that 1) it lets movements run without an estop, and 2) it does not show up on the HTTP endpoints |
…-when-Estop-is-physically-or-logically-engaged # Conflicts: # api/src/opentrons/config/advanced_settings.py # api/src/opentrons/config/feature_flags.py # api/tests/opentrons/config/test_advanced_settings_migration.py
|
With latest commits, tested that the |
shlokamin
deleted the
RCORE-809-Block-access-to-all-robot-movement-when-Estop-is-physically-or-logically-engaged
branch
TamarZanzouri
pushed a commit
that referenced
this pull request
Sep 13, 2023
) * Raise exceptions if the estop is not in the DISENGAGED state during any movements * Added nicer error messages * Added feature flag to enable movement without an estop * Fixed broken test * un-expose the estop setting over HTTP * Add the estopNotRequired setting back to definitions but make it hidden, and set up Robot Server to actually hide settings that should be hidden * Fix settings migration tests * If the estopNotRequired setting is enabled, always return that the estop is present
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.
Overview
If the OT-3 Estop is engaged, or if there is no Estop plugged in, the robot should not be allowed to move. This includes the "logically engaged" state, where an Estop has been pressed but not acknowledged.
We block these movements with a simple function wrapper that checks the current estop status. If it is anything other than "disengaged," the OT3Controller will refuse to move and it will raise one of our nice exceptions about it.
Test Plan
On an OT-3, I tested moving the Estop state machine through all 4 of its states and sending a POST request to home the robot. In any state other than Disengaged, the robot responded with an error with the expected error message (with the correct error code!)
Changelog
estopNotRequiredthat will allow the hardware controller to function with no estop connected.Review requests
Do we need a way to disable this? I'm mostly thinking of any hardware-testing stuff that might not have an estop connected.
Risk assessment
If there are situations where we need to run OT3API functions without an Estop, this is pretty annoying.
Also, we should NOT merge this until the App supports acknowledging and clearing the Estop state. I will wait on @koji to decide when it is ready :)