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): don't stop reading from CAN after most exceptions #12963

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Jun 22, 2023

Overview

The CAN messenger reader task will exit if it gets any exceptions. We actually want to keep reading unless the task is cancelled. There are a few exceptions that are normal to receive and we don't want to completely stop reading.

Test Plan

Changelog

Review requests

Risk assessment

@fsinapi fsinapi requested a review from sfoster1 June 22, 2023 19:17
@fsinapi fsinapi requested a review from a team as a code owner June 22, 2023 19:17
@fsinapi fsinapi self-assigned this Jun 22, 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.

Looks much better to me!

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #12963 (4da95ae) into internal-release_0.12.0 (3424f42) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.12.0   #12963      +/-   ##
===========================================================
- Coverage                    73.29%   73.29%   -0.01%     
===========================================================
  Files                         2317     2317              
  Lines                        63528    63528              
  Branches                      6977     6977              
===========================================================
- Hits                         46564    46563       -1     
- Misses                       15308    15309       +1     
  Partials                      1656     1656              
Flag Coverage Δ
hardware 58.67% <83.33%> (-0.02%) ⬇️
notify-server 89.13% <ø> (ø)

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

Impacted Files Coverage Δ
...pentrons_hardware/drivers/can_bus/can_messenger.py 85.32% <83.33%> (ø)

... and 1 file with indirect coverage changes

@fsinapi fsinapi merged commit 12e30e0 into internal-release_0.12.0 Jun 22, 2023
@fsinapi fsinapi deleted the mask_can_errors_0.12.0 branch June 22, 2023 19:55
sfoster1 added a commit that referenced this pull request Jul 10, 2023
Now that we can express error codes in all our client interfaces, let's start raising them.

This PR makes the opentrons_hardware module use the new exceptions that have the new error codes. There's a couple new error codes, but mostly all this does is raise different exceptions in the same places.

The two exception is in the can messenger and the movegrouprunner.
CAN Messenger

The CAN messenger has this asyncio task that reads stuff asynchronously from the bus and passes it to registered listeners. In addition, if a message is an error message it would raise an exception. But, it's a task, so that exception doesn't go anywhere. #12963 made this exception just logged and swallowed but, like... why does it exist? So we removed it.
MoveGroupRunner

A similar issue happens in MoveGroupRunner, where a background task accumulates responses and then pops them on over to the caller. This works the same now, but we can also move the move stop condition checking inside that loop. We can also now properly handle having multiple errors in a move group!

Note: You need to run make setup-py again since now hardware depends on shared-data.
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.

2 participants