Add TX16S UI harness - #7337
Conversation
57f162e to
cfadba6
Compare
📝 WalkthroughWalkthroughAdds optional native-test build gating, simulator stdin automation with JSON commands and PPM screenshots, and a Python UI harness exposing CLI and MCP interfaces for TX16S flows, fixtures, screenshots, and simulator lifecycle management. ChangesSimulator UI harness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant McpServer
participant HarnessService
participant SdlAutomationSession
participant sdl_simu
MCPClient->>McpServer: Request a harness tool
McpServer->>HarnessService: Dispatch the requested operation
HarnessService->>SdlAutomationSession: Start simulator or execute action
SdlAutomationSession->>sdl_simu: Send newline-delimited automation command
sdl_simu-->>SdlAutomationSession: Return JSON status or screenshot metadata
SdlAutomationSession-->>HarnessService: Return operation result
HarnessService-->>McpServer: Return tool result
McpServer-->>MCPClient: Return JSON-RPC response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
radio/src/targets/simu/sdl_simu.cpp (2)
266-270: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueInitialize all fields of the SDL event.
It's good practice to zero-initialize the event struct to prevent pushing uninitialized memory to the SDL event queue.
🧹 Proposed fix
} else if (command == "stop") { - SDL_Event event; + SDL_Event event{}; event.type = SDL_QUIT;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/simu/sdl_simu.cpp` around lines 266 - 270, Zero-initialize the SDL_Event local in the command == "stop" branch before setting its type and pushing it, ensuring all fields are initialized while preserving the existing automation_reply_ok flow.
244-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asynchronous execution for duration-based commands.
Using
SDL_Delayblocks the main SDL event loop. While acceptable for a simple testing harness, this freezes the window rendering and prevents OS event processing during the wait, which might cause the OS to flag the window as unresponsive. Consider moving the wait logic to the Python harness by exposing explicitkey_down/key_upcommands in the future.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/simu/sdl_simu.cpp` around lines 244 - 248, Update the duration-based “wait” handling in the command-processing flow to avoid blocking the SDL main event loop with SDL_Delay; move timing to an asynchronous/non-blocking mechanism while preserving the existing duration clamping and automation_reply_ok behavior, or expose the required key_down/key_up commands so the Python harness can handle the wait.tools/ui-harness/edgetx_ui/core.py (1)
195-208: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure pipes are closed and process is fully reaped.
Leaving
stdinandstdoutunclosed can lead to file descriptor leaks orResourceWarnings. Additionally, after callingkill(), it's recommended to callwait()to ensure the child process is reaped and doesn't remain a zombie.♻️ Proposed refactor for cleanup logic
def stop(self) -> dict[str, Any]: if not self.process: return {"running": False} if self.process.poll() is None: try: self.command("stop", timeout=2.0) except HarnessError: self.process.terminate() try: self.process.wait(timeout=3.0) except subprocess.TimeoutExpired: self.process.kill() + self.process.wait() + + if self.process.stdin: + self.process.stdin.close() + if self.process.stdout: + self.process.stdout.close() + return {"running": False}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/ui-harness/edgetx_ui/core.py` around lines 195 - 208, Update the process cleanup in stop so stdin and stdout are closed after the process exits, and call wait again after kill to fully reap the child. Preserve the existing terminate and timeout behavior while ensuring cleanup also occurs when the process is already stopped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/targets/simu/sdl_simu.cpp`:
- Around line 249-255: Update the screenshot_ppm command handling to use
std::getline on the input stream instead of formatted extraction, capturing the
complete path including spaces. Preserve the existing empty-path validation and
automation_reply_error behavior.
- Around line 276-297: Replace the select()/std::cin combination in
automation_poll_stdin with non-blocking POSIX read() calls into a static buffer.
Accumulate partial input across polls, extract and pass each complete
newline-terminated command to automation_handle_command, and ensure polling
never blocks the SDL event loop or loses commands already read from the pipe.
In `@tools/ui-harness/edgetx_ui/core.py`:
- Around line 217-234: Update the chunked stdout-reading logic in the deadline
loop to accumulate raw bytes rather than decoding each os.read result
independently. Split the byte buffer on newline bytes, then decode only complete
lines with UTF-8 replacement handling; preserve partial trailing bytes for the
next chunk and retain the existing stripping and empty-line filtering behavior.
---
Nitpick comments:
In `@radio/src/targets/simu/sdl_simu.cpp`:
- Around line 266-270: Zero-initialize the SDL_Event local in the command ==
"stop" branch before setting its type and pushing it, ensuring all fields are
initialized while preserving the existing automation_reply_ok flow.
- Around line 244-248: Update the duration-based “wait” handling in the
command-processing flow to avoid blocking the SDL main event loop with
SDL_Delay; move timing to an asynchronous/non-blocking mechanism while
preserving the existing duration clamping and automation_reply_ok behavior, or
expose the required key_down/key_up commands so the Python harness can handle
the wait.
In `@tools/ui-harness/edgetx_ui/core.py`:
- Around line 195-208: Update the process cleanup in stop so stdin and stdout
are closed after the process exits, and call wait again after kill to fully reap
the child. Preserve the existing terminate and timeout behavior while ensuring
cleanup also occurs when the process is already stopped.
🪄 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 Plus
Run ID: bfae56bb-ac59-4fda-b493-1da4bc76644b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignorecmake/NativeTargets.cmakepyproject.tomlradio/src/CMakeLists.txtradio/src/targets/simu/arg_parser.cppradio/src/targets/simu/arg_parser.hradio/src/targets/simu/sdl_simu.cpptools/ui-harness/README.mdtools/ui-harness/edgetx-mcptools/ui-harness/edgetx-uitools/ui-harness/edgetx_ui/__init__.pytools/ui-harness/edgetx_ui/cli.pytools/ui-harness/edgetx_ui/core.pytools/ui-harness/edgetx_ui/mcp_server.pytools/ui-harness/edgetx_ui/png.pytools/ui-harness/fixtures/settings-tx16s/MODELS/labels.ymltools/ui-harness/fixtures/settings-tx16s/MODELS/model1.ymltools/ui-harness/fixtures/settings-tx16s/RADIO/radio.ymltools/ui-harness/flows/tx16s-smoke.json
| } else if (command == "screenshot_ppm") { | ||
| std::string path; | ||
| in >> path; | ||
| if (path.empty()) { | ||
| automation_reply_error("missing screenshot path"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle spaces in screenshot paths.
Using in >> path extracts up to the first whitespace, which will truncate paths containing spaces. Use std::getline to safely capture the full rest of the line as the path.
💚 Proposed fix
} else if (command == "screenshot_ppm") {
std::string path;
- in >> path;
+ std::getline(in >> std::ws, path);
if (path.empty()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (command == "screenshot_ppm") { | |
| std::string path; | |
| in >> path; | |
| if (path.empty()) { | |
| automation_reply_error("missing screenshot path"); | |
| return; | |
| } | |
| } else if (command == "screenshot_ppm") { | |
| std::string path; | |
| std::getline(in >> std::ws, path); | |
| if (path.empty()) { | |
| automation_reply_error("missing screenshot path"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@radio/src/targets/simu/sdl_simu.cpp` around lines 249 - 255, Update the
screenshot_ppm command handling to use std::getline on the input stream instead
of formatted extraction, capturing the complete path including spaces. Preserve
the existing empty-path validation and automation_reply_error behavior.
| static void automation_poll_stdin() | ||
| { | ||
| if (!automation_stdio) return; | ||
|
|
||
| #if defined(_WIN32) | ||
| return; | ||
| #else | ||
| timeval timeout = {0, 0}; | ||
| fd_set readfds; | ||
| FD_ZERO(&readfds); | ||
| FD_SET(STDIN_FILENO, &readfds); | ||
|
|
||
| int ready = select(STDIN_FILENO + 1, &readfds, nullptr, nullptr, &timeout); | ||
| if (ready > 0 && FD_ISSET(STDIN_FILENO, &readfds)) { | ||
| std::string line; | ||
| if (std::getline(std::cin, line)) { | ||
| automation_handle_command(line); | ||
| } | ||
| } | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid mixing select() with std::cin to prevent deadlocks and blocking.
std::cin uses an internal stream buffer. If multiple commands arrive in the OS pipe simultaneously, std::cin reads them into its buffer. std::getline will return the first command, but subsequent calls to select() will block (since the OS pipe is now empty), trapping the remaining commands in the buffer until new data arrives. Additionally, if only a partial line arrives, std::getline will block the main SDL event loop until a newline is received.
Use non-blocking POSIX read() into a static buffer instead.
🛠️ Proposed fix to manually buffer lines
static void automation_poll_stdin()
{
if (!automation_stdio) return;
`#if` defined(_WIN32)
return;
`#else`
timeval timeout = {0, 0};
fd_set readfds;
FD_ZERO(&readfds);
FD_SET(STDIN_FILENO, &readfds);
int ready = select(STDIN_FILENO + 1, &readfds, nullptr, nullptr, &timeout);
if (ready > 0 && FD_ISSET(STDIN_FILENO, &readfds)) {
- std::string line;
- if (std::getline(std::cin, line)) {
- automation_handle_command(line);
- }
+ static std::string buffer;
+ char chunk[256];
+ ssize_t bytes = read(STDIN_FILENO, chunk, sizeof(chunk));
+ if (bytes > 0) {
+ buffer.append(chunk, bytes);
+ size_t pos;
+ while ((pos = buffer.find('\n')) != std::string::npos) {
+ std::string line = buffer.substr(0, pos);
+ buffer.erase(0, pos + 1);
+ if (!line.empty() && line.back() == '\r') line.pop_back();
+ automation_handle_command(line);
+ }
+ }
}
`#endif`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void automation_poll_stdin() | |
| { | |
| if (!automation_stdio) return; | |
| #if defined(_WIN32) | |
| return; | |
| #else | |
| timeval timeout = {0, 0}; | |
| fd_set readfds; | |
| FD_ZERO(&readfds); | |
| FD_SET(STDIN_FILENO, &readfds); | |
| int ready = select(STDIN_FILENO + 1, &readfds, nullptr, nullptr, &timeout); | |
| if (ready > 0 && FD_ISSET(STDIN_FILENO, &readfds)) { | |
| std::string line; | |
| if (std::getline(std::cin, line)) { | |
| automation_handle_command(line); | |
| } | |
| } | |
| #endif | |
| } | |
| static void automation_poll_stdin() | |
| { | |
| if (!automation_stdio) return; | |
| `#if` defined(_WIN32) | |
| return; | |
| `#else` | |
| timeval timeout = {0, 0}; | |
| fd_set readfds; | |
| FD_ZERO(&readfds); | |
| FD_SET(STDIN_FILENO, &readfds); | |
| int ready = select(STDIN_FILENO + 1, &readfds, nullptr, nullptr, &timeout); | |
| if (ready > 0 && FD_ISSET(STDIN_FILENO, &readfds)) { | |
| static std::string buffer; | |
| char chunk[256]; | |
| ssize_t bytes = read(STDIN_FILENO, chunk, sizeof(chunk)); | |
| if (bytes > 0) { | |
| buffer.append(chunk, bytes); | |
| size_t pos; | |
| while ((pos = buffer.find('\n')) != std::string::npos) { | |
| std::string line = buffer.substr(0, pos); | |
| buffer.erase(0, pos + 1); | |
| if (!line.empty() && line.back() == '\r') line.pop_back(); | |
| automation_handle_command(line); | |
| } | |
| } | |
| } | |
| `#endif` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@radio/src/targets/simu/sdl_simu.cpp` around lines 276 - 297, Replace the
select()/std::cin combination in automation_poll_stdin with non-blocking POSIX
read() calls into a static buffer. Accumulate partial input across polls,
extract and pass each complete newline-terminated command to
automation_handle_command, and ensure polling never blocks the SDL event loop or
loses commands already read from the pipe.
| deadline = time.monotonic() + timeout | ||
| recent_lines: deque[str] = deque(maxlen=200) | ||
| buffer = "" | ||
| stdout_fd = self.process.stdout.fileno() | ||
| while time.monotonic() < deadline: | ||
| remaining = max(0.0, deadline - time.monotonic()) | ||
| ready, _, _ = select.select([stdout_fd], [], [], remaining) | ||
| if not ready: | ||
| break | ||
| chunk = os.read(stdout_fd, 4096) | ||
| if not chunk: | ||
| break | ||
| buffer += chunk.decode("utf-8", errors="replace") | ||
| while "\n" in buffer: | ||
| line, buffer = buffer.split("\n", 1) | ||
| line = line.strip() | ||
| if not line: | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent multi-byte character corruption during chunked reading.
Decoding raw byte chunks directly into strings can corrupt multi-byte UTF-8 sequences if they happen to be split across the 4096-byte chunk boundary. Instead, accumulate raw bytes and decode complete lines after splitting by a newline byte.
🐛 Proposed fix for safe string decoding
deadline = time.monotonic() + timeout
recent_lines: deque[str] = deque(maxlen=200)
- buffer = ""
+ buffer = b""
stdout_fd = self.process.stdout.fileno()
while time.monotonic() < deadline:
remaining = max(0.0, deadline - time.monotonic())
ready, _, _ = select.select([stdout_fd], [], [], remaining)
if not ready:
break
chunk = os.read(stdout_fd, 4096)
if not chunk:
break
- buffer += chunk.decode("utf-8", errors="replace")
- while "\n" in buffer:
- line, buffer = buffer.split("\n", 1)
- line = line.strip()
+ buffer += chunk
+ while b"\n" in buffer:
+ line_bytes, buffer = buffer.split(b"\n", 1)
+ line = line_bytes.decode("utf-8", errors="replace").strip()
if not line:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deadline = time.monotonic() + timeout | |
| recent_lines: deque[str] = deque(maxlen=200) | |
| buffer = "" | |
| stdout_fd = self.process.stdout.fileno() | |
| while time.monotonic() < deadline: | |
| remaining = max(0.0, deadline - time.monotonic()) | |
| ready, _, _ = select.select([stdout_fd], [], [], remaining) | |
| if not ready: | |
| break | |
| chunk = os.read(stdout_fd, 4096) | |
| if not chunk: | |
| break | |
| buffer += chunk.decode("utf-8", errors="replace") | |
| while "\n" in buffer: | |
| line, buffer = buffer.split("\n", 1) | |
| line = line.strip() | |
| if not line: | |
| continue | |
| deadline = time.monotonic() + timeout | |
| recent_lines: deque[str] = deque(maxlen=200) | |
| buffer = b"" | |
| stdout_fd = self.process.stdout.fileno() | |
| while time.monotonic() < deadline: | |
| remaining = max(0.0, deadline - time.monotonic()) | |
| ready, _, _ = select.select([stdout_fd], [], [], remaining) | |
| if not ready: | |
| break | |
| chunk = os.read(stdout_fd, 4096) | |
| if not chunk: | |
| break | |
| buffer += chunk | |
| while b"\n" in buffer: | |
| line_bytes, buffer = buffer.split(b"\n", 1) | |
| line = line_bytes.decode("utf-8", errors="replace").strip() | |
| if not line: | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ui-harness/edgetx_ui/core.py` around lines 217 - 234, Update the
chunked stdout-reading logic in the deadline loop to accumulate raw bytes rather
than decoding each os.read result independently. Split the byte buffer on
newline bytes, then decode only complete lines with UTF-8 replacement handling;
preserve partial trailing bytes for the next chunk and retain the existing
stripping and empty-line filtering behavior.
|
Thank you, @onliner10, for the work and architecture in this PR. While reviewing overlap raised on #7646, it became clear that #7337 provides the stronger generic foundation, while #7646 explored some complementary requirements around Windows, reset/reload, state injection, and capture after redraw. I opened #7668 as a collaborative consolidation draft rather than continuing a competing protocol. Its first commit is a detailed keep/redesign/drop matrix and implementation plan. It intentionally preserves #7337's generic harness direction, addresses the current review findings, narrows the first implementation, and includes an explicit authorship/credit policy. I would be glad to collaborate directly and preserve Git authorship for reused work. Nothing in this proposal requires you to close or change this PR now; your feedback on #7668's protocol boundary and contribution approach would be very welcome. |
Add a dependency-free binary-pipe session for the implemented start, ping, and stop lifecycle, with strict response correlation and bounded cleanup tests.\n\nThe reusable host-session and CLI direction builds on EdgeTX#7337 by Mateusz Urban. This implementation is substantially rewritten for protocol v1, Windows-compatible reader threads, and deterministic process cleanup; no coherent source block is imported.
Add bounded status and describe responses backed by the simulator session state, then validate discovery and poll first-frame readiness from the cross-platform host session. This consolidates the host lifecycle and discovery direction from EdgeTX#7337 with the guarded epoch and first-frame state model from EdgeTX#7646. Credit to @onliner10 and @pfeerick for the two approaches brought together here.
Implement Phase 4 of the consolidated simulator automation plan: target-filtered key, rotary, and touch primitives; an asynchronous real-LCD frame barrier; host-side timed composites; release cleanup; and focused native/Python coverage. This combines the portable host-side composition direction from EdgeTX#7337 with the direct simulator helpers and explicit touch transitions explored in EdgeTX#7646. Thanks to Mateusz Urban (@onliner10) for the original harness direction; review remains explicitly invited in EdgeTX#7668.
Capture a strictly newer RGB565 framebuffer after an LVGL invalidation, publish deterministic PPM artifacts without replacement, and convert them to verified PNG metadata in the host client. This combines the portable PPM and client direction from EdgeTX#7337 with the static-screen invalidation approach from EdgeTX#7646. Thanks to @onliner10 for the original harness and capture direction.
2 participants
Fixes: N/A
Summary of Changes
This adds a repeatable TX16S UI automation harness for simulator-driven UI work.
The goal is to make future color LCD/touch UI changes easier to review, easier to automate, and safer to regression-test before touching production UI code.
Why This Is Being Added
EdgeTX UI work currently depends heavily on manually launching the simulator, navigating screens by hand, and capturing host-window screenshots. That makes visual regression checks hard to repeat and easy to skew by local window scale, focus, or setup state.
This harness provides a small, opt-in control plane around the existing simulator so contributors can build, navigate, and capture framebuffer screenshots in a reproducible way.
It also makes EdgeTX much easier to work on with AI coding agents. The simulator can be exposed as an MCP server, so an agent can start a TX16S simulator session, press keys, rotate, tap, wait, capture screenshots, and inspect UI state without relying on manual host-window interaction. That should make UI work more reviewable and less speculative.
This is intended as groundwork for future TX16S-class color touchscreen UI improvements. It does not redesign the UI and does not change model storage, EEPROM format, Lua APIs, or normal simulator behavior.
What Changed
--automation-stdioto the SDL simulator as an opt-in automation mode.status, key press, long press, rotary, touch, wait, screenshot, and stop.simuLcdCopy, avoiding host-window screenshots.tools/ui-harness/edgetx-uiCLI for build, smoke, and JSON flow execution.tools/ui-harness/edgetx-mcpas a stdio MCP wrapper using the same Python core as the CLI.home,menu, andreturn-homescreenshots.uvproject metadata so contributors can run the harness with a known Python dependency set.EDGE_TX_BUILD_TESTSso this harness build path can avoid fetching/building native tests when they are not needed.Contribution / Review Notes
--automation-stdiois passed.EDGE_TX_BUILD_TESTSoption only disables them when explicitly set toOFF.PCB=X10,PCBREV=TX16S), but the harness is structured so more color LCD/touch targets can be added later.Verification
The smoke flow produced three valid 480x272 PNG screenshots from the simulator framebuffer:
homemenureturn-homeSummary by CodeRabbit
New Features
Build & Configuration
Documentation