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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions api/src/opentrons/hardware_control/ot3api.py
Original file line number Diff line number Diff line change
Expand Up @@ -775,12 +775,12 @@ async def _update_position_estimation(
"""
Function to update motor estimation for a set of axes
"""
if axes is None:
axes = [ax for ax in Axis]

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

else:
checked_axes = [ax for ax in Axis]
await self._backend.update_motor_estimation(checked_axes)
axes = [ax for ax in axes if self._backend.axis_is_present(ax)]

await self._backend.update_motor_estimation(axes)

# Global actions API
def pause(self, pause_type: PauseType) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ async def update_axis_position_estimations(self, axes: Sequence[Axis]) -> None:
"""Update the specified axes' position estimators from their encoders.

This will allow these axes to make a non-home move even if they do not currently have
a position estimation (unless there is no tracked poition from the encoders, as would be
a position estimation (unless there is no tracked position from the encoders, as would be
true immediately after boot).

Axis encoders have less precision than their position estimators. Calling this function will
Expand All @@ -19,6 +19,8 @@ async def update_axis_position_estimations(self, axes: Sequence[Axis]) -> None:

This function updates only the requested axes. If other axes have bad position estimation,
moves that require those axes or attempts to get the position of those axes will still fail.
Axes that are not currently available (like a plunger for a pipette that is not connected)
will be ignored.
"""
...

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ class UpdatePositionEstimatorsParams(BaseModel):
"""Payload required for an UpdatePositionEstimators command."""

axes: List[MotorAxis] = Field(
..., description="The axes for which to update the position estimators."
...,
description=(
"The axes for which to update the position estimators."
" Any axes that are not physically present will be ignored."
),
)


Expand Down
29 changes: 21 additions & 8 deletions api/tests/opentrons/hardware_control/test_ot3_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2037,23 +2037,36 @@ def set_mock_plunger_configs() -> None:


@pytest.mark.parametrize(
"axes",
[[Axis.X], [Axis.X, Axis.Y], [Axis.X, Axis.Y, Axis.P_L], None],
("axes_in", "axes_present", "expected_axes"),
[
([Axis.X, Axis.Y], [Axis.X, Axis.Y], [Axis.X, Axis.Y]),
([Axis.X, Axis.Y], [Axis.Y, Axis.Z_L], [Axis.Y]),
(None, list(Axis), list(Axis)),
(None, [Axis.Y, Axis.Z_L], [Axis.Y, Axis.Z_L]),
],
)
async def test_update_position_estimation(
ot3_hardware: ThreadManager[OT3API],
hardware_backend: OT3Simulator,
axes: List[Axis],
axes_in: List[Axis],
axes_present: List[Axis],
expected_axes: List[Axis],
) -> None:
def _axis_is_present(axis: Axis) -> bool:
return axis in axes_present

with patch.object(
hardware_backend,
"update_motor_estimation",
AsyncMock(spec=hardware_backend.update_motor_estimation),
) as mock_update:
await ot3_hardware._update_position_estimation(axes)
if axes is None:
axes = [ax for ax in Axis]
mock_update.assert_called_once_with(axes)
) as mock_update, patch.object(
hardware_backend,
"axis_is_present",
Mock(spec=hardware_backend.axis_is_present),
) as mock_axis_is_present:
mock_axis_is_present.side_effect = _axis_is_present
await ot3_hardware._update_position_estimation(axes_in)
mock_update.assert_called_once_with(expected_axes)


async def test_refresh_positions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,7 @@ def _listener_filter(arbitration_id: ArbitrationId) -> bool:
log.warning("Update motor position estimation timed out")
raise CommandTimedOutError(
"Update motor position estimation timed out",
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.

)

return data
Loading