Begin update to new protocol #247
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 PR updates the communication protocol to use a new header-based format for commands and responses. It introduces a new Sequence diagram for get_version with new protocolsequenceDiagram
participant SL as ScienceLab
participant C as Connection
SL->>C: get_version()
C->>C: exchange(COMMON + GET_VERSION)
C->>C: pack header (cmd, len(data))
C->>C: write(header + data)
C->>C: read(header.size)
C->>C: unpack header (status, response_size)
alt status != 0
C-->>SL: raise Exception(status)
else
C->>C: read(response_size)
C-->>SL: version
end
SL-->>SL: version.rstrip(b"\x00").decode("utf-8")
SL-->>SL: return version
Sequence diagram for get_firmware_version with new protocolsequenceDiagram
participant SL as ScienceLab
participant C as Connection
SL->>C: get_firmware_version()
C->>C: exchange(COMMON + GET_FW_VERSION)
C->>C: pack header (cmd, len(data))
C->>C: write(header + data)
C->>C: read(header.size)
C->>C: unpack header (status, response_size)
alt status != 0
C-->>SL: raise Exception(status)
else
C->>C: read(response_size)
C-->>SL: major, minor, patch
end
SL-->>SL: return FirmwareVersion(major, minor, patch)
Updated class diagram for ConnectionclassDiagram
class Connection {
+write(data: bytes) int
+exchange(cmd: bytes, data: bytes) bytes
+get_byte() int
+get_version() str
+get_firmware_version() FirmwareVersion
}
class FirmwareVersion {
+major: int
+minor: int
+patch: int
}
Connection -- FirmwareVersion: returns
Updated class diagram for ScienceLabclassDiagram
class ScienceLab {
-device: ConnectionHandler
+version: str
+logic_analyzer: LogicAnalyzer
+oscilloscope: Oscilloscope
+waveform_generator: WaveformGenerator
+datalogger: Datalogger
+multimeter: Multimeter
+power_source: PowerSource
+i2c: I2C
+spi: SPIMaster
+cap_meter: CapacitanceMeter
+res_meter: ResistanceMeter
+freq_counter: FrequencyCounter
+rgb_led(colors: List, output: str, order: str)
-_read_program_address(address: int)
}
ScienceLab -- ConnectionHandler: has a
ScienceLab -- LogicAnalyzer: has a
ScienceLab -- Oscilloscope: has a
ScienceLab -- WaveformGenerator: has a
ScienceLab -- Datalogger: has a
ScienceLab -- Multimeter: has a
ScienceLab -- PowerSource: has a
ScienceLab -- I2C: has a
ScienceLab -- SPIMaster: has a
ScienceLab -- CapacitanceMeter: has a
ScienceLab -- ResistanceMeter: has a
ScienceLab -- FrequencyCounter: has a
note for ScienceLab "version attribute added in constructor"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
I need this for FOSSASIA Summit workshop, so I'm merging it into a new 'develop' branch. |
![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 timeout to the
readcalls inexchangeto prevent indefinite blocking. - The removal of
send_bytecalls is good, but ensure all usages are replaced with the newexchangemethod.
Here's what I looked at during the review
- 🟡 General issues: 2 issues 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.
|
|
||
| Parameters | ||
| ---------- | ||
| cmd : int |
There was a problem hiding this comment.
suggestion: Align cmd parameter type in docstring and implementation.
The docstring currently indicates that cmd is an int, whereas the implementation expects a bytes object (and unpacks it using CP.ShortInt.unpack). Consider updating the docstring so that it accurately reflects the required type.
Suggested change
| cmd : int | |
| cmd : bytes |
Comment on lines
+93
to
+94
| if status: | ||
| raise Exception(status) |
There was a problem hiding this comment.
suggestion: Raise a more descriptive exception on status error.
Instead of raising a generic Exception with the status code, consider raising a custom exception or adding a more descriptive error message. This could aid in debugging and handling errors in client code.
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 PR updates pslab-python to use the new protocol currently being worked on here: fossasia/pslab-firmware#180
The new protocol has two major changes from the old one:
This flowchart summarizes the new protocol:
Summary by Sourcery
Updates the communication protocol to use a new header-based format for commands and responses. This change involves modifying the structure of data transmission and reception, including the introduction of a header containing command codes, data sizes, and status codes. The update also includes changes to how version information is retrieved from the device.
New Features:
Enhancements: