Skip to content
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

fix(hardware): clear move group from node if move condition is met #12986

Conversation

ahiuchingau
Copy link
Contributor

Overview

The expected-stall move group times out whenever a stall is detected before the final move block, even though this is an acceptable behavior. This fixes it by clearing and finishing the move group whenever a move block is completed with a stall.

@ahiuchingau ahiuchingau requested a review from a team as a code owner June 26, 2023 21:46
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #12986 (8170fd3) into internal-release_0.13.0 (ecc3d81) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.13.0   #12986      +/-   ##
===========================================================
+ Coverage                    72.82%   72.83%   +0.01%     
===========================================================
  Files                         2364     2364              
  Lines                        64467    64514      +47     
  Branches                      7191     7191              
===========================================================
+ Hits                         46945    46988      +43     
- Misses                       15842    15847       +5     
+ Partials                      1680     1679       -1     
Flag Coverage Δ
app 71.46% <ø> (+<0.01%) ⬆️
hardware 59.19% <100.00%> (+0.18%) ⬆️
notify-server 89.13% <ø> (ø)
protocol-designer 44.44% <ø> (ø)
step-generation 85.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ons_hardware/hardware_control/move_group_runner.py 92.57% <100.00%> (-0.32%) ⬇️

... and 5 files with indirect coverage changes

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check looks good, but is there a way we could add a quick test for this scenario?

Copy link
Contributor

@fsinapi fsinapi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think this should just clear out the one single node from the move group, and still wait until every node has reported that it's done. The way this is written, other axes could still be moving after the one axis reports its stall, and the robot could move on and start a new move before they stop moving.

@ahiuchingau ahiuchingau changed the base branch from edge to internal-release_0.13.0 June 27, 2023 16:22
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, I think we don't want to do this only if the stop condition is stall - I think this should happen any time we're stopped_by_condition no matter what the condition is

@@ -422,6 +422,17 @@ def _handle_move_completed(
)
self._should_stop = True
self._event.set()
if (
stop_cond == MoveStopCondition.stall
and ack_id == MoveAckId.stopped_by_condition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want this to just be ack_id == MoveAckId.stopped_by_condition whether or not the stop condition is stall; I think you're going to get similar behavior from home commands, including the home backoffs, and the tip action motors, and definitely the stop-on-sync commands from calibration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right!

Copy link
Contributor

@fsinapi fsinapi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM!

@ahiuchingau ahiuchingau requested a review from sfoster1 June 27, 2023 21:16
@ahiuchingau ahiuchingau changed the title fix(hardware): clear move group when the expected stall is detected fix(hardware): clear move group from node if move condition is met Jun 27, 2023
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty looks good to me!

@ahiuchingau ahiuchingau force-pushed the hardware_finish-move-group-when-stall-condition-is-met branch from c5d49d8 to 8170fd3 Compare June 29, 2023 15:37
@ahiuchingau ahiuchingau merged commit 4f1efe7 into internal-release_0.13.0 Jun 29, 2023
@ahiuchingau ahiuchingau deleted the hardware_finish-move-group-when-stall-condition-is-met branch June 29, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants