XVC: event-driven refactor + CI regression test#1182
XVC: event-driven refactor + CI regression test#1182ruck314 wants to merge 5 commits intopre-releasefrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the Xilinx XVC server implementation to be event-driven (blocking queue + self-pipe wake) and adds an end-to-end Python test harness plus CI gating to prevent regressions.
Changes:
- Replaces polling/timeouts with blocking waits and a wake-fd for fast shutdown; adds
getPort()and expands GIL management around blocking sections. - Adds pure-stdlib XVC client + deterministic FakeJtag emulator and a comprehensive pytest suite (unit framing + E2E protocol coverage).
- Adds a C++ smoke test and a dedicated “Rogue XVC Protocol Tests” CI step.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/rogue/protocols/xilinx/Xvc.cpp |
Event-driven refactor, wake-fd shutdown, GIL handling, getPort() binding. |
src/rogue/protocols/xilinx/XvcServer.cpp |
Listener loop updated to block in select() and wake via self-pipe. |
src/rogue/protocols/xilinx/XvcConnection.cpp |
Connection read loop updated to block in select(), use wake-fd, and avoid SIGPIPE. |
src/rogue/protocols/xilinx/JtagDriver.cpp |
Makes done_ atomic and uses atomic loads in retry loop. |
include/rogue/protocols/xilinx/Xvc.h |
Adds wake-fd/server members, atomic threadEn_, and getPort() API doc. |
include/rogue/protocols/xilinx/XvcServer.h |
Updates ctor/run signatures for wake-fd + atomic run flag; adds getPort(). |
include/rogue/protocols/xilinx/XvcConnection.h |
Updates ctor/readTo contract for wake-fd and shutdown semantics. |
include/rogue/protocols/xilinx/JtagDriver.h |
Makes done_ atomic and updates isDone() accordingly. |
tests/protocols/xilinx/xvc_client.py |
Pure-stdlib XVC wire client used by tests (no Rogue import). |
tests/protocols/xilinx/fake_jtag.py |
Deterministic scan-chain emulator (ris.Master/Slave) for stream-graph tests. |
tests/protocols/xilinx/conftest.py |
XVC E2E fixtures: starts Xvc(0), wires FakeJtag, yields bound port. |
tests/protocols/xilinx/test_xvc_e2e.py |
End-to-end protocol tests over real TCP against the C++ Xvc server. |
tests/protocols/xilinx/test_xvc_client_framing.py |
Socketpair-based byte-level framing/parse tests for XvcClient. |
tests/protocols/xilinx/test_fake_jtag.py |
Unit tests for FakeJtag oracle + frame layout + error paths. |
tests/python/protocols/xilinx/test_xvc_api.py |
Python API smoke: preserved surface, getPort(), and basic GIL-release check. |
tests/conftest.py |
Adds xdist-aware port-range slicing and an xvc_server fixture. |
tests/cpp/protocols/xilinx/test_xvc_smoke.cpp |
Doctest smoke for stop latency (wake-fd) with/without a client. |
tests/cpp/protocols/xilinx/CMakeLists.txt |
Registers the new C++ smoke test. |
tests/cpp/protocols/CMakeLists.txt |
Adds protocols subdir when Python is enabled. |
tests/cpp/CMakeLists.txt |
Includes protocols tests under the NO_PYTHON guard. |
docs/src/built_in_modules/protocols/xilinx/index.rst |
Documents Vivado default port and dynamic port assignment via getPort(). |
.github/workflows/rogue_ci.yml |
Adds a dedicated XVC protocol test step in Ubuntu + macOS jobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- XvcConnection::readTo retries select() on EINTR (matches XvcServer::run) so a benign signal no longer tears down the client connection. - Xvc::start guards against second start with a clear GeneralError; rogue::Queue::stop is irreversible so restart on the same instance is unsupported. This also makes the wake-fd drain question moot — the read end never gets re-armed under a new start. - Xvc::xfer uses separate txFrame/rxFrame shared_ptrs so the TX frame is never released inside the GilRelease scope (no overwrite during the GIL-released section). - Tag the C++ server thread via pthread_setname_np for OS-level diagnostics (htop, gdb info threads). - Replace the Python thread-name leak filter with an observable socket-unreachability assertion: after _stop(), the kernel-assigned port must refuse new connections. The old filter was vacuous because the C++ std::thread is invisible to Python's threading.enumerate(). - Smoke test drops the racy pick_free_port() helper and uses Xvc(0) + getPort() instead, eliminating the TOCTOU window between bind/close and Xvc rebind.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Close leaked FDs in XvcServer ctor when setsockopt/bind/listen throw after socket() succeeds - Close leaked FD in XvcConnection ctor when setsockopt throws after accept() succeeds - Log errno on select() error in XvcServer::run() for diagnostics - Distinguish shutdown (-1) from select error (-2) in readTo() so fill()/run() report accurate error messages - Make Xvc::start() transactional: defer started_/threadEn_ flags until server construction and thread spawn both succeed; clean up on failure - Fix misleading wake-fd drain comment in Xvc::stop()
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pre-release #1182 +/- ##
===============================================
+ Coverage 56.05% 56.10% +0.04%
===============================================
Files 70 70
Lines 8137 8137
Branches 1209 1209
===============================================
+ Hits 4561 4565 +4
+ Misses 3293 3291 -2
+ Partials 283 281 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
readTo(): ::read() returning -1 (EINTR, ECONNRESET) collided with the -1 shutdown sentinel. Wrap in an EINTR retry loop and map remaining socket errors to -2 so callers never confuse a read failure with a wakeFd shutdown signal. test_parallel_workers: add threading.Barrier so all 4 Xvc instances stay alive until every port is recorded, eliminating the theoretical TOCTOU window where the OS could recycle a released ephemeral port.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace post-stop() drain loop with queue_.reset() to avoid infinite spin when queue is non-empty (Queue::pop returns default after stop). - Cache bound port in atomic<uint32_t> so getPort() never races with stop()'s server_.reset(). - Mark instance done and close listener in runThread catch blocks so xfer() callers unblock and new TCP clients are refused on init/run failure. - Retry send() on EINTR in XvcConnection::flush() (mirrors readTo()).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/rogue/protocols/xilinx/XvcServer.cpp:52
sockaddr_in a;is not zero-initialized before being passed tobind(). Onlysin_family/sin_addr/sin_portare set, leaving the padding/unused fields uninitialized, which can trip MSan/valgrind and is technically UB. Initialize withsockaddr_in a{};(ormemset(&a, 0, sizeof(a))) before assigning fields.
struct sockaddr_in a;
int yes = 1;
a.sin_family = AF_INET;
a.sin_addr.s_addr = INADDR_ANY;
a.sin_port = htons(port);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard sys.path.insert in xvc_server fixture to avoid duplicates - Drop flaky lower-bound assertion on time.sleep() in test_gil_release - Catch Exception instead of BaseException in client thread wrapper
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Xvc::start(): reset boundPort_ to 0 in the catch block when std::thread creation throws, so getPort() doesn't return a stale port from a failed start. - XvcServer::run(): throw GeneralError on unrecoverable select() failure instead of silently breaking. The throw propagates to runThread()'s existing catch blocks which set done_=true, queue_.stop(), and server_.reset() — so xfer() callers unblock and new TCP clients are refused. The previous silent break left threadEn_ true with no accept loop running. Both issues flagged by GitHub Copilot review on PR #1182.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor XVC/JTAG driver to event-driven architecture with hardened code quality: RAII thread ownership, widened arithmetic to prevent overflow, explicit narrowing casts, virtual destructors, and defensive guards throughout. Add JTAG emulator (fake_jtag.py) and comprehensive test suite covering XVC client framing, end-to-end flows, and C++ smoke tests.
The xilinx conftest.py was adding its directory to sys.path[0], which caused pytest to resolve 'from conftest import wait_for' to the wrong conftest module during full test collection.
Summary
rogue.protocols.xilinx.Xvc: replacesusleep(1000)+ queue polling with blockingQueue::pop, self-pipe wakeFd for promptselect()shutdown (< 50 ms),getPort()accessor,MSG_NOSIGNALin write paths, GIL release across blocking sections. Public Python API unchanged.XvcClient, deterministicFakeJtag(ris.Master+ris.Slave) scan-chain emulator, pytest E2E cases coveringgetinfo/settck/shift(widths 1,7,8,9,15,16,17,32,64), chunking oversupVecLen_, clean teardown (listening socket unreachable after_stop()), unknown-command handling, parallel-worker safety via port 0 +getPort(). Picked up automatically by the existingRogue Tests(pytest) andRogue Native C++ Tests(ctest) steps inrogue_ci.ymlon Ubuntu and macOS — full_build_test gatesgen_release/conda_build_lib/docker_build_roguevia the existingneeds:chain.tstateontoXvc::runThreadviaScopedGiland wraps theselect()sites inXvcServer::run/XvcConnection::readToinGilReleaseso Pythonris.Slavesubclasses can call back intoself._sendFramefrom native-thread context without the fatalPyThreadState_Getcrash.tests/**/__init__.pymarkers that turnedtests/into a package and brokefrom conftest import MemoryRootintests/core/plus a name collision between the twoxilinxtest dirs under-n auto.Test plan
./scripts/run_linters.shclean (flake8 + cpplint,compileallgreen)cmake -S . -B build -DROGUE_INSTALL=local -DROGUE_BUILD_TESTS=ON && cmake --build build -j && cmake --build build --target install)ctest --test-dir build -L cpppytest -n auto --dist loadfile tests/protocols/xilinx/ tests/python/protocols/xilinx/pytest -n auto --dist loadfile -m "not perf"