Summit/blink/skeleton #249
Merged
Conversation
This file contains 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 a new Sequence diagram for get_version methodsequenceDiagram
participant Device as PSLab Device
participant Connection as ConnectionHandler
activate Device
Device->>Connection: exchange(COMMON + GET_VERSION)
activate Connection
Connection->>Device: Header + Data
Device-->>Connection: Status + Response
deactivate Connection
Device-->>Device: Decode version
Device-->>Device: Return version
deactivate Device
Sequence diagram for rgb_led methodsequenceDiagram
participant ScienceLab
participant ConnectionHandler
ScienceLab->>ConnectionHandler: exchange(cmd, args)
activate ConnectionHandler
ConnectionHandler->>ScienceLab: Header + Data
ScienceLab-->>ConnectionHandler: Status + Response
deactivate ConnectionHandler
Updated class diagram for ConnectionHandlerclassDiagram
class ConnectionHandler {
+write(data: bytes) int
+read(size: int) bytes
+exchange(cmd: bytes, data: bytes) bytes
+get_byte() int
+get_version() str
+get_firmware_version() FirmwareVersion
}
Class diagram for blink functionsclassDiagram
class blink {
+blink(psl: pslab.ScienceLab, color: tuple[int, int, int], period: int) None
+blink_c(psl: pslab.ScienceLab, color: tuple[int, int, int], period: int) None
}
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:
- The new
exchangemethod looks useful; consider using it in more places to reduce code duplication. - It looks like the
rgb_ledfunction insciencelab.pycould be simplified by using the newexchangemethod.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 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.
| @@ -70,6 +70,36 @@ def write(self, data: bytes) -> int: | |||
| """ | |||
| ... | |||
|
|
|||
| def exchange(self, cmd: bytes, data: bytes = b"") -> bytes: | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the exchange method into smaller helper functions to separate the header processing and response validation steps, improving code modularity and readability without altering functionality .
The exchange method now does too many things in one go. Consider splitting up the responsibilities into separate helper functions. For example, separate out the header processing and response validation:
def _process_header(self) -> int:
"""Process header, raise exception on status error, and return expected response size."""
status, response_size = CP.Header.unpack(self.read(CP.Header.size))
if status:
raise Exception(status)
return response_size
def _validate_response(self, response: bytes, expected_size: int) -> bytes:
"""Ensure the full response was received, or raise TimeoutError."""
if len(response) < expected_size:
raise TimeoutError
return responseThen update exchange to use these helpers while retaining functionality:
def exchange(self, cmd: bytes, data: bytes = b"") -> bytes:
(cmd_int,) = CP.ShortInt.unpack(cmd)
header = CP.Header.pack(cmd_int, len(data))
self.write(header + data)
response_size = self._process_header()
response = self.read(response_size)
return self._validate_response(response, response_size)This refactoring maintains all functionality while reducing complexity in exchange.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @bessman - I've reviewed your changes - here's some feedback:
Overall Comments:
- The new
exchangemethod looks useful; consider using it in other places where you're sending commands and data. - It looks like you're using
CP.COMMON + CP.GET_VERSIONas a command. Consider defining a constant for this.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
Comment on lines
+98
to
+99
| if len(response) < response_size: | ||
| raise TimeoutError |
There was a problem hiding this comment.
suggestion: Consider providing more context when raising TimeoutError.
Including details such as the expected response size and the actual number of bytes received would aid in debugging timeout issues.
Suggested change
| if len(response) < response_size: | |
| raise TimeoutError | |
| if len(response) < response_size: | |
| raise TimeoutError(f"Timeout error: expected {response_size} bytes but received {len(response)} bytes") |
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.
This provided a code skeleton for the PSLab Development 101 workshop at FOSSASIA Summit 2025.
It shall not be merged into main.
Summary by Sourcery
New Features: