Add Renode emulator driver for embedded target simulation#533
Add Renode emulator driver for embedded target simulation#533vtz wants to merge 5 commits intojumpstarter-dev:mainfrom
Conversation
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
📝 WalkthroughWalkthroughAdds a new Renode driver package to Jumpstarter: async Renode telnet monitor client, RenodePower and RenodeFlasher drivers, composite Renode driver and RPC client/CLI, tests, package metadata, examples, README, and an ADR documenting design decisions and risks. Changes
Sequence DiagramssequenceDiagram
participant CLI
participant RenodeDriver as Renode Driver
participant Power as RenodePower
participant Monitor as RenodeMonitor
participant RenodeProc as Renode Process
participant PTY as PTY Terminal
CLI->>RenodeDriver: on()
RenodeDriver->>Power: on()
Power->>Power: pick free port
Power->>RenodeProc: spawn renode --monitor-port=<port>
Power->>Monitor: connect(host=localhost, port)
Monitor->>RenodeProc: TCP connection
Monitor->>Monitor: read until prompt
Monitor-->>Power: connected
Power->>Monitor: execute("machine LoadPlatform ...")
Monitor->>RenodeProc: send command + newline
Monitor->>Monitor: read until prompt
Monitor-->>Power: response
Power->>PTY: create PTY for UART
Power->>Monitor: execute("connector ...")
Monitor-->>Power: connector ok
alt firmware configured
Power->>Monitor: execute("sysbus LoadELF <path>")
Monitor-->>Power: load ok
end
Power->>Monitor: execute("start")
Monitor-->>Power: running
Power-->>RenodeDriver: on() complete
RenodeDriver-->>CLI: Ready
sequenceDiagram
participant CLI
participant RenodeDriver as Renode Driver
participant Flasher as RenodeFlasher
participant Monitor as RenodeMonitor
participant RenodeProc as Renode Process
CLI->>RenodeDriver: flash(firmware_resource)
RenodeDriver->>Flasher: flash(resource)
Flasher->>Flasher: write resource -> temp file
Flasher->>Flasher: record firmware path & load_command
alt simulation running
Flasher->>Monitor: execute("sysbus LoadELF <path>")
Monitor->>RenodeProc: perform load
Monitor-->>Flasher: success
Flasher->>Monitor: execute("machine Reset")
Monitor-->>Flasher: reset ok
end
Flasher-->>RenodeDriver: flash() complete
RenodeDriver-->>CLI: Firmware stored/loaded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.py (1)
22-31: Accept the remainder of the CLI line as the monitor command.Most Renode monitor commands contain spaces, so this only works naturally when the whole command is shell-quoted. Using a variadic Click argument makes
... monitor sysbus LoadELF@foo.elf`` behave as expected.Proposed fix
def cli(self): base = super().cli() `@base.command`(name="monitor") - `@click.argument`("command") - def monitor_command(command): + `@click.argument`("command", nargs=-1) + def monitor_command(command: tuple[str, ...]): """Send a command to the Renode monitor.""" - result = self.monitor_cmd(command) + result = self.monitor_cmd(" ".join(command)) if result.strip(): click.echo(result.strip())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.py` around lines 22 - 31, The CLI monitor handler currently defines a single-word argument and should accept the remainder of the CLI line; update the Click argument in cli() for the nested monitor_command (the `@base.command` handler) to use a variadic argument (click.argument("command", nargs=-1)) and then join the tuple into a single string before calling monitor_cmd (e.g., command_str = " ".join(command) and pass command_str to self.monitor_cmd) so multi-word Renode monitor commands work without shell-quoting.python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py (1)
298-310: Consider renaming test for clarity.The test name
test_power_close_calls_offis misleading sinceclose()doesn't actually calloff()— it directly terminates the process without sending thequitcommand or disconnecting the monitor. Consider renaming totest_power_close_terminates_processto accurately reflect the behavior being tested.Suggested rename
`@pytest.mark.anyio` - async def test_power_close_calls_off(self): - """close() terminates the process.""" + async def test_power_close_terminates_process(self): + """close() terminates the process directly without monitor cleanup.""" driver = _make_driver()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py` around lines 298 - 310, Rename the test function to reflect actual behavior: change the test named test_power_close_calls_off to test_power_close_terminates_process and update its reference in the file (the async test function that calls RenodePower.close and asserts mock_process.terminate was called and power._process is None) so the name accurately describes that close() terminates the process rather than calling off().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py`:
- Around line 99-127: The block that starts the Renode process with Popen and
then performs monitor setup (Popen -> self._process, RenodeMonitor,
self._monitor.connect, and subsequent self._monitor.execute calls) must be
wrapped in a try/except so any exception during monitor connection or commands
will teardown the spawned subprocess to avoid leaking it and leaving
self._process set. Modify the code so after creating self._process you use try:
... to run the RenodeMonitor connect and all execute calls (including load and
start), and in except Exception as e: ensure you cleanly stop the subprocess
(call self._process.terminate()/kill(), wait() and close pipes), set
self._process = None, then re-raise the exception; reference the symbols Popen,
self._process, RenodeMonitor, self._monitor.connect, self._monitor.execute and
ensure cleanup mirrors what off() would do.
- Around line 98-99: The Popen invocation in driver.py is piping
stdin/stdout/stderr (self._process = Popen(..., stdin=PIPE, stdout=PIPE,
stderr=PIPE)) which are never read and can cause the Renode process to block;
change those three arguments to use subprocess.DEVNULL instead so the child
won't deadlock (import DEVNULL if needed) — update the Popen call where
self._process is created and ensure any related code expecting piped streams is
removed or adjusted.
- Around line 144-148: The shutdown path is blocking because it calls the
synchronous Popen.wait(timeout=5) (used alongside self._process.terminate() and
except TimeoutExpired -> self._process.kill()) inside an async context (off());
change this to a non-blocking approach by running the blocking wait/killing
logic off the event loop (e.g., wrap the wait/kill sequence in a thread via
asyncio.to_thread or AnyIO's thread-runner) or replace it with an async poll
loop using self._process.poll() plus async sleep and then call kill if still
running; update the code around self._process.terminate(), self._process.wait(),
TimeoutExpired, and self._process.kill() accordingly so off() never blocks the
event loop.
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py`:
- Around line 43-52: The execute method currently returns raw Renode responses;
update execute (in jumpstarter_driver_renode.monitor.Monitor.execute) to inspect
the text returned by _read_until_prompt and, if it indicates a command failure
(e.g., contains "ERROR", "Error", "Failed", or matches Renode's error prefix),
raise RenodeMonitorError with the full response instead of returning it; keep
logging the request/response but throw RenodeMonitorError to stop initialization
(adjust tests like test_monitor_execute_error_response and callers such as
RenodePower.on which rely on execute's behavior to handle failures).
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.py`:
- Around line 22-31: The CLI monitor handler currently defines a single-word
argument and should accept the remainder of the CLI line; update the Click
argument in cli() for the nested monitor_command (the `@base.command` handler) to
use a variadic argument (click.argument("command", nargs=-1)) and then join the
tuple into a single string before calling monitor_cmd (e.g., command_str = "
".join(command) and pass command_str to self.monitor_cmd) so multi-word Renode
monitor commands work without shell-quoting.
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py`:
- Around line 298-310: Rename the test function to reflect actual behavior:
change the test named test_power_close_calls_off to
test_power_close_terminates_process and update its reference in the file (the
async test function that calls RenodePower.close and asserts
mock_process.terminate was called and power._process is None) so the name
accurately describes that close() terminates the process rather than calling
off().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d349f643-86a8-49ef-8781-30c12c5cb166
📒 Files selected for processing (12)
python/docs/source/contributing/adr/0001-renode-integration.mdpython/docs/source/contributing/adr/index.mdpython/docs/source/reference/package-apis/drivers/renode.mdpython/packages/jumpstarter-driver-renode/.gitignorepython/packages/jumpstarter-driver-renode/README.mdpython/packages/jumpstarter-driver-renode/examples/exporter.yamlpython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/__init__.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.pypython/packages/jumpstarter-driver-renode/pyproject.toml
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py
Show resolved
Hide resolved
- Add structured Design Decisions (DD-N) section to JEP template following the ADR pattern used in the project (e.g., ADR-0001 from PR #533) - Add Consequences section (positive/negative/risks) to JEP template - Mark all template sections as mandatory, optional, or conditional - Document the relationship between JEPs and ADRs in JEP-0000 - Add SpecKit and ADR references to Prior Art in JEP-0000 - Add agent instructions and document conventions to jeps/README.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce jumpstarter-driver-renode, a composite driver that enables Renode-based virtual hardware targets in Jumpstarter. Users can define any Renode-supported platform (STM32, S32K, Nucleo H753ZI, etc.) via exporter YAML configuration without modifying driver code. Key components: - RenodeMonitor: async telnet client for the Renode monitor interface - RenodePower: manages Renode process lifecycle and simulation control - RenodeFlasher: firmware loading via sysbus LoadELF / LoadBinary - RenodeClient: composite client with CLI extension for monitor commands Includes ADR-0001 documenting architectural decisions (control interface, UART exposure, configuration model, firmware loading strategy). Made-with: Cursor
- Replace PIPE with DEVNULL in Popen to prevent deadlocks (pipes were never read) - Wrap monitor setup in try-except to teardown subprocess on failure, preventing process leaks - Use anyio.to_thread.run_sync for blocking wait() in off() to avoid blocking the event loop - Raise RenodeMonitorError on error responses instead of silently returning error text - Accept multi-word monitor commands in CLI via nargs=-1 - Rename test_power_close_calls_off to test_power_close_terminates_process - Add docstrings across all public APIs Made-with: Cursor
Restructure the Renode integration ADR to follow the JEP template format: metadata table, Abstract, Motivation, Rejected Alternatives, Prior Art, Implementation History, and References sections. The DD-N design decisions and Consequences sections were already aligned. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py (2)
40-72: Simplify the running-state check inflash().Line 62 uses
hasattr(self.parent.children["power"], "_process"), but_processis always defined as a dataclass field withdefault=None. This check always returnsTrue, making it misleading. The effective guard isif monitor is not Noneon line 64.Consider replacing with a more explicit check:
Proposed fix
- if hasattr(self.parent.children["power"], "_process"): - monitor = self.parent.children["power"]._monitor - if monitor is not None: + power = self.parent.children["power"] + if power._process is not None and power._monitor is not None: + monitor = power._monitor + if True: # already checked aboveOr more simply:
- if hasattr(self.parent.children["power"], "_process"): - monitor = self.parent.children["power"]._monitor - if monitor is not None: + monitor = self.parent.children["power"]._monitor + if monitor is not None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py` around lines 40 - 72, The hasattr(self.parent.children["power"], "_process") check in RenodeFlasher.flash is misleading because _process is always present; remove that hasattr guard and instead explicitly ensure the power child and its monitor exist before hot-loading: fetch the power child (e.g. power = self.parent.children.get("power")), check power is not None and power._monitor is not None, then assign monitor = power._monitor and run monitor.execute(...) as currently written; update the RenodeFlasher.flash method to use these explicit checks (referencing RenodeFlasher.flash, self.parent.children["power"], _monitor and _process to locate the code).
166-177: Monitor socket not explicitly closed in synchronousclose().The
close()method setsself._monitor = Nonewithout closing the underlying TCP socket. While the socket will likely be broken when Renode terminates, it's not explicitly cleaned up.Since
close()must be synchronous (per the learning, it's called during driver teardown outside an async context) anddisconnect()is async, a potential workaround is to directly close the socket if available:Possible enhancement
def close(self): """Synchronous cleanup for use during driver teardown.""" if self._process is not None: if self._monitor is not None: + # Best-effort sync close of the socket + if self._monitor._stream is not None: + try: + self._monitor._stream._transport.close() + except Exception: + pass self._monitor = NoneThis is a minor improvement — the socket will be cleaned up when Renode terminates anyway, but explicit cleanup is cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py` around lines 166 - 177, The close() method currently nulled out self._monitor without closing the underlying socket; modify close() to synchronously close the monitor if present (e.g., call self._monitor.close() or the monitor's appropriate close/shutdown method) inside a try/except to swallow/LOG any exceptions, then set self._monitor = None, and proceed to terminate the process as before; reference the close() method and the self._monitor attribute (and keep disconnect() untouched since it is async).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py`:
- Around line 318-398: The async iterator mocks in TestRenodeFlasher tests
(test_flash_stores_firmware_path, test_flash_while_running_sends_load_and_reset,
test_flash_custom_load_command) use AsyncMock(return_value=iter(...)) which
doesn't implement async iteration; update each patched flasher.resource mock
(the mock_res used in those with patch.object(flasher, "resource")) to implement
async iteration by assigning mock_res.__aiter__ = lambda self: self and
mock_res.__anext__ = AsyncMock(side_effect=[<chunks...>, StopAsyncIteration()])
so the async for chunk in res: loops in RenodeFlasher.flash() iterate correctly
and yield the desired byte chunks.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py`:
- Around line 40-72: The hasattr(self.parent.children["power"], "_process")
check in RenodeFlasher.flash is misleading because _process is always present;
remove that hasattr guard and instead explicitly ensure the power child and its
monitor exist before hot-loading: fetch the power child (e.g. power =
self.parent.children.get("power")), check power is not None and power._monitor
is not None, then assign monitor = power._monitor and run monitor.execute(...)
as currently written; update the RenodeFlasher.flash method to use these
explicit checks (referencing RenodeFlasher.flash, self.parent.children["power"],
_monitor and _process to locate the code).
- Around line 166-177: The close() method currently nulled out self._monitor
without closing the underlying socket; modify close() to synchronously close the
monitor if present (e.g., call self._monitor.close() or the monitor's
appropriate close/shutdown method) inside a try/except to swallow/LOG any
exceptions, then set self._monitor = None, and proceed to terminate the process
as before; reference the close() method and the self._monitor attribute (and
keep disconnect() untouched since it is async).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 381d75b0-27c9-41e5-9df3-3869a5a95ad9
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
python/docs/source/contributing/adr/0001-renode-integration.mdpython/docs/source/contributing/adr/index.mdpython/docs/source/reference/package-apis/drivers/renode.mdpython/packages/jumpstarter-driver-renode/.gitignorepython/packages/jumpstarter-driver-renode/README.mdpython/packages/jumpstarter-driver-renode/examples/exporter.yamlpython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/__init__.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.pypython/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.pypython/packages/jumpstarter-driver-renode/pyproject.toml
✅ Files skipped from review due to trivial changes (7)
- python/docs/source/reference/package-apis/drivers/renode.md
- python/packages/jumpstarter-driver-renode/.gitignore
- python/docs/source/contributing/adr/index.md
- python/packages/jumpstarter-driver-renode/examples/exporter.yaml
- python/packages/jumpstarter-driver-renode/pyproject.toml
- python/packages/jumpstarter-driver-renode/README.md
- python/docs/source/contributing/adr/0001-renode-integration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.py
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py
Show resolved
Hide resolved
|
@kirkbrauer ADR-0001 has been restructured to align with the JEP template format from #423 (metadata table, Abstract, Motivation, Rejected Alternatives, Prior Art, Implementation History, References, and license footer). The DD-N design decisions and Consequences sections were already in the right shape. |
Replace incorrect AsyncMock(return_value=iter([...])) with idiomatic __aiter__/__anext__ pattern. The old pattern silently yielded nothing (with RuntimeWarning), the new one properly iterates the chunks. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py (1)
241-248: Strengthen idempotency tests with observable assertions.Both tests currently pass on “no exception” only. Add explicit assertions that no startup/shutdown side effects were triggered (or assert expected warning logs), so regressions are caught deterministically.
🧪 Suggested direction
`@pytest.mark.anyio` async def test_power_on_idempotent(self): """Second on() call logs warning and does nothing.""" driver = _make_driver() power: RenodePower = driver.children["power"] power._process = MagicMock() - await power.on() + with patch("jumpstarter_driver_renode.driver._find_renode") as mock_find_renode: + await power.on() + mock_find_renode.assert_not_called() `@pytest.mark.anyio` async def test_power_off_idempotent(self): """Second off() call logs warning and does nothing.""" driver = _make_driver() power: RenodePower = driver.children["power"] power._process = None - await power.off() + await power.off() # no-op path + assert power._process is NoneAs per coding guidelines:
python/packages/jumpstarter-driver-*/jumpstarter_driver_*/driver_test.py: “Each driver package must include comprehensive tests indriver_test.py.”Also applies to: 290-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py` around lines 241 - 248, The idempotency tests (e.g., test_power_on_idempotent and the corresponding power.off test around lines 290-297) currently only await calls; enhance them to assert observable behavior: verify that RenodePower._process (accessed via power._process) was not called when calling power.on() or power.off() a second time, and assert that the expected warning log was emitted (mock the logger or use caplog) to ensure the function logged a warning rather than performing startup/shutdown side effects; update both tests to include these assertions so regressions are caught deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py`:
- Around line 445-449: The test test_renode_pty_path should use
pathlib.Path-based assertions to mirror the driver implementation: replace the
string suffix check and manual substring check with a single Path
equality/assertion that Path(driver._pty) == Path(driver._tmp_dir.name) / "pty"
(or assert Path(driver._pty).resolve() == (Path(driver._tmp_dir.name) /
"pty").resolve()) so the test uses the same Path construction as the driver
(referencing driver._pty and driver._tmp_dir).
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py`:
- Around line 241-248: The idempotency tests (e.g., test_power_on_idempotent and
the corresponding power.off test around lines 290-297) currently only await
calls; enhance them to assert observable behavior: verify that
RenodePower._process (accessed via power._process) was not called when calling
power.on() or power.off() a second time, and assert that the expected warning
log was emitted (mock the logger or use caplog) to ensure the function logged a
warning rather than performing startup/shutdown side effects; update both tests
to include these assertions so regressions are caught deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91eeec4c-c940-43e5-87c4-52dfc245331c
📒 Files selected for processing (1)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py
Outdated
Show resolved
Hide resolved
Mirror the driver's Path construction instead of string suffix matching. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py (2)
266-267: Use exact call-count assertion forquit.
assert_called_with("quit")won’t fail ifquitis sent multiple times.🔧 Tighten assertion
- mock_monitor.execute.assert_called_with("quit") + mock_monitor.execute.assert_called_once_with("quit")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py` around lines 266 - 267, Replace the loose assertion on the monitor's quit call with an exact one: change the check using mock_monitor.execute.assert_called_with("quit") to assert it was called exactly once (e.g., mock_monitor.execute.assert_called_once_with("quit")) so the test fails if "quit" was sent multiple times; keep the existing mock_monitor.disconnect.assert_called_once() as-is.
241-248: Strengthen idempotency tests with side-effect assertions.These tests currently only verify “no exception,” so regressions that accidentally re-run startup/shutdown paths would still pass.
🔧 Proposed test hardening
`@pytest.mark.anyio` async def test_power_on_idempotent(self): """Second on() call logs warning and does nothing.""" driver = _make_driver() power: RenodePower = driver.children["power"] power._process = MagicMock() - await power.on() + with patch("jumpstarter_driver_renode.driver.Popen") as mock_popen, patch( + "jumpstarter_driver_renode.driver.RenodeMonitor" + ) as mock_monitor_cls: + await power.on() + + mock_popen.assert_not_called() + mock_monitor_cls.assert_not_called() @@ `@pytest.mark.anyio` async def test_power_off_idempotent(self): """Second off() call logs warning and does nothing.""" driver = _make_driver() power: RenodePower = driver.children["power"] power._process = None + power._monitor = AsyncMock(spec=RenodeMonitor) await power.off() + power._monitor.execute.assert_not_called() + power._monitor.disconnect.assert_not_called()As per coding guidelines,
python/packages/jumpstarter-driver-*/jumpstarter_driver_*/driver_test.py: Each driver package must include comprehensive tests indriver_test.py.Also applies to: 290-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py` around lines 241 - 248, The idempotency tests (e.g., test_power_on_idempotent) only await power.on() but don't assert that startup side-effects weren't executed; update the test to assert that the mocked process and startup methods were not called and that a warning was logged instead: after creating driver via _make_driver(), use the existing MagicMock on power._process and assert its start/execute/send-like methods were not invoked and that the logger (or warning call) on RenodePower was called once; apply the same pattern to the corresponding power.off idempotent test (the similar block around lines 290-297) so both on() and off() tests verify no side-effects and a warning was emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py`:
- Around line 266-267: Replace the loose assertion on the monitor's quit call
with an exact one: change the check using
mock_monitor.execute.assert_called_with("quit") to assert it was called exactly
once (e.g., mock_monitor.execute.assert_called_once_with("quit")) so the test
fails if "quit" was sent multiple times; keep the existing
mock_monitor.disconnect.assert_called_once() as-is.
- Around line 241-248: The idempotency tests (e.g., test_power_on_idempotent)
only await power.on() but don't assert that startup side-effects weren't
executed; update the test to assert that the mocked process and startup methods
were not called and that a warning was logged instead: after creating driver via
_make_driver(), use the existing MagicMock on power._process and assert its
start/execute/send-like methods were not invoked and that the logger (or warning
call) on RenodePower was called once; apply the same pattern to the
corresponding power.off idempotent test (the similar block around lines 290-297)
so both on() and off() tests verify no side-effects and a warning was emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2ecf2f-4b37-4a32-befa-52ee533ea3e7
📒 Files selected for processing (1)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py
ADR-0001 → JEP Conversion: Renode IntegrationI took a pass at converting the Renode integration ADR ( Where to put itPer the JEP process defined here, the file should live at: (JEP-0010 is the first non-reserved number, since 0000-0009 are reserved for process/meta-JEPs.) If the original ADR is also kept, it should move to The converted JEPClick to expand full JEP-0010 (click to expand)JEP-0010: Renode Emulator Driver for MCU Target Simulation
Abstract (mandatory)This JEP proposes integrating the Renode emulation Motivation (mandatory)Jumpstarter provides a driver-based framework for interacting with There is growing demand for microcontroller-class virtual targets The high-level alternative of extending the QEMU driver for MCU targets User Stories (optional)
Constraints
Reference TargetsThe initial targets for validation are:
Proposal (mandatory)The Exporter ConfigurationA target is defined entirely in YAML. Here is an STM32F407 example: apiVersion: jumpstarter.dev/v1alpha1
kind: ExporterConfig
metadata:
name: renode-stm32f407
export:
ecu:
type: jumpstarter_driver_renode.driver.Renode
config:
platform: "platforms/boards/stm32f4_discovery-kit.repl"
uart: "sysbus.usart2"For targets that require register pokes or other setup before firmware config:
platform: "/path/to/s32k388_renode.repl"
uart: "sysbus.uart0"
extra_commands:
- "sysbus WriteDoubleWord 0x40090030 0x0301"Configuration Parameters
Python APIFrom test code or a leased client session: with serve(Renode(
platform="platforms/boards/stm32f4_discovery-kit.repl",
uart="sysbus.usart2",
)) as target:
target.flasher.flash("/path/to/firmware.elf")
target.power.on()
# target.console is a standard PySerial/pexpect interface
target.power.off()The
Power Lifecycle
CLIThe driver extends the composite CLI with a Multi-word commands are joined automatically. API / Protocol Changes (if applicable)This JEP introduces a new driver package. No existing gRPC Hardware Considerations (if applicable)Although Renode is a virtual target, several hardware-boundary
Design Decisions (mandatory for Standards Track)DD-1: Control Interface -- Telnet MonitorAlternatives considered:
Decision: Telnet monitor. Rationale: It is the lowest-common-denominator interface that works DD-2: UART Exposure -- PTY TerminalAlternatives considered:
Decision: PTY as the primary interface. Rationale: Consistency with the QEMU driver, which uses DD-3: Configuration Model -- Managed ModeAlternatives considered:
Decision: Managed mode as primary, with an Rationale: Managed mode gives the driver full control over the UART DD-4: Firmware Loading -- Deferred to FlashAlternatives considered:
Decision: Option 1 -- Rationale: This matches the QEMU driver's semantic where you flash Design Details (mandatory for Standards Track)ArchitectureThe driver follows Jumpstarter's composite driver pattern: The Monitor Client (
|
| New JEP Section | What it adds |
|---|---|
| User Stories | Three concrete user personas (firmware dev, test author, platform integrator) |
| Proposal | Teaching-oriented description of the driver: exporter YAML examples, config parameter table, Python API usage, power lifecycle walkthrough, CLI commands |
| API / Protocol Changes | Explicit statement that no existing interfaces are modified |
| Hardware Considerations | PTY requirement, no privileged access needed, timing/timeout behavior, crash cleanup |
| Design Details | Architecture diagram (composite tree), monitor client internals, process lifecycle, data flow diagram, error handling strategy, concurrency model |
| Test Plan | Itemized inventory of all 28 unit tests by category, E2E test description, skip/xfail conditions, manual verification steps |
| Backward Compatibility | Explicit "new package, no concerns" statement |
| Future Possibilities | Socket fallback, .resc script mode, multi-machine simulation, CI integration |
What the ADR expresses that the JEP template doesn't naturally accommodate
Two sections from the ADR have no direct JEP template equivalent and are preserved with <!-- NOTE --> comments:
-
Constraints — a scannable checklist of hard requirements (must use composite pattern, must use anyio, config-only targets, etc.). In the JEP template these get scattered across Motivation and Design Decisions, losing their cohesion as a verification checklist.
-
Reference Targets — specific MCU boards used as scope/acceptance criteria (STM32F407, S32K388, Nucleo H753ZI). These serve as an implicit scope boundary ("we're building for these") that doesn't fit neatly into Test Plan (which asks "how do you verify?") or Motivation (which asks "why?").
Observation on scope
Per JEP-0000's own criteria, new drivers that follow the existing scaffold and don't modify framework interfaces do not require a JEP. The Renode driver follows the composite driver pattern exactly and introduces no new interfaces. This makes it a good ADR candidate but arguably not a JEP candidate — which is actually a validation that JEP-0000 draws the line in the right place.
That said, having this conversion exercise is useful to see how the two formats relate and whether the JEP template's Constraints and Reference Targets gaps should be addressed.
|
@vtz can we convert the ADR into a JEP which we are trying to push at the community level?, there is no community decission to use ADRs, and that may get confusing to keep both in repo. |
| reason="Renode not installed", | ||
| ) | ||
| @pytest.mark.xfail( | ||
| platform.system() == "Darwin" and os.getenv("GITHUB_ACTIONS") == "true", |
There was a problem hiding this comment.
why?, I think this is just copying from other places.
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| shutil.which("renode") is None, |
There was a problem hiding this comment.
can we modify the .github workflows for python testing to install renode so this is rested in CI?
| async def flash(self, source, load_command: str | None = None): | ||
| """Flash firmware to the simulated MCU. | ||
|
|
||
| If the simulation is not yet running, stores the firmware for | ||
| loading during power-on. If already running, loads the firmware | ||
| and resets the machine. | ||
| """ | ||
| firmware_path = self.parent._tmp_dir.name + "/firmware" | ||
| async with await FileWriteStream.from_path(firmware_path) as stream: | ||
| async with self.resource(source) as res: | ||
| async for chunk in res: | ||
| await stream.send(chunk) | ||
|
|
||
| cmd = load_command or "sysbus LoadELF" | ||
| self.parent._firmware_path = firmware_path | ||
| self.parent._load_command = cmd | ||
|
|
||
| if hasattr(self.parent.children["power"], "_process"): | ||
| monitor = self.parent.children["power"]._monitor | ||
| if monitor is not None: | ||
| await monitor.execute(f'{cmd} @"{firmware_path}"') | ||
| await monitor.execute("machine Reset") |
There was a problem hiding this comment.
[HIGH] The load_command parameter in flash() is client-supplied and interpolated directly into a monitor command without validation. A remote client can inject arbitrary monitor commands by passing something like load_command='logFile @"/tmp/exfil"\nsysbus LoadELF'. The injected value is also persisted in self.parent._load_command and replayed during subsequent power.on() calls.
Suggested fix: validate load_command against an allowlist of known Renode load commands (e.g., sysbus LoadELF, sysbus LoadBinary, sysbus LoadSymbolsFrom) and raise ValueError if it doesn't match.
AI-generated, human reviewed
| async def monitor_cmd(self, command: str) -> str: | ||
| """Send an arbitrary command to the Renode monitor.""" | ||
| power: RenodePower = self.children["power"] | ||
| if power._monitor is None: | ||
| raise RuntimeError("Renode is not running") | ||
| return await power._monitor.execute(command) |
There was a problem hiding this comment.
[HIGH] monitor_cmd forwards arbitrary strings to the Renode telnet monitor without any filtering. The Renode monitor supports commands that interact with the host filesystem (logFile, include, CreateFileTerminal, LoadPlatformDescription), so any authenticated Jumpstarter client can use these to read or write files on the host. For comparison, the existing QEMU driver does not expose an unrestricted QMP passthrough.
Suggested fix: implement an allowlist of permitted monitor commands, or gate this export behind an explicit allow_raw_monitor: true configuration flag that defaults to false.
AI-generated, human reviewed
| async def connect(self, host: str, port: int, timeout: float = 10) -> None: | ||
| """Connect to the Renode monitor, retrying until the prompt appears.""" | ||
| with fail_after(timeout): | ||
| while True: | ||
| try: | ||
| self._stream = await connect_tcp(host, port) | ||
| self._buffer = b"" | ||
| await self._read_until_prompt() | ||
| logger.info("connected to Renode monitor at %s:%d", host, port) | ||
| return | ||
| except OSError: | ||
| await sleep(0.5) |
There was a problem hiding this comment.
[HIGH] TCP socket streams leak on retry iterations in connect(). When connect_tcp succeeds but _read_until_prompt() subsequently fails with an OSError, the except clause catches it and the loop retries. On the next iteration, self._stream = await connect_tcp(...) overwrites the reference to the previous stream without closing it. This is a realistic scenario during Renode startup when the telnet port accepts connections before the monitor is ready.
Suggested fix: close the existing stream before retrying:
except OSError:
if self._stream is not None:
await self._stream.aclose()
self._stream = None
await sleep(0.5)AI-generated, human reviewed
| async def execute(self, command: str) -> str: | ||
| """Send a command and return the response text (excluding the prompt). | ||
|
|
||
| Raises RenodeMonitorError if the response indicates a command failure. | ||
| """ | ||
| if self._stream is None: | ||
| raise RuntimeError("not connected to Renode monitor") | ||
|
|
||
| logger.debug("monitor> %s", command) | ||
| await self._stream.send(f"{command}\n".encode()) | ||
| response = await self._read_until_prompt() | ||
| logger.debug("monitor< %s", response.strip()) | ||
|
|
||
| stripped = response.strip() | ||
| if stripped and any(stripped.startswith(m) for m in self._ERROR_MARKERS): | ||
| raise RenodeMonitorError(stripped) | ||
|
|
||
| return response |
There was a problem hiding this comment.
[HIGH] execute() has no timeout and can block an async task indefinitely. It calls _read_until_prompt() which runs an unbounded while True loop. If Renode stops sending data but keeps the TCP connection open (process hang, simulation deadlock), self._stream.receive(4096) blocks forever. Unlike connect() which wraps in fail_after(timeout), execute() has no cancellation scope.
Suggested fix: wrap the read in a configurable timeout:
async def execute(self, command: str, timeout: float = 30) -> str:
...
with fail_after(timeout):
response = await self._read_until_prompt()
...AI-generated, human reviewed
| dependencies = [ | ||
| "jumpstarter", | ||
| "jumpstarter-driver-composite", | ||
| "jumpstarter-driver-network", | ||
| "jumpstarter-driver-opendal", | ||
| "jumpstarter-driver-power", | ||
| "jumpstarter-driver-pyserial", | ||
| ] |
There was a problem hiding this comment.
[HIGH] Missing workspace source entry in the top-level python/pyproject.toml. Every driver package in the workspace has an explicit entry in [tool.uv.sources] (e.g., jumpstarter-driver-qemu = { workspace = true }), but the new jumpstarter-driver-renode package has no such entry. Without it, other workspace packages listing jumpstarter-driver-renode as a dependency won't resolve to the local workspace copy during development, causing build failures since the package doesn't exist on PyPI yet.
Suggested fix: add jumpstarter-driver-renode = { workspace = true } to [tool.uv.sources] in python/pyproject.toml, alphabetically after jumpstarter-driver-qemu.
AI-generated, human reviewed
| async def off(self) -> None: | ||
| """Stop simulation, disconnect monitor, and terminate the Renode process.""" | ||
| if self._process is None: | ||
| self.logger.warning("already powered off, ignoring request") | ||
| return | ||
|
|
||
| if self._monitor is not None: | ||
| try: | ||
| await self._monitor.execute("quit") | ||
| except Exception: | ||
| pass | ||
| await self._monitor.disconnect() | ||
| self._monitor = None | ||
|
|
||
| self._process.terminate() | ||
| try: | ||
| await to_thread.run_sync(self._process.wait, 5) | ||
| except TimeoutExpired: | ||
| self._process.kill() | ||
| self._process = None |
There was a problem hiding this comment.
[LOW] off() leaves inconsistent state if terminate() raises on an already-exited process. ProcessLookupError propagates without executing self._process = None, leaving the driver in a state where subsequent off() calls fail and on() short-circuits.
Suggested fix: wrap the terminate/wait/kill sequence in try/finally to always set self._process = None.
AI-generated, human reviewed
| def _find_free_port() -> int: | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| s.bind(("127.0.0.1", 0)) | ||
| return s.getsockname()[1] |
There was a problem hiding this comment.
[LOW] TOCTOU race in _find_free_port(): the ephemeral port is discovered by binding and immediately releasing a socket. A local process could race to bind the same port before Renode does. The QEMU driver avoids this by using Unix domain sockets.
Suggested fix: switch to a Unix domain socket for the monitor connection if Renode supports it, or use --port 0 if Renode supports reporting its actual bound port.
AI-generated, human reviewed
| async def on(self) -> None: | ||
| """Start Renode, connect monitor, configure platform, and begin simulation.""" | ||
| if self._process is not None: | ||
| self.logger.warning("already powered on, ignoring request") | ||
| return | ||
|
|
||
| renode_bin = _find_renode() | ||
| port = self.parent.monitor_port or _find_free_port() | ||
| self.parent._active_monitor_port = port | ||
|
|
||
| cmdline = [ | ||
| renode_bin, | ||
| "--disable-xwt", | ||
| "--plain", | ||
| "--port", | ||
| str(port), | ||
| ] | ||
|
|
||
| self.logger.info("starting Renode: %s", " ".join(cmdline)) | ||
| self._process = Popen(cmdline, stdin=DEVNULL, stdout=DEVNULL, stderr=DEVNULL) | ||
|
|
||
| self._monitor = RenodeMonitor() | ||
| try: | ||
| await self._monitor.connect("127.0.0.1", port) | ||
|
|
||
| machine = self.parent.machine_name | ||
| await self._monitor.execute(f'mach create "{machine}"') | ||
| await self._monitor.execute( | ||
| f'machine LoadPlatformDescription @"{self.parent.platform}"' | ||
| ) | ||
|
|
||
| pty_path = self.parent._pty | ||
| await self._monitor.execute( | ||
| f'emulation CreateUartPtyTerminal "term" "{pty_path}"' | ||
| ) | ||
| await self._monitor.execute( | ||
| f"connector Connect {self.parent.uart} term" | ||
| ) | ||
|
|
||
| for cmd in self.parent.extra_commands: | ||
| await self._monitor.execute(cmd) | ||
|
|
||
| if self.parent._firmware_path: | ||
| load_cmd = self.parent._load_command or "sysbus LoadELF" | ||
| await self._monitor.execute( | ||
| f'{load_cmd} @"{self.parent._firmware_path}"' | ||
| ) | ||
|
|
||
| await self._monitor.execute("start") | ||
| self.logger.info("Renode simulation started") | ||
| except Exception: | ||
| await self.off() | ||
| raise |
There was a problem hiding this comment.
[LOW] RenodePower.on() handles multiple responsibilities in ~53 lines: idempotency check, binary location, port allocation, command-line construction, process spawn, monitor connection, machine creation, platform loading, UART setup, extra commands, firmware loading, simulation start, and error cleanup.
Suggested fix: extract the monitor setup sequence (lines 110-133) into a private method like _configure_simulation() to improve readability.
AI-generated, human reviewed
| # --------------------------------------------------------------------------- | ||
| # 5.1 RenodeMonitor unit tests | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
[LOW] Six decorative separator comments reference internal plan task numbers (5.1-5.6) and restate what the immediately-following class name already says (e.g., # 5.1 RenodeMonitor unit tests followed by class TestRenodeMonitor:). These add noise without information.
Suggested fix: remove all six separator blocks. The class and function names already convey the grouping.
AI-generated, human reviewed
| @@ -0,0 +1,562 @@ | |||
| from __future__ import annotations | |||
|
|
|||
There was a problem hiding this comment.
[LOW] Several minor paths lack test coverage: (1) _find_renode() error path when binary not on PATH; (2) RenodePower.read() raising NotImplementedError; (3) Renode.monitor_cmd() success path; (4) close() timeout-kill branch; (5) E2E test only exercises start/stop, not flash or console interaction.
Suggested fix: add targeted tests for each: (1) mock shutil.which returning None; (2) parallel to test_dump_not_implemented; (3) mock monitor and verify forwarding; (4) mock wait raising TimeoutExpired; (5) extend E2E with flash and monitor_cmd when Renode is available.
AI-generated, human reviewed
3 participants
Summary
jumpstarter-driver-renode, a composite driver that enables Renode-based virtual hardware targets in JumpstarterRenodeMonitor(async telnet client),RenodePower(process lifecycle),RenodeFlasher(firmware loading viasysbus LoadELF/LoadBinary), andRenodeClient(composite client with CLI extension)Design Decisions
Key architectural decisions are documented in
python/docs/source/contributing/adr/0001-renode-integration.md:anyio.connect_tcpextra_commandsfor customizationflash()thenon()semanticFiles Changed
python/packages/jumpstarter-driver-renode/— Full driver package (monitor, driver, client, tests, examples)python/docs/source/contributing/adr/— ADR index and ADR-0001 for Renode integrationpython/docs/source/reference/package-apis/drivers/renode.md— Docs symlink to driver READMETest Plan
RenodeMonitor(connect, execute, close, timeout, error handling)RenodePower(on/off, firmware loading, UART configuration)RenodeFlasher(flash, format detection, not-running error)Renodeconfig validation (missing platform, defaults, UART config)serve()(requires Renode installed, marked withskipif)python-tests.yaml(follow-up)Made with Cursor