From 0acc0178f8a6b0c62e8522371162e682fe52d269 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Sun, 20 Nov 2022 21:59:44 +0000 Subject: [PATCH 1/4] Add test to cover sensor reader temporary failure This will give us more confidence when we refactor these exception handlers in the next commits. --- src/pms/core/reader.py | 2 +- tests/core/test_reader.py | 42 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/pms/core/reader.py b/src/pms/core/reader.py index 1cabf5d..9640eae 100644 --- a/src/pms/core/reader.py +++ b/src/pms/core/reader.py @@ -165,7 +165,7 @@ def __call__(self, *, raw: Optional[bool] = None): except (SensorWarmingUp, InconsistentObservation) as e: logger.debug(e) time.sleep(5) - except SensorWarning as e: # pragma: no cover + except SensorWarning as e: logger.debug(e) self.serial.reset_input_buffer() else: diff --git a/tests/core/test_reader.py b/tests/core/test_reader.py index 34d9dbf..e9ef886 100644 --- a/tests/core/test_reader.py +++ b/tests/core/test_reader.py @@ -107,6 +107,31 @@ def passive_read(n): ) +@pytest.fixture +def mock_sensor_temp_failure(mock_serial): + def passive_read(n): + if n == 1: + # first return garbage data (bad checksum) + return ( + b"BM\x00\x1c" # expected header + + b"\0" * 26 # payload (to total 32 bytes) + + b"\x00\xFF" # checksum + ) + else: + # then behave like the original stub again + return ( + b"BM\x00\x1c" # expected header + + b".........................." # payload + + b"\x05W" # checksum + ) + + mock_serial.stub( + name="passive_read", + receive_bytes=b"BM\xe2\x00\x00\x01q", + send_fn=passive_read, + ) + + @pytest.fixture def sensor_reader_factory(monkeypatch, mock_sensor): def factory( @@ -204,6 +229,23 @@ def test_sensor_reader_warm_up( assert len(obs) == 1 +def test_sensor_reader_temp_failure( + mock_sensor, + sensor_reader_factory, + mock_sensor_temp_failure, +): + sensor_reader = sensor_reader_factory() + + with sensor_reader as r: + obs = list(r()) + + # check one sample still acquired + assert len(obs) == 1 + + # check two samples were attempted + assert mock_sensor.stubs["passive_read"].calls == 2 + + def test_sensor_reader_sensor_mismatch(mock_sensor, sensor_reader_factory): sensor_reader = sensor_reader_factory() From 1f73b890c7bd90c18ea7d2beff30b013ebfa43be Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Sat, 19 Nov 2022 22:19:32 +0000 Subject: [PATCH 2/4] Simplify exception handling for SensorReader This clarifies that both exceptions relate to a temporary condition where it makes sense to wait before trying again. --- src/pms/__init__.py | 10 ++++++++-- src/pms/core/reader.py | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/pms/__init__.py b/src/pms/__init__.py index 4549e10..cd8635f 100644 --- a/src/pms/__init__.py +++ b/src/pms/__init__.py @@ -21,13 +21,19 @@ class WrongMessageChecksum(SensorWarning): pass -class SensorWarmingUp(SensorWarning): +class SensorNotReady(SensorWarning): + """Sensor not ready to read: observations not reliable""" + + pass + + +class SensorWarmingUp(SensorNotReady): """Empty message: throw away observation and wait until sensor warms up""" pass -class InconsistentObservation(SensorWarning): +class InconsistentObservation(SensorNotReady): """Message payload makes no sense: throw away observation""" pass diff --git a/src/pms/core/reader.py b/src/pms/core/reader.py index 9640eae..85e93d5 100644 --- a/src/pms/core/reader.py +++ b/src/pms/core/reader.py @@ -18,7 +18,7 @@ from serial import Serial from typer import progressbar -from pms import InconsistentObservation, SensorWarmingUp, SensorWarning, logger +from pms import SensorNotReady, SensorWarning, logger from pms.core import Sensor, Supported from .types import ObsData @@ -162,7 +162,7 @@ def __call__(self, *, raw: Optional[bool] = None): try: obs = self.sensor.decode(buffer) - except (SensorWarmingUp, InconsistentObservation) as e: + except SensorNotReady as e: logger.debug(e) time.sleep(5) except SensorWarning as e: From d7af689848691d664ef8ff9bd5d36ebc4f192ced Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Sat, 26 Nov 2022 13:16:04 +0000 Subject: [PATCH 3/4] Simplify API to allow granular reader operation In response to [^1]. Previously it was hard to operate the reader classes from calling code that separates resource management ("open" / "close") and the actual polling of data ("read_one"). This exposes new APIs to make it easier to use reader classes in such a granular way. Implementation notes: - Reinstating a type hint for __enter__ of "-> Self" will become possible in Python 3.11. - "start" / "stop" were discussed in relation to the Sensor being operated by a SensorReader, but these concepts don't make much sense for the MessageReader class, so I've used "open" / "close" instead. [^1]: https://github.com/avaldebe/PyPMS/pull/32#issuecomment-1324702383 --- src/pms/core/reader.py | 28 ++++++++++++++++++++-------- tests/core/test_reader.py | 4 ++-- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/pms/core/reader.py b/src/pms/core/reader.py index 85e93d5..a9d6d1f 100644 --- a/src/pms/core/reader.py +++ b/src/pms/core/reader.py @@ -50,7 +50,7 @@ def hexdump(self, line: Optional[int] = None) -> str: return f"{offset:08x}: {hex} {dump}" -class Reader(AbstractContextManager): +class Reader: @abstractmethod def __call__(self, *, raw: Optional[bool]) -> Iterator[Union[RawData, ObsData]]: """ @@ -60,6 +60,21 @@ def __call__(self, *, raw: Optional[bool]) -> Iterator[Union[RawData, ObsData]]: """ ... + @abstractmethod + def open(self) -> None: + ... + + @abstractmethod + def close(self) -> None: + ... + + def __enter__(self): + self.open() + return self + + def __exit__(self, exception_type, exception_value, traceback) -> None: + self.close() + class SensorReader(Reader): """Read sensor messages from serial port @@ -119,7 +134,7 @@ def _pre_heat(self): # only pre-heat the firs time self.pre_heat = 0 - def __enter__(self) -> "SensorReader": + def open(self) -> None: """Open serial port and sensor setup""" if not self.serial.is_open: logger.debug(f"open {self.serial.port}") @@ -143,9 +158,7 @@ def __enter__(self) -> "SensorReader": logger.error(f"Sensor is not {self.sensor.name}") raise UnableToRead("Sensor failed validation") - return self - - def __exit__(self, exception_type, exception_value, traceback) -> None: + def close(self) -> None: """Put sensor to sleep and close serial port""" logger.debug(f"sleep {self.sensor}") buffer = self._cmd("sleep") @@ -188,14 +201,13 @@ def __init__(self, path: Path, sensor: Sensor, samples: Optional[int] = None) -> self.sensor = sensor self.samples = samples - def __enter__(self) -> "MessageReader": + def open(self) -> None: logger.debug(f"open {self.path}") self.csv = self.path.open() reader = DictReader(self.csv) self.data = (row for row in reader if row["sensor"] == self.sensor.name) - return self - def __exit__(self, exception_type, exception_value, traceback) -> None: + def close(self) -> None: logger.debug(f"close {self.path}") self.csv.close() diff --git a/tests/core/test_reader.py b/tests/core/test_reader.py index e9ef886..208b23e 100644 --- a/tests/core/test_reader.py +++ b/tests/core/test_reader.py @@ -12,12 +12,12 @@ def __init__(self, raise_on_enter=False): def __call__(self): raise NotImplemented - def __enter__(self): + def open(self): if self.raise_on_enter: raise reader.UnableToRead() self.entered = True - def __exit__(self, *_args): + def close(self): self.exited = True From 75c54c3c07771ed2da622ac6850a26d0abc172cb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 2 Dec 2022 20:42:12 +0000 Subject: [PATCH 4/4] Add max_retries flag for SensorReader.__call__ In response to [^1]. [^1]: https://github.com/avaldebe/PyPMS/pull/33#discussion_r1038131161 --- src/pms/core/reader.py | 9 +++++++++ tests/core/test_reader.py | 27 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/pms/core/reader.py b/src/pms/core/reader.py index a9d6d1f..3306d17 100644 --- a/src/pms/core/reader.py +++ b/src/pms/core/reader.py @@ -93,6 +93,7 @@ def __init__( interval: Optional[int] = None, samples: Optional[int] = None, timeout: Optional[float] = None, + max_retries: Optional[int] = None, ) -> None: """Configure serial port""" self.sensor = sensor if isinstance(sensor, Sensor) else Sensor[sensor] @@ -101,6 +102,7 @@ def __init__( self.serial.port = port self.serial.baudrate = self.sensor.baud self.serial.timeout = timeout or 5 # max time to wake up sensor + self.max_retries = max_retries self.interval = interval self.samples = samples logger.debug( @@ -169,6 +171,7 @@ def __call__(self, *, raw: Optional[bool] = None): """Passive mode reading at regular intervals""" sample = 0 + failures = 0 while self.serial.is_open: try: buffer = self._cmd("passive_read") @@ -176,9 +179,15 @@ def __call__(self, *, raw: Optional[bool] = None): try: obs = self.sensor.decode(buffer) except SensorNotReady as e: + failures += 1 + if self.max_retries is not None and failures > self.max_retries: + raise logger.debug(e) time.sleep(5) except SensorWarning as e: + failures += 1 + if self.max_retries is not None and failures > self.max_retries: + raise logger.debug(e) self.serial.reset_input_buffer() else: diff --git a/tests/core/test_reader.py b/tests/core/test_reader.py index 208b23e..429ac71 100644 --- a/tests/core/test_reader.py +++ b/tests/core/test_reader.py @@ -1,5 +1,6 @@ import pytest +import pms from pms.core import reader from pms.core.sensor import Sensor from tests.conftest import captured_data @@ -139,6 +140,7 @@ def factory( samples=0, # exit immediately interval=None, sensor="PMSx003", # match with stubs + max_retries=None, ): sensor_reader = reader.SensorReader( port=mock_sensor.port, @@ -146,6 +148,7 @@ def factory( interval=interval, sensor=sensor, timeout=0.01, # low to avoid hanging on failure + max_retries=max_retries, ) # https://github.com/pyserial/pyserial/issues/625 @@ -229,6 +232,18 @@ def test_sensor_reader_warm_up( assert len(obs) == 1 +def test_sensor_reader_warm_up_exhaust_retries( + mock_sensor, + sensor_reader_factory, + mock_sensor_warm_up, +): + sensor_reader = sensor_reader_factory(max_retries=0) + + with sensor_reader as r: + with pytest.raises(pms.SensorWarmingUp): + list(r()) + + def test_sensor_reader_temp_failure( mock_sensor, sensor_reader_factory, @@ -246,6 +261,18 @@ def test_sensor_reader_temp_failure( assert mock_sensor.stubs["passive_read"].calls == 2 +def test_sensor_reader_temp_failure_exhaust_retries( + mock_sensor, + sensor_reader_factory, + mock_sensor_temp_failure, +): + sensor_reader = sensor_reader_factory(max_retries=0) + + with sensor_reader as r: + with pytest.raises(pms.SensorWarning): + list(r()) + + def test_sensor_reader_sensor_mismatch(mock_sensor, sensor_reader_factory): sensor_reader = sensor_reader_factory()