Add Renode emulator driver for embedded target simulation#533
Add Renode emulator driver for embedded target simulation#533mangelajo merged 10 commits intojumpstarter-dev:mainfrom
Conversation
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Renode driver package and docs: async Renode telnet monitor client, RenodePower and RenodeFlasher drivers, composite Renode driver and RPC client/CLI, tests, package metadata, examples, README, ADR, and CI steps to install Renode. 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 <platform>")
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("<LoadELF command> <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("<LoadELF command> <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
- 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>
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
|
@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. |
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
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.
There was a problem hiding this comment.
You're right — this was copied from the QEMU test pattern. Removed in c311ab6.
There was a problem hiding this comment.
Could you clarify what you mean? This comment has no line reference so it's hard to tell which part of driver_test.py you're referring to. Are you asking about a specific test pattern or the overall test structure?
|
|
||
|
|
||
| @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?
There was a problem hiding this comment.
Done in c311ab6. Added Renode installation steps for both Linux (deb) and macOS (brew) to the python-tests workflow.
| 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
There was a problem hiding this comment.
Fixed in c311ab6. execute() now wraps the read in fail_after(timeout) with a default of 30 seconds.
| 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
There was a problem hiding this comment.
This is true, let's follow up on this, but not on this PR.
| 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
There was a problem hiding this comment.
it's passing our complexity checks anyway :D
| # --------------------------------------------------------------------------- | ||
| # 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py (1)
184-189:⚠️ Potential issue | 🟡 MinorWrap terminate/wait/kill in try-finally to handle already-exited process.
If the Renode process exits before
off()is called,terminate()raisesProcessLookupError, leavingself._processset. Subsequentoff()calls fail andon()short-circuits with "already powered on".🛡️ Proposed fix
- self._process.terminate() - try: - await to_thread.run_sync(self._process.wait, 5) - except TimeoutExpired: - self._process.kill() - self._process = None + try: + self._process.terminate() + try: + await to_thread.run_sync(self._process.wait, 5) + except TimeoutExpired: + self._process.kill() + except ProcessLookupError: + pass # Process already exited + finally: + self._process = 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 184 - 189, The off() shutdown sequence can raise ProcessLookupError if the Renode process already exited, leaving self._process set and breaking subsequent off()/on() logic; wrap the terminate()/wait()/kill() sequence in a try/finally so self._process is always cleared, and specifically catch ProcessLookupError around self._process.terminate() to treat an already-exited process as success; keep the existing TimeoutExpired handling for wait and call kill() only if terminate succeeded, then set self._process = None in the finally block to ensure state is cleared for future on()/off() calls.
🧹 Nitpick comments (1)
.github/workflows/python-tests.yaml (1)
87-91: Use a consistent Renode version policy across OS runners.Line 79 is pinned to a specific Linux build, but Lines 87-90 install the latest Homebrew formula. This can cause cross-platform drift and flaky matrix results.
Consider either pinning both OSes to the same tested Renode version or adding an explicit version gate after macOS install (fail if outside the validated range).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-tests.yaml around lines 87 - 91, The macOS step "Install Renode (macOS)" currently installs the latest Homebrew renode while the Linux runner is pinned to a specific Renode build, causing cross-OS drift; update the "Install Renode (macOS)" step to either install the same pinned Renode version as the Linux install (use the same version identifier used in the Linux Renode install step) or add a follow-up check step that queries the installed renode --version and fails the job if the version is outside the validated range so both runners run the identical tested Renode release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/python-tests.yaml:
- Around line 76-81: Replace the unverified download in the "Install Renode
(Linux)" step: either upgrade to Renode >=1.16.1 and use its published SHA256
checksum to verify the downloaded .deb before installing (capture the checksum
and run sha256sum -c) and make macOS use the same pinned release (avoid floating
`brew install renode`), or if you must remain on 1.15.3, add an explicit inline
comment documenting the accepted supply-chain risk and keep the install behavior
consistent across runners; target the wget URL and the "Install Renode (Linux)"
step and ensure /tmp/renode.deb is integrity-checked or the risk is clearly
documented.
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py`:
- Around line 196-206: The close() method can raise ProcessLookupError when
calling self._process.terminate() if the process already exited, preventing
self._process from being cleared; wrap the terminate()/wait()/kill() sequence in
a try/except/finally that catches ProcessLookupError (and reuses the existing
TimeoutExpired handling) and always sets self._process = None, and ensure
self._monitor is cleared before attempting to terminate (referencing the close
method, self._process, self._monitor, terminate, wait, kill, and TimeoutExpired
to locate the code).
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py`:
- Around line 184-189: The off() shutdown sequence can raise ProcessLookupError
if the Renode process already exited, leaving self._process set and breaking
subsequent off()/on() logic; wrap the terminate()/wait()/kill() sequence in a
try/finally so self._process is always cleared, and specifically catch
ProcessLookupError around self._process.terminate() to treat an already-exited
process as success; keep the existing TimeoutExpired handling for wait and call
kill() only if terminate succeeded, then set self._process = None in the finally
block to ensure state is cleared for future on()/off() calls.
---
Nitpick comments:
In @.github/workflows/python-tests.yaml:
- Around line 87-91: The macOS step "Install Renode (macOS)" currently installs
the latest Homebrew renode while the Linux runner is pinned to a specific Renode
build, causing cross-OS drift; update the "Install Renode (macOS)" step to
either install the same pinned Renode version as the Linux install (use the same
version identifier used in the Linux Renode install step) or add a follow-up
check step that queries the installed renode --version and fails the job if the
version is outside the validated range so both runners run the identical tested
Renode release.
🪄 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: 8fc21ebe-d4da-4988-a3cc-ec5571332521
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/python-tests.yamlpython/packages/jumpstarter-all/pyproject.tomlpython/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/pyproject.toml
✅ Files skipped from review due to trivial changes (2)
- python/packages/jumpstarter-all/pyproject.toml
- python/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py
| def close(self): | ||
| """Synchronous cleanup for use during driver teardown.""" | ||
| if self._process is not None: | ||
| if self._monitor is not None: | ||
| self._monitor = None | ||
| self._process.terminate() | ||
| try: | ||
| self._process.wait(timeout=5) | ||
| except TimeoutExpired: | ||
| self._process.kill() | ||
| self._process = None |
There was a problem hiding this comment.
Same ProcessLookupError issue in synchronous close().
The close() method has the same issue as off(): if the process has already exited, terminate() raises ProcessLookupError and self._process = None is never executed.
🛡️ Proposed fix
def close(self):
"""Synchronous cleanup for use during driver teardown."""
if self._process is not None:
if self._monitor is not None:
self._monitor = None
- self._process.terminate()
try:
- self._process.wait(timeout=5)
- except TimeoutExpired:
- self._process.kill()
- self._process = None
+ self._process.terminate()
+ try:
+ self._process.wait(timeout=5)
+ except TimeoutExpired:
+ self._process.kill()
+ except ProcessLookupError:
+ pass # Process already exited
+ finally:
+ self._process = 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 196 - 206, The close() method can raise ProcessLookupError when
calling self._process.terminate() if the process already exited, preventing
self._process from being cleared; wrap the terminate()/wait()/kill() sequence in
a try/except/finally that catches ProcessLookupError (and reuses the existing
TimeoutExpired handling) and always sets self._process = None, and ensure
self._monitor is cleared before attempting to terminate (referencing the close
method, self._process, self._monitor, terminate, wait, kill, and TimeoutExpired
to locate the code).
There was a problem hiding this comment.
yes, you are agreeing with @raballew comment's up above.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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
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
Mirror the driver's Path construction instead of string suffix matching. Made-with: Cursor
- Validate load_command against allowlist of known Renode load commands - Gate monitor_cmd behind allow_raw_monitor flag (default: false) - Auto-detect ELF vs binary firmware via magic bytes - Add timeout to monitor execute() with fail_after - Close leaked TCP streams on connect() retry - Reject newline characters in monitor commands - Check error markers per-line instead of just first line - Constrain prompt matching to registered machine names - Fix platform description path quoting (@path not @"path") - Add jumpstarter-driver-renode to workspace sources and jumpstarter-all - Add Renode installation to CI workflow (Linux + macOS) - Remove speculative xfail marker from E2E test Made-with: Cursor
Move the Renode integration design document from python/docs/source/contributing/adr/ to the JEP directory at docs/internal/jeps/, following the JEP template from PR jumpstarter-dev#423. Adds mandatory sections (Proposal, Design Details, Test Plan, Backward Compatibility, User Stories, Future Possibilities) and a new DD-5 for security decisions (allow_raw_monitor, load_command allowlist, newline rejection). Made-with: Cursor
|
|
||
| ```yaml | ||
| export: | ||
| Renode: |
There was a problem hiding this comment.
| Renode: | |
| renode: |
| a disk image first, then power on. It also allows re-flashing between | ||
| power cycles without restarting the Renode process. The `RenodeFlasher` | ||
| additionally supports hot-loading: if the simulation is already running, | ||
| `flash()` sends the load command and resets the machine. |
There was a problem hiding this comment.
not to be fixed in this PR: yep, also has the limitation of not being able to open the serial port until it's started. But I am thinking of options for qemu, and then this. We can just make the open wait on the ptty to show up.
| authenticated client poses a risk. The `load_command` parameter in | ||
| `flash()` is separately validated against an allowlist of known Renode | ||
| load commands. Newline characters are rejected in all monitor commands | ||
| to prevent command injection. |
|
This is in a good mergeable state, I will do that and follow up with the collected feedback here #556 myself |
mangelajo
dismissed
raballew’s stale review
I have collected feedback on #556 and will handle it right after merging.
| - name: Install Renode (macOS) | ||
| if: runner.os == 'macOS' | ||
| run: | | ||
| brew install renode/tap/renode | ||
|
|
There was a problem hiding this comment.
| - name: Install Renode (macOS) | |
| if: runner.os == 'macOS' | |
| run: | | |
| brew install renode/tap/renode | |
| # NOTE: not testing renode on macOS, as the install rebuilds renode and it is too slow. | |
| # - name: Install Renode (macOS) | |
| # if: runner.os == 'macOS' | |
| # run: | | |
| # brew install renode/tap/renode | |
Closes #556 MEDIUM fixes: - Fix close() SocketStream leak: add RenodeMonitor.close_sync() using SocketAttribute.raw_socket for best-effort synchronous cleanup - Eliminate private attribute traversal across sibling drivers: add RenodePower.is_running property and send_monitor_command() public method so RenodeFlasher and Renode no longer access _process/_monitor - Make shared firmware state explicit: add Renode.set_firmware() method instead of directly writing parent._firmware_path/_load_command - Fix off() inconsistent state on terminate() failure: wrap process cleanup in try/finally so _process is always reset to None - Add error/cleanup path tests: on() mid-startup failure cleanup, connect() persistent OSError timeout, _read_until_prompt empty receive - Fix idempotency tests: add assertions that Popen/RenodeMonitor are NOT called on duplicate on()/off() calls LOW fixes: - Replace dead hasattr guard with power.is_running check - Extract _configure_simulation() from on() to reduce method length - Add TOCTOU race comment to _find_free_port() - Remove decorative separator comments from tests - Add test coverage: _find_renode not on PATH, read() NotImplementedError, monitor_cmd() success path, close() timeout-kill, close_sync(), is_running property, set_firmware() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## Summary Addresses the open follow-up items from issue #556 (Renode driver review of PR #533). Fix LoadElf, and monitor error detection. ### MEDIUM fixes - **`close()` leaks SocketStream**: Added `RenodeMonitor.close_sync()` that uses `SocketAttribute.raw_socket` for best-effort synchronous socket cleanup during driver teardown - **Private attribute traversal across sibling drivers**: Added `RenodePower.is_running` property and `send_monitor_command()` public method — `RenodeFlasher.flash()` and `Renode.monitor_cmd()` no longer access `_process`/`_monitor` directly - **Shared mutable parent state for firmware loading**: Added `Renode.set_firmware()` method as the explicit interface instead of `RenodeFlasher` writing `parent._firmware_path`/`_load_command` directly - **`off()` inconsistent state on `terminate()` failure**: Wrapped process cleanup in `try/finally` so `_process` is always reset to `None`, even if `terminate()` raises `ProcessLookupError` - **Test gaps — error/cleanup paths**: Added tests for `on()` mid-startup failure cleanup, `connect()` persistent `OSError` → `TimeoutError`, `_read_until_prompt` with empty `receive()` → `ConnectionError` - **Idempotency tests lack assertions**: `test_power_on_idempotent` and `test_power_off_idempotent` now assert that `Popen`/`RenodeMonitor` were NOT called ### LOW fixes - **Dead `hasattr` guard**: Replaced with `power.is_running` (which checks `_process is not None and _monitor is not None`) - **`on()` method too long**: Extracted monitor setup into `_configure_simulation()` - **TOCTOU race in `_find_free_port()`**: Added comment documenting the race and noting Renode doesn't yet support Unix domain sockets - **Decorative separator comments in tests**: Removed all six `# 5.x ...` section separators - **Minor test coverage gaps**: Added tests for `_find_renode()` when binary not on PATH, `RenodePower.read()` `NotImplementedError`, `monitor_cmd()` success path, `close()` timeout-kill branch, `close_sync()`, `is_running` property, `set_firmware()` Co-authored-by: Ambient Code Bot <bot@ambient-code.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- 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>
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