feat: Add MockHandler for hardware-less simulation#269
feat: Add MockHandler for hardware-less simulation#269hitarthium wants to merge 3 commits intofossasia:mainfrom
Conversation
- Implements MockHandler class with full I/O simulation. - Updates ScienceLab to support 'mock=True' initialization. - Fixes type handling for byte/int compatibility in simulation.
Reviewer's GuideAdds a MockHandler implementation and integrates a mock mode into ScienceLab, enabling hardware-less simulation and programmable device responses compatible with existing instrument classes. Sequence diagram for ScienceLab initialization in mock modesequenceDiagram
actor Developer
participant ScienceLab
participant MockHandler
Developer->>ScienceLab: ScienceLab(device=None, mock=True)
activate ScienceLab
ScienceLab->>MockHandler: MockHandler()
activate MockHandler
MockHandler-->>ScienceLab: instance
ScienceLab->>MockHandler: open()
MockHandler-->>ScienceLab: connected=True
ScienceLab-->>Developer: ScienceLab instance with
deactivate MockHandler
deactivate ScienceLab
Sequence diagram for registering and using a mock responsesequenceDiagram
actor Developer
participant ScienceLab
participant MockHandler
Developer->>ScienceLab: device.register_response(b"01", b"99")
activate ScienceLab
ScienceLab->>MockHandler: register_response(command=b"01", response=b"99")
MockHandler-->>ScienceLab: response_map updated
Developer->>ScienceLab: device.write(b"01")
ScienceLab->>MockHandler: write(data=b"01")
activate MockHandler
MockHandler->>MockHandler: match command in response_map
MockHandler->>MockHandler: read_buffer.extend(b"99")
MockHandler-->>ScienceLab: bytes_written=1
Developer->>ScienceLab: device.read(1)
ScienceLab->>MockHandler: read(size=1)
MockHandler->>MockHandler: pop 1 byte from read_buffer
MockHandler-->>ScienceLab: b"99"
ScienceLab-->>Developer: b"99"
deactivate MockHandler
deactivate ScienceLab
Class diagram for ScienceLab mock integration and MockHandlerclassDiagram
class ScienceLab {
+ConnectionHandler device
+str firmware
+LogicAnalyzer logic_analyzer
+Oscilloscope oscilloscope
+WaveformGenerator waveform_generator
+ScienceLab(device ConnectionHandler, mock bool)
}
class ConnectionHandler {
}
class MockHandler {
+bool connected
+str port
+Dict~bytes,bytes~ response_map
+bytearray read_buffer
+Logger logger
+MockHandler(port str)
+open(port str) void
+close() void
+write(data bytes) int
+read(size int) bytes
+clear_buffer() void
+send_byte(val int)
+send_byte(val bytes)
+send_int(val int) void
+read_byte() int
+read_int() int
+get_ack() bool
+register_response(command bytes, response bytes) void
+inject_data(data bytes) void
}
class LogicAnalyzer {
}
class Oscilloscope {
}
class WaveformGenerator {
}
ScienceLab --> ConnectionHandler : uses
ScienceLab --> MockHandler : uses when_mock_true
ScienceLab --> LogicAnalyzer : composes
ScienceLab --> Oscilloscope : composes
ScienceLab --> WaveformGenerator : composes
MockHandler ..|> ConnectionHandler : mimics_interface
File-Level Changes
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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pslab/sciencelab.py:37-41` </location>
<code_context>
- def __init__(self, device: ConnectionHandler | None = None):
- self.device = device if device is not None else autoconnect()
- self.firmware = self.device.get_firmware_version()
+ def __init__(self, device: ConnectionHandler | None = None, mock: bool = False):
+ if mock:
+ self.device = MockHandler()
+ self.device.open()
+ self.firmware = "MOCK_FW_1.0" # Fake firmware version
+ else:
+ self.device = device if device is not None else autoconnect()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify interaction between `device` and `mock` to avoid surprising behavior.
With `mock=True`, any provided `device` is ignored and a `MockHandler` is always used, which can hide misconfigurations. Consider either raising when both `device` and `mock` are set, or defining clear precedence (e.g., prefer `device` when given, otherwise use `MockHandler` when `mock=True`) and documenting it.
</issue_to_address>
### Comment 2
<location> `pslab/mock_handler.py:25-27` </location>
<code_context>
+ self.read_buffer = bytearray()
+ self.logger = logging.getLogger(__name__)
+
+ def open(self, port: Optional[str] = None) -> None:
+ self.connected = True
+ self.logger.info(f"MockHandler connected on {self.port}")
+
+ def close(self) -> None:
</code_context>
<issue_to_address>
**issue:** Use the `port` parameter in `open` or remove it to avoid confusion.
Right now `open` accepts `port` but ignores it and always logs `self.port`. If you want to mirror the real handler, consider updating `self.port` when a non-`None` value is passed (and log that), otherwise drop the parameter so the API isn’t misleading.
</issue_to_address>
### Comment 3
<location> `pslab/mock_handler.py:55-60` </location>
<code_context>
+ if not self.connected:
+ raise RuntimeError("Attempted to read from closed MockHandler.")
+
+ if len(self.read_buffer) < size:
+ self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.")
+ # Return what we have, then empty bytes
+ data = self.read_buffer[:]
+ self.read_buffer.clear()
+ return bytes(data)
+
+ data = self.read_buffer[:size]
</code_context>
<issue_to_address>
**suggestion:** Align partial-read behavior and inline comment in `read`, or make behavior closer to the real handler.
In this branch, the comment (`"Return what we have, then empty bytes"`) doesn’t match the behavior: we return once with the available data and clear the buffer. Also, returning fewer bytes than requested may not align with the real handler’s semantics (e.g., blocking until enough data is available or raising). Please either update the comment to describe the current behavior, or adjust the implementation to better match the real handler so callers can rely on consistent read semantics.
```suggestion
if len(self.read_buffer) < size:
self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.")
# Return all currently available bytes and clear the buffer
data = self.read_buffer[:]
self.read_buffer.clear()
return bytes(data)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Updated MockHandler.open to use the port argument. - Clarified partial read behavior in MockHandler.read. - Added safety check in ScienceLab to prevent mixed device/mock arguments.
There was a problem hiding this comment.
Pull request overview
Adds a mock/simulated connection path to pslab-python so ScienceLab can be instantiated without a physical PSLab device, enabling hardware-less development/testing.
Changes:
- Add
pslab/mock_handler.pyimplementing a mock communication handler with injectable responses/buffered reads. - Extend
ScienceLab.__init__with amockflag to select the mock handler instead ofautoconnect(). - Provide basic read/write/send helpers and response registration utilities for simulation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
pslab/sciencelab.py |
Adds mock initialization path and wires ScienceLab to use MockHandler in simulation mode. |
pslab/mock_handler.py |
New mock handler implementation intended to mimic the device connection interface with programmable responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, device: ConnectionHandler | None = None, mock: bool = False): | ||
| """ | ||
| Initialize the ScienceLab interface. | ||
|
|
||
| :param device: The connection handler (real hardware). | ||
| :param mock: If True, use the MockHandler for simulation. | ||
| :raises ValueError: If both 'device' and 'mock=True' are provided. | ||
| """ | ||
| if mock: | ||
| if device is not None: | ||
| raise ValueError("Cannot initialize ScienceLab with both a physical device and mock=True.") | ||
|
|
||
| self.device = MockHandler() | ||
| self.device.open() | ||
| self.firmware = "MOCK_FW_1.0" # Fake firmware version | ||
| else: | ||
| self.device = device if device is not None else autoconnect() | ||
| self.firmware = self.device.get_firmware_version() |
There was a problem hiding this comment.
New mock mode/MockHandler functionality is not covered by tests. The repo already has a substantial pytest suite; adding a small unit test that instantiates ScienceLab(mock=True), registers a response, writes a command, and asserts the read/ack behavior would prevent regressions and validate the intended hardware-less workflow.
| def get_ack(self) -> bool: | ||
| """Simulates receiving an acknowledgement from the device.""" | ||
| return True |
There was a problem hiding this comment.
get_ack() returns bool, but the real ConnectionHandler.get_ack() returns an int bitfield and callers sometimes bit-shift it (e.g., >> 4 in I2C/NRF code) or test ACK bits. Returning True coerces to 1, which can change behavior. For compatibility, return an int ACK value consistent with the protocol (typically 0x01 for ACK), and consider matching the exception behavior on timeout if needed.
| def get_ack(self) -> bool: | |
| """Simulates receiving an acknowledgement from the device.""" | |
| return True | |
| def get_ack(self) -> int: | |
| """Simulates receiving an acknowledgement from the device as an integer bitfield.""" | |
| return 0x01 |
| if port: | ||
| self.port = port | ||
| self.connected = True | ||
| self.logger.info(f"MockHandler connected on {self.port}") |
There was a problem hiding this comment.
PEP 8 spacing: add a blank line between open() and close() method definitions to keep method boundaries clear and satisfy formatters/linters.
| self.logger.info(f"MockHandler connected on {self.port}") | |
| self.logger.info(f"MockHandler connected on {self.port}") |
| from .mock_handler import MockHandler | ||
| import time | ||
| from typing import Iterable, List |
There was a problem hiding this comment.
Import ordering: relative/local import from .mock_handler import MockHandler is placed before standard-library imports (time, typing). The rest of the file groups stdlib imports first, then pslab imports; please move this import to the existing pslab.* import block to keep import grouping consistent (and to satisfy common linters).
| self.device = MockHandler() | ||
| self.device.open() | ||
| self.firmware = "MOCK_FW_1.0" # Fake firmware version | ||
| else: | ||
| self.device = device if device is not None else autoconnect() | ||
| self.firmware = self.device.get_firmware_version() |
There was a problem hiding this comment.
In mock mode, self.firmware is set to a string ("MOCK_FW_1.0"), but the rest of the codebase treats ScienceLab.firmware as a FirmwareVersion (e.g., self.firmware.major is used later in this file and elsewhere). Initializing with mock=True will raise AttributeError when any code checks .major/.minor/.patch. Consider setting self.firmware to a FirmwareVersion instance (and optionally mirroring it onto self.device.firmware if other components expect it).
| """ | ||
| Initialize the ScienceLab interface. | ||
|
|
||
| :param device: The connection handler (real hardware). | ||
| :param mock: If True, use the MockHandler for simulation. | ||
| :raises ValueError: If both 'device' and 'mock=True' are provided. | ||
| """ |
There was a problem hiding this comment.
The new __init__ docstring uses reST :param/:raises fields, but the repository enforces NumPy-style docstrings (see [pydocstyle] convention = numpy in tox.ini, and existing docstrings in this file using Parameters/Returns). This will likely fail docstyle checks; please convert this docstring to NumPy style to match the codebase convention.
| raise ValueError("Cannot initialize ScienceLab with both a physical device and mock=True.") | ||
|
|
||
| self.device = MockHandler() | ||
| self.device.open() |
There was a problem hiding this comment.
ScienceLab uses ConnectionHandler (connect/disconnect) semantics elsewhere, but mock mode calls MockHandler.open(). To keep the handler interchangeable with ConnectionHandler (and to reduce special-casing), consider implementing connect()/disconnect() on MockHandler and calling connect() here instead of open() (or at least provide connect = open / disconnect = close aliases).
| self.device.open() | |
| # Ensure MockHandler provides a ConnectionHandler-like API | |
| if not hasattr(self.device, "connect") and hasattr(self.device, "open"): | |
| self.device.connect = self.device.open # type: ignore[attr-defined] | |
| if not hasattr(self.device, "disconnect") and hasattr(self.device, "close"): | |
| self.device.disconnect = self.device.close # type: ignore[attr-defined] | |
| self.device.connect() |
| if len(self.read_buffer) < size: | ||
| self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.") | ||
| # Return what we have, then empty bytes | ||
| data = self.read_buffer[:] | ||
| self.read_buffer.clear() | ||
| return bytes(data) | ||
|
|
||
| data = self.read_buffer[:size] | ||
| self.read_buffer = self.read_buffer[size:] | ||
| return bytes(data) | ||
|
|
There was a problem hiding this comment.
MockHandler.read() contains duplicated logic after an unconditional return (the block starting at line 73 is unreachable). This is dead code and can hide bugs; please remove the unreachable duplicate branch and keep a single, well-defined partial-read behavior.
| if len(self.read_buffer) < size: | |
| self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.") | |
| # Return what we have, then empty bytes | |
| data = self.read_buffer[:] | |
| self.read_buffer.clear() | |
| return bytes(data) | |
| data = self.read_buffer[:size] | |
| self.read_buffer = self.read_buffer[size:] | |
| return bytes(data) |
| class MockHandler: | ||
| """ | ||
| A mock communication handler that simulates the PSLab hardware. | ||
| """ | ||
|
|
There was a problem hiding this comment.
MockHandler is described as mimicking the ConnectionHandler interface, but it does not currently implement the same surface area: ConnectionHandler requires connect()/disconnect() and provides helpers like get_byte()/get_int() that the instrument code calls extensively. As written, passing MockHandler into instruments will fail at runtime when they call _device.get_int() / _device.get_byte() (e.g., oscilloscope, multimeter, logic analyzer). Consider subclassing pslab.connection.connection.ConnectionHandler and implementing connect/disconnect + read/write, or add compatible aliases (connect=open, disconnect=close, get_byte=read_byte, get_int=read_int, etc.) so instruments can run unchanged.
| if len(self.read_buffer) < size: | ||
| self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.") | ||
| # Return what we have, then empty bytes | ||
| data = self.read_buffer[:] | ||
| self.read_buffer.clear() | ||
| return bytes(data) | ||
|
|
||
| data = self.read_buffer[:size] | ||
| self.read_buffer = self.read_buffer[size:] | ||
| return bytes(data) | ||
|
|
There was a problem hiding this comment.
This statement is unreachable.
| if len(self.read_buffer) < size: | |
| self.logger.warning(f"MockHandler: Requested {size} bytes, but only {len(self.read_buffer)} available.") | |
| # Return what we have, then empty bytes | |
| data = self.read_buffer[:] | |
| self.read_buffer.clear() | |
| return bytes(data) | |
| data = self.read_buffer[:size] | |
| self.read_buffer = self.read_buffer[size:] | |
| return bytes(data) |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks for the PR. A hardware less mock mode is a useful direction for testing and onboarding.
Before we can review this for merge, we need a few things:
-
Please open an issue describing the intended behaviour and scope, then link it in the PR description using standard GitHub best practice, for example:
Fixes # -
There is a functional blocker: in mock mode
ScienceLab.firmwareis set to a plain string, but the codebase treats it as a FirmwareVersion like object and accesses.major/.minor/.patch. Please change mock mode to return a proper FirmwareVersion instance (and keep behaviour consistent with the non mock path). -
Please match the repo docstring style. The new
__init__docstring uses:paramsyntax, but we use NumPy style docstrings. Please convert it to the existing convention. -
Please attach a short screencast showing this working on your screen:
- how you run it
- a minimal snippet using
ScienceLab(mock=True) - at least one instrument call that exercises the mock handler
- your full test command output (pytest or tox)
Once the issue is linked, the firmware type is fixed, the docstring matches conventions, and we have a screencast proving the main path works, we can continue with a maintainer review.
|
Small process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. The team will review your PR. Thank you for your contribution and cooperation. |
- Replaced string firmware with MockFirmwareVersion object. - Updated docstrings to NumPy style. - Fixed get_ack return type to int. - Cleaned up read() method.
2 participants
Description
This PR introduces a
MockHandlerto thepslab-pythonlibrary. It allows developers to initialize theScienceLabclass in a simulation mode (mock=True), enabling testing and development without a physical PSLab device connected.Changes Made
pslab/mock_handler.pywhich mimics theConnectionHandlerinterface.pslab/sciencelab.pyto support amockboolean argument in__init__.read,write,send_byte, and signal injection methods to allow programmatic control over "hardware" responses.How to Test