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

feat(hardware): Hardware error codes #13009

Merged
merged 20 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 2 additions & 4 deletions api/src/opentrons/hardware_control/ot3api.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,7 @@
InstrumentDict,
GripperDict,
)
from opentrons_hardware.hardware_control.motion_planning.move_utils import (
MoveConditionNotMet,
)


from .status_bar_state import StatusBarStateController

Expand Down Expand Up @@ -1227,7 +1225,7 @@ async def _home(self, axes: Sequence[OT3Axis]) -> None:
except ZeroLengthMoveError:
self._log.info(f"{axis} already at home position, skip homing")
continue
except (MoveConditionNotMet, Exception) as e:
except BaseException as e:
self._log.exception(f"Homing failed: {e}")
self._current_position.clear()
raise
Expand Down
9 changes: 4 additions & 5 deletions api/tests/opentrons/hardware_control/test_moves.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
)
from opentrons.hardware_control.types import OT3Axis

from opentrons_shared_data.errors.exceptions import MoveConditionNotMetError


async def test_controller_must_home(hardware_api):
abs_position = types.Point(30, 20, 10)
Expand Down Expand Up @@ -100,12 +102,9 @@ async def test_home(ot3_hardware, mock_home):


async def test_home_unmet(ot3_hardware, mock_home):
from opentrons_hardware.hardware_control.motion_planning.move_utils import (
MoveConditionNotMet,
)

mock_home.side_effect = MoveConditionNotMet()
with pytest.raises(MoveConditionNotMet):
mock_home.side_effect = MoveConditionNotMetError()
with pytest.raises(MoveConditionNotMetError):
await ot3_hardware.home([OT3Axis.X])
assert ot3_hardware.gantry_load == GantryLoad.LOW_THROUGHPUT
mock_home.assert_called_once_with([OT3Axis.X], GantryLoad.LOW_THROUGHPUT)
Expand Down
3 changes: 2 additions & 1 deletion hardware/Pipfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ numpy = "==1.21.2"
pydantic = "==1.8.2"

[dev-packages]
pytest = "==6.1.0"
pytest = "==7.0.1"
pytest-lazy-fixture = "==0.6.3"
pytest-cov = "==2.10.1"
mypy = "==0.910"
Expand All @@ -25,6 +25,7 @@ types-mock = "==4.0.1"
hypothesis = "~=6.36.1"
pytest-asyncio = "~=0.18"
matplotlib = "*"
opentrons-shared-data = { editable = true, path = "../shared-data/python" }

[requires]
python_version = "3.7"
731 changes: 444 additions & 287 deletions hardware/Pipfile.lock

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions hardware/mypy.ini
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[mypy]
show_error_codes = True
warn_unused_configs = True
strict = True

[mypy-can.*]
Expand Down
19 changes: 13 additions & 6 deletions hardware/opentrons_hardware/drivers/binary_usb/bin_serial.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
"""The usb binary protocol over serial transport."""
import asyncio
import logging
import concurrent.futures
from functools import partial
from typing import Optional, Type, Tuple

import serial # type: ignore[import]
from serial.tools.list_ports import comports # type: ignore[import]
from functools import partial

from opentrons_shared_data.errors.exceptions import InternalUSBCommunicationError

from opentrons_hardware.firmware_bindings.messages.binary_message_definitions import (
BinaryMessageDefinition,
get_binary_definition,
)
import asyncio
import logging
import concurrent.futures

from typing import Optional, Type, Tuple

from opentrons_hardware.firmware_bindings import utils
from opentrons_hardware.firmware_bindings.binary_constants import BinaryMessageId

Expand All @@ -37,7 +41,10 @@ def find_and_connect(
"""Initialize a serial connection to a usb device that uses the binary messaging protocol."""
_port_name = self._find_serial_port(vid, pid)
if _port_name is None:
raise IOError("unable to find serial device")
raise InternalUSBCommunicationError(
message="unable to find serial device",
detail={"vid": str(vid), "pid": str(pid)},
)
self._vid = vid
self._pid = pid
self._baudrate = baudrate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@

import logging

from opentrons_shared_data.errors.exceptions import (
EnumeratedError,
InternalUSBCommunicationError,
PythonException,
)

from opentrons_hardware.drivers.binary_usb.bin_serial import SerialUsbDriver
from opentrons_hardware.firmware_bindings.binary_constants import BinaryMessageId
from opentrons_hardware.drivers.errors import USBCommunicationError


from opentrons_hardware.firmware_bindings.messages.binary_message_definitions import (
Expand Down Expand Up @@ -106,10 +111,14 @@ async def send(self, message: BinaryMessageDefinition) -> bool:
return bool(
(await self._drive.write(message=message)) == message.get_size()
)
except USBCommunicationError:
except EnumeratedError:
raise
except Exception as exc:
raise USBCommunicationError(exc=exc) from exc
except BaseException as exc:
raise InternalUSBCommunicationError(
message="Error in USB send",
detail={"message": str(message)},
wrapping=[PythonException(exc)],
)

async def __aenter__(self) -> BinaryMessenger:
"""Start messenger."""
Expand All @@ -124,9 +133,9 @@ async def __aexit__(
if exc_val:
# type ignore because it's unclear what the type of exc_tb should be
log.error(format_exception(exc_type, exc_val, exc_tb)) # type: ignore
if isinstance(exc_val, USBCommunicationError):
if isinstance(exc_val, EnumeratedError):
raise exc_val
raise USBCommunicationError(exc=exc_val)
raise PythonException(exc_val)

def start(self) -> None:
"""Start the reader task."""
Expand Down Expand Up @@ -165,15 +174,18 @@ def remove_listener(self, listener: BinaryMessageListenerCallback) -> None:
self._listeners.pop(listener)

async def _read_task_shield(self) -> None:
try:
await self._read_task()
except asyncio.CancelledError:
pass
except USBCommunicationError:
raise
except Exception as exc:
log.exception("Exception in read")
raise USBCommunicationError(exc=exc) from exc
while True:
try:
await self._read_task()
except (asyncio.CancelledError, StopAsyncIteration):
return
except (InternalUSBCommunicationError) as e:
log.exception(f"Nonfatal error in USB read task: {e}")
continue
except BaseException as e:
# Log this separately if it's some unknown error
log.exception(f"Unexpected error in USB read task: {e}")
continue

async def _read_task(self) -> None:
"""Read task."""
Expand Down Expand Up @@ -214,11 +226,15 @@ async def send_and_receive(
listener = SendAndReceiveListener(self, response_type, timeout)
try:
return await listener.send_and_receive(message)
except USBCommunicationError:
except EnumeratedError:
raise
except Exception as exc:
except BaseException as exc:
log.exception("Exception in send_and_receive")
raise USBCommunicationError(exc=exc) from exc
raise InternalUSBCommunicationError(
message="Exception in internal USB send_and_receive",
detail={"message": str(message)},
wrapping=[PythonException(exc=exc)],
)


class BinaryWaitableCallback:
Expand Down
73 changes: 28 additions & 45 deletions hardware/opentrons_hardware/drivers/can_bus/can_messenger.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,41 +16,33 @@

import logging

from opentrons_hardware.drivers.errors import CANCommunicationError
from opentrons_shared_data.errors.exceptions import (
CanbusCommunicationError,
EnumeratedError,
PythonException,
)
from opentrons_hardware.drivers.can_bus.abstract_driver import AbstractCanDriver
from opentrons_hardware.firmware_bindings.arbitration_id import (
ArbitrationId,
ArbitrationIdParts,
)
from opentrons_hardware.firmware_bindings.message import CanMessage
from opentrons_hardware.firmware_bindings.utils.binary_serializable import (
BinarySerializable,
)

from opentrons_hardware.firmware_bindings.constants import (
NodeId,
MessageId,
FunctionCode,
ErrorSeverity,
ErrorCode,
)

from opentrons_hardware.firmware_bindings.messages.message_definitions import (
Acknowledgement,
ErrorMessage,
)

from opentrons_hardware.firmware_bindings.messages.messages import (
MessageDefinition,
get_definition,
)

from opentrons_hardware.firmware_bindings.messages.payloads import ErrorMessagePayload

from opentrons_hardware.firmware_bindings.utils import BinarySerializableException

from .errors import AsyncHardwareError, CanError

log = logging.getLogger(__name__)


Expand Down Expand Up @@ -238,11 +230,13 @@ async def _send(self, node_id: NodeId, message: MessageDefinition) -> None:
await self._drive.send(
message=CanMessage(arbitration_id=arbitration_id, data=data)
)
except CANCommunicationError:
except EnumeratedError:
raise
except Exception as exc:
log.exception("Exception in CAN send")
raise CANCommunicationError(exc=exc) from exc
raise CanbusCommunicationError(
message="Exception in canbus.send", wrapping=[PythonException(exc)]
)

async def ensure_send_exclusive(
self,
Expand Down Expand Up @@ -293,11 +287,13 @@ async def _ensure_send(
)
try:
return await listener.send_and_verify_recieved()
except CANCommunicationError:
except EnumeratedError:
raise
except Exception as exc:
log.exception("Exception in CAN ensure_send")
raise CANCommunicationError(exc=exc) from exc
raise CanbusCommunicationError(
message="Exception in CAN ensure_send", wrapping=[PythonException(exc)]
)

async def __aenter__(self: CanMessenger) -> CanMessenger:
"""Start messenger."""
Expand All @@ -313,9 +309,10 @@ async def __aexit__(
"""Stop messenger."""
await self.stop()
if exc_val:
if isinstance(exc_val, CANCommunicationError):
if isinstance(exc_val, EnumeratedError):
raise exc_val
raise CANCommunicationError(exc=exc_val)
# Don't want a specific error here because this wraps other code
raise PythonException(exc_val)

def start(self) -> None:
"""Start the reader task."""
Expand Down Expand Up @@ -354,12 +351,8 @@ async def _read_task_shield(self) -> None:
await self._read_task()
except (asyncio.CancelledError, StopAsyncIteration):
return
except (CANCommunicationError, AsyncHardwareError, CanError) as e:
log.exception(f"Nonfatal error in CAN read task: {e}")
continue
except Exception as e:
# Log this separately if it's some unknown error
log.exception(f"Unexpected error in CAN read task: {e}")
except BaseException:
log.exception("Exception in read")
continue

async def _read_task(self) -> None:
Expand All @@ -375,37 +368,27 @@ async def _read_task(self) -> None:
f"Received <--\n\tarbitration_id: {message.arbitration_id},\n\t"
f"payload: {build}"
)
handled = False
for listener, filter in self._listeners.values():
if filter and not filter(message.arbitration_id):
log.debug("message ignored by filter")
continue
listener(message_definition(payload=build), message.arbitration_id) # type: ignore[arg-type]
if (
message.arbitration_id.parts.message_id
== MessageId.error_message
):
await self._handle_error(build)
handled = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that we could get in a state where a listener accepts an error message but for whatever reason, it doesn't have an error handling method and hence not raise it?

In that case, would this error message just be thrown away since the can messenger thinks it was handled by a listener?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. All I can really say is, like, "don't do that"

if not handled:
if (
message.arbitration_id.parts.message_id
== MessageId.error_message
):
log.error(f"Asynchronous error message ignored: {message}")
else:
log.info(f"Message ignored: {message}")
except BinarySerializableException:
log.exception(f"Failed to build from {message}")
else:
log.error(f"Message {message} is not recognized.")
raise StopAsyncIteration

async def _handle_error(self, build: BinarySerializable) -> None:
err_msg = ErrorMessage(payload=build) # type: ignore[arg-type]
error_payload: ErrorMessagePayload = err_msg.payload
err_msg.log_error(log)

if error_payload.message_index == 0:
log.error(
f"error {str(err_msg)} recieved is asyncronous, raising exception"
)
raise AsyncHardwareError(
"Async firmware error: " + str(err_msg),
ErrorCode(error_payload.error_code.value),
ErrorSeverity(error_payload.severity.value),
)

@property
def exclusive_writer(self) -> asyncio.Lock:
"""A caller may acquire this context manager to temporarily gain exclusive control of the bus.
Expand Down
8 changes: 5 additions & 3 deletions hardware/opentrons_hardware/drivers/can_bus/driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
from typing import Optional, Union, Dict, Any
import concurrent.futures

from opentrons_shared_data.errors.exceptions import CANBusBusError
from can import Notifier, Bus, AsyncBufferedReader, Message

from opentrons_hardware.firmware_bindings.arbitration_id import ArbitrationId
from opentrons_hardware.firmware_bindings.message import CanMessage
from .errors import ErrorFrameCanError
from .abstract_driver import AbstractCanDriver
from .settings import calculate_fdcan_parameters, PCANParameters

Expand Down Expand Up @@ -124,12 +124,14 @@ async def read(self) -> CanMessage:
A can message

Raises:
ErrorFrameCanError
CANBusBusError
"""
m: Message = await self._reader.get_message()
if m.is_error_frame:
log.error("Error frame encountered")
raise ErrorFrameCanError(message=repr(m))
raise CANBusBusError(
message="Error frame encountered", detail={"frame": repr(m)}
)

return CanMessage(
arbitration_id=ArbitrationId(id=m.arbitration_id), data=m.data
Expand Down
Loading