chore(hardware): Add some logging around what I think is causing some can errors #17117
Conversation
hardware/opentrons_hardware/hardware_control/move_group_runner.py
Outdated
Show resolved
Hide resolved
| @@ -284,6 +285,7 @@ async def _ensure_send( | |||
| expected_nodes = list(self._known_nodes) | |||
| else: | |||
| expected_nodes = [node_id] | |||
| log.warning(f"Setting expected nodes to {expected_nodes}") | |||
There was a problem hiding this comment.
this is going to flood the logs no?
There was a problem hiding this comment.
If everything is being done right this should never be in the logs, if we start seeing it, that mean's we are not setting up the can messages correctly and we need to hook up the "expected nodes" argument to this method.
There was a problem hiding this comment.
can we delete the one on line 282 then
There was a problem hiding this comment.
Messages and logic LGTM, modulo @vegano1's question above.
I don't see any way that bool(self._moves) could disagree with len(self._moves), though.
I agree but we have seen a few cases in ABR where this actually seems to be the case. I'm hoping this log either reveals an issue with python garbage collecting or shows us that we need to look elsewhere for the error. |
| @@ -284,6 +285,7 @@ async def _ensure_send( | |||
| expected_nodes = list(self._known_nodes) | |||
| else: | |||
| expected_nodes = [node_id] | |||
| log.warning(f"Setting expected nodes to {expected_nodes}") | |||
There was a problem hiding this comment.
can we delete the one on line 282 then
| @@ -154,6 +154,7 @@ async def send_and_verify_recieved(self) -> ErrorCode: | |||
| log.error( | |||
| f"Message did not receive ack for message index {self._message.payload.message_index}" | |||
| ) | |||
| log.error(f"Missing node {self._expected_nodes}") | |||
There was a problem hiding this comment.
can we condense this and the previous message
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 checkingif 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.Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment