Skip to content

docs: propose FrameSync tickbox+auto-election and flag missing config… #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrewsg3 merged 1 commit into from
Aug 5, 2026
Merged

docs: propose FrameSync tickbox+auto-election and flag missing config… #263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ These are account/settings actions on github.com, not fixable by editing the rep
### Medium priority — reliability / UX

- [x] **CI never runs on PRs / lints with flake8 / excludes examples / testpaths gap** (found 2026-07-09 review) — all four resolved: PR triggers were already present on `main`/`staging`/`develop`; CI now runs `ruff` (dev dependency, converged from flake8); `src/modules/examples` is no longer excluded (fixing this required correcting `[tool.ruff] target-version` from the false `py38` to `py311` — the old value made `match` statements register as syntax errors); `testpaths` already included `src/modules/tests`. One real bug the newly-included directory caught immediately: `src/modules/examples/arduino/arduino_module.py` subclassed `Command` without importing it — fixed. Note `arduino_module.py` has other latent issues beyond that (e.g. `ArduinoCommand` methods reference `self.callbacks`, which nothing ever sets — the base `Command` class populates `self.commands`) not yet addressed. 2026-07-31 review found it's worse than latent: `ArduinoCommand.__init__` calls `super().__init__()` with 4 positional args but base `Command.__init__` only takes one, so `ArduinoModule()` raises `TypeError` immediately on construction — it cannot be instantiated at all. `self.export.set_callbacks(...)` and `self.service.controller_ip` also reference things that don't exist (`Export` has no such method; should be `self.network`). This looks like fully dead code superseded by `apa_arduino_module.py` — consider deleting rather than patching further.
- [ ] **`web.py`: no config change is blocked while a session is `ACTIVE`** (found 2026-08-05, while discussing the FrameSync auto-election idea below) — `save_module_config`, `apply_section_to_cameras`, `apply_section_to_type`, `reset_module_config`, and `save_controller_config` (all `web.py`) each call only `_require_auth()`; none check session state. The frontend doesn't gate this either — `CameraConfigCard.jsx`'s sync-mode dropdown only disables the "Server" option when *another camera already holds it*, never based on whether a recording is in progress. This is reachable today (an operator can already edit any module's config mid-recording with no warning) and becomes actively dangerous once FrameSync election exists, since a `sync_mode` change only takes effect through a camera reconfigure/stream-restart (see `camera_base.py` in the FrameSync item below) — flipping it mid-session would restart that camera's stream mid-recording. Proposed fix: `recording.py`'s `_busy_modules()` already computes "module IDs currently in an ACTIVE session" (used today only for the session-overlap check, itself flagged elsewhere as unlocked) but is private to `RecordingManager` and not exposed via `facade.py`. Add a `facade.get_busy_module_ids()` (or `is_module_recording(module_id)`) built on it, gate the module-config-write handlers above on it (reject with a clear error rather than silently no-op), and mirror the same check in the frontend to disable the settings form outright during an active session — same pattern already used for the disabled-server-option case in `CameraConfigCard.jsx`. Whether `save_controller_config` should also be gated fleet-wide (any session ACTIVE anywhere) rather than per-module is a separate call — changing export/Samba config mid-recording could break the active export pipeline too, so leaning toward yes, but worth confirming before building rather than assuming.
- [ ] **`web.py:400`: broken timestamp format in legacy `send_command start_recording` path** (found 2026-07-09 review) — `strftime("%Y%M%d_%H%m%s")` has month/minute swapped and non-portable `%s`; should be `"%Y%m%d_%H%M%S"`.
- [ ] **`web.py`: NAS exported-recordings listing is broken** (found 2026-07-09 review) — `get_nas_recordings()` scans `/mnt/nas` but calls `mount_nas()` which mounts at `/mnt/controller_export`, so the scan never sees the mount. Also logs one INFO line per file found — a journal flood on large shares. Feature appears dead; either fix the mount point or remove it.
- [ ] **`pyproject.toml` hygiene** (found 2026-07-09 review) — `requires-python = ">=3.8"` is false (code needs 3.11+); `pytest` is a runtime dependency (belongs in dev extras only). (The `[tool.ruff]` deprecated top-level `select`/`ignore` part of this is now fixed — moved to `[tool.ruff.lint]`.) 2026-07-31 review: `[tool.mypy] python_version` is separately still `"3.8"` — a distinct stale claim from the `requires-python` one, since ruff's `target-version` was already fixed to `py311` but mypy's own target version check was missed.
Expand Down Expand Up @@ -308,6 +309,11 @@ These are larger structural issues that require significant refactoring. Recorde
- **TTL/event timeseries plot** — genuinely from zero. No charting library exists anywhere in the frontend (`package.json` has no Chart.js/Plotly/D3/recharts); picking one and building a first timeseries component is its own small design decision, independent of the video work.
Recommendation if this moves forward: ship video-layout compositing as its own PR first (it already has a working prototype and reuses nothing speculative), then treat spectrogram overlay and TTL charting as separate follow-on PRs — bundling all three into one v1 risks stalling on the two components that don't have a head start.

- [ ] **Camera FrameSync: replace the manual server/client dropdown with a tickbox + controller-side election** (proposed 2026-08-05) — today `camera.sync_mode` (`none`/`server`/`client`, libcamera software sync — see the Camera framesync Hardware gotcha, unrelated to PTP) is a per-camera dropdown in `CameraConfigCard.jsx`, and the fleet-wide invariant "at most one server" is enforced only cosmetically: the dropdown disables the "Server" option once another camera holds it (`CameraConfigCard.jsx:174-175,436-437`), but nothing stops a raw `save_module_config` socket event from setting `sync_mode: "server"` on two modules — module and controller both accept it silently. Idea: a single "Enable FrameSync" checkbox per camera; the controller elects exactly one enabled+online camera as the transmitter (server) and configures the rest as receivers (client), rather than the operator picking a role directly.
- **Feasible with mostly-existing plumbing**: the controller already sees every camera's live `sync_mode` via `ModuleConfigState.true_config` (`modules.py`), and `web.py:895-944` already does "elect one, push differently to the rest" — it propagates fps/sensor_mode_index from the server to every client whenever either side's config is saved. Auto-election is a natural extension of that same code path (pick the transmitter, push `sync_mode` itself instead of just fps/sensor_mode_index).
- **Re-election on transmitter dropout is tolerable (agreed 2026-08-05), provided it can't produce two transmitters.** The risk: the elected transmitter drops (heartbeat timeout — already debounced, see the `modules.py` online/offline hysteresis fix above), the controller elects a replacement among the survivors and pushes `sync_mode: server` to it — but the original module's *own local config* still says `server` from before it dropped, and nothing corrects it until the controller explicitly reconciles. If it reconnects before that reconciliation happens, both modules believe they're the transmitter simultaneously. Fix: module reconnection (`module_back_online`, see "Session state has no durability" above) must explicitly re-push the controller's *currently-elected* sync role as part of reconciliation — not just resume recording state — so a recovering module is corrected before it can act on its stale local role.
- **Interacts with, but isn't solved by, the config-lock gap above.** `sync_mode` only takes effect through a camera reconfigure/stream-restart, not a live control (`camera_base.py:279-309,419-437`), so both a manual tickbox flip and an automatic re-election briefly restart the affected camera's stream. Blocking operator-initiated config changes during an active session doesn't prevent re-election-on-dropout — that's a controller-driven reaction to a module going offline, not an operator save — so this needs its own accepted-tradeoff note: a transmitter dropping mid-recording already means that camera's own recording is interrupted, so restarting the newly-elected transmitter's stream at the same moment is a bounded, already-degraded situation, not a new failure mode this feature introduces.

### Tests

- [x] **Config merge has no unit tests** — `_merge_defaults`, `_merge_dicts`, `_merge_internal_defaults`, and `reset_to_defaults` are all untested; add `pytest` cases covering each merge path and edge cases (stale keys, `_`-prefix re-application).
Expand Down
Loading