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(api): Skip updating position estimators for axes that are not present #16804

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Nov 13, 2024

Overview

Fixes RQA-3569.

Test Plan and Hands on Testing

  • Follow the steps to reproduce in RQA-3569. The robot should not complain about any missing nodes; the unsafe/updatePositionEstimators commands should succeed.

Changelog

  • At the hardware API level, if the caller tries to update the position estimations for axes that aren't physically present, just skip those axes.
  • Also, remove a prepareToAspirate command from the drop tip wizard. This was causing the wizard to fail with a "no tip attached" error. @mjhuff, @sfoster1 and I couldn't remember why we added it in the first place in fix(app): use a slow blowout in DTWiz #15869.

Review requests

  • On the robot I tested with, I noticed that gripper_g always timed out, even with a gripper attached. Is that expected, or should we investigate further?
  • Any reason not to delete the prepareToAspirate command?

Risk assessment

Medium. We're not so sure about the prepareToAspirate command. If it breaks something, it'll probably break something somewhere in error recovery.

@SyntaxColoring SyntaxColoring requested review from ryanthecoder and a team November 13, 2024 22:10
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner November 13, 2024 22:10

if axes:
checked_axes = [ax for ax in axes if ax in Axis]
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Nov 13, 2024

Choose a reason for hiding this comment

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

[ax for ax in axes if ax in Axis] looked like a no-op to me. Was there a hidden intent to that, or was that just a small oversight?

Copy link
Member

Choose a reason for hiding this comment

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

That’s for possibly filtering out things that aren’t axis enums. It might be holdover ot2 code where there’s chronic string/enum confusion

detail={
"missing-nodes": ", ".join(
node.name for node in set(nodes).difference(set(data))
)
},
detail={"missing-node": node.name},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When any node was missing, the error message here said that node, plus all the nodes that would have come after it, and that we didn't attempt. This narrows it down to just the node that actually timed out.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner November 13, 2024 22:31
@SyntaxColoring SyntaxColoring requested review from shlokamin and removed request for a team November 13, 2024 22:31
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

App portion is good, thanks for updating. If there's a reason we were prepareToAspirateing, I certainly don't remember it now.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner November 13, 2024 22:45
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.

Definitely correct and makes sense, thanks!


if axes:
checked_axes = [ax for ax in axes if ax in Axis]
Copy link
Member

Choose a reason for hiding this comment

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

That’s for possibly filtering out things that aren’t axis enums. It might be holdover ot2 code where there’s chronic string/enum confusion

@SyntaxColoring SyntaxColoring merged commit 91b40ae into chore_release-8.2.0 Nov 14, 2024
67 checks passed
@SyntaxColoring SyntaxColoring deleted the not_all_axes branch November 14, 2024 14:19
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.

3 participants