Repository navigation
feat(player): add opt-in removal of long recorded gaps - #2117
timothyanderson096-ocdealcheck wants to merge 1 commit into
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. |
Sorry, but this is likely not, what you want. The example Maybe I really didn't get behind the use-case - but this approach looks wrong to me. |
|
Thanks — my example mixed recorded timestamps with elapsed playback time without making the distinction clear. The intended example is:
The playback columns are idealized seconds from replay start, not rewritten Message.timestamp values or offsets from the preceding frame. The repeated 0.25 means no deliberate sleep between the B and C send calls. MessageSync yields the original Message objects in input order, and can.player calls bus.send sequentially. The recorded timestamps remain 0, 0.25, 5.25 and 5.5. I rechecked the unchanged head: 45 focused tests pass, and a separate probe through the real VirtualBus queues preserves send/receive order and those timestamps with removal enabled and disabled. That establishes application/VirtualBus ordering, not physical CAN transmission order or real-time precision. Removing waits can create bursts; the proposed option removes the wait requested in #1646 without adding a minimum inter-frame delay or a hardware ordering guarantee. I have clarified the PR description to make those boundaries explicit. Timothy Anderson — Spotted Ridge Engineering / OC Labs · GDN (Global Decision Network) |
|
@codex review
Retrying the automated review that could not complete on the unchanged PR
head.
On Mon, 26 Oct 2026 21:37:08 -0700, "chatgpt-codex-connector[bot]" < ***@***.***> wrote:
chatgpt-codex-connector[bot] left a comment (hardbyte/python-can#2117) <!--
codex-pull-request-review-summary --> <!-- codex-review-state:v1
{"code_review":{"commit_sha":"c7850ea4a1d6594aa31c60eae2801af8b1193c38","has_any_findings":null,"has_unresolved_findings":null,"status":"running"},"security_review":{"commit_sha":null,"has_any_findings":null,"has_unresolved_findings":null,"status":null}}
--> ## Codex Review Summary This comment shows the latest Codex review
activity on this pull request. | Review | Status | Commit | Review trigger
| | --- | --- | --- | --- | | 📝 **Code Review** | 🔄 **Running** since
<relative-time
datetime="2026-10-27T04:36:42.222789Z">2026-10-27T04:36:42.222789Z</relative-time>
| `c7850ea` | PR opened | <details> <summary>ℹ️ About Codex in
GitHub</summary> <br/> [Your team has set up Codex to review pull requests
in this repo](https://chatgpt.com/codex/cloud/settings/general). Reviews
are triggered when you - Open a pull request for review - Mark a draft as
ready - Comment ***@***.*** review" or ***@***.*** security review". Codex reacts
with 👀 while any review is running, comments if it has suggestions, and
reacts with 👍 once all reviews finish with no findings. </details> --
Reply to this email directly or view it on GitHub:
#2117 (comment)
You are receiving this because you authored the thread. Message ID:
***@***.***>
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary of Changes
Add keyword-only
MessageSync(skip_gaps=True)and player--remove-gapsto remove remaining recorded-timestamp waits greater than the existingskipthreshold. Defaults continue capping the wait.skip=0disables removal/capping, and fixed-gap playback is unchanged. Existing four positional arguments and the CLI--skabbreviation remain valid. Add API/CLI documentation, deterministic regressions and a news fragment.For recorded
Message.timestampvalues0, .25, 5.25, 5.5withskip=1, the idealized elapsed playback times measured from replay start are0, .25, 1.25, 1.5with the default cap and0, .25, .25, .5with opt-in removal. These are neither rewritten message timestamps nor offsets from the preceding frame. The original Message objects, their recorded timestamps and their iteration order are retained; can.player calls bus.send sequentially. The repeated.25means no deliberate wait between those two send calls. Physical bus ordering and real-time scheduling precision are not guaranteed by this change. The threshold is applied to the remaining wait after consumer processing time; equal-threshold waits are retained and subsequent intervals follow the shortened timeline. CLI options go before the input filename, consistent with its existing configuration-remainder parsing.Related Issues / Pull Requests
Related to #1646, reported by @yomannnn. @zariiii9003 described this as an enhancement and invited a PR; @hartkopp explained the distinction between capping and removing gaps. API/flag naming is proposed, not previously approved. This does not promise full compatibility with can-utils canplayer.
Current overlapping #1936 adds pcapng/compression readers in the same module without changing MessageSync. Merged #1374 supplies the existing perf-counter/consumer-time scheduling; retain that behavior and credit its author @zariiii9003.
Type of Change
Checklist
Additional Notes
Validation recorded on 25 October 2026, CPython 3.13.16 / Linux:
:shell:option. Review caught this; the final revision restores the option and passes strict HTML/doctest docs and the final 45-test focused run.a1ed7aa; finalfb3be02changes only the RST option location. Python/tests/news and installed runtime module hashes match the final source. Final affected docs/focused gates were rerun.On 27 October, public head
c7850ea4a1d6594aa31c60eae2801af8b1193c38, full tree102f5319975abf23f9885206b135fddba995319d, was recovered exactly from the tested clean final candidatefb3be02b05ccab2a568812aae981d64f74615e69on unchanged mainb4f82ab. Saved raw archive hash, original/final test XML, independent review, docs output and equal full-suite failure identities were checked. No tests were rerun during this publication refresh.Review follow-up on 27 October for hartkopp's timing/order question: the unchanged public head passes the 45 focused tests again on CPython 3.12.14 / Linux. An additional private probe using the real VirtualBus send/receive queues preserves input/send/receive ID order and original recorded timestamps for both default capping and opt-in removal. The probe uses a simulated scheduler clock and does not establish physical CAN transmission order. No production or upstream test source changed for this clarification.
Hardware, real-time scheduling guarantees, other OS/interpreter matrix and direct can-utils comparison were not validated. Existing negative/nonfinite timing semantics are outside this feature.
Prepared with AI assistance and independently inspected during the original preparation.
Timothy Anderson — Spotted Ridge Engineering / OC Labs
Independent software verification · GDN (Global Decision Network)
timothyanderson096@gmail.com · Engineering evidence