Add WLAN support #242
Merged
Add WLAN support #242
+784
−632
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces WLAN support by adding a new Updated class diagram for ConnectionHandler and its subclassesclassDiagram
class ConnectionHandler {
<<abstract>>
+connect()
+disconnect()
+read(numbytes: int): bytes
+write(data: bytes): int
+get_byte(): int
+get_int(): int
+get_long(): int
+send_byte(data: int | bytes)
+send_int(data: int | bytes)
+send_long(data: int | bytes)
+get_ack(): int
+get_version(): str
+get_firmware_version(): FirmwareVersion
}
class SerialHandler {
-_port: str
-_ser: serial.Serial
+port: str
+baudrate: int
+timeout: float
+connect()
+disconnect()
+read(number_of_bytes: int): bytes
+write(data: bytes): int
}
class WLANHandler {
-_host: str
-_port: int
-_timeout: float
-_sock: socket.socket
+host: int
+port: int
+timeout: float
+connect()
+disconnect()
+read(numbytes: int): bytes
+write(data: bytes): int
}
ConnectionHandler <|-- SerialHandler
ConnectionHandler <|-- WLANHandler
note for ConnectionHandler "Abstract base class for PSLab control interfaces"
note for SerialHandler "Interface for controlling a PSLab over a serial port"
note for WLANHandler "Interface for controlling a PSLab over WLAN"
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 @bessman - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a factory function or classmethod to
ConnectionHandlerto handle the connection logic instead ofautoconnect(). - The
detectfunction could return a list of disconnectedConnectionHandlerinstances, allowing the user to choose which device to connect to.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -3,7 +3,7 @@ | |||
| from serial.tools.list_ports_common import ListPortInfo | |||
|
|
|||
| import pslab.protocol as CP | |||
| from pslab.serial_handler import detect, SerialHandler | |||
| from pslab.connection import detect, SerialHandler | |||
There was a problem hiding this comment.
issue (testing): Missing test for autoconnect
It seems like autoconnect is a new function, but I don't see any tests for it in this PR. We should add tests to ensure it functions correctly, including cases where no devices are found and where multiple devices are found.
| import pslab.protocol as CP | ||
|
|
||
|
|
||
| class ADCBufferMixin: |
There was a problem hiding this comment.
issue (complexity): Consider creating a helper function to encapsulate the common device communication steps in fetch_buffer and fill_buffer to reduce code duplication.
You can reduce the duplication in the while loops by abstracting the common device communication steps into a helper function. For example, you can create an internal method (e.g. _buffer_transaction) that handles sending the common header and command, plus the extra parameters. Then call this helper in both fetch_buffer and fill_buffer. For instance:
def _buffer_transaction(self, command: int, idx: int, count: int, extra_send: callable = None):
self._device.send_byte(CP.COMMON)
self._device.send_byte(command)
self._device.send_int(idx)
self._device.send_int(count)
if extra_send:
extra_send()
result = None
if command == CP.RETRIEVE_BUFFER:
# when fetching the buffer, gather the responses
result = [self._device.get_int() for _ in range(count)]
self._device.get_ack()
return resultThen modify your methods as:
def fetch_buffer(self, samples: int, starting_position: int = 0):
received = []
buf_size = 128
remaining = samples
idx = starting_position
while remaining > 0:
samps = min(remaining, buf_size)
chunk = self._buffer_transaction(
CP.RETRIEVE_BUFFER,
idx,
samps
)
received += chunk
remaining -= samps
idx += samps
return received
def fill_buffer(self, data: list[int], starting_position: int = 0):
buf_size = 128
idx = starting_position
remaining = len(data)
while remaining > 0:
samps = min(remaining, buf_size)
# supply a lambda to send the data chunk
self._buffer_transaction(
CP.FILL_BUFFER,
idx,
samps,
extra_send=lambda: [self._device.send_int(value) for value in data[idx:idx+samps]]
)
idx += samps
remaining -= sampsThis refactoring abstracts away the common patterns in device communication while preserving functionality and reducing complexity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
This pull request introduces WLAN support for PSLab devices by refactoring the connection logic to support multiple connection types. It introduces a new
ConnectionHandlerabstract base class and implementsSerialHandlerandWLANHandlerclasses to handle serial and WLAN connections, respectively. TheScienceLabclass is updated to use theConnectionHandlerinterface, allowing it to work with different connection types.Enhancements:
ConnectionHandlerabstract base class to define the common interface for all connection types.SerialHandlerandWLANHandlerclasses to handle serial and WLAN connections, respectively.ScienceLabclass to use theConnectionHandlerinterface, allowing it to work with different connection types.pslab.serial_handlermodule and redirect users to the newpslab.connectionmodule.