Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!live_tx_running()) { | ||
| stop_sample_clock(sample_clock); | ||
| continue; |
There was a problem hiding this comment.
If the publisher is already awake while handle_line() executes START, it can consume g_start_request after that flag is stored but before the subsequent g_running=true store. This new check then observes the old stopped state, stops the clock, and discards the only request; the control task afterwards marks the device RUNNING and reports START accepted, but no timer is started and the publisher may block indefinitely. Publish these fields as one state transition, or at least make the running state visible before the request can be consumed.
AGENTS.md reference: AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
| static constexpr std::uint32_t kNoReader = 3U; | ||
| std::array<SvLiveSignalState, 3> banks_{}; | ||
| mutable std::atomic<std::uint32_t> reader_index_{kNoReader}; | ||
| std::array<SvLiveSignalState, 4> banks_{}; |
There was a problem hiding this comment.
Index 3 is now a valid bank, but it remains the kNoReader sentinel. When bank 3 is the only inactive unpinned slot—for example, active bank 2 while two concurrent readers still pin banks 0 and 1—the loop selects 3 and the following sentinel check returns false, so the advertised spare bank is never usable and a valid signal update is dropped. Use banks_.size() or another out-of-range value as the sentinel.
AGENTS.md reference: AGENTS.md:L120-L127
Useful? React with 👍 / 👎.
| publisher.join(); | ||
| diagnostics.join(); | ||
| CHECK(committed > 100U); | ||
| CHECK(read_count.load() > 100U); |
There was a problem hiding this comment.
The main thread can finish all 20,000 publishes and set done before either newly created reader receives meaningful CPU time; each reader then performs only its mandatory first iteration and this assertion fails with a count of 2. Thread scheduling provides no minimum progress guarantee here, so the regression test is flaky and also does not deterministically exercise reads overlapping writes. Add a startup barrier and require a controlled number of reader iterations before ending the test.
AGENTS.md reference: AGENTS.md:L230-L234
Useful? React with 👍 / 👎.
|
Additional concrete GUI-vs-firmware semantic defect discovered during end-to-end audit (19 October 2026): QML |
1 participant
Audit scope (GUI + ESP32-P4 + PTP AUTO)
Verified source at PR #247 HEAD f3e4456:
SETto an immutable generation viaSvLiveSignalBank, read by 4 kHz publisher and console diagnostics. Physical values, PTP lock time and network signal parity require fresh PCAP and hardware logs.P0 defects found and corrected
reader_index_was only safe if all snapshot readers serialized; firmware uses concurrent publisher and console diagnostics, so the second reader can replace first reader protection while writer republishes to a bank still copied by the first. Replace with 4 bank slots + per-bank atomic reader pin counts. No mutex, allocation or I/O in the 4kHz snapshot path.g_start_request. Explicit STOP now cancels pending START; publisher checks cancellation before and after build.g_running=trueand claim START accepted, then fail profile/template preparation while staying logically RUNNING with no SV. Publisher nowlive_control_force_stop()+ GPTimer teardown on all profile/template failures.Regression
smpSynch=1, no loosened clock gate, no silent GUI claim of SV wire parity. PR remains unmerged pending physical 4I4V and PTP acceptance.Remaining P0 hardening
Serial
SETs are independent firmware updates. A fully atomic, firmware-confirmed scenario generation commit + CRC/profile hash and independent capture-side exact comparison is needed to prove every START signal matches visible table; don't call that solved by queue exclusivity. Use physical tests to verify 4000 fps, zero gaps, RMS and phase, VLAN/APPID/svID/quality, AUTO stable LOCAL <=10s, hot-plug and reboot.