sigrok - logic analyzer driver #22
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update pyproject.toml files to adjust raw-options root paths from '../..' to '../../..' to account for monorepo subdirectory.
Update multiversion.sh to use correct paths with python/ prefix in worktree structure.
- Update Python container files for monorepo build paths - Copy Kind cluster config with dex nodeport pre-configured - Configure controller and e2e values with certificate placeholder - Patch deploy_with_helm.sh to support EXTRA_VALUES for Helm overlay pattern
- Add unified GitHub Actions workflows with path filters - Configure dependabot for all package ecosystems - Remove old .github directories from subdirectories - Install e2e test scripts (setup-e2e.sh, run-e2e.sh, tests.bats)
also, VCD tests should not expect channel mapping when not interacting with sigrok-cli, since sigrok-cli is the one performing mappings.
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new Jumpstarter driver package for Sigrok integration, enabling logic analyzer and protocol capture via sigrok-cli. The package provides device scanning, configurable single-shot and streaming captures with multiple output formats (CSV, VCD, BITS, ASCII), protocol decoder support, and format parsers with automatic timestamp calculations. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as SigrokClient
participant Driver as Sigrok Driver
participant CLI as sigrok-cli
participant Parser as Format Parser
User->>Client: capture(config)
Client->>Driver: call("capture", config)
Driver->>Driver: _build_capture_command(config)
Note over Driver: Construct CLI args:<br/>channels, sample_rate,<br/>triggers, decoders
Driver->>CLI: Execute with output file
CLI->>CLI: Capture samples
CLI-->>Driver: Write output (CSV/VCD/etc)
Driver->>Driver: Read & base64-encode
Driver-->>Client: CaptureResult (data_b64)
Client-->>User: CaptureResult
User->>Client: result.decode()
Client->>Parser: parse_csv/parse_vcd(data)
Parser->>Parser: Parse format headers
Parser->>Parser: Yield Sample objects
Parser-->>Client: Iterator[Sample]
Client-->>User: Decoded samples
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py`:
- Around line 67-82: The _extract_csv_data_lines function currently returns
blank lines which later produce empty rows and cause zip(..., strict=True) to
raise ValueError; update _extract_csv_data_lines to also skip
empty/whitespace-only lines (after strip) so it only appends non-empty CSV data
lines (i.e., keep the existing comment and analog-preview checks in
_extract_csv_data_lines and add a guard that continues if line == "" to avoid
emitting empty rows).
In
`@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py`:
- Around line 304-305: Replace the strict float equality assertion on
sample.time with an approximate comparison using pytest.approx to avoid
precision errors: change the assertion that compares sample.time to
sample.sample * 0.00001 (the current line referencing sample.time and
sample.sample) to use pytest.approx(...). Ensure pytest is imported in
jumpstarter_driver_sigrok/driver_test.py if not already present and use
pytest.approx(sample.sample * 0.00001) (or an explicit tolerance via rel/abs) in
the assert.
In
`@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py`:
- Around line 161-182: _channel_args currently returns no -C when self.channels
is empty or when selected_names contain device keys (e.g., "D0") that aren't
values in self.channels, causing selections to be ignored; modify _channel_args
and related logic so selected_names is matched against both user names and
device keys: when self.channels is empty, build channel_map from selected_names
by treating each name as a device (mapping name=name), and when filtering a
non-empty self.channels include entries where either the user name (value) or
the device name (key) is in selected_names; also update _resolve_channel to
accept device names by returning the device key when present (falling back to
existing name->device lookup), ensuring trigger/decoder pin mapping honors
selections and unmapped device names.
- Around line 107-112: The subprocess is created with
stderr=asyncio.subprocess.PIPE but never read, which can deadlock; in the code
around the asyncio.create_subprocess_exec call (search for the
self.logger.debug("streaming sigrok-cli: %s", " ".join(cmd)) and the process =
await asyncio.create_subprocess_exec(...) line in
jumpstarter_driver_sigrok/driver.py), either change stderr to None to inherit
the parent stderr or add a concurrent reader that drains process.stderr (e.g.,
spawn an asyncio.create_task that reads and discards/logs stderr lines) so the
stderr pipe cannot fill and stall the sigrok-cli stream.
In `@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py`:
- Around line 22-61: The parser currently only handles value changes when they
appear on the same line as the timestamp; change the main parsing loop (the
second loop that uses sample_idx, timescale_multiplier and channel_map) to treat
a line starting with "#" as a timestamp marker: record the current timestamp,
then iterate subsequent lines until the next "#" (or EOF) and for each
non-empty, non-directive line parse it as a value-change and yield a sample with
sample_idx incremented; additionally handle $dumpvars by detecting a "$dumpvars"
directive and consuming lines until the matching "$end", parsing each inner
value-change the same way. Use/extend the existing helpers (_parse_timescale,
_parse_vcd_timestamp_line or add a new _parse_vcd_value_change) and keep
channel_map/timescale_multiplier usage, assigning sample number via sample_idx
for each yielded change.
In `@python/packages/jumpstarter-driver-sigrok/pyproject.toml`:
- Around line 19-21: The source_archive URL under
[tool.hatch.metadata.hooks.vcs.urls] is pointing to "jumpstarter-dev/repo" which
is incorrect; update the source_archive value to use the correct repository name
"jumpstarter-dev/monorepo" while preserving the {commit_hash} placeholder (i.e.,
change source_archive =
"https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" to reference
"monorepo") so archive links resolve correctly.
In `@python/packages/jumpstarter-driver-sigrok/README.md`:
- Around line 17-28: The README's export YAML for Sigrok is missing the required
config: wrapper; update the example so the sigrok export block nests the driver
settings under config (i.e., export -> sigrok -> type/driver/conn/channels
remain but are placed inside a config: mapping) so it matches the schema used by
examples/exporter.yaml and the Sigrok exporter class
(jumpstarter_driver_sigrok.driver.Sigrok).
Comment on lines
+67
to
+82
| def _extract_csv_data_lines(lines: list[str]) -> list[str]: | ||
| """Extract actual CSV data lines, skipping comments and analog preview lines.""" | ||
| data_lines = [] | ||
|
|
||
| for _i, line in enumerate(lines): | ||
| line = line.strip() | ||
| # Skip comment lines | ||
| if line.startswith(";"): | ||
| continue | ||
| # Skip analog preview lines (contain colon, not CSV comma-separated) | ||
| if ":" in line and "," not in line: | ||
| continue | ||
| # This is CSV data | ||
| data_lines.append(line) | ||
|
|
||
| return data_lines |
Contributor
There was a problem hiding this comment.
Skip empty CSV lines to avoid strict-zip failures.
Blank lines produce empty rows and zip(..., strict=True) raises ValueError, aborting decode. Filter empty lines here (or before parsing rows).
🐛 Suggested fix
@@
- line = line.strip()
+ line = line.strip()
+ if not line:
+ continue🤖 Prompt for AI Agents
In `@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py`
around lines 67 - 82, The _extract_csv_data_lines function currently returns
blank lines which later produce empty rows and cause zip(..., strict=True) to
raise ValueError; update _extract_csv_data_lines to also skip
empty/whitespace-only lines (after strip) so it only appends non-empty CSV data
lines (i.e., keep the existing comment and analog-preview checks in
_extract_csv_data_lines and add a guard that continues if line == "" to avoid
emitting empty rows).
Comment on lines
+304
to
+305
| # Verify timing progresses (1/100kHz = 0.00001s per sample) | ||
| assert sample.time == sample.sample * 0.00001 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the full test file to understand context and imports
wc -l "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py"Repository: jumpstarter-dev/monorepo
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check imports and the specific lines mentioned
head -30 "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py"
echo "---"
sed -n '290,320p' "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py"Repository: jumpstarter-dev/monorepo
Length of output: 1849
🏁 Script executed:
#!/bin/bash
# Also check where sample.time and sample.sample are defined
grep -n "sample.time\|sample.sample" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py" -B 2 -A 2Repository: jumpstarter-dev/monorepo
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Check if pytest is imported
grep -n "import pytest\|from pytest" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py"Repository: jumpstarter-dev/monorepo
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find the Sample class definition
rg "class Sample" --type pyRepository: jumpstarter-dev/monorepo
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check for other similar assertions in the test file that compare floats
grep -n "assert.*==" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py" | head -20Repository: jumpstarter-dev/monorepo
Length of output: 475
🏁 Script executed:
#!/bin/bash
# Look at the structure around line 305 more carefully
sed -n '280,330p' "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py"Repository: jumpstarter-dev/monorepo
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Find the Sample class definition in common.py
grep -n "class Sample" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py" -A 20Repository: jumpstarter-dev/monorepo
Length of output: 909
🏁 Script executed:
#!/bin/bash
# Look for where sample.time is set in the decode logic
grep -rn "sample.time\|\.time\s*=" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/" --include="*.py" | grep -v "test" | head -20Repository: jumpstarter-dev/monorepo
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for where Sample instances are created
rg "Sample\(" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/" -t py --max-count 20 -B 3 -A 1Repository: jumpstarter-dev/monorepo
Length of output: 1266
🏁 Script executed:
#!/bin/bash
# Look for CSV decoding logic where Sample objects are created
grep -n "def decode" "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py" -A 50 | head -80Repository: jumpstarter-dev/monorepo
Length of output: 2629
🏁 Script executed:
#!/bin/bash
# Find the CaptureResult class and its decode method
sed -n '100,250p' "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/common.py"Repository: jumpstarter-dev/monorepo
Length of output: 4248
🏁 Script executed:
#!/bin/bash
# Check the VCD parser too
cat "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py" | head -100Repository: jumpstarter-dev/monorepo
Length of output: 3403
🏁 Script executed:
#!/bin/bash
# Read the CSV parser
cat "python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/csv.py"Repository: jumpstarter-dev/monorepo
Length of output: 4167
Use pytest.approx() for float equality to avoid potential precision issues.
Float comparisons should use approximate equality in tests. Even though the calculation (idx × 0.00001) may work in this specific case, using pytest.approx() is a safer practice that guards against future precision issues from different parsing paths or rounding behavior.
🔧 Suggested fix
- assert sample.time == sample.sample * 0.00001
+ assert sample.time == pytest.approx(sample.sample * 0.00001, rel=1e-6, abs=1e-12)🤖 Prompt for AI Agents
In
`@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver_test.py`
around lines 304 - 305, Replace the strict float equality assertion on
sample.time with an approximate comparison using pytest.approx to avoid
precision errors: change the assertion that compares sample.time to
sample.sample * 0.00001 (the current line referencing sample.time and
sample.sample) to use pytest.approx(...). Ensure pytest is imported in
jumpstarter_driver_sigrok/driver_test.py if not already present and use
pytest.approx(sample.sample * 0.00001) (or an explicit tolerance via rel/abs) in
the assert.
Comment on lines
+107
to
+112
| self.logger.debug("streaming sigrok-cli: %s", " ".join(cmd)) | ||
| process = await asyncio.create_subprocess_exec( | ||
| *cmd, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py | head -150 | tail -100Repository: jumpstarter-dev/monorepo
Length of output: 4393
🏁 Script executed:
rg "create_subprocess_exec" -A 5 -B 5Repository: jumpstarter-dev/monorepo
Length of output: 29324
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py | sed -n '1,150p'Repository: jumpstarter-dev/monorepo
Length of output: 7281
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py | sed -n '150,250p'Repository: jumpstarter-dev/monorepo
Length of output: 3601
🌐 Web query:
asyncio.create_subprocess_exec stderr PIPE not read blocking deadlock
💡 Result:
Cause
- OS pipe buffers are limited. If a child writes enough to stderr (or stdout) and your code isn't reading it, the child blocks when the buffer is full and you can deadlock waiting for process exit. This is documented in Python's asyncio subprocess docs. [1][2]
Fixes (pick one)
-
Use proc.communicate(): it concurrently reads stdout+stderr and waits for exit. Preferred when you want all output. Example:
proc = await asyncio.create_subprocess_exec(..., stdout=PIPE, stderr=PIPE)
stdout, stderr = await proc.communicate()stdout/stderr are bytes. [1]
-
Drain streams concurrently: create tasks to read stdout and stderr at the same time so neither pipe fills. Example pattern:
proc = await asyncio.create_subprocess_exec(..., stdout=PIPE, stderr=PIPE)
t1 = asyncio.create_task(proc.stdout.read()) # or readexactly/readuntil/iter_lines
t2 = asyncio.create_task(proc.stderr.read())
await asyncio.wait([t1, t2])
await proc.wait()
out = t1.result(); err = t2.result()
This is useful if you need streaming handling (processing lines as they arrive). [1][3] -
Redirect stderr to stdout if you only need one stream:
proc = await asyncio.create_subprocess_exec(..., stdout=PIPE, stderr=STDOUT)
Then read proc.stdout only. [1] -
Avoid pipes if you don't need the data (set stdout/stderr=None) or write output to files to remove piping limits.
Notes
- Don't rely on sequential reads (await proc.wait(); await proc.stderr.read()) — that can deadlock if the child waits for its stderr to be drained. Use communicate() or concurrent readers. [1][2]
Sources
- Python asyncio subprocess docs (warning about deadlocks and communicate()). [1]
- Practical explanation / troubleshooting for asyncio.subprocess.PIPE. [2]
Drain or inherit stderr to prevent streaming stalls.
The code creates stderr=PIPE but never reads from it. If sigrok-cli writes to stderr, the pipe buffer will fill and block the process, stalling the stream. Python's asyncio subprocess documentation explicitly warns against relying on sequential reads after process.wait() as this can deadlock. Either inherit stderr by setting stderr=None (to inherit the parent's stderr) or drain it concurrently with stdout.
🐛 Minimal fix to avoid backpressure
@@
process = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
- stderr=asyncio.subprocess.PIPE,
+ stderr=None,
)🤖 Prompt for AI Agents
In
`@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py`
around lines 107 - 112, The subprocess is created with
stderr=asyncio.subprocess.PIPE but never read, which can deadlock; in the code
around the asyncio.create_subprocess_exec call (search for the
self.logger.debug("streaming sigrok-cli: %s", " ".join(cmd)) and the process =
await asyncio.create_subprocess_exec(...) line in
jumpstarter_driver_sigrok/driver.py), either change stderr to None to inherit
the parent stderr or add a concurrent reader that drains process.stderr (e.g.,
spawn an asyncio.create_task that reads and discards/logs stderr lines) so the
stderr pipe cannot fill and stall the sigrok-cli stream.
Comment on lines
+161
to
+182
| def _channel_args(self, selected_names: list[str] | None) -> list[str]: | ||
| """Build channel selection/renaming args for sigrok-cli. | ||
|
|
||
| Args: | ||
| selected_names: Optional list of semantic names to include | ||
|
|
||
| Returns: | ||
| List of args like ["-C", "D0=vcc,D1=cs,D2=miso"] | ||
| """ | ||
| if not self.channels: | ||
| return [] | ||
|
|
||
| # Filter channels if specific names requested | ||
| if selected_names: | ||
| selected_lower = {name.lower() for name in selected_names} | ||
| filtered = {dev: user for dev, user in self.channels.items() if user.lower() in selected_lower} | ||
| else: | ||
| filtered = self.channels | ||
|
|
||
| # Build channel map: device_name=user_name | ||
| channel_map = ",".join(f"{dev}={user}" for dev, user in filtered.items()) | ||
| return ["-C", channel_map] if channel_map else [] |
Contributor
There was a problem hiding this comment.
Channel selection can be silently ignored.
When self.channels is empty or when selections are device names/unmapped channels (e.g., D0, A0), _channel_args drops the selection and returns no -C arg, so captures include all channels. _resolve_channel also rejects unmapped device names, which breaks triggers/decoder pin mapping.
🐛 Suggested fix to honor selections and device names
@@
- if not self.channels:
- return []
-
- # Filter channels if specific names requested
- if selected_names:
- selected_lower = {name.lower() for name in selected_names}
- filtered = {dev: user for dev, user in self.channels.items() if user.lower() in selected_lower}
- else:
- filtered = self.channels
-
- # Build channel map: device_name=user_name
- channel_map = ",".join(f"{dev}={user}" for dev, user in filtered.items())
- return ["-C", channel_map] if channel_map else []
+ if selected_names:
+ resolved = [self._resolve_channel(name) for name in selected_names]
+ if self.channels:
+ channel_map = ",".join(f"{dev}={self.channels.get(dev, dev)}" for dev in resolved)
+ else:
+ channel_map = ",".join(resolved)
+ return ["-C", channel_map] if channel_map else []
+
+ if not self.channels:
+ return []
+
+ # Build channel map: device_name=user_name
+ channel_map = ",".join(f"{dev}={user}" for dev, user in self.channels.items())
+ return ["-C", channel_map] if channel_map else []
@@
- # If already a device channel name, return as-is
- if candidate in self.channels:
- return candidate
+ if not self.channels:
+ return candidate
+ if candidate in self.channels:
+ return candidate
@@
- raise ValueError(f"Channel '{name_or_dn}' not found in channel map {self.channels}")
+ if candidate[:1].isalpha() and candidate[1:].isdigit():
+ return candidate
+ raise ValueError(f"Channel '{name_or_dn}' not found in channel map {self.channels}")Also applies to: 266-286
🤖 Prompt for AI Agents
In
`@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/driver.py`
around lines 161 - 182, _channel_args currently returns no -C when self.channels
is empty or when selected_names contain device keys (e.g., "D0") that aren't
values in self.channels, causing selections to be ignored; modify _channel_args
and related logic so selected_names is matched against both user names and
device keys: when self.channels is empty, build channel_map from selected_names
by treating each name as a device (mapping name=name), and when filtering a
non-empty self.channels include entries where either the user name (value) or
the device name (key) is in selected_names; also update _resolve_channel to
accept device names by returning the device key when present (falling back to
existing name->device lookup), ensuring trigger/decoder pin mapping honors
selections and unmapped device names.
Comment on lines
+22
to
+61
| text = data.decode("utf-8") | ||
| lines = text.strip().split("\n") | ||
|
|
||
| # Parse VCD header to extract timescale and channel mapping | ||
| timescale_multiplier = 1e-9 # Default: 1 unit = 1 ns = 1e-9 seconds | ||
| channel_map: dict[str, str] = {} # symbol → channel name | ||
|
|
||
| for line in lines: | ||
| line = line.strip() | ||
|
|
||
| # Parse timescale (e.g., "$timescale 1 us $end" means 1 unit = 1000 ns) | ||
| if line.startswith("$timescale"): | ||
| timescale_multiplier = _parse_timescale(line) | ||
|
|
||
| # Parse variable definitions (e.g., "$var wire 1 ! D0 $end") | ||
| if line.startswith("$var"): | ||
| parts = line.split() | ||
| if len(parts) >= 5: | ||
| symbol = parts[3] # e.g., "!" | ||
| channel = parts[4] # e.g., "D0" | ||
| channel_map[symbol] = channel | ||
|
|
||
| if line == "$enddefinitions $end": | ||
| break | ||
|
|
||
| # Parse and yield value changes one by one | ||
| sample_idx = 0 | ||
|
|
||
| for line in lines: | ||
| line = line.strip() | ||
| if not line or line.startswith("$"): | ||
| continue | ||
|
|
||
| # Timestamp line (e.g., "#100 1! 0" 1#") | ||
| if line.startswith("#"): | ||
| sample_data = _parse_vcd_timestamp_line(line, timescale_multiplier, channel_map) | ||
| if sample_data is not None: | ||
| sample_data["sample"] = sample_idx | ||
| yield sample_data | ||
| sample_idx += 1 |
Contributor
There was a problem hiding this comment.
Handle multi-line value changes and $dumpvars blocks.
VCD commonly emits timestamps on their own line and value changes on subsequent lines (including $dumpvars). The current loop only parses changes when they’re on the same line as #, which will drop most samples for standard VCD output.
🛠️ Suggested fix (parse value lines between timestamps)
- # Parse and yield value changes one by one
- sample_idx = 0
-
- for line in lines:
- line = line.strip()
- if not line or line.startswith("$"):
- continue
-
- # Timestamp line (e.g., "#100 1! 0" 1#")
- if line.startswith("#"):
- sample_data = _parse_vcd_timestamp_line(line, timescale_multiplier, channel_map)
- if sample_data is not None:
- sample_data["sample"] = sample_idx
- yield sample_data
- sample_idx += 1
+ # Parse value changes across timestamp blocks (supports multi-line changes)
+ sample_idx = 0
+ current_time_s: float | None = None
+ current_values: dict[str, int | float] = {}
+
+ for line in lines:
+ line = line.strip()
+ if not line:
+ continue
+ if line.startswith("$"):
+ continue
+
+ if line.startswith("#"):
+ # Flush previous timestamp block
+ if current_time_s is not None and current_values:
+ yield {"sample": sample_idx, "time": current_time_s, "values": current_values}
+ sample_idx += 1
+ current_values = {}
+
+ parts = line.split(maxsplit=1)
+ time_str = parts[0][1:]
+ if time_str:
+ current_time_s = int(time_str) * timescale_multiplier
+ else:
+ current_time_s = 0.0
+
+ # Inline values on the same line (if present)
+ if len(parts) > 1:
+ _parse_vcd_value_changes(parts[1], channel_map, current_values)
+ continue
+
+ # Value change line (may appear after # or inside $dumpvars)
+ if current_time_s is None:
+ current_time_s = 0.0
+ _parse_vcd_value_changes(line, channel_map, current_values)
+
+ # Flush final block
+ if current_time_s is not None and current_values:
+ yield {"sample": sample_idx, "time": current_time_s, "values": current_values}🤖 Prompt for AI Agents
In `@python/packages/jumpstarter-driver-sigrok/jumpstarter_driver_sigrok/vcd.py`
around lines 22 - 61, The parser currently only handles value changes when they
appear on the same line as the timestamp; change the main parsing loop (the
second loop that uses sample_idx, timescale_multiplier and channel_map) to treat
a line starting with "#" as a timestamp marker: record the current timestamp,
then iterate subsequent lines until the next "#" (or EOF) and for each
non-empty, non-directive line parse it as a value-change and yield a sample with
sample_idx incremented; additionally handle $dumpvars by detecting a "$dumpvars"
directive and consuming lines until the matching "$end", parsing each inner
value-change the same way. Use/extend the existing helpers (_parse_timescale,
_parse_vcd_timestamp_line or add a new _parse_vcd_value_change) and keep
channel_map/timescale_multiplier usage, assigning sample number via sample_idx
for each yielded change.
Comment on lines
+19
to
+21
| [tool.hatch.metadata.hooks.vcs.urls] | ||
| Homepage = "https://jumpstarter.dev" | ||
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" |
Contributor
There was a problem hiding this comment.
Verify repository name in source archive URL.
The source_archive URL references jumpstarter-dev/repo, but based on the PR URL, the repository appears to be named jumpstarter-dev/monorepo. This mismatch would result in broken archive links.
Suggested fix
[tool.hatch.metadata.hooks.vcs.urls]
Homepage = "https://jumpstarter.dev"
-source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip"
+source_archive = "https://github.com/jumpstarter-dev/monorepo/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.
Suggested change
| [tool.hatch.metadata.hooks.vcs.urls] | |
| Homepage = "https://jumpstarter.dev" | |
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | |
| [tool.hatch.metadata.hooks.vcs.urls] | |
| Homepage = "https://jumpstarter.dev" | |
| source_archive = "https://github.com/jumpstarter-dev/monorepo/archive/{commit_hash}.zip" |
🤖 Prompt for AI Agents
In `@python/packages/jumpstarter-driver-sigrok/pyproject.toml` around lines 19 -
21, The source_archive URL under [tool.hatch.metadata.hooks.vcs.urls] is
pointing to "jumpstarter-dev/repo" which is incorrect; update the source_archive
value to use the correct repository name "jumpstarter-dev/monorepo" while
preserving the {commit_hash} placeholder (i.e., change source_archive =
"https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" to reference
"monorepo") so archive links resolve correctly.
Comment on lines
+17
to
+28
| ```yaml | ||
| export: | ||
| sigrok: | ||
| type: jumpstarter_driver_sigrok.driver.Sigrok | ||
| driver: fx2lafw # sigrok driver (demo, fx2lafw, rigol-ds, etc.) | ||
| conn: auto # optional: USB VID.PID, serial path, or "auto" for auto-detect | ||
| channels: # optional: map device channels to friendly names | ||
| D0: clk | ||
| D1: mosi | ||
| D2: miso | ||
| D3: cs | ||
| ``` |
Contributor
There was a problem hiding this comment.
Exporter YAML example likely missing config: wrapper.
The example here differs from examples/exporter.yaml in this PR, which nests driver settings under config:. If the schema expects config, this doc will mislead users.
📝 Proposed fix
export:
sigrok:
type: jumpstarter_driver_sigrok.driver.Sigrok
- driver: fx2lafw # sigrok driver (demo, fx2lafw, rigol-ds, etc.)
- conn: auto # optional: USB VID.PID, serial path, or "auto" for auto-detect
- channels: # optional: map device channels to friendly names
- D0: clk
- D1: mosi
- D2: miso
- D3: cs
+ config:
+ driver: fx2lafw # sigrok driver (demo, fx2lafw, rigol-ds, etc.)
+ conn: auto # optional: USB VID.PID, serial path, or "auto" for auto-detect
+ channels: # optional: map device channels to friendly names
+ D0: clk
+ D1: mosi
+ D2: miso
+ D3: cs📝 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.
Suggested change
| ```yaml | |
| export: | |
| sigrok: | |
| type: jumpstarter_driver_sigrok.driver.Sigrok | |
| driver: fx2lafw # sigrok driver (demo, fx2lafw, rigol-ds, etc.) | |
| conn: auto # optional: USB VID.PID, serial path, or "auto" for auto-detect | |
| channels: # optional: map device channels to friendly names | |
| D0: clk | |
| D1: mosi | |
| D2: miso | |
| D3: cs | |
| ``` |
🤖 Prompt for AI Agents
In `@python/packages/jumpstarter-driver-sigrok/README.md` around lines 17 - 28,
The README's export YAML for Sigrok is missing the required config: wrapper;
update the example so the sigrok export block nests the driver settings under
config (i.e., export -> sigrok -> type/driver/conn/channels remain but are
placed inside a config: mapping) so it matches the schema used by
examples/exporter.yaml and the Sigrok exporter class
(jumpstarter_driver_sigrok.driver.Sigrok).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.