Skip to content

Commit

Permalink
chore(hardware): Add some logging around what I think is causing some…
Browse files Browse the repository at this point in the history
… can errors (#17117)

<!--
Thanks for taking the time to open a Pull Request (PR)! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

GitHub provides robust markdown to format your PR. Links, diagrams,
pictures, and videos along with text formatting make it possible to
create a rich and informative PR. For more information on GitHub
markdown, see:


https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview

We occasionally get some ABR issues where the can bus says it's not
receiving ACKs or that a move group says it didn't get all expected
nodes, but reports [] as the missing nodes.

I think the second bug is because we're checking with `if not
self._moves[group_id]` instead of checking `if
len(self._moves[group_id]) == 0:` Which should be equivalent but I think
there may be some python cleanup bug that reports the list as true due
to a remnant that hasn't been garbage collected yet.
<!--
Describe your PR at a high level. State acceptance criteria and how this
PR fits into other work. Link issues, PRs, and other relevant resources.
-->

## Test Plan and Hands on Testing

<!--
Describe your testing of the PR. Emphasize testing not reflected in the
code. Attach protocols, logs, screenshots and any other assets that
support your testing.
-->

## Changelog

<!--
List changes introduced by this PR considering future developers and the
end user. Give careful thought and clear documentation to breaking
changes.
-->

## Review requests

<!--
- What do you need from reviewers to feel confident this PR is ready to
merge?
- Ask questions.
-->

## Risk assessment

<!--
- Indicate the level of attention this PR needs.
- Provide context to guide reviewers.
- Discuss trade-offs, coupling, and side effects.
- Look for the possibility, even if you think it's small, that your
change may affect some other part of the system.
- For instance, changing return tip behavior may also change the
behavior of labware calibration.
- How do your unit tests and on hands on testing mitigate this PR's
risks and the risk of future regressions?
- Especially in high risk PRs, explain how you know your testing is
enough.
-->
  • Loading branch information
ryanthecoder authored Jan 10, 2025
1 parent b13cd27 commit 02b6a42
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 2 deletions.
6 changes: 4 additions & 2 deletions hardware/opentrons_hardware/drivers/can_bus/can_messenger.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ async def send_and_verify_recieved(self) -> ErrorCode:
)
except asyncio.TimeoutError:
log.error(
f"Message did not receive ack for message index {self._message.payload.message_index}"
f"Message did not receive ack for message index {self._message.payload.message_index} Missing node(s) {self._expected_nodes}"
)
return ErrorCode.timeout
finally:
Expand Down Expand Up @@ -278,12 +278,14 @@ async def _ensure_send(
exclusive: bool = False,
) -> ErrorCode:
if len(expected_nodes) == 0:
log.warning("Expected Nodes should have been specified")
if node_id == NodeId.broadcast:
if not expected_nodes:
expected_nodes = list(self._known_nodes)
else:
expected_nodes = [node_id]
log.warning(
f"Expected Nodes should have been specified, Setting expected nodes to {expected_nodes}"
)

listener = AcknowledgeListener(
can_messenger=self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,10 @@ def _remove_move_group(
f"Received completion for {node_id} group {group_id} seq {seq_id}"
f", which {'is' if in_group else 'isn''t'} in group"
)
if self._moves[group_id] and len(self._moves[group_id]) == 0:
log.error(
f"Python bug proven if check {bool(not self._moves[group_id])} len check {len(self._moves[group_id]) == 0}"
)
if not self._moves[group_id]:
log.debug(f"Move group {group_id+self._start_at_index} has completed.")
self._event.set()
Expand Down

0 comments on commit 02b6a42

Please sign in to comment.