Sigrok driver: Logic Analyzers and Oscilloscopes #774
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new jumpstarter-driver-sigrok package: a Sigrok driver (sigrok-cli) implementation with client API, Pydantic models, CSV/VCD parsers, tests, docs, packaging, and CI/Docker changes to install sigrok-cli. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as SigrokClient
participant Driver as Sigrok
participant CLI as sigrok-cli
participant Parser as Parser (CSV/VCD/BITS/ASCII)
Client->>Driver: capture(config)
activate Driver
Driver->>Driver: build sigrok-cli args (driver, conn, channels, rate, triggers, decoders)
Driver->>CLI: run subprocess (sync)
activate CLI
CLI-->>Driver: stdout (capture bytes/base64)
deactivate CLI
Driver->>Parser: decode(data, output_format)
activate Parser
Parser-->>Driver: samples / mapping / ascii / bits
deactivate Parser
Driver-->>Client: CaptureResult (with decoded payload or raw data)
deactivate Driver
Client->>Driver: capture_stream(config)
activate Driver
Driver->>CLI: start async subprocess (stream)
activate CLI
loop while streaming
CLI-->>Driver: stream chunk
Driver-->>Client: yield chunk
end
CLI-->>Driver: stream end
deactivate CLI
deactivate Driver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mangelajo
force-pushed
the
sigrok-driver
branch
from
a867cec to
12b1f2d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/jumpstarter-driver-sigrok/.gitignore (1)
1-3: Consider enhancing .gitignore with additional Python development artifacts.The current entries cover coverage reports and bytecode, which is a good start. However, for a complete and maintainable Python package, consider adding entries for common artifacts from testing, packaging, and virtual environment workflows.
Apply this diff to enhance the .gitignore:
__pycache__/ .coverage coverage.xml +*.pyc +*.pyo +*.egg-info/ +dist/ +build/ +.pytest_cache/ +.tox/ +.venv/ +venv/ +htmlcov/ +.mypy_cache/Alternatively, if the root
.gitignorealready covers these patterns, you may rely on those and keep this file minimal for driver-specific overrides only.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (1)
124-124: Unused variable_multiplier.The
_multipliervariable is destructured fromtest_casesbut never used in the test loop.- for timescale_str, _multiplier, expected_time_ns in test_cases: + for timescale_str, _, expected_time_ns in test_cases:packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
88-109: Add error handling for malformed timestamps.If
time_strcontains non-numeric characters (malformed VCD),int(time_str)at line 96 will raiseValueError, crashing the parser. Consider wrapping in try-except for robustness:# Skip empty time lines if not time_str: return None - time_units = int(time_str) + try: + time_units = int(time_str) + except ValueError: + return None + current_time_ns = time_units * timescale_multiplierpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (2)
69-84: Minor: Unused loop variable_i.The
enumerateis unnecessary since_iis never used. Consider simplifying:- for _i, line in enumerate(lines): + for line in lines:
116-140:strict=Truemay crash on malformed CSV rows.If a CSV row has a different number of columns than
channel_names,zip(..., strict=True)raisesValueError. This could happen with malformed CSV data from sigrok. Consider handling this gracefully:- for channel, value in zip(channel_names, row, strict=True): + for channel, value in zip(channel_names, row, strict=False): + if not channel: + continueOr wrap in try-except at the call site in
parse_csv.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (1)
7-37: LGTM! Clean client implementation with proper Pydantic validation.The client correctly:
- Uses
CaptureResult.model_validate()for type-safe deserialization- Delegates to
self.call()andself.streamingcall()for RPC- Mirrors the driver's public API surface
Minor: Consider adding return type annotation to
capture_stream:- def capture_stream(self, config: CaptureConfig | dict): + def capture_stream(self, config: CaptureConfig | dict) -> Iterator[bytes]:This would require
from typing import Iteratororfrom collections.abc import Iterator.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (1)
87-116: LGTM with a minor suggestion.Good async implementation with proper cleanup and timeout handling. Consider logging
stderron process exit for debugging purposes, since it's already being captured (Line 97).packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (1)
8-19: Consider usingStrEnumfor type safety.Using a plain class with string constants works, but
StrEnum(Python 3.11+) orenum.Enumwould provide better type safety and IDE support.from enum import StrEnum class OutputFormat(StrEnum): CSV = "csv" BITS = "bits" ASCII = "ascii" BINARY = "binary" SRZIP = "srzip" VCD = "vcd"If supporting Python < 3.11, you can use
str, Enumas base classes instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/sigrok.md(1 hunks)packages/jumpstarter-driver-sigrok/-(1 hunks)packages/jumpstarter-driver-sigrok/.gitignore(1 hunks)packages/jumpstarter-driver-sigrok/README.md(1 hunks)packages/jumpstarter-driver-sigrok/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/__init__.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py(1 hunks)packages/jumpstarter-driver-sigrok/pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/__init__.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/__init__.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-sigrok/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-sigrok/pyproject.toml
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
🧠 Learnings (18)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: All driver packages should have a symlink in `docs/source/reference/package-apis/drivers/` pointing to the driver's README.md
Applied to files:
docs/source/reference/package-apis/drivers/index.mddocs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/README.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/index.mdpackages/jumpstarter-driver-sigrok/.gitignoredocs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-09-06T05:25:18.184Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 608
File: packages/jumpstarter-cli/jumpstarter_cli/run.py:0-0
Timestamp: 2025-09-06T05:25:18.184Z
Learning: Zombie process reaping in jumpstarter is useful beyond just PID 1 cases, as drivers can leave behind processes in various execution contexts.
Applied to files:
docs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
docs/source/reference/package-apis/drivers/index.mddocs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/README.mdpackages/jumpstarter-driver-sigrok/pyproject.tomlpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/index.mddocs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-sigrok/.gitignoredocs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-10-06T15:44:51.111Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.111Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter-driver-sigrok/examples/exporter.yaml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
docs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/pyproject.tomlpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
docs/source/reference/package-apis/drivers/sigrok.mdpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-sigrok/README.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Each package must have its own README.md documentation
Applied to files:
packages/jumpstarter-driver-sigrok/README.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to pyproject.toml : Top-level `pyproject.toml` should define the UV workspace and include all packages
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to pyproject.toml : All packages in `packages/*` and `examples/*` directories must be included as workspace members in the top-level `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
🧬 Code graph analysis (5)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
data(67-70)decode(72-101)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (4)
CaptureResult(53-134)OutputFormat(8-19)Sample(22-26)decode(72-101)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py (3)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (3)
client(36-37)Sigrok(23-271)capture(65-85)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (1)
SigrokClient(8-36)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (5)
CaptureConfig(39-50)CaptureResult(53-134)OutputFormat(8-19)data(67-70)decode(72-101)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (3)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
parse_csv(8-51)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
parse_vcd(6-62)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (3)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (6)
CaptureConfig(39-50)DecoderConfig(29-36)OutputFormat(8-19)all(18-19)data(67-70)decode(72-101)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(57-274)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (5)
scan(11-12)get_driver_info(29-30)get_channel_map(32-33)list_output_formats(35-36)capture(14-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (41)
packages/jumpstarter-driver-sigrok/examples/exporter.yaml (1)
1-21: LGTM!The example configuration demonstrates the Sigrok driver setup clearly with appropriate placeholders for sensitive values and practical channel mappings.
docs/source/reference/package-apis/drivers/sigrok.md (1)
1-1: LGTM!Correctly follows the project convention of symlinking driver documentation to the package README. Based on learnings, all driver packages should have a symlink pointing to their README.md.
docs/source/reference/package-apis/drivers/index.md (2)
74-75: LGTM!The Sigrok driver is appropriately categorized under "Debug and Programming Drivers" and follows the established documentation format.
109-109: LGTM!Toctree entry correctly added in alphabetical order.
packages/jumpstarter-driver-sigrok/README.md (2)
1-8: LGTM!Comprehensive documentation that clearly explains the driver's capabilities, installation, and configuration options.
183-190: Clarify whether values dictionary keys use device names or mapped channel names.The example configuration maps
A0: CH1andA1: CH2, but the code accesses values using device names (A0,A1). Confirm that the values dictionary keys are device names regardless of the channel mapping configuration, and update the example with a comment explaining this behavior if it may confuse users.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/__init__.py (1)
1-11: LGTM!Clean public API surface exposing all necessary types for users of the Sigrok driver.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (3)
8-111: LGTM!Excellent comprehensive test coverage for the VCD parser, including single/multi-character identifiers, timescale variations, X/Z state handling, binary vectors, and real (analog) values.
144-168: LGTM!Good edge case coverage for empty timestamp lines in VCD files.
170-232: LGTM!Thorough boundary testing for VCD identifier encoding (single-char, two-char, three-char) aligning with libsigrok's vcd_identifier() implementation.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (3)
6-62: LGTM! Well-structured VCD parser with clear separation of concerns.The main
parse_vcdfunction correctly:
- Parses header to extract timescale and channel mapping
- Stops header parsing at
$enddefinitions- Processes value changes with proper timing
112-153: LGTM! Value change parsing handles multiple VCD formats correctly.The parser handles single-bit, binary, and real value formats. Note that
x/z(undefined/high-impedance) states are mapped to0, which is a reasonable simplification for typical logic analysis use cases.
155-225: LGTM! Helper functions are well-implemented with proper error handling.Good defensive coding with
try-exceptblocks returning safe defaults (0for binary,0.0for real) on parse failures.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (3)
8-51: LGTM! Well-structured CSV parser with clear documentation.The main
parse_csvfunction correctly:
- Parses sample rate for timing calculation
- Skips comments and analog preview lines
- Infers channel names from type header
- Builds samples with timing information
54-66: Edge case:1GHzwould raiseValueError.The function checks for
GHZsuffix but the condition at line 60 usesf"{suffix}HZ"which isGHZ. However, for input like"1GHZ", it matches and returns correctly. The issue is with mixed case like"1Ghz"- afterupper()it becomes"1GHZ"which is handled.Actually, looking more carefully - this is correct. Approving.
87-113: LGTM! Channel name inference handles digital/analog/unknown types appropriately.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (6)
1-35: LGTM! Well-structured test fixtures with proper client-server pattern.The fixtures correctly:
- Configure demo driver with channel mappings
- Use
serve()context manager for client-server boundary testing- Enable testing serialization through the wire
37-69: LGTM! Good coverage of basic capture functionality and model validation.Tests correctly verify that
CaptureResultis a proper Pydantic model withbytesdata, not raw base64 strings.
71-168: LGTM! Good format-specific tests with proper VCD decoding verification.Tests cover default format behavior, CSV output, and analog channel handling. The analog test correctly creates a separate driver with analog channel mappings.
170-218: LGTM! Dict config test validates Pydantic coercion, streaming test appropriately skipped.Good documentation in the skip reason explaining sigrok-cli's stdout streaming limitations.
220-263: LGTM! Metadata tests correctly don't require sigrok-cli.These tests verify dict/list serialization through the client-server boundary without invoking sigrok-cli.
265-489: LGTM! Comprehensive decode test coverage across all supported formats.Tests verify:
- CSV/VCD produce
Sampleobjects with timing- ASCII returns string
- BITS returns dict with bit sequences
- Binary raises
NotImplementedError- Both digital and analog channels work
Based on learnings, this comprehensive test coverage aligns with driver implementation requirements.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py (3)
1-39: LGTM! Test fixtures properly configured for CSV format testing.Note:
SigrokClientimport at line 7 is used for type annotation in test function signatures, which is appropriate.
62-131: LGTM! Timing and channel type tests are well-designed.The tests correctly verify:
- Timing calculation:
10,000nsper sample at100kHz- Analog values parsed as
int/float- Mixed digital/analog capture works
41-60: Assertion may be inconsistent with channel naming.The config uses semantic names
["vcc", "cs"]but the assertion checks for device names"D0"/"D1". Clarify whether the intent is to verify that semantic names are preserved in decoded output (in which case the assertion should check for"vcc"or"cs"), or if the CSV parser infers device names from the data regardless of input channel mapping (in which case the assertion is correct but the comment should be clearer about this behavior).packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (10)
1-19: LGTM!The imports are well-organized and the
find_sigrok_cliutility function properly validates the presence of the sigrok-cli executable with a clear error message.
22-37: LGTM!The class structure follows the expected pattern for jumpstarter drivers. The
client()classmethod correctly returns the import path. Per coding guidelines, driver packages must implement adriver.pyfile containing the driver implementation - this is properly implemented.
41-54: LGTM!The
scanandget_driver_infomethods are straightforward. Usingcheck=Truewithsubprocess.runwill propagate errors appropriately.
60-62: LGTM!Simple delegation to
OutputFormat.all().
119-145: LGTM!The command builder methods are well-structured. Good separation of concerns between capture and streaming commands.
146-168: LGTM!The channel argument building with filtering and mapping is well implemented. Case-insensitive matching for user-friendly names is a good UX choice.
169-188: LGTM!Good default behavior: 1000 samples when not specified for non-continuous captures. The sigrok-cli requirement for
--samples,--frames,--time, or--continuousis handled correctly.
190-249: LGTM!The trigger and decoder argument handling is well implemented. The auto-mapping for common protocols (SPI, I2C, UART) is a helpful convenience feature. The recursive flattening of stacked decoders handles nested configurations correctly.
251-271: LGTM!Channel resolution with fallback to device names and case-insensitive matching is robust. The explicit
ValueErrorwith a descriptive message aids debugging.
70-73: Add error handling for subprocess failures.
subprocess.runis called withoutcheck=Trueorcapture_output=True. If sigrok-cli fails, the error will be silently ignored and the subsequentoutfile.read_bytes()will fail with an obscure error (file not found or empty).self.logger.debug("running sigrok-cli: %s", " ".join(cmd)) - subprocess.run(cmd, check=True) + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"sigrok-cli failed: {result.stderr}")Likely an incorrect or invalid review comment.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (6)
22-26: LGTM!Clean Pydantic model for sample data with appropriate types.
29-36: LGTM!The forward reference for recursive
stackis handled correctly with the string annotation"DecoderConfig".
39-50: LGTM!Comprehensive configuration model with sensible defaults and good documentation via Field descriptions.
53-70: LGTM!Good use of base64 encoding for reliable JSON transport. The
dataproperty provides convenient access to the raw bytes.
72-101: LGTM!The decode method handles format-specific parsing well with appropriate error handling for unsupported formats. The local imports avoid circular dependencies.
103-134: LGTM!The bits parsing correctly handles sigrok-cli's multi-line wrapping behavior by accumulating bits across repeated channel names. The filtering
if b in "01"gracefully handles any non-bit characters.
packages/jumpstarter-driver-sigrok/-
Outdated
| @@ -0,0 +1 @@ | |||
| ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ����������������������������������������ݾ�������������������ٶ������������������������������������� No newline at end of file | |||
There was a problem hiding this comment.
Remove this corrupted/garbage file.
This file named - contains only non-ASCII garbage characters and appears to be accidentally included. This could be the result of a mishandled shell command (e.g., cat - > file). It should be removed from the repository.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-sigrok/- around lines 1 to 1, the file is
corrupted and contains non-ASCII/garbage content; remove this accidental file
from the repository by deleting packages/jumpstarter-driver-sigrok/- and ensure
no other references to it remain (remove from index/commit, update .gitignore if
needed to prevent re-creation), then commit the deletion with a clear message
like "chore: remove stray corrupted file '-'".
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py
Outdated
Show resolved
Hide resolved
| [project] | ||
| name = "jumpstarter-driver-sigrok" | ||
| dynamic = ["version", "urls"] | ||
| description = "Jumpstarter driver wrapping sigrok-cli for logic analyzer and oscilloscope support" | ||
| readme = "README.md" | ||
| license = "Apache-2.0" | ||
| authors = [ | ||
| { name = "Miguel Angel Ajo Pelayo", email = "miguelangel@ajo.es" } | ||
| ] | ||
| requires-python = ">=3.11" | ||
| dependencies = [ | ||
| "jumpstarter", | ||
| ] | ||
|
|
||
| [tool.hatch.version] | ||
| source = "vcs" | ||
| raw-options = { 'root' = '../../'} | ||
|
|
||
| [tool.hatch.metadata.hooks.vcs.urls] | ||
| Homepage = "https://jumpstarter.dev" | ||
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | ||
|
|
||
| [tool.pytest.ini_options] | ||
| addopts = "--cov --cov-report=html --cov-report=xml" | ||
| log_cli = true | ||
| log_cli_level = "INFO" | ||
| testpaths = ["jumpstarter_driver_sigrok"] | ||
| asyncio_default_fixture_loop_scope = "function" | ||
|
|
||
| [build-system] | ||
| requires = ["hatchling", "hatch-vcs", "hatch-pin-jumpstarter"] | ||
| build-backend = "hatchling.build" | ||
|
|
||
| [tool.hatch.build.hooks.pin_jumpstarter] | ||
| name = "pin_jumpstarter" | ||
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "pytest-cov>=6.0.0", | ||
| "pytest>=8.3.3", | ||
| "pytest-asyncio>=0.24.0", | ||
| ] |
There was a problem hiding this comment.
Missing jumpstarter.drivers entry point registration.
Per the project guidelines, driver packages must register via the jumpstarter.drivers entry point in pyproject.toml. This enables automatic driver discovery.
Add the entry point configuration:
+[project.entry-points."jumpstarter.drivers"]
+sigrok = "jumpstarter_driver_sigrok"
+
[tool.hatch.version]
source = "vcs"Based on learnings, driver packages must register via the jumpstarter.drivers entry point.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [project] | |
| name = "jumpstarter-driver-sigrok" | |
| dynamic = ["version", "urls"] | |
| description = "Jumpstarter driver wrapping sigrok-cli for logic analyzer and oscilloscope support" | |
| readme = "README.md" | |
| license = "Apache-2.0" | |
| authors = [ | |
| { name = "Miguel Angel Ajo Pelayo", email = "miguelangel@ajo.es" } | |
| ] | |
| requires-python = ">=3.11" | |
| dependencies = [ | |
| "jumpstarter", | |
| ] | |
| [tool.hatch.version] | |
| source = "vcs" | |
| raw-options = { 'root' = '../../'} | |
| [tool.hatch.metadata.hooks.vcs.urls] | |
| Homepage = "https://jumpstarter.dev" | |
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | |
| [tool.pytest.ini_options] | |
| addopts = "--cov --cov-report=html --cov-report=xml" | |
| log_cli = true | |
| log_cli_level = "INFO" | |
| testpaths = ["jumpstarter_driver_sigrok"] | |
| asyncio_default_fixture_loop_scope = "function" | |
| [build-system] | |
| requires = ["hatchling", "hatch-vcs", "hatch-pin-jumpstarter"] | |
| build-backend = "hatchling.build" | |
| [tool.hatch.build.hooks.pin_jumpstarter] | |
| name = "pin_jumpstarter" | |
| [dependency-groups] | |
| dev = [ | |
| "pytest-cov>=6.0.0", | |
| "pytest>=8.3.3", | |
| "pytest-asyncio>=0.24.0", | |
| ] | |
| [project] | |
| name = "jumpstarter-driver-sigrok" | |
| dynamic = ["version", "urls"] | |
| description = "Jumpstarter driver wrapping sigrok-cli for logic analyzer and oscilloscope support" | |
| readme = "README.md" | |
| license = "Apache-2.0" | |
| authors = [ | |
| { name = "Miguel Angel Ajo Pelayo", email = "miguelangel@ajo.es" } | |
| ] | |
| requires-python = ">=3.11" | |
| dependencies = [ | |
| "jumpstarter", | |
| ] | |
| [project.entry-points."jumpstarter.drivers"] | |
| sigrok = "jumpstarter_driver_sigrok" | |
| [tool.hatch.version] | |
| source = "vcs" | |
| raw-options = { 'root' = '../../'} | |
| [tool.hatch.metadata.hooks.vcs.urls] | |
| Homepage = "https://jumpstarter.dev" | |
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | |
| [tool.pytest.ini_options] | |
| addopts = "--cov --cov-report=html --cov-report=xml" | |
| log_cli = true | |
| log_cli_level = "INFO" | |
| testpaths = ["jumpstarter_driver_sigrok"] | |
| asyncio_default_fixture_loop_scope = "function" | |
| [build-system] | |
| requires = ["hatchling", "hatch-vcs", "hatch-pin-jumpstarter"] | |
| build-backend = "hatchling.build" | |
| [tool.hatch.build.hooks.pin_jumpstarter] | |
| name = "pin_jumpstarter" | |
| [dependency-groups] | |
| dev = [ | |
| "pytest-cov>=6.0.0", | |
| "pytest>=8.3.3", | |
| "pytest-asyncio>=0.24.0", | |
| ] |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-sigrok/pyproject.toml around lines 1 to 42, the
package lacks the required jumpstarter.drivers entry point registration so the
driver won’t be auto-discovered; add a [project.entry-points] table (or extend
it if present) and register a jumpstarter.drivers entry mapping the driver name
to the package module/class (e.g., "sigrok =
jumpstarter_driver_sigrok:SigrokDriver") so the jumpstarter registry can load
this driver at runtime, then bump any metadata if needed and run a local install
to verify discovery.
|
|
||
| [tool.hatch.metadata.hooks.vcs.urls] | ||
| Homepage = "https://jumpstarter.dev" | ||
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" |
There was a problem hiding this comment.
Incorrect repository URL in source_archive.
The URL uses repo instead of the actual repository name.
-source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip"
+source_archive = "https://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | |
| source_archive = "https://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip" |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-sigrok/pyproject.toml around line 21, the
source_archive URL incorrectly uses "repo" as the repository name; update the
URL to use the actual repository name (e.g., "jumpstarter-driver-sigrok") so it
reads
https://github.com/jumpstarter-dev/jumpstarter-driver-sigrok/archive/{commit_hash}.zip,
keeping the {commit_hash} placeholder intact.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
65-74: Picosecond timescale precision issue previously flagged.This issue was identified in a previous review. When timescale is in picoseconds (e.g.,
$timescale 100 ps $end), the integer conversion results in 0, causing all timestamps to collapse.packages/jumpstarter-driver-sigrok/pyproject.toml (2)
14-17: Missingjumpstarter.driversentry point registration.Per coding guidelines, driver packages must register via the
jumpstarter.driversentry point for automatic driver discovery.Add the entry point configuration:
dependencies = [ "jumpstarter", ] +[project.entry-points."jumpstarter.drivers"] +sigrok = "jumpstarter_driver_sigrok" + [tool.hatch.version]
19-22: Incorrect repository URL in source_archive.The URL uses
repoinstead of the actual repository namejumpstarter.-source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" +source_archive = "https://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip"
🧹 Nitpick comments (6)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (1)
113-142: Timescale test logic could be clearer.The test correctly validates different timescale units, but the conditional at line 129 (
#{100 if expected_time_ns else 0}) makes the test somewhat harder to follow. The test cases at lines 117-122 include expected times that aren't directly asserted. Consider simplifying to just verify the first sample at time 0, or add explicit assertions for the second sample's time.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (2)
6-62: Sample rate parameter documented as unused.The
sample_rateparameter is correctly documented as unused since VCD has its own timescale. Consider adding_prefix or using# noqacomment to indicate intentionally unused parameter, or add a note in the function signature.-def parse_vcd(data: bytes, sample_rate: str) -> list[dict]: +def parse_vcd(data: bytes, sample_rate: str) -> list[dict]: # noqa: ARG001 - sample_rate unused, kept for API consistency
77-109: Consider error handling for malformed timestamps.Line 96
int(time_str)could raiseValueErrorif the VCD file contains malformed timestamp data. Consider wrapping in try/except for robustness, similar to how binary/real value parsing handles errors.# Skip empty time lines if not time_str: return None - time_units = int(time_str) + try: + time_units = int(time_str) + except ValueError: + return None # Skip malformed timestamp lines current_time_ns = time_units * timescale_multiplierpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (1)
17-27: Consider adding return type annotation forcapture_stream.The generator function lacks a return type annotation. Adding it would improve type checking and IDE support.
- def capture_stream(self, config: CaptureConfig | dict): + def capture_stream(self, config: CaptureConfig | dict) -> Iterator[bytes]:And add the import at the top:
from collections.abc import Iteratorpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
8-19: Consider using Enum for type safety and better tooling support.The constants pattern works but Python's
Enumis more idiomatic for this use case, providing better type checking, IDE autocompletion, and iteration support.Apply this diff to refactor to Enum:
+from enum import Enum + + -class OutputFormat: - """Constants for sigrok output formats.""" - CSV = "csv" - BITS = "bits" - ASCII = "ascii" - BINARY = "binary" - SRZIP = "srzip" - VCD = "vcd" - - @classmethod - def all(cls) -> list[str]: - return [cls.CSV, cls.BITS, cls.ASCII, cls.BINARY, cls.SRZIP, cls.VCD] +class OutputFormat(str, Enum): + """Constants for sigrok output formats.""" + CSV = "csv" + BITS = "bits" + ASCII = "ascii" + BINARY = "binary" + SRZIP = "srzip" + VCD = "vcd" + + @classmethod + def all(cls) -> list[str]: + return [member.value for member in cls]
103-134: Consider explicit validation for the bits string format.Line 127 silently filters out non-0/1 characters, which may hide malformed data. While this approach handles whitespace and format quirks gracefully, explicit validation could help catch unexpected format issues during development.
If stricter validation is desired, consider:
# Parse bits from this line - bits = [int(b) for b in bits_str if b in "01"] + # Strip whitespace but validate format + bits_str_clean = bits_str.replace(" ", "").replace("\t", "") + if not all(b in "01" for b in bits_str_clean): + raise ValueError(f"Invalid bits format for {channel_name}: {bits_str}") + bits = [int(b) for b in bits_str_clean]Alternatively, retain the current lenient approach if Sigrok output contains expected non-binary characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/sigrok.md(1 hunks)packages/jumpstarter-driver-sigrok/.gitignore(1 hunks)packages/jumpstarter-driver-sigrok/README.md(1 hunks)packages/jumpstarter-driver-sigrok/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/__init__.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py(1 hunks)packages/jumpstarter-driver-sigrok/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-sigrok/.gitignore
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/source/reference/package-apis/drivers/sigrok.md
- packages/jumpstarter-driver-sigrok/examples/exporter.yaml
- packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py
- packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py
- packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/init.py
- packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
- packages/jumpstarter-driver-sigrok/README.md
🧰 Additional context used
📓 Path-based instructions (5)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-sigrok/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-sigrok/pyproject.toml
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/pyproject.tomlpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/pyproject.tomldocs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/pyproject.tomlpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.tomldocs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.tomldocs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to pyproject.toml : Top-level `pyproject.toml` should define the UV workspace and include all packages
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-sigrok/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: All driver packages should have a symlink in `docs/source/reference/package-apis/drivers/` pointing to the driver's README.md
Applied to files:
docs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-09-06T05:25:18.184Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 608
File: packages/jumpstarter-cli/jumpstarter_cli/run.py:0-0
Timestamp: 2025-09-06T05:25:18.184Z
Learning: Zombie process reaping in jumpstarter is useful beyond just PID 1 cases, as drivers can leave behind processes in various execution contexts.
Applied to files:
docs/source/reference/package-apis/drivers/index.md
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py
🧬 Code graph analysis (4)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
parse_csv(8-51)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
parse_vcd(6-62)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (3)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (7)
CaptureConfig(39-50)CaptureResult(53-134)OutputFormat(8-19)data(67-70)decode(72-101)all(18-19)Sample(22-26)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (8)
Sigrok(23-271)client(36-37)scan(42-46)capture(65-85)capture_stream(88-115)get_driver_info(49-54)get_channel_map(57-58)list_output_formats(61-62)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (6)
scan(11-12)capture(14-15)capture_stream(17-27)get_driver_info(29-30)get_channel_map(32-33)list_output_formats(35-36)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (3)
OutputFormat(8-19)Sample(22-26)decode(72-101)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (2)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
data(67-70)decode(72-101)packages/jumpstarter/jumpstarter/config/client.py (1)
channel(123-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (24)
docs/source/reference/package-apis/drivers/index.md (1)
74-75: Documentation index entries for Sigrok driver are syntactically correct and properly positioned.The new entry at lines 74-75 follows the established formatting pattern for driver entries in the "Debug and Programming Drivers" section, and the toctree addition at line 109 maintains alphabetical ordering.
However, verification of the accompanying
sigrok.mdfile and the driver package itself could not be completed due to repository access limitations. Confirm thatdocs/source/reference/package-apis/drivers/sigrok.mdexists and contains the external link to the driver's README as required by the project structure guidelines, and that thejumpstarter-driver-sigrokpackage has been properly added to the codebase.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (7)
1-7: LGTM! Clean imports and proper dependency setup.The imports are well-organized and the test file correctly imports from the package's internal modules. The use of
shutil.whichfor checking sigrok-cli availability is appropriate.
10-34: Well-structured fixtures with clear documentation.The fixtures demonstrate good practices: semantic channel naming (vcc, cs, miso, etc.) that maps to protocol analyzer requirements, and proper use of the
serve()context manager for client-server testing.
37-68: Good type validation and serialization checks.These tests correctly verify that
CaptureResultis a proper Pydantic model (not just a dict) after client-server serialization, and that thedataproperty correctly decodes from base64 to bytes. This is important for catching serialization issues.
130-168: Good coverage of analog channel functionality.The test appropriately creates a separate driver instance with analog channel mappings and validates both the capture flow and CSV content. The fallback assertions handle the demo driver's output variability well.
265-302: Excellent timing verification in CSV decode test.The assertion on line 299 (
assert sample.time_ns == sample.sample * 10_000) correctly validates the timing calculation based on sample rate (1/100kHz = 10μs = 10,000ns). This is a valuable correctness check.
193-217: Appropriate skip with documented reason.The streaming test is correctly skipped with a clear explanation of the sigrok-cli limitation. This preserves the test for when it becomes supported while avoiding false failures.
220-262: Correctly omitted skipif for non-CLI tests.These tests (
test_get_driver_info,test_get_channel_map,test_list_output_formats) correctly don't require the skipif decorator since they test driver configuration and metadata that doesn't invoke sigrok-cli.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py (4)
1-6: LGTM! Clean test module setup.Good import structure for unit testing the VCD parser through the
CaptureResult.decode()integration path.
8-111: Comprehensive VCD parser coverage with appropriate tolerances.Excellent test that covers all VCD value formats (single-bit, binary vectors, real/analog), identifier types (single-char, multi-char), and special states (X/Z converted to 0). The float comparisons correctly use appropriate tolerances for different magnitudes.
144-168: Good edge case coverage for empty timestamps.This test validates that the parser correctly handles and skips empty timestamp lines (
#\n) in VCD data, which is an important robustness check.
170-232: Excellent boundary testing for VCD identifiers.The test thoroughly validates the parser's handling of libsigrok's VCD identifier encoding scheme, including single-character (0-93), two-letter (94-769), and three-letter (770+) identifiers. This is critical for supporting large channel counts.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (3)
1-5: LGTM! Clean module docstring and import.The module is well-structured with a clear purpose documented in the docstring.
112-153: Clean value parsing implementation.The function correctly handles all VCD value formats with a clear state machine approach. The X/Z to 0 conversion is appropriate for practical signal analysis. The silent skip of unknown characters at lines 151-152 is reasonable for robustness.
155-225: LGTM! Robust helper functions with proper error handling.The helper functions correctly handle edge cases:
_parse_binary_valuecatchesValueErrorfor invalid binary strings (line 189-191), and_parse_real_valuesupports scientific notation and catchesValueError(lines 220-223). Good defensive coding.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/client.py (2)
1-9: LGTM! Clean client class structure.The
SigrokClientcorrectly extendsDriverClientand uses the@dataclass(kw_only=True)decorator consistent with the Jumpstarter client pattern. The import ofCaptureConfigandCaptureResultfrom.commonenables proper type validation.
11-37: Well-implemented client methods with proper validation.The
capture()method correctly usesCaptureResult.model_validate()to ensure the server response is properly deserialized into a Pydantic model. Thecapture_stream()generator pattern is appropriate for streaming data.packages/jumpstarter-driver-sigrok/pyproject.toml (2)
1-14: Package metadata looks good.The package follows the naming convention (
jumpstarter-driver-sigrok), uses Apache-2.0 license, and declares the requiredjumpstarterdependency as per coding guidelines.
23-42: LGTM! Build and test configuration is appropriate.The pytest configuration, build system, and dev dependencies are correctly set up. The
testpathscorrectly points to the package directory, and the build hooks include the requiredhatch-pin-jumpstarter.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (5)
22-26: LGTM!Clean Pydantic model with appropriate types for both digital and analog channel values.
29-36: LGTM!Proper use of forward reference for the recursive
stackfield, enabling nested decoder configurations.
39-50: LGTM!Well-documented configuration model with sensible defaults. VCD as default output format is appropriate for its efficiency with sparse data.
66-70: LGTM!Clean property implementation with local import for base64 decoding.
72-101: LGTM!Well-structured decode method with format-specific handling, proper model validation, and clear error messages for unsupported formats.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (1)
68-70: Fix return type annotation: keys are strings, not integers.The return type annotation
dict[int, str]doesn't match thechannelsfield typedict[str, str](Line 31). This will cause type checking failures.Apply this diff:
@export - def get_channel_map(self) -> dict[int, str]: + def get_channel_map(self) -> dict[str, str]: return self.channels
🧹 Nitpick comments (4)
.github/workflows/pytest.yaml (1)
54-59: Version pinning recommended for sigrok-cli to ensure reproducible CI builds.The sigrok-cli installations don't specify versions, which could lead to inconsistent test behavior if sigrok-cli releases a breaking change or if the demo driver's output format changes between versions. Since tests rely on the sigrok demo driver (per PR description), pinning to a known-good version would improve build stability.
Consider pinning sigrok-cli to a stable version:
- - name: Install sigrok-cli (Linux) + - name: Install sigrok-cli (Linux) if: runner.os == 'Linux' run: | sudo apt-get update - sudo apt-get install -y sigrok-cli + sudo apt-get install -y sigrok-cli=0.7.2-1and
- - name: Install sigrok-cli (macOS) + - name: Install sigrok-cli (macOS) if: runner.os == 'macOS' run: | - brew install sigrok-cli + brew install sigrok-cli@0.7.2(Adjust version numbers to match the validated version.)
Also applies to: 65-69
Dockerfile (1)
10-10: Verify Docker and CI build consistency for sigrok-cli versions.The Dockerfile installs sigrok-cli via
dnf(Fedora), while CI environments useapt-get(Linux) andbrew(macOS). These package managers may install different sigrok-cli versions, which could cause inconsistent test results or demo driver behavior. If version pinning is added to CI (recommended above), consider applying the same version constraint in the Dockerfile for consistency.Once a validated sigrok-cli version is selected for CI, update the Dockerfile to pin the same version:
-RUN dnf install -y python3 ustreamer libusb1 android-tools python3-libgpiod sigrok-cli && \ +RUN dnf install -y python3 ustreamer libusb1 android-tools python3-libgpiod sigrok-cli-0.7.2-1 && \(Adjust version to match the pinned CI version.)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (2)
43-47: Consider removing redundant skipif decorators.The module-level
pytestmarkat Line 10 already skips all tests whensigrok-cliis unavailable. The individual@pytest.mark.skipifdecorators on lines 43, 50, 77, etc. are redundant and add unnecessary clutter.Apply this pattern throughout the file:
-@pytest.mark.skipif(which("sigrok-cli") is None, reason="sigrok-cli not installed") def test_scan_demo_driver(demo_client):
280-280: Consider moving imports to module level.Several test functions import
OutputFormatandSamplefrom.commoninside the function body (lines 280, 319, 345, 385, 425, 473), even though.commonis already imported at Line 5. Moving these to the module-level import would reduce duplication.Apply this diff at the module level:
-from .common import CaptureConfig, CaptureResult, OutputFormat +from .common import CaptureConfig, CaptureResult, OutputFormat, SampleThen remove the local imports from within the test functions.
Also applies to: 319-319, 345-345, 385-385, 425-425, 473-473
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pytest.yaml(1 hunks)Dockerfile(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (8)
Sigrok(25-286)client(46-47)scan(52-58)capture(77-98)capture_stream(101-129)get_driver_info(61-66)get_channel_map(69-70)list_output_formats(73-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (21)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (2)
1-13: LGTM!The module-level setup correctly guards all tests with
pytestmarkto skip whensigrok-cliis unavailable, preventing false failures in environments without the dependency.
16-40: LGTM!The fixtures properly set up a demo driver with semantic channel names and use the
serve()pattern for client-server testing, which aligns with the project's testing conventions.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (19)
1-22: LGTM!The imports are well-organized, and the
find_sigrok_cli()utility correctly usesshutil.which()to locate the executable in PATH.
24-31: LGTM!The class definition properly extends
Driverand uses@dataclass(kw_only=True), which is a good practice for classes with multiple optional parameters. The field types are correctly annotated.
33-43: LGTM!The
__post_init__method safely handles the parent class initialization, and_ensure_executable()provides a clear error message whensigrok-cliis unavailable.
45-47: LGTM!The
client()classmethod correctly returns the client path string, following the driver pattern for client registration.
51-58: LGTM!The
scan()method correctly ensures the executable is available before running the command and usessubprocess.run()withcheck=Truefor proper error handling.
60-66: LGTM!The
get_driver_info()method provides a clean dictionary interface for driver metadata.
72-74: LGTM!The method correctly delegates to
OutputFormat.all()to provide a list of supported output formats.
76-98: LGTM!The
capture()method properly handles configuration validation, subprocess execution, and cleanup. The use of base64 encoding ensures reliable transport of binary data across the client-server boundary, and thefinallyblock guarantees cleanup.
100-129: LGTM!The async
capture_stream()method follows best practices for subprocess management: it terminates gracefully, waits with a timeout, and kills if necessary. The chunk size of 4096 bytes is reasonable for streaming.
133-144: LGTM!The command builder correctly composes the sigrok-cli command from modular helper methods and returns all necessary components for execution and cleanup.
146-153: LGTM!The streaming command builder appropriately uses
-o -for stdout output and passescontinuous=Trueto configure continuous capture mode.
155-159: LGTM!The base driver args builder correctly handles the optional connection parameter.
161-182: LGTM!The channel argument builder properly handles channel filtering and mapping with case-insensitive comparison, and the implementation follows sigrok-cli's expected format.
184-203: LGTM!The configuration builder correctly handles the mutually exclusive options required by sigrok-cli (--samples, --frames, --time, or --continuous) and provides a sensible default of 1000 samples.
205-212: LGTM!The trigger argument builder correctly resolves channel names and formats trigger conditions for sigrok-cli.
214-235: LGTM!The decoder argument builder properly handles stacked decoders, pin mappings, options, and annotations, following sigrok-cli's protocol decoder syntax.
237-243: LGTM!The decoder flattening logic correctly handles hierarchical decoder stacks through recursion.
245-264: LGTM!The decoder channel resolution provides intelligent defaults for common protocols (SPI, I2C, UART) and gracefully handles cases where auto-mapping isn't possible.
266-286: LGTM!The channel resolution logic properly handles both device channel names and user-friendly names with case-insensitive matching, and provides a clear error message when a channel isn't found.
mangelajo
changed the title
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
I would like to test this PR. Do we have a sigrok capable device available?
| sigrok: | ||
| type: jumpstarter_driver_sigrok.driver.Sigrok | ||
| driver: fx2lafw # sigrok driver (demo, fx2lafw, rigol-ds, etc.) | ||
| conn: null # optional: USB VID.PID, serial path, or null for auto |
There was a problem hiding this comment.
i do not like the implicit behavior of null, cant we just use auto instead
There was a problem hiding this comment.
done, auto is much better 👍
also, VCD tests should not expect channel mapping when not interacting with sigrok-cli, since sigrok-cli is the one performing mappings.
this is a very very cheap option: https://es.aliexpress.com/item/1005007349261117.html (based on the fx2lafw chip) also anything from this list: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-sigrok/README.md (1)
183-190: Inconsistent channel names in Example 3.The exporter config maps
A0: CH1andA1: CH2, but the code accessessample.values.get('A0', ...). Based on the documentation, if channel mapping is used, sigrok-cli outputs the mapped names (CH1,CH2). The access should use the mapped names:- print(f" CH1: {sample.values.get('A0', 'N/A')}V") - print(f" CH2: {sample.values.get('A1', 'N/A')}V") + print(f" CH1: {sample.values.get('CH1', 'N/A')}V") + print(f" CH2: {sample.values.get('CH2', 'N/A')}V")Alternatively, if the intent is to show device names, remove the channel mapping from the exporter config.
🧹 Nitpick comments (11)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
126-131: Unknown/high-impedance states silently mapped to 0.VCD states
x(unknown) andz(high-impedance) are meaningful in digital logic but are currently mapped to0. This loses information that could be important for debugging tri-state buses or initialization issues.Consider preserving these states:
if char in "01xzXZ": symbol, new_i = _extract_symbol(values_str, i + 1) if symbol in channel_map: channel = channel_map[symbol] - current_values[channel] = 1 if char == "1" else 0 + if char == "1": + current_values[channel] = 1 + elif char == "0": + current_values[channel] = 0 + else: + current_values[channel] = char.lower() # 'x' or 'z' i = new_iThis would require updating the type hint for
current_valuestodict[str, int | float | str].packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
126-136:strict=Truein zip may raise ValueError on malformed CSV rows.If a CSV row has a different number of columns than expected (malformed data, trailing commas, etc.), this will raise
ValueErrorand abort parsing. Consider graceful handling:- for channel, value in zip(channel_names, row, strict=True): + for channel, value in zip(channel_names, row): + # Skip if we run out of either channels or values value = value.strip()Or catch and skip malformed rows with a warning.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (1)
8-19: Consider usingStrEnumfor type safety.Using a class with string constants works, but
StrEnumprovides better type checking and IDE support:from enum import StrEnum class OutputFormat(StrEnum): CSV = "csv" BITS = "bits" ASCII = "ascii" BINARY = "binary" SRZIP = "srzip" VCD = "vcd"This allows
OutputFormat.VCD == "vcd"to still work while enabling proper enum validation.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py (1)
13-38: Fixtures are duplicated withdriver_test.py.The
demo_driver_instanceanddemo_clientfixtures are identical to those indriver_test.py. Consider moving them to a sharedconftest.pyfile to reduce duplication and ensure consistency.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (1)
43-47: Redundantskipifdecorators.The
pytestmarkat lines 10-13 already appliesskipif(which("sigrok-cli") is None, ...)to all tests in this module. The individual@pytest.mark.skipifdecorators on each test are redundant and can be removed for cleaner code.-@pytest.mark.skipif(which("sigrok-cli") is None, reason="sigrok-cli not installed") def test_scan_demo_driver(demo_client):Apply this to all tests with the redundant decorator.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (6)
51-58: Capture stderr for better error diagnostics.Line 55 contains a redundant assertion since
_ensure_executable()already raisesFileNotFoundErrorif the executable is None.More importantly,
subprocess.rundoesn't capture stderr, making it difficult to diagnose sigrok-cli failures.Apply this diff to improve error handling:
@export def scan(self) -> str: """List devices for the configured driver.""" self._ensure_executable() - assert self.executable is not None cmd = [self.executable, "--driver", self.driver, "--scan"] - result = subprocess.run(cmd, capture_output=True, text=True, check=True) + result = subprocess.run(cmd, capture_output=True, text=True, check=True) return result.stdoutNote:
capture_output=Truealready captures both stdout and stderr, so the current implementation is actually fine for stderr. The redundant assertion should still be removed.
76-98: Consider capturing stderr for better error diagnostics.Similar to the
scanmethod, whensubprocess.runfails (line 85), stderr output from sigrok-cli would help diagnose issues. However,capture_outputis not specified here.Apply this diff to capture stderr:
try: self.logger.debug("Running sigrok-cli: %s", " ".join(cmd)) - subprocess.run(cmd, check=True) + subprocess.run(cmd, capture_output=True, check=True)This allows sigrok-cli error messages to be included in the
CalledProcessErrorexception whencheck=Truefails.
114-130: Avoid terminating already-finished process.The finally block unconditionally calls
process.terminate()even if the process completed successfully (when the while loop breaks due to EOF). This is harmless but inefficient.Apply this diff to only terminate running processes:
finally: - process.terminate() + if process.returncode is None: + process.terminate() try: await asyncio.wait_for(process.wait(), timeout=5) except asyncio.TimeoutError: process.kill()
155-159: Redundant assertion.Line 156 contains an assertion that
self.executable is not None. This is redundant because all callers (captureandcapture_stream) already invoke_ensure_executable()which raises if the executable is None.def _base_driver_args(self) -> list[str]: - assert self.executable is not None if self.conn and self.conn != "auto": return [self.executable, "-d", f"{self.driver}:conn={self.conn}"] return [self.executable, "-d", self.driver]
184-203: Consider making default sample count configurable.Line 200 hardcodes a default of 1000 samples when neither
cfg.samplesnor continuous mode is specified. This magic number could be:
- Extracted to a class-level constant for easier configuration
- Documented in the
CaptureConfigmodel's docstringExample refactor:
+ DEFAULT_SAMPLES = 1000 + def _config_args(self, cfg: CaptureConfig, *, continuous: bool = False) -> list[str]: parts = [f"samplerate={cfg.sample_rate}"] if cfg.pretrigger is not None: parts.append(f"pretrigger={cfg.pretrigger}") args: list[str] = [] if parts: args += ["-c", ",".join(parts)] # sigrok-cli requires one of: --samples, --frames, --time, or --continuous # If samples is explicitly specified, use that even for streaming if cfg.samples is not None: args.extend(["--samples", str(cfg.samples)]) elif continuous: args.append("--continuous") else: # Default to 1000 samples if not specified - args.extend(["--samples", "1000"]) + args.extend(["--samples", str(self.DEFAULT_SAMPLES)]) return args
245-264: Consider extracting decoder pin defaults to a constant.Lines 250-254 contain hardcoded pin mappings for common protocols. Extracting these to a class-level constant would make the code more maintainable and easier to extend.
+ DECODER_PIN_DEFAULTS = { + "spi": ["clk", "mosi", "miso", "cs"], + "i2c": ["scl", "sda"], + "uart": ["rx", "tx"], + } + def _resolve_decoder_channels(self, decoder: DecoderConfig) -> dict[str, str]: if decoder.channels: return decoder.channels # Best-effort auto-mapping based on common decoder pin names - defaults = { - "spi": ["clk", "mosi", "miso", "cs"], - "i2c": ["scl", "sda"], - "uart": ["rx", "tx"], - } - pins = defaults.get(decoder.name.lower()) + pins = self.DECODER_PIN_DEFAULTS.get(decoder.name.lower()) if not pins: return {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/jumpstarter-driver-sigrok/README.md(1 hunks)packages/jumpstarter-driver-sigrok/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py(1 hunks)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-sigrok/examples/exporter.yaml
- packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd_test.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: All driver packages should have a symlink in `docs/source/reference/package-apis/drivers/` pointing to the driver's README.md
Applied to files:
packages/jumpstarter-driver-sigrok/README.md
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-sigrok/README.mdpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-sigrok/README.mdpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-sigrok/README.mdpackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.pypackages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py
🧬 Code graph analysis (4)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
data(109-112)decode(114-148)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
parse_csv(9-49)packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
parse_vcd(8-61)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (2)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (6)
CaptureConfig(81-92)DecoderConfig(71-78)OutputFormat(8-19)all(18-19)data(109-112)decode(114-148)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(57-274)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
data(109-112)decode(114-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py (1)
98-111: Channel name inference looks reasonable.The type detection logic handles sigrok's known output types (
logic,V DC,mV, etc.) appropriately. The fallback toCH{n}for unknown types is a sensible default.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (1)
114-148: Decode method implementation is well-structured.The heterogeneous return type is intentional and well-documented. The lazy evaluation via generators for CSV/VCD is memory-efficient for large captures.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv_test.py (1)
41-61: Tests are well-structured.Good coverage of CSV format basics with proper assertions on result types and decoded data structure.
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py (2)
136-174: Good test design for analog-specific configuration.This test correctly creates its own driver instance with analog channel mappings rather than reusing the digital-focused
demo_clientfixture. This ensures proper isolation and explicit test intent.
448-462: Well-designed negative test.Good practice to verify that unsupported formats raise the expected
NotImplementedError, ensuring the API contract is clear.packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py (2)
68-70: Previous type mismatch has been resolved.The return type annotation now correctly matches the
channelsfield type (dict[str, str]). The issue raised in the previous review has been addressed.
266-286: LGTM!The channel resolution logic correctly handles both device names (e.g., "D0") and user-friendly names (e.g., "clk") with case-insensitive matching. The error handling is clear and informative.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (2)
8-19: Consider using Enum for better type safety.The current constants-based approach works, but converting
OutputFormatto a string Enum would provide better type checking, IDE autocomplete, and validation.Example refactor using StrEnum:
+from enum import StrEnum + +class OutputFormat(StrEnum): -class OutputFormat: """Constants for sigrok output formats.""" - CSV = "csv" - BITS = "bits" - ASCII = "ascii" - BINARY = "binary" - SRZIP = "srzip" - VCD = "vcd" + CSV = "csv" + BITS = "bits" + ASCII = "ascii" + BINARY = "binary" + SRZIP = "srzip" + VCD = "vcd" - @classmethod - def all(cls) -> list[str]: - return [cls.CSV, cls.BITS, cls.ASCII, cls.BINARY, cls.SRZIP, cls.VCD]Note:
StrEnumis available in Python 3.11+. For earlier versions, usestr, Enumas base classes.
125-129: Consider moving base64 import to module level.The
dataproperty works correctly, but the import inside the method is unconventional. Movingfrom base64 import b64decodeto the top-level imports would be more idiomatic and slightly more efficient.from __future__ import annotations +from base64 import b64decode from typing import Any, IteratorThen update the property:
@property def data(self) -> bytes: """Get the captured data as bytes (auto-decodes from base64).""" - from base64 import b64decode return b64decode(self.data_b64)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (6)
packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py (6)
1-6: LGTM!Module imports are clean and appropriate for the Pydantic models and type hints used throughout the file.
22-68: LGTM!The
Samplemodel is well-implemented with appropriate time formatting. The_format_timemethod correctly handles the full range from femtoseconds to seconds, including the zero-time edge case and appropriate unit selection.
71-78: LGTM!The
DecoderConfigmodel correctly uses forward references for the recursivestackfield, allowing nested decoder configurations.
81-92: LGTM!The
CaptureConfigmodel has sensible defaults and clear field descriptions. The choice of VCD as the default output format is appropriate given its efficiency for change-based captures.
131-165: LGTM!The
decode()method appropriately dispatches to format-specific parsers. The lazy imports forcsvandvcdparsers (lines 150, 154) are likely intentional to avoid circular dependencies, as those modules probably import fromcommon.py.
167-198: LGTM!The
_parse_bits()method correctly parses sigrok-cli's bits format, including:
- Handling multi-line wrapping for large sample counts
- Filtering non-bit characters (line 191), which is appropriate for parsing text output
- Mapping device names to user-friendly names via
channel_map
2 participants
Initial sigrok support, it doesn't support decoders yet which will come in a follow up PR.
Testing relies on the sigrok demo driver which produces test data output.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores / CI
✏️ Tip: You can customize this high-level summary in your review settings.