Add MockHandler and allow ScienceLab(mock=True)#274
Add MockHandler and allow ScienceLab(mock=True)#274minhpham1810 wants to merge 2 commits intofossasia:mainfrom
Conversation
Reviewer's GuideImplements a mock connection backend and a mock mode for ScienceLab that bypasses hardware autoconnection and instrument initialization, enabling instantiation and firmware queries without a physical PSLab device. Sequence diagram for ScienceLab initialization in mock modesequenceDiagram
actor Client
participant ScienceLab
participant MockHandler
Client->>ScienceLab: __init__(device=None, mock=True)
alt device is None and mock is True
ScienceLab->>MockHandler: create MockHandler()
activate MockHandler
MockHandler-->>ScienceLab: instance
deactivate MockHandler
ScienceLab->>MockHandler: get_firmware_version()
activate MockHandler
MockHandler->>MockHandler: write(command_bytes)
MockHandler->>MockHandler: _maybe_respond()
MockHandler-->>MockHandler: queue firmware bytes
MockHandler-->>ScienceLab: read(firmware_bytes)
deactivate MockHandler
ScienceLab-->>Client: ScienceLab instance (instruments set to None)
end
Updated class diagram for ScienceLab mock mode and MockHandlerclassDiagram
class ConnectionHandler {
<<abstract>>
+connect() None
+disconnect() None
+read(numbytes int) bytes
+write(data bytes) int
+get_firmware_version() str
}
class MockHandler {
+version str
-_rx deque
-_tx bytearray
-_fw tuple
-_connected bool
+MockHandler(version str, fw tuple)
+connect() None
+disconnect() None
+read(numbytes int) bytes
+write(data bytes) int
-_queue(payload bytes) None
-_maybe_respond() None
}
class ScienceLab {
+device ConnectionHandler
+firmware str
+logic_analyzer LogicAnalyzer
+oscilloscope Oscilloscope
+waveform_generator WaveformGenerator
+pwm_generator PWMGenerator
+multimeter Multimeter
+power_supply PowerSupply
+ScienceLab(device ConnectionHandler, mock bool)
}
class LogicAnalyzer {
+device ConnectionHandler
+LogicAnalyzer(device ConnectionHandler)
}
class Oscilloscope {
+device ConnectionHandler
+Oscilloscope(device ConnectionHandler)
}
class WaveformGenerator {
+device ConnectionHandler
+WaveformGenerator(device ConnectionHandler)
}
class PWMGenerator {
+device ConnectionHandler
+PWMGenerator(device ConnectionHandler)
}
class Multimeter {
+device ConnectionHandler
+Multimeter(device ConnectionHandler)
}
class PowerSupply {
+device ConnectionHandler
+PowerSupply(device ConnectionHandler)
}
MockHandler --|> ConnectionHandler
ScienceLab o--> ConnectionHandler : device
ScienceLab o--> LogicAnalyzer : logic_analyzer
ScienceLab o--> Oscilloscope : oscilloscope
ScienceLab o--> WaveformGenerator : waveform_generator
ScienceLab o--> PWMGenerator : pwm_generator
ScienceLab o--> Multimeter : multimeter
ScienceLab o--> PowerSupply : power_supply
LogicAnalyzer --> ConnectionHandler : uses
Oscilloscope --> ConnectionHandler : uses
WaveformGenerator --> ConnectionHandler : uses
PWMGenerator --> ConnectionHandler : uses
Multimeter --> ConnectionHandler : uses
PowerSupply --> ConnectionHandler : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ScienceLab.__init__, themockflag is silently ignored when adeviceis provided; consider either raising if both are set or clearly documenting/enforcing thatdevicetakes precedence so callers don’t get surprising behavior. - In mock mode all instrument attributes are set to
None, which can easily break existing code that expects these to be usable objects; consider making their types explicitly optional and/or providing lazy construction or clearer failure modes when accessed undermock=True. - The
MockHandlermaintains an internal_connectedflag that is never consulted byread/write; either enforce connection state in these methods (e.g., raising if not connected) or remove the flag to avoid confusion about its semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ScienceLab.__init__`, the `mock` flag is silently ignored when a `device` is provided; consider either raising if both are set or clearly documenting/enforcing that `device` takes precedence so callers don’t get surprising behavior.
- In mock mode all instrument attributes are set to `None`, which can easily break existing code that expects these to be usable objects; consider making their types explicitly optional and/or providing lazy construction or clearer failure modes when accessed under `mock=True`.
- The `MockHandler` maintains an internal `_connected` flag that is never consulted by `read`/`write`; either enforce connection state in these methods (e.g., raising if not connected) or remove the flag to avoid confusion about its semantics.
## Individual Comments
### Comment 1
<location> `pslab/connection/mock.py:101-103` </location>
<code_context>
+
+ # GET_VERSION: ConnectionHandler.get_version reads 9 bytes
+ # and checks b"PSLab"
+ if cmd == CP.GET_VERSION:
+ self._tx = self._tx[2:]
+ self._queue(self.version.encode("utf-8")[:9].ljust(9, b" "))
+ continue
+
</code_context>
<issue_to_address>
**suggestion (testing):** The version string is truncated/padded to 9 bytes; confirming this matches protocol expectations might avoid subtle bugs.
The mock forces a 9-byte `GET_VERSION` reply via `[:9].ljust(9, b" ")`. If the real device’s version length or encoding differs, tests may diverge from real behavior. Prefer deriving the length from a shared protocol constant (e.g., in `CP` or `ConnectionHandler`), or add an assertion that the mock length matches what the production handler expects.
Suggested implementation:
```python
from collections import deque
import pslab.protocol as CP
from pslab.connection.connection import ConnectionHandler
# Length of the GET_VERSION reply in bytes.
# Prefer a shared protocol constant if available to keep the mock in sync
# with the real ConnectionHandler implementation.
GET_VERSION_REPLY_LEN = getattr(CP, "GET_VERSION_REPLY_LEN", 9)
```
```python
# GET_VERSION: ConnectionHandler.get_version reads GET_VERSION_REPLY_LEN bytes
# and checks b"PSLab"
if cmd == CP.GET_VERSION:
self._tx = self._tx[2:]
version_bytes = self.version.encode("utf-8")
reply_len = GET_VERSION_REPLY_LEN
assert reply_len > 0, "GET_VERSION_REPLY_LEN must be positive"
self._queue(version_bytes[:reply_len].ljust(reply_len, b" "))
continue
```
If the production protocol already defines a constant length for the GET_VERSION payload (for example, `CP.GET_VERSION_REPLY_LEN` or something similar), it would be better to:
1. Add that constant to `pslab.protocol` (if it does not exist yet).
2. Ensure `GET_VERSION_REPLY_LEN` in this mock module is set directly from that constant (and possibly drop the `getattr(..., 9)` fallback to fail fast if the protocol changes without updating the mock).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pslab/connection/mock.py
Outdated
| if cmd == CP.GET_VERSION: | ||
| self._tx = self._tx[2:] | ||
| self._queue(self.version.encode("utf-8")[:9].ljust(9, b" ")) |
There was a problem hiding this comment.
suggestion (testing): The version string is truncated/padded to 9 bytes; confirming this matches protocol expectations might avoid subtle bugs.
The mock forces a 9-byte GET_VERSION reply via [:9].ljust(9, b" "). If the real device’s version length or encoding differs, tests may diverge from real behavior. Prefer deriving the length from a shared protocol constant (e.g., in CP or ConnectionHandler), or add an assertion that the mock length matches what the production handler expects.
Suggested implementation:
from collections import deque
import pslab.protocol as CP
from pslab.connection.connection import ConnectionHandler
# Length of the GET_VERSION reply in bytes.
# Prefer a shared protocol constant if available to keep the mock in sync
# with the real ConnectionHandler implementation.
GET_VERSION_REPLY_LEN = getattr(CP, "GET_VERSION_REPLY_LEN", 9) # GET_VERSION: ConnectionHandler.get_version reads GET_VERSION_REPLY_LEN bytes
# and checks b"PSLab"
if cmd == CP.GET_VERSION:
self._tx = self._tx[2:]
version_bytes = self.version.encode("utf-8")
reply_len = GET_VERSION_REPLY_LEN
assert reply_len > 0, "GET_VERSION_REPLY_LEN must be positive"
self._queue(version_bytes[:reply_len].ljust(reply_len, b" "))
continueIf the production protocol already defines a constant length for the GET_VERSION payload (for example, CP.GET_VERSION_REPLY_LEN or something similar), it would be better to:
- Add that constant to
pslab.protocol(if it does not exist yet). - Ensure
GET_VERSION_REPLY_LENin this mock module is set directly from that constant (and possibly drop thegetattr(..., 9)fallback to fail fast if the protocol changes without updating the mock).
There was a problem hiding this comment.
Pull request overview
This PR adds a MockHandler implementation and introduces a mock=True parameter to ScienceLab to enable hardware-free testing and development. The MockHandler provides a minimal in-memory implementation of the ConnectionHandler interface that simulates protocol responses for version queries.
Changes:
- Introduced
MockHandlerclass with basic protocol handling for GET_VERSION and GET_FW_VERSION commands - Added
mockparameter toScienceLab.__init__()to enable mock mode, skipping autoconnect and instrument initialization - Added test coverage for mock mode initialization
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
pslab/connection/mock.py |
New MockHandler class providing in-memory protocol simulation for hardware-free operation |
pslab/connection/__init__.py |
Export MockHandler in all and add import statement |
pslab/sciencelab.py |
Add mock parameter to support initialization without hardware; conditionally skip instrument initialization |
tests/test_sciencelab_mock.py |
Test verifying ScienceLab can be instantiated with mock=True without calling autoconnect |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pslab/sciencelab.py
Outdated
| # mock mode initializes without hardware; instruments are not initialized. | ||
|
|
There was a problem hiding this comment.
This comment is positioned awkwardly between the firmware retrieval and the conditional initialization logic. Consider either moving it to be a docstring parameter description or rephrasing it as an inline comment closer to the conditional logic, such as: "# In mock mode, skip instrument initialization to avoid hardware dependencies".
| def __init__(self, version: str = "PSLab V6 ", fw=(3, 0, 0)) -> None: | ||
| self._rx = deque() # bytes to be read | ||
| self._tx = bytearray() # bytes written by client | ||
| self.version = version # convenient attribute for callers | ||
| self._fw = fw | ||
| self._connected = False |
There was a problem hiding this comment.
The __init__ method should have a Parameters section in its docstring following the NumPy docstring conventions used throughout the codebase. The version and fw parameters should be documented. For example:
Parameters
version : str, optional
Version string to return when queried. Default is "PSLab V6 ".
fw : tuple of int, optional
Firmware version tuple (major, minor, patch). Default is (3, 0, 0).
tests/test_sciencelab_mock.py
Outdated
| # It should initialize and provide a firmware version object. | ||
| assert psl.firmware.major >= 0 | ||
| assert psl.firmware.minor >= 0 | ||
| assert psl.firmware.patch >= 0 |
There was a problem hiding this comment.
The test assertions for firmware version only check that values are >= 0, which would pass even if the mock handler returns (0, 0, 0) due to protocol handling failures. Consider strengthening the test to verify specific expected values, such as assert psl.firmware.major == 3 and assert psl.firmware.minor == 0 and assert psl.firmware.patch == 0, which match the default firmware version in MockHandler.
| # It should initialize and provide a firmware version object. | |
| assert psl.firmware.major >= 0 | |
| assert psl.firmware.minor >= 0 | |
| assert psl.firmware.patch >= 0 | |
| # It should initialize and provide the expected mock firmware version object. | |
| assert psl.firmware.major == 3 | |
| assert psl.firmware.minor == 0 | |
| assert psl.firmware.patch == 0 |
| @@ -34,15 +34,31 @@ class ScienceLab: | |||
| nrf : pslab.peripherals.NRF24L01 | |||
There was a problem hiding this comment.
The Attributes section of the class docstring lists the instruments but doesn't indicate they can be None in mock mode. Consider updating the docstring to reflect this, for example by adding "These attributes are None when initialized with mock=True." after the attributes list, or by documenting each attribute as "instrument_name : ClassName or None".
| nrf : pslab.peripherals.NRF24L01 | |
| nrf : pslab.peripherals.NRF24L01 | |
| These attributes are set to None when initialized with mock=True. |
| def __init__(self, device: ConnectionHandler | None = None, mock: bool = False): | ||
| if device is not None: | ||
| self.device = device | ||
| elif mock: | ||
| self.device = MockHandler() | ||
| else: | ||
| self.device = autoconnect() |
There was a problem hiding this comment.
When both device and mock=True are provided, the device parameter silently takes precedence and the mock parameter is ignored. This behavior may be unexpected for users. Consider either raising a ValueError when both are provided, or documenting this precedence in the docstring if it's intentional.
pslab/connection/__init__.py
Outdated
| from .connection import ConnectionHandler | ||
| from ._serial import SerialHandler | ||
| from .wlan import WLANHandler | ||
| from pslab.connection.mock import MockHandler |
There was a problem hiding this comment.
The import statement uses an absolute path from pslab.connection.mock import MockHandler while other imports in this file use relative imports (e.g., from .connection import ConnectionHandler). For consistency with the existing import style, this should be changed to from .mock import MockHandler.
| from pslab.connection.mock import MockHandler | |
| from .mock import MockHandler |
pslab/connection/mock.py
Outdated
| if self._tx[0] != CP.COMMON: | ||
| # Drop unknown leading bytes | ||
| self._tx.pop(0) | ||
| continue | ||
|
|
||
| cmd = self._tx[1] |
There was a problem hiding this comment.
The comparison logic is incorrect. self._tx[0] returns an integer (since _tx is a bytearray), but CP.COMMON, CP.GET_VERSION, and CP.GET_FW_VERSION are bytes objects created with Byte.pack(). Comparing an integer with a bytes object will always be False, so the mock handler won't correctly detect commands. The comparisons should unpack the protocol constants to get the integer values, for example: if self._tx[0] != CP.Byte.unpack(CP.COMMON)[0]:.
pslab/connection/mock.py
Outdated
| if cmd == CP.GET_VERSION: | ||
| self._tx = self._tx[2:] | ||
| self._queue(self.version.encode("utf-8")[:9].ljust(9, b" ")) | ||
| continue | ||
|
|
||
| # GET_FW_VERSION: reads 3 bytes (major, minor, patch) | ||
| if cmd == CP.GET_FW_VERSION: | ||
| self._tx = self._tx[2:] | ||
| major, minor, patch = self._fw | ||
| self._queue(bytes([major, minor, patch])) | ||
| continue |
There was a problem hiding this comment.
The comparison logic is incorrect. self._tx[1] (an integer) is being compared with CP.GET_VERSION and CP.GET_FW_VERSION (bytes objects). This will always be False. The comparisons should unpack the protocol constants to get the integer values, for example: if cmd == CP.Byte.unpack(CP.GET_VERSION)[0]:.
| @@ -34,15 +34,31 @@ class ScienceLab: | |||
| nrf : pslab.peripherals.NRF24L01 | |||
There was a problem hiding this comment.
The docstring for the ScienceLab class should document the new mock parameter. Following the pattern seen in other classes in the codebase (e.g., LogicAnalyzer), a Parameters section should be added documenting both the device and mock parameters. For example:
Parameters
device : ConnectionHandler, optional
Connection handler for communicating with the PSLab device. If not provided, a new one will be created via autoconnect.
mock : bool, optional
If True, use a mock handler instead of connecting to physical hardware. Instruments will not be instantiated in mock mode. The default is False.
| nrf : pslab.peripherals.NRF24L01 | |
| nrf : pslab.peripherals.NRF24L01 | |
| Parameters | |
| ---------- | |
| device : ConnectionHandler, optional | |
| Connection handler for communicating with the PSLab device. If not | |
| provided, a new one will be created via autoconnect. | |
| mock : bool, optional | |
| If True, use a mock handler instead of connecting to physical | |
| hardware. Instruments will not be instantiated in mock mode. The | |
| default is False. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Unknown command under CP.COMMON: drop and stop | ||
| break |
There was a problem hiding this comment.
When an unknown command is encountered under CP.COMMON, the loop breaks instead of continuing to process remaining bytes. This means if multiple commands are written in sequence and one is unknown, subsequent commands won't be processed. Consider using continue after incrementing past the unknown command, or at minimum, consuming the unknown command bytes to prevent them from blocking future command processing.
| # Unknown command under CP.COMMON: drop and stop | |
| break | |
| # Unknown command under CP.COMMON: drop it and continue parsing | |
| self._tx = self._tx[2:] | |
| continue |
| def read(self, numbytes: int) -> bytes: | ||
| """Read bytes from the internal receive buffer. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| numbytes : int | ||
| Number of bytes to read. | ||
|
|
||
| Returns | ||
| ------- | ||
| bytes | ||
| Bytes read from the receive buffer (may be shorter if insufficient data | ||
| is available). | ||
| """ | ||
| out = bytearray() | ||
| while len(out) < numbytes and self._rx: | ||
| out.append(self._rx.popleft()) | ||
| return bytes(out) |
There was a problem hiding this comment.
When insufficient bytes are available in the receive buffer, this method returns fewer bytes than requested instead of blocking or raising an exception. This differs from the behavior of real connection handlers (which would timeout). Consider documenting this behavior, or optionally raising an exception when insufficient data is available to help catch test setup issues early.
| `write()` so higher-level code can be exercised without an actual device. | ||
| """ | ||
|
|
||
| def __init__(self, version: str = "PSLab V6 ", fw=(3, 0, 0)) -> None: |
There was a problem hiding this comment.
The fw parameter is missing a type annotation. Consider adding tuple[int, int, int] to be consistent with other parameters and improve type safety.
| def __init__(self, version: str = "PSLab V6 ", fw=(3, 0, 0)) -> None: | |
| def __init__(self, version: str = "PSLab V6 ", fw: tuple[int, int, int] = (3, 0, 0)) -> None: |
| Firmware version as a (major, minor, patch) tuple. Default is (3, 0, 0). | ||
| """ | ||
| self._rx = deque() # bytes to be read | ||
| self._tx = bytearray() # bytes written by client | ||
| self.version = version # convenient attribute for callers | ||
| self._fw = fw |
There was a problem hiding this comment.
The fw parameter is not validated. If non-integer values or values outside the valid byte range (0-255) are provided, it will cause an error later when constructing the response in _maybe_respond(). Consider adding validation in __init__ to ensure fw contains three integers in the valid range, or document this requirement in the docstring.
| Firmware version as a (major, minor, patch) tuple. Default is (3, 0, 0). | |
| """ | |
| self._rx = deque() # bytes to be read | |
| self._tx = bytearray() # bytes written by client | |
| self.version = version # convenient attribute for callers | |
| self._fw = fw | |
| Firmware version as a (major, minor, patch) tuple of integers in the | |
| range 0–255. Default is (3, 0, 0). | |
| """ | |
| self._rx = deque() # bytes to be read | |
| self._tx = bytearray() # bytes written by client | |
| self.version = version # convenient attribute for callers | |
| # Validate firmware version: must be three integers in byte range 0–255 | |
| if not isinstance(fw, (tuple, list)) or len(fw) != 3: | |
| raise ValueError( | |
| "fw must be a tuple or list of three integers in the range 0–255" | |
| ) | |
| for part in fw: | |
| if not isinstance(part, int) or not (0 <= part <= 255): | |
| raise ValueError( | |
| "fw must contain three integers in the range 0–255" | |
| ) | |
| self._fw = tuple(fw) |
| if not mock: # In mock mode, skip instrument initialization to avoid hardware dependencies | ||
| self.logic_analyzer = LogicAnalyzer(device=self.device) | ||
| self.oscilloscope = Oscilloscope(device=self.device) | ||
| self.waveform_generator = WaveformGenerator(device=self.device) | ||
| self.pwm_generator = PWMGenerator(device=self.device) | ||
| self.multimeter = Multimeter(device=self.device) | ||
| self.power_supply = PowerSupply(device=self.device) | ||
| else: | ||
| self.logic_analyzer = None | ||
| self.oscilloscope = None | ||
| self.waveform_generator = None | ||
| self.pwm_generator = None | ||
| self.multimeter = None | ||
| self.power_supply = None |
There was a problem hiding this comment.
The conditional logic here only checks the mock parameter, which can lead to incorrect behavior when both device and mock are provided. For example:
ScienceLab(device=MockHandler(), mock=False)would attempt to initialize instruments with a MockHandlerScienceLab(device=real_device, mock=True)would skip instrument initialization despite having a real device
The logic should determine whether to initialize instruments based on the actual type of the device, not just the mock parameter. Consider checking isinstance(self.device, MockHandler) instead of relying solely on the mock parameter value.
1 participant
Add MockHandler and support ScienceLab(mock=True)
Fixes #273
This PR adds a minimal
MockHandlerimplementation ofConnectionHandlerand introduces amock=Trueoption toScienceLab.When
mock=True:ScienceLabinitializes without callingautoconnect()A
MockHandleris used instead of real hardwareInstruments are not instantiated to avoid hardware-dependent calls
This allows
ScienceLabto be created in environments without a physical PSLab device, making testing and development easier while preserving existing behavior.Tests
Added a unit test ensuring
autoconnect()is not called in mock modeVerified firmware retrieval works via the mock handler
Summary by Sourcery
Introduce a mockable ScienceLab configuration that can operate without connecting to physical PSLab hardware.
New Features:
Tests: