diff --git a/CHANGELOG.md b/CHANGELOG.md index 55b95c1..cb5703f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.3.1] - 2026-07-29 — Fix OSC send-failure misclassification + +See [specs/planning/T069-fix-osc-send-failure-misclassification.md](specs/planning/T069-fix-osc-send-failure-misclassification.md). + +### Fixed + +* **`FadeMotion::evalAndSend`** (`src/motion/FadeMotion.cpp`): `lo_send()` returns the + number of bytes sent (a positive, non-zero int) on success and `-1` on failure — never + `0`. The failure check used `ret != 0`, which misclassified every successful send as a + failure and killed any fade with `duration_ms` greater than ~25ms once + `kOscFailureThreshold` (5 consecutive ticks) was reached. Changed to `ret < 0`. +* Documented the `OscSendFn` return-value contract on both declarations + (`src/motion/FadeMotion.h`, `src/motion/MotionRegistry.h`) to prevent test mocks (or + future callers) from reintroducing the POSIX-style "0 = success" assumption. +* Added a `WARNING` log on individual OSC send failures in `FadeMotion::evalAndSend`, and + a `DEBUG` log in `MotionRegistry::tick` when a motion recovers from a transient failure + streak — prior visibility was limited to the terminal `MotionError:"osc_send_failed"`. + ## [0.3.0] - 2026-05-13 — Phase H: OSC Input Transport Replaces the NNG bus-client inbound command transport with a localhost UDP OSC listener. diff --git a/CLAUDE.md b/CLAUDE.md index 6292635..c4879c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,44 +1,26 @@ -# gradient-motion-engine Development Guidelines +# gradient-motion-engine -Auto-generated from all feature plans. Last updated: 2026-04-23 +Part of the **CUEMS** ecosystem — see the [`cuems-RELATIONS`](https://github.com/stagesoft/cuems-RELATIONS) repo for the system index, architecture diagram, and protocol/port map. -## Active Technologies -- C++17 (GCC, `-Wall -O3 -pthread`) + None (C++ standard library only — ``, ``, ``, ``, ``) (002-gradient-curves) -- C++17 (GCC, `-Wall -O3 -pthread`) + mtcreceiver v2.0.0 (submodule, pinned at `59fc76e`), (004-adapt-mtc-tick-v2) -- N/A (in-memory adapter; no persistence) (004-adapt-mtc-tick-v2) -- C++17 (GCC, `-Wall -O3 -pthread`) + NNG 1.10.1 (`libnng-dev`, C API — `nng_bus0_open`, (005-nng-bus-client) -- N/A — all state is in-memory. The queue is a fixed-size array; (005-nng-bus-client) -- C++17 (GCC, `-Wall -O3 -pthread`) + liblo (OSC sending), NNG 1.10.1 (already linked), nlohmann-json (already linked), RtMidi via mtcreceiver submodule (already linked) (006-fade-registry-tick-loop) -- N/A — all state in-memory (`std::unordered_map` inside `FadeRegistry`, fixed SPSC queue for status) (006-fade-registry-tick-loop) +## Role -- C++17 (GCC, `-Wall -O3 -pthread`) (001-phase0-scaffold) +Timecode-driven motion and gradient evaluation engine with OSC output — runs as the daemon **`gradient-motiond`** (unit wired by cuems-common). C++17 (GCC, `-Wall -O3 -pthread`). MTC-synced via the `mtcreceiver` submodule; receives commands from the CUEMS engine over localhost UDP OSC and sends OSC out, both via `liblo`. Also uses nlohmann-json. -## Project Structure +**Engine-side client:** `cuems-engine`'s `GradientClient` (`players/GradientClient.py`) is a fire-and-forget UDP OSC client targeting `gradient_osc_port` (7100 in `settings.xml`). Commands: `/gradient/start_fade`, `/gradient/cancel_motion `, `/gradient/cancel_all`. The engine delegates cue fades here (loop-cue fades, ActionCue fades via `ActionHandler`) — the engine's loop only supervises; the fade curve itself is evaluated by this daemon. -```text -src/ -tests/ -``` +## Active technologies (per feature history) -## Commands +- C++17 (GCC, `-Wall -O3 -pthread`), C++ standard library only for the core (``, ``, ``, ``, ``) — `001-phase0-scaffold`, `002-gradient-curves`. +- `mtcreceiver` v2.0.0 (submodule) — `004-adapt-mtc-tick-v2`. +- liblo (OSC), nlohmann-json, RtMidi via mtcreceiver — `006-fade-registry-tick-loop`. All state in-memory (`MotionRegistry` map + fixed SPSC command queue). +- liblo UDP OSC listener (`OscServer`, `127.0.0.1:`) as the inbound transport — `007-osc-input-transport`. Superseded the NNG bus client of `005-nng-bus-client`; `libnng` is **no longer a build or runtime dependency** (removed in v0.3.0, commit `538d992`), and the outbound NNG status channel is gone — motion status events are logged only. -# Add commands for C++17 (GCC, `-Wall -O3 -pthread`) +## Build & release -## Code Style +Standard C++ submodule build (`git submodule update --init`, cmake/make). Release lineage: `rc_1` carries the fleet-wide MTC >24h work + `24h_extended_support` tag (tip `069f951`, 24h `mtcreceiver` `8a30d05`); `main` is the development line. -C++17 (GCC, `-Wall -O3 -pthread`): Follow standard conventions +For additional per-feature context (project structure, shell commands), read the current plan at [specs/007-osc-input-transport/plan.md](specs/007-osc-input-transport/plan.md). Non-code artifacts follow the same `specs/planning/` convention as cuems-utils. -## Recent Changes -- 006-fade-registry-tick-loop: Added C++17 (GCC, `-Wall -O3 -pthread`) + liblo (OSC sending), NNG 1.10.1 (already linked), nlohmann-json (already linked), RtMidi via mtcreceiver submodule (already linked) -- 005-nng-bus-client: Added C++17 (GCC, `-Wall -O3 -pthread`) + NNG 1.10.1 (`libnng-dev`, C API — `nng_bus0_open`, -- 004-adapt-mtc-tick-v2: Added C++17 (GCC, `-Wall -O3 -pthread`) + mtcreceiver v2.0.0 (submodule, pinned at `59fc76e`), +## Field notes - - - - - -For additional context about technologies to be used, project structure, -shell commands, and other important information, read the current plan: -[specs/007-osc-input-transport/plan.md](specs/007-osc-input-transport/plan.md) - +- Uses the shared `mtcreceiver` submodule — the 2s-resync-skip fix (`aa44894`) and the raw-wire-MTC timebase apply here too; see the mtcreceiver CLAUDE.md. diff --git a/CMakeLists.txt b/CMakeLists.txt index 8dc76f9..4169743 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,7 +25,7 @@ cmake_minimum_required(VERSION 3.10) # Allow target_link_libraries() on targets defined in other directories # (needed to link cuemslogger into mtcreceiver from the root CMakeLists) cmake_policy(SET CMP0079 NEW) -project(gradient-motion-engine VERSION 0.3.0 LANGUAGES CXX) +project(gradient-motion-engine VERSION 0.3.1 LANGUAGES CXX) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -39,6 +39,11 @@ add_compile_options(-Wall -O3 -pthread) # =========================================================== option(ENABLE_CUEMS_LOGGER "Build with CuemsLogger syslog backend" ON) option(BUILD_DAEMON "Build the gradient-motiond daemon (requires rtmidi)" ON) +# Default ON so plain `cmake ..` and the CI TEST/COVERAGE stages keep building +# the suite. The packaging BUILD stage (debian/rules) turns it OFF: a .deb only +# ships the daemon, and the latency-budgeted benchmarks are unfit for shared +# runners. +option(BUILD_TESTS "Build the unit/integration test suite" ON) # =========================================================== # External dependencies via pkg-config @@ -134,8 +139,12 @@ add_subdirectory(src) # =========================================================== # Tests # =========================================================== -enable_testing() -add_subdirectory(tests) +if(BUILD_TESTS) + enable_testing() + add_subdirectory(tests) +else() + message(STATUS "BUILD_TESTS=OFF — test suite not configured") +endif() # =========================================================== # Version header (generated from project VERSION above) @@ -176,10 +185,9 @@ if(BUILD_DAEMON) target_include_directories(gradient-motiond PRIVATE ${LIBLO_INCLUDE_DIRS}) endif() - if(ENABLE_CUEMS_LOGGER) - target_compile_definitions(gradient-motiond PRIVATE HAVE_CUEMS_LOGGER) - target_link_libraries(gradient-motiond PRIVATE cuemslogger) - endif() + # HAVE_CUEMS_LOGGER + cuemslogger now arrive via gradient_motion's + # PUBLIC usage requirements (src/CMakeLists.txt) — armed on the library + # so all its objects and every consumer share one logging backend. # Install rules — places the daemon at /bin/gradient-motiond. # debian/rules sets CMAKE_INSTALL_PREFIX=/usr so the .deb ships diff --git a/cuemslogger b/cuemslogger index c39e2d8..6463cd9 160000 --- a/cuemslogger +++ b/cuemslogger @@ -1 +1 @@ -Subproject commit c39e2d82fedf82ce997e490735a85863e096ae21 +Subproject commit 6463cd90a682b2b66fd5a4d11f94987e9e43303e diff --git a/daemon/comms/OscServer.cpp b/daemon/comms/OscServer.cpp index 8687ce2..8522107 100644 --- a/daemon/comms/OscServer.cpp +++ b/daemon/comms/OscServer.cpp @@ -40,8 +40,8 @@ struct OscServer::Impl { bool started = false; static void errorHandler(int num, const char* msg, const char* where) { - std::fprintf(stderr, "ERROR OscServer: liblo error %d — %s (path: %s)\n", - num, msg ? msg : "", where ? where : ""); + GME_LOG_ERROR("OscServer: liblo error " + std::to_string(num) + " — " + + (msg ? msg : "") + " (path: " + (where ? where : "") + ")"); } // Per-address callback entry point (called on the liblo network thread). @@ -84,11 +84,11 @@ struct OscServer::Impl { break; } } catch (const std::exception& e) { - std::fprintf(stderr, "ERROR OscServer: exception in callback for %s: %s\n", - path, e.what()); + GME_LOG_ERROR("OscServer: exception in callback for " + std::string(path) + + ": " + e.what()); } catch (...) { - std::fprintf(stderr, "ERROR OscServer: unknown exception in callback for %s\n", - path); + GME_LOG_ERROR("OscServer: unknown exception in callback for " + + std::string(path)); } return 0; // 0 = handled; do not try further methods } @@ -125,8 +125,7 @@ bool OscServer::start() { impl_->server_thread = lo_server_thread_new(port_str.c_str(), Impl::errorHandler); if (!impl_->server_thread) { - std::fprintf(stderr, "FATAL OscServer: failed to bind UDP port %s\n", - port_str.c_str()); + GME_LOG_CRITICAL("OscServer: failed to bind UDP port " + port_str); return false; } @@ -143,7 +142,7 @@ bool OscServer::start() { Impl::onMessage, impl_.get()); if (lo_server_thread_start(impl_->server_thread) != 0) { - std::fprintf(stderr, "FATAL OscServer: lo_server_thread_start failed\n"); + GME_LOG_CRITICAL("OscServer: lo_server_thread_start failed"); lo_server_thread_free(impl_->server_thread); impl_->server_thread = nullptr; return false; diff --git a/daemon/comms/OscServer.h b/daemon/comms/OscServer.h index c59215b..8abf8ae 100644 --- a/daemon/comms/OscServer.h +++ b/daemon/comms/OscServer.h @@ -26,7 +26,8 @@ namespace comms { * wire contract, parses them via `parseFadeOscCommand`, and on `ParseResult::Ok` * pushes to the `LockFreeQueue` supplied at construction. * - * Namespace: `gme::daemon::comms` (matches NngBusClient convention). + * Namespace: `gme::daemon::comms` — the convention for daemon-specific + * inbound-transport clients. * * ## Threading * diff --git a/debian/changelog b/debian/changelog index 4c5a3d1..cc17494 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,13 @@ +cuems-gradient-motiond (0.3.1-1) unstable; urgency=medium + + * Fix FadeMotion::evalAndSend misclassifying every successful lo_send() + as a failure (ret != 0 instead of ret < 0), which silently killed + every fade with duration_ms > ~25ms via the 5-consecutive-failure + threshold. Add OscSendFn return-value contract docs and warning-level + logging on individual send failures. + + -- Adrià Masip Wed, 29 Jul 2026 19:00:53 +0200 + cuems-gradient-motiond (0.3.0-1) unstable; urgency=medium * Phase H — OSC Input Transport (spec 007-osc-input-transport). diff --git a/debian/rules b/debian/rules index aacbcd5..5978129 100755 --- a/debian/rules +++ b/debian/rules @@ -6,9 +6,44 @@ export DEB_BUILD_MAINT_OPTIONS = hardening=+all %: dh $@ +# --- Submodule guard ------------------------------------------------------- +# The daemon links mtcreceiver and cuemslogger, which live in git submodules. +# On a runner that checked out without `submodules: recursive` CMake aborts +# midway with a FATAL_ERROR; fail up front with the actual remedy instead. override_dh_auto_configure: + @for m in mtcreceiver cuemslogger; do \ + test -f "$$m/CMakeLists.txt" || { \ + echo "ERROR: submodule '$$m' is missing from the source tree."; \ + echo " Run 'git submodule update --init --recursive', or"; \ + echo " check out with actions/checkout submodules: recursive."; \ + exit 1; }; \ + done dh_auto_configure -- \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_INSTALL_PREFIX=/usr \ -DBUILD_DAEMON=ON \ -DBUILD_TESTS=OFF + +# --- Packaging BUILD stage: no tests here --------------------------------- +# In the CI/CD pipeline the suite is exercised in dedicated, earlier stages +# (TEST -> COVERAGE) against the same tree; this BUILD stage only has to emit +# a deterministic .deb. Running dh_auto_test here would both duplicate that +# work and fail non-deterministically on shared/virtualized runners (GitHub +# Actions et al.): `test_motion_registry_bench` and `bench_osc_latency` are +# latency-budgeted, the MTC tests want an ALSA sequencer that a container may +# not expose, and `bench_osc_latency` writes its results back into the source +# tree (tests/bench_results/), which dirties a packaging build. +# +# Note this override is what actually skips the run: -DBUILD_TESTS=OFF above +# keeps the suite from being *compiled*, and this keeps it from being *run*, +# so the stage stays correct even if the suite is re-enabled locally. +# +# Escape hatch: `RUN_DEB_TESTS=1 dpkg-buildpackage ...` forces the suite for a +# local package build. An explicit `DEB_BUILD_OPTIONS=nocheck` still wins, +# because dh_auto_test honours it internally. +override_dh_auto_test: +ifeq ($(RUN_DEB_TESTS),1) + dh_auto_test +else + @echo "dh_auto_test: skipped in packaging BUILD stage — tests run in the CI TEST/COVERAGE stages (set RUN_DEB_TESTS=1 to force)" +endif diff --git a/mtcreceiver b/mtcreceiver index 59fc76e..20ab95b 160000 --- a/mtcreceiver +++ b/mtcreceiver @@ -1 +1 @@ -Subproject commit 59fc76edb4ded90296c1aa904a9540c1aabf2ab5 +Subproject commit 20ab95b96baa9693a6c59e786a1bbd7b27beaae0 diff --git a/specs/planning/T069-fix-osc-send-failure-misclassification.md b/specs/planning/T069-fix-osc-send-failure-misclassification.md new file mode 100644 index 0000000..486074c --- /dev/null +++ b/specs/planning/T069-fix-osc-send-failure-misclassification.md @@ -0,0 +1,288 @@ + + +# T069 — Fix OSC Send-Failure Misclassification in `FadeMotion::evalAndSend` + +**Issued by**: cuems-engine debugging session (`fix/gradient-engine-action` branch) +**Target repo**: `gradient-motion-engine` +**Primary file**: `src/motion/FadeMotion.cpp` +**Context version**: v0.3.0 (`debian/changelog` top entry `cuems-gradient-motiond (0.3.0-1)`) +**Date**: 2026-07-29 +**Status**: plan — not yet applied. To be validated per the checklist below, then folded into the package (changelog entry + version bump) once green. + +--- + +## Background + +While debugging a separate, already-fixed `node_name`-filter bug in cuems-engine +(`GradientClient` was sending the node's UUID instead of the daemon's +`--node-name`), we confirmed the daemon now correctly *receives* +`/gradient/start_fade`. But the downstream OSC tick to the target player +(`osc_host:osc_port:osc_path`) never arrives, or arrives for only a handful of +ticks before the fade silently dies. + +### Root cause (verified, not hypothesized) + +`FadeMotion::evalAndSend` (`src/motion/FadeMotion.cpp:81-89`): + +```cpp +const int ret = oscSend_(osc_target_, osc_path.c_str(), value); // -> lo_send() +... +r.failed = (ret != 0); +``` + +`oscSend_` defaults to `gme::osc::sendFloat` (`src/osc/OscSender.cpp:20-24`), +which forwards directly to liblo's `lo_send()`. Per `/usr/include/lo/lo.h:128`, +`lo_send` returns **`-1` on failure**; on success it returns the number of +bytes sent — **never `0`**. This was verified empirically, not just read from +the header comment: + +```c +// compiled against the installed liblo.so.7 (0.32-2) +lo_address addr = lo_address_new("127.0.0.1", "9999"); +int ret = lo_send(addr, "/test/path", "f", 0.5f); +printf("lo_send returned: %d\n", ret); // -> 20 +``` + +So `ret != 0` is **true on every successful send**, not just on failures. +`MotionRegistry::tick` (`src/motion/MotionRegistry.cpp:173-186`) increments +`m.consecutive_osc_failures` whenever `r.failed`, and at +`kOscFailureThreshold = 5` (`src/motion/MotionRegistry.h:74-81`, "at 200 Hz +this equals 25 ms") emits `MotionError:"osc_send_failed"` and removes the +motion. Net effect: **every fade with `duration_ms` greater than ~25 ms is +killed almost immediately after starting**, having sent only a handful of +ticks near `start_value`. The daemon logs this at INFO level — confirmed live +against `cuems-gradient-motiond` on the test box via +`journalctl -u cuems-gradient-motiond`, which showed +`GradientEngine: MotionError motion_id=... reason=osc_send_failed` within +~25 ms of the corresponding `start_fade`. + +### Why the existing test suite didn't catch it + +Every `OscSendFn` mock in `tests/test_fade_motion.cpp` and +`tests/test_motion_registry.cpp` uses the **opposite, POSIX-style** +convention — `return 0;` for success, `return -1;` for injected failure +(e.g. `tests/test_fade_motion.cpp:150,167,189,213,228,321,361,380,398,496,510,524` +and the explicit failure-injection tests at `:443-446,470-473`). No existing +test ever passes a positive "success" value through the send function, so +nothing exercises the real liblo return convention. `MotionRegistry.h:93` / +`FadeMotion.h:65` declare `OscSendFn = std::function` with no documented return-value contract — that omission is +the actual gap that let the mismatch through code review. + +### Fix + +`FadeMotion.cpp:88`: `r.failed = (ret != 0);` → `r.failed = (ret < 0);` + +--- + +## Execution checklist + +### 1. Checkout + +```bash +cd /disk/Projects/StageLab/gradient-motion-engine +git status # confirm clean tree before starting +git checkout -b fix/osc-send-failure-threshold-t069 +``` + +### 2. Regression test FIRST (TDD — confirm it fails before the fix) + +Add to `tests/test_fade_motion.cpp`, in the US4 section (after +`test_us4_transient_failure_recovery`, ~line 487), following the existing +`TestCtx`/`ASSERT_TRUE` idiom: + +```cpp +static bool test_us4_real_lo_send_success_not_misclassified() { + // Regression for T069: liblo's lo_send() returns bytes-sent (a positive, + // non-zero int) on success, never 0. A mock that mirrors this — instead + // of the POSIX-style "0 = success" convention every other test in this + // file uses — must NOT trip the failure counter. + TestCtx ctx; + auto send = [](lo_address, const char*, float) -> int { return 20; }; + auto reg = ctx.makeRegWithSend(send); + + auto cmd = TestCtx::makeCmd("freal", "linear", 0.0f, 1.0f, 10000.0f, 0); + reg->apply(cmd); + + for (int i = 0; i < gme::motion::MotionRegistry::kOscFailureThreshold; ++i) { + reg->tick(i * 10); + } + + ASSERT_TRUE(reg->size() == 1, + "us4_real_lo_send: fade must survive N ticks of a real " + "liblo-style positive return value"); + + for (auto& r : ctx.emitted) + ASSERT_TRUE(r.reason != "osc_send_failed", + "us4_real_lo_send: no spurious osc_send_failed"); + return true; +} +``` + +Register it in `main()`'s `tests[]` array (~line 468): +```cpp +{"us4_real_lo_send_not_misclassified", test_us4_real_lo_send_success_not_misclassified}, +``` + +Build and run — **this test must FAIL on the current code** (proves the +regression test actually exercises the bug before touching production code): + +```bash +cmake -B build -DCMAKE_BUILD_TYPE=Debug +cmake --build build -j"$(nproc)" +ctest --test-dir build --output-on-failure -R test_fade_motion +``` +Expected: `FAIL [us4_real_lo_send: ...]` and the ctest case for +`test_fade_motion` reports failure. If it passes before the fix, stop — +something about the reproduction is wrong and the fix must not proceed on a +false premise. + +### 3. Apply the fix + +`src/motion/FadeMotion.cpp:88`: +```diff +- r.failed = (ret != 0); ++ r.failed = (ret < 0); +``` + +Re-run the same build/test command. All of `test_fade_motion` (including the +new case and the two pre-existing US4 tests, which still use the POSIX-style +mock and must keep passing since `-1 < 0` is still `true`) must pass: + +```bash +cmake --build build -j"$(nproc)" +ctest --test-dir build --output-on-failure +``` + +Run the **full** suite, not just `test_fade_motion` — confirm no other test +implicitly depended on the old (buggy) comparison: +```bash +ctest --test-dir build --output-on-failure +``` + +### 4. Document the return-value contract (prevents recurrence) + +Add a one-line contract note to the `OscSendFn` declarations so the next +person wiring a mock doesn't repeat the mismatch: + +- `src/motion/FadeMotion.h:65` (near the `OscSendFn` typedef) +- `src/motion/MotionRegistry.h:93` (same) + +```cpp +// Return convention matches liblo's lo_send(): >= 0 (bytes sent) on +// success, < 0 on failure. NEVER assume 0 == success — lo_send never +// returns exactly 0 for a non-empty message. +using OscSendFn = std::function; +``` + +### 5. Additional logging + +The current failure path logs only the terminal `MotionError:"osc_send_failed"` +at INFO once the threshold is hit — there is no visibility into individual +tick-level send failures leading up to it, which made this bug hard to +distinguish from "daemon never received the command at all" during the +original investigation. Add: + +- **`FadeMotion::evalAndSend`** (`FadeMotion.cpp`, right after the `oscSend_` + call): on `ret < 0`, emit a `GME_LOG_WARNING` (via a callback or a direct + include of `logging.h` — check whether `FadeMotion` currently has any + logging dependency; if not, prefer threading a lightweight log callback + through `MotionRegistry::tick` rather than adding a hard dependency from + `src/motion/` on `daemon/`'s logging, to keep the library/daemon split + intact per `GradientEngine.cpp`'s comment about `libgradient_motion` vs the + daemon binary) including `motion_id`, `osc_path`, and the raw `ret` value: + ``` + WARNING FadeMotion: osc send failed motion_id= path= lo_send_ret= + ``` + Rate-limit or dedupe if this proves noisy in practice (a real dead target + will log this every tick for `kOscFailureThreshold` ticks before removal — + 5 lines is fine, no rate-limiting needed at that volume). +- **`MotionRegistry::tick`** (`MotionRegistry.cpp:173-186`): when a motion + recovers from a nonzero `consecutive_osc_failures` count back to 0 (i.e. + transient failure that self-healed), log at DEBUG: + `DEBUG MotionRegistry: motion_id= osc send recovered after failures`. + This distinguishes "always worked" from "flaked and recovered" in the + journal without adding INFO-level noise to the common case. + +### 6. Version bump + +Source of truth: `CMakeLists.txt:28` → `project(gradient-motion-engine VERSION 0.3.0 ...)`. +This is a bug fix, not a feature — bump the patch component per the existing +convention (`git log` shows prior bumps as a single commit touching +`CMakeLists.txt` + `debian/changelog` + `CHANGELOG.md` together): + +- `CMakeLists.txt:28`: `VERSION 0.3.0` → `VERSION 0.3.1` +- `debian/changelog`: new top entry: + ``` + cuems-gradient-motiond (0.3.1-1) unstable; urgency=medium + + * Fix FadeMotion::evalAndSend misclassifying every successful lo_send() + as a failure (ret != 0 instead of ret < 0), which silently killed + every fade with duration_ms > ~25ms via the 5-consecutive-failure + threshold. Add OscSendFn return-value contract docs and warning-level + logging on individual send failures. + + -- + ``` +- `CHANGELOG.md`: new `## [0.3.1] - 2026-07-29` entry under a `### Fixed` + heading, referencing this plan file (`specs/planning/T069-...md`). + +### 7. Build the package + +Per `debian/rules` (standard `dh $@`, configured via +`override_dh_auto_configure` with `-DBUILD_TESTS=OFF` for the release build): + +```bash +cd /disk/Projects/StageLab/gradient-motion-engine +dpkg-buildpackage -us -uc -b +``` + +This produces `../cuems-gradient-motiond_0.3.1-1_amd64.deb` (and the +`-dbgsym` companion) in the parent directory, matching the naming pattern of +the currently-installed `cuems-gradient-motiond_0.3.0-1_amd64.deb`. + +### 8. Pre-deploy validation (before this file is "passed into the package") + +- [ ] `ctest --test-dir build --output-on-failure` — full suite green, + including the new regression test. +- [ ] New regression test fails on a clean revert of the one-line fix + (sanity-check the test isn't vacuously passing). +- [ ] `dpkg-buildpackage` completes without error; `dpkg -c` on the resulting + `.deb` shows the updated `/usr/bin/gradient-motiond`. +- [ ] Manual smoke test on the dev/test box (`10.16.10.3`, per prior session): + install the new `.deb`, restart `cuems-gradient-motiond`, replay the + same raw `/gradient/start_fade` probe used earlier in this + investigation (correct `node_name`, `duration_ms` well above 25ms — + e.g. 5000), and confirm via `journalctl -u cuems-gradient-motiond + --since ` that `MotionComplete` appears **after** roughly + `duration_ms` has elapsed (not ~25ms later), with **no** + `MotionError:"osc_send_failed"` in between. + - [ ] Independently confirm the downstream player actually received the + ticks (e.g. a throwaway UDP listener on the probe's `osc_port`, or + observing the real player's parameter change) — the journal check + alone only proves the daemon *thinks* it succeeded now, which is + exactly the class of assumption that caused this bug in the first + place; corroborate with an actual received packet. + +### 9. Rollback plan + +If the fix regresses something unexpected: `dpkg -i` the previously-built +`cuems-gradient-motiond_0.3.0-1_amd64.deb` (already present in +`/disk/Projects/StageLab/`) and restart the service. No schema/data +migration is involved — this is a pure logic fix with no persistent state, +so rollback is a straight binary swap. + +--- + +## Related artifacts + +| Artifact | Location | +|---|---| +| Daemon source of the bug | `src/motion/FadeMotion.cpp:81-89` | +| Threshold logic | `src/motion/MotionRegistry.cpp:173-186`, `MotionRegistry.h:74-81` | +| liblo contract | `/usr/include/lo/lo.h:122-130` | +| Existing test harness pattern | `tests/test_fade_motion.cpp` (`TestCtx`, `test_us4_osc_failure_threshold`) | +| cuems-engine node_name fix (separate, already applied) | cuems-engine `src/cuemsengine/players/GradientClient.py`, `NodeEngine.py::_resolve_gradient_node_name` | diff --git a/specs/planning/T070-cicd-github-actions-suite.md b/specs/planning/T070-cicd-github-actions-suite.md new file mode 100644 index 0000000..c4ff3fb --- /dev/null +++ b/specs/planning/T070-cicd-github-actions-suite.md @@ -0,0 +1,336 @@ + + +# T070 — Layered CI/CD Suite (GitHub Actions): TEST → COVERAGE → DOCUMENT → BUILD → DEPLOY + +**Issued by**: packaging session following [T069](T069-fix-osc-send-failure-misclassification.md) +**Target repo**: `gradient-motion-engine` +**Primary files**: `.github/workflows/*.yml`, `tests/CMakeLists.txt`, `README.md` +**Context version**: v0.3.1 (`debian/changelog` top entry `cuems-gradient-motiond (0.3.1-1)`) +**Date**: 2026-07-29 +**Status**: plan — not yet applied. +**Prerequisite (already landed)**: commit `9f1e0be` *"build(deb): make packaging build runner-safe; add real BUILD_TESTS option"* — the BUILD stage below depends on it. + +--- + +## Background + +The repo currently has two disconnected workflows: + +| File | Does | Gap | +|---|---|---| +| `.github/workflows/tests.yml` | Build (Debug, `--coverage`) → modprobe ALSA → `ctest` (all 9 tests) → lcov → Codecov | Conflates TEST and COVERAGE; gates on the latency benchmarks, which are unfit for shared runners | +| `.github/workflows/docs.yml` | mkdocs + Doxygen → GitHub Pages | Runs independently of test outcome; can publish docs for a tree whose tests failed | + +There is **no BUILD stage and no DEPLOY stage** — `.deb` packages are today built by hand on a workstation (that is how `cuems-gradient-motiond_0.3.1-1_amd64.deb` was produced) and moved to the fleet by hand. Nothing verifies that the version in `CMakeLists.txt`, `debian/changelog`, and `CHANGELOG.md` agree, which is exactly the kind of three-file bump T069 §6 performed manually. + +The goal is a single layered pipeline where each stage gates the next. + +--- + +## Constraints discovered in this repo (do not re-derive these) + +1. **Two tests are latency-budgeted and will flake on shared runners.** + `bench_osc_latency` (label `bench`) and `test_motion_registry_bench` (label + `integration`) assert p99 budgets that a noisy-neighbour GitHub runner + misses. They must be collected, not gated on. +2. **`test_motion_registry_bench` is mislabelled.** It carries + `LABELS "integration"` (`tests/CMakeLists.txt:179`) despite being a + benchmark, so `ctest -LE bench` does *not* exclude it. Relabel it to + `bench`; that makes label-based gating actually work and leaves + `integration` meaning only `test_osc_server_integration`, which is a + legitimate gate. +3. **`bench_osc_latency` writes into the source tree.** + `BENCH_RESULTS_FILE="${CMAKE_SOURCE_DIR}/tests/bench_results/osc_latency.txt"` + (`tests/CMakeLists.txt:154`) — running it leaves the checkout dirty. That + file is dirty in the working tree right now for exactly this reason. +4. **Submodules are mandatory.** `mtcreceiver` and `cuemslogger` are needed by + the daemon; CMake hard-fails without them (`CMakeLists.txt:70-84`). Every + job needs `submodules: recursive`. +5. **`DOXYGEN_WARN_AS_ERROR YES`** (`CMakeLists.txt:114`) — the docs target is + already a strict gate; use it as one. +6. **Badge URLs are pinned to workflow filenames.** `README.md:15` points at + `actions/workflows/tests.yml/badge.svg` and `:17` at `docs.yml`. Renaming + or deleting either file silently breaks the badge. See §Decisions. +7. **The fleet is Debian, the default runner is Ubuntu.** `${shlibs:Depends}` + resolves against the *build* distro, so an `ubuntu-latest` build stamps + Ubuntu-flavoured dependencies (`liblo7`, `librtmidi6` version ranges) into + a package destined for Debian nodes. The BUILD stage must run in a + `debian:bookworm` container. +8. **`DEB_BUILD_MAINT_OPTIONS = hardening=+all`** is already set in + `debian/rules`; no runner-side hardening flags are needed. + +--- + +## Target pipeline + +``` + ┌────────────────────────────────────┐ + push / PR ──────▶│ 1. TEST (gating, fast) │ + └───────────────┬────────────────────┘ + │ needs + ┌──────────────────────┼──────────────────────┐ + ▼ ▼ ▼ + ┌──────────────────┐ ┌───────────────────┐ ┌────────────────────┐ + │ 2. COVERAGE │ │ 3. DOCUMENT │ │ 2b. BENCH │ + │ lcov → Codecov │ │ Doxygen + mkdocs │ │ non-gating, upload │ + │ + coverage badge │ │ + badges │ │ results as artifact│ + └────────┬─────────┘ └─────────┬─────────┘ └────────────────────┘ + └──────────┬───────────┘ + ▼ + ┌──────────────────────────────┐ + │ 4. BUILD (debian:bookworm) │ + │ dpkg-buildpackage → .deb │ + │ + version-consistency guard │ + └──────────────┬───────────────┘ + ▼ (tag v* / release published only) + ┌──────────────────────────────┐ + │ 5. DEPLOY │ + │ attach .deb to GH Release │ + └──────────────────────────────┘ +``` + +Stages 2, 2b and 3 run in parallel once TEST is green. DEPLOY is the only +stage restricted by trigger. + +--- + +## Execution checklist + +### 0. Branch + +```bash +git checkout main && git pull +git checkout -b feat/cicd-pipeline-t070 +``` + +### 1. Make label-based gating meaningful (prerequisite edit) + +`tests/CMakeLists.txt:179`: + +```diff +- set_tests_properties(test_motion_registry_bench PROPERTIES LABELS "integration") ++ # Latency-budgeted: grouped with the other benchmark so CI can gate on ++ # `ctest -LE bench` and collect these separately (see T070). ++ set_tests_properties(test_motion_registry_bench PROPERTIES LABELS "bench") +``` + +Verify the split is what the pipeline expects: + +```bash +cmake -B build && ctest --test-dir build -N -LE bench # expect 7 tests +cmake -B build && ctest --test-dir build -N -L bench # expect 2 tests +``` + +### 2. Make the benchmark output path overridable (constraint 3) + +So the bench job does not dirty the checkout. `tests/CMakeLists.txt:153-155`: + +```diff ++set(BENCH_RESULTS_DIR "${CMAKE_SOURCE_DIR}/tests/bench_results" ++ CACHE PATH "Directory bench_osc_latency writes its results to") + target_compile_definitions(bench_osc_latency PRIVATE +- BENCH_RESULTS_FILE="${CMAKE_SOURCE_DIR}/tests/bench_results/osc_latency.txt" ++ BENCH_RESULTS_FILE="${BENCH_RESULTS_DIR}/osc_latency.txt" + ) +``` + +CI then passes `-DBENCH_RESULTS_DIR=${{ runner.temp }}/bench` and uploads that +directory as an artifact. Default behaviour for local developers is unchanged. + +### 3. `.github/workflows/ci.yml` — stages 1–4 + +Replaces `tests.yml`. **Keep the filename decision from §Decisions in mind +before deleting `tests.yml`.** Structure (SPDX header as in the existing +workflows): + +```yaml +name: CI +on: + push: + branches: [main, rc_1] + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + workflow_dispatch: + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read +``` + +**Job `test`** (gating) — `ubuntu-latest`: +- `actions/checkout@v4` with `submodules: recursive` +- apt: `build-essential cmake pkg-config librtmidi-dev liblo-dev nlohmann-json3-dev libtinyxml2-dev` +- `ccache` via `hendrikmuhs/ccache-action@v1` keyed on `${{ runner.os }}-ci` +- `sudo modprobe snd-seq snd-seq-dummy || true` (keep the `|| true` from + `tests.yml:52` — Azure kernels lack the modules; commit `0c2af7b` fixed + precisely this) +- configure `-DCMAKE_BUILD_TYPE=Debug`, build, then + `ctest --test-dir build --output-on-failure -LE bench` + +**Job `bench`** (non-gating) — `needs: test`, `continue-on-error: true`: +- same setup, `-DBENCH_RESULTS_DIR=${{ runner.temp }}/bench` +- `ctest --test-dir build --output-on-failure -L bench` +- `actions/upload-artifact@v4` for the results dir, `if: always()` +- Rationale: a p99 miss on a shared runner is not a defect in this daemon, but + the trend data is worth keeping. Do **not** let this job block the pipeline. + +**Job `coverage`** — `needs: test`: +- lift the lcov + Codecov steps verbatim from `tests.yml:56-77` (the + `--ignore-errors inconsistent` flags and the `/usr/*`, `*/tests/*`, + `*/mtcreceiver/*`, `*/cuemslogger/*` exclusions are already tuned — do not + re-tune them here) +- run coverage over `-LE bench` only, so the flaky benches cannot fail the job +- keep `fail_ci_if_error: false` and the existing `CODECOV_TOKEN` secret + +**Job `docs`** — `needs: test`: +- apt `doxygen`, `pip install mkdocs==1.6.1 mkdocs-material mkdoxy` (pin as + `docs.yml:44` already does) +- `mkdocs build` — with `DOXYGEN_WARN_AS_ERROR YES` this fails on any + undocumented symbol, which is the intended DOCUMENT gate +- upload the site as an artifact; **do not deploy to Pages here** — Pages + deployment stays in `docs.yml` (see §Decisions) + +**Job `package`** — `needs: [coverage, docs]`, `container: debian:bookworm` +(constraint 7): +- `apt-get install -y --no-install-recommends build-essential devscripts + debhelper cmake pkg-config librtmidi-dev liblo-dev nlohmann-json3-dev + libtinyxml2-dev git ca-certificates` +- checkout with `submodules: recursive` (inside a container, `git` must be + installed *before* checkout for submodules to work — install it in a + pre-step or use `actions/checkout@v4`'s bundled fallback) +- **version-consistency guard** (see §4 below) — run this *before* building +- `dpkg-buildpackage -b -us -uc` +- move `../*.deb ../*.changes ../*.buildinfo` into `artifacts/` and upload + +Because commit `9f1e0be` landed, this job needs no test-related workarounds: +`override_dh_auto_test` already skips the suite, `-DBUILD_TESTS=OFF` is now a +real option so the suite is not even compiled, and the submodule guard in +`override_dh_auto_configure` produces a legible error if checkout misbehaves. + +### 4. Version-consistency guard + +A shell step in `package` (and reused by DEPLOY) asserting the three sources of +truth agree — this is the drift T069 §6 had to reconcile by hand: + +```bash +cmake_ver=$(grep -oP 'project\(gradient-motion-engine VERSION \K[0-9.]+' CMakeLists.txt) +deb_ver=$(dpkg-parsechangelog -S Version | cut -d- -f1) +chg_ver=$(grep -oP '^## \[\K[0-9.]+' CHANGELOG.md | head -1) +test "$cmake_ver" = "$deb_ver" && test "$cmake_ver" = "$chg_ver" || { + echo "Version drift: CMakeLists=$cmake_ver debian/changelog=$deb_ver CHANGELOG=$chg_ver"; exit 1; } +``` + +On a tag build, additionally assert `"v$cmake_ver" = "$GITHUB_REF_NAME"`. + +### 5. `.github/workflows/release.yml` — stage 5 (DEPLOY) + +```yaml +on: + push: + tags: ['v*'] +permissions: + contents: write # required to create/attach to the Release +``` + +- Reuse the `package` job (extract it into a reusable workflow called with + `workflow_call` from both `ci.yml` and `release.yml`, so the release artifact + is built by identical steps to the ones CI exercises on every push). +- `softprops/action-gh-release@v2` attaching `*.deb`, `*.changes`, + `*.buildinfo`, with the body generated from the matching `CHANGELOG.md` + section. +- Guard: refuse to publish if the version guard (§4) fails, and if the tag is + not on `main` or `rc_1`. + +### 6. Badges (the DOCUMENT deliverable) + +`README.md:14-17` currently carries License, Tests, codecov, and Docs badges. +Update/extend to match the new suite: + +| Badge | Source | +|---|---| +| License GPLv3 | unchanged (`README.md:14`) | +| CI | `actions/workflows/ci.yml/badge.svg` — **replaces** the `tests.yml` badge | +| codecov | unchanged — already token-pinned (`README.md:16`) | +| Docs | unchanged (`docs.yml`) | +| Release | `https://img.shields.io/github/v/release/stagesoft/gradient-motion-engine` | +| Debian package | `https://img.shields.io/github/v/tag/...?label=deb` or a shields endpoint badge fed by the release asset name | + +Note `cuems-engine` renders its coverage badge via `genbadge` pushed to +`gh-pages/badges` (`ci.yml:112-125` there) because it has no Codecov token; +this repo *does* have one (`README.md:16`), so use the Codecov badge and do +**not** replicate the genbadge/gh-pages mechanism. + +--- + +## Decisions required before implementation + +1. **DEPLOY target.** No APT repository infrastructure exists anywhere in the + ecosystem checkout (searched for `reprepro`/`aptly`/`packagecloud`/ + `cloudsmith`/`deb.cuems` — no hits). This plan therefore assumes **GitHub + Releases as the artifact of record**, with fleet nodes installing via + `dpkg -i` of a downloaded asset, mirroring how `cuems-engine` publishes to + PyPI on `release: published`. If a fleet APT repo is intended instead + (`reprepro` on a Stagelab host, signed with a repo key), stage 5 changes + shape: it needs a GPG signing secret, `dput`/`reprepro includedeb`, and the + `-us -uc` flags in `debian/rules` invocations become real signing. **Confirm + which before implementing stage 5**; stages 1–4 are unaffected either way. +2. **Workflow filename.** `README.md:15` badge is pinned to `tests.yml`. + Either (a) name the new pipeline `tests.yml` to preserve the badge and the + branch-protection rule that likely references the "Tests" check, or + (b) name it `ci.yml` (matching `cuems-engine`) and update both the README + badge *and* any required-status-check config in repo settings in the same + change. Recommendation: **(b)**, for ecosystem consistency — but it requires + a settings touch that CI cannot do for itself. +3. **Pages ownership.** `docs.yml` owns the `pages` concurrency group and the + `github-pages` environment. Keep it as the sole deployer (this plan's `docs` + job only validates and uploads an artifact) to avoid two workflows racing + for the same environment. + +--- + +## Verification checklist + +- [ ] `ctest -N -LE bench` reports 7 tests; `-L bench` reports 2 (§1). +- [ ] `cmake -B build -DBENCH_RESULTS_DIR=/tmp/b && ctest -L bench` leaves + `git status` clean (§2). +- [ ] Open a throwaway PR: `test` gates, `bench` runs red-but-non-blocking, + `coverage` and `docs` run in parallel, `package` produces a `.deb` + artifact downloadable from the run summary. +- [ ] `dpkg-deb -I` on the CI-produced `.deb` shows `Version: 0.3.1-1` and + Debian-flavoured `Depends:` — compare against the locally built package + to confirm constraint 7 is actually addressed. +- [ ] Deliberately bump only `CMakeLists.txt` and confirm the version guard + fails the `package` job (§4). Revert. +- [ ] Push tag `v0.3.1-test` on a scratch branch → release workflow attaches + assets → delete the test release and tag. +- [ ] All README badges render green after merge (§6). + +## Rollback + +Purely additive to `.github/workflows/` plus two `tests/CMakeLists.txt` label +and path edits. Revert the merge commit; nothing in the daemon, the library, or +the package contents changes. The only externally visible state is any GitHub +Release created by a real tag push — delete the release and tag if a bad one +ships. + +--- + +## Related artifacts + +| Artifact | Location | +|---|---| +| Runner-safe packaging (prerequisite) | commit `9f1e0be`, `debian/rules`, `CMakeLists.txt:41-46` | +| Existing test workflow | `.github/workflows/tests.yml` | +| Existing docs workflow | `.github/workflows/docs.yml` | +| Ecosystem CI reference (lint→test→badge) | `cuems-engine/.github/workflows/ci.yml` | +| Ecosystem publish reference (release-triggered) | `cuems-engine/.github/workflows/pypi-publish.yml` | +| Test labels / bench paths | `tests/CMakeLists.txt:17,36,51,68,91,112,133,154,158,179` | +| Package metadata | `debian/control`, `debian/changelog` | +| Badges | `README.md:14-17` | +| Prior task | [T069](T069-fix-osc-send-failure-misclassification.md) | diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 2f434e3..d43a66c 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -24,6 +24,20 @@ target_include_directories(gradient_motion PUBLIC ${CMAKE_SOURCE_DIR} ) +# Arm the CuemsLogger backend on the LIBRARY, not just the daemon executable. +# Previously only mtcreceiver and gradient-motiond carried HAVE_CUEMS_LOGGER, +# so every GME_LOG_* compiled into this library fell back to the std::cerr +# emitter — under systemd those lines are stamped PRIORITY=6 and invisible to +# `cuems-logs -e` — and GradientEngine.cpp, listed in BOTH targets, was +# compiled twice with two different logging backends. PUBLIC on purpose: the +# daemon and the test binaries then share one backend (side effect: ctest +# output for the 5 test binaries goes to syslog when cuemslogger is enabled). +# The TARGET guard covers BUILD_DAEMON=OFF, where cuemslogger is never added. +if(ENABLE_CUEMS_LOGGER AND TARGET cuemslogger) + target_compile_definitions(gradient_motion PUBLIC HAVE_CUEMS_LOGGER) + target_link_libraries(gradient_motion PUBLIC cuemslogger) +endif() + target_link_libraries(gradient_motion PUBLIC nlohmann_json::nlohmann_json ) diff --git a/src/engine/GradientEngine.cpp b/src/engine/GradientEngine.cpp index dfb486c..7bf10dc 100644 --- a/src/engine/GradientEngine.cpp +++ b/src/engine/GradientEngine.cpp @@ -41,7 +41,7 @@ GradientEngine::~GradientEngine() { bool GradientEngine::initialize(const GradientEngineConfig& config) { if (initialized_) return true; - // --- Build MotionRegistry (status events → logged, no NNG) --- + // --- Build MotionRegistry (status events → logged, no outbound channel) --- registry_ = std::make_unique( tickSource_, [](gme::signal::StatusKind k, diff --git a/src/gradient/CurveFactory.cpp b/src/gradient/CurveFactory.cpp index a75513e..d781d0e 100644 --- a/src/gradient/CurveFactory.cpp +++ b/src/gradient/CurveFactory.cpp @@ -17,6 +17,7 @@ #include "EaseOutCurve.h" #include "SCurve.h" #include "ResampledCurve.h" +#include "daemon/logging.h" namespace gme { namespace gradient { @@ -54,8 +55,8 @@ CurveFactory::createCurve(const std::string& type, inner = std::make_unique(); } else { - std::cerr << "[CurveFactory] Unknown curve type: '" - << type << "' — returning nullopt\n"; + GME_LOG_WARNING("CurveFactory: unknown curve type '" + std::string(type) + + "' — returning nullopt"); return std::nullopt; } diff --git a/src/motion/FadeMotion.cpp b/src/motion/FadeMotion.cpp index a636685..3d5e7ad 100644 --- a/src/motion/FadeMotion.cpp +++ b/src/motion/FadeMotion.cpp @@ -15,6 +15,7 @@ #include #include +#include namespace gme { namespace motion { @@ -79,13 +80,19 @@ EvalResult FadeMotion::evalAndSend(long mtc_ms) { // Send OSC const int ret = oscSend_(osc_target_, osc_path.c_str(), value); + if (ret < 0) { + std::fprintf(stderr, + "WARNING FadeMotion: osc send failed motion_id=%s path=%s " + "lo_send_ret=%d\n", + motion_id.c_str(), osc_path.c_str(), ret); + } // Update last_sent_value unconditionally (registry removes on failure anyway) last_sent_value = value; EvalResult r; r.completed = (t >= 1.0f); - r.failed = (ret != 0); + r.failed = (ret < 0); r.failure_reason = r.failed ? "osc_send_failed" : nullptr; return r; } diff --git a/src/motion/FadeMotion.h b/src/motion/FadeMotion.h index e4c4ce7..ad58223 100644 --- a/src/motion/FadeMotion.h +++ b/src/motion/FadeMotion.h @@ -61,7 +61,13 @@ namespace motion { */ class FadeMotion final : public IMotion { public: - /** @brief Function type matching `gme::osc::sendFloat`. */ + /** + * @brief Function type matching `gme::osc::sendFloat`. + * + * Return convention matches liblo's `lo_send()`: `>= 0` (bytes sent) on + * success, `< 0` on failure. NEVER assume `0` == success — `lo_send` + * never returns exactly `0` for a non-empty message. + */ using OscSendFn = std::function; /** @@ -124,7 +130,7 @@ class FadeMotion final : public IMotion { * `consecutive_osc_failures` — that is managed by `MotionRegistry::tick`. * * @param mtc_ms Current MTC head position in milliseconds. - * @return `EvalResult { completed=(t>=1.0), failed=(ret!=0) }`. + * @return `EvalResult { completed=(t>=1.0), failed=(ret<0) }`. * * @throws Never. */ diff --git a/src/motion/MotionFactory.cpp b/src/motion/MotionFactory.cpp index 7e94e64..eab6d90 100644 --- a/src/motion/MotionFactory.cpp +++ b/src/motion/MotionFactory.cpp @@ -17,6 +17,7 @@ #include #include +#include "daemon/logging.h" namespace gme { namespace motion { @@ -29,8 +30,8 @@ static std::unique_ptr makeFadeMotion(const gme::signal::FadeCommand& c : cmd.curve_params; auto curveOpt = gme::gradient::CurveFactory::createCurve(cmd.curve_type, params); if (!curveOpt) { - std::fprintf(stderr, "WARNING MotionFactory: unknown curve type '%s' " - "(motion_id=%s)\n", cmd.curve_type.c_str(), cmd.motion_id.c_str()); + GME_LOG_WARNING("MotionFactory: unknown curve type '" + cmd.curve_type + + "' (motion_id=" + cmd.motion_id + ")"); ctx.emitStatus(gme::signal::StatusKind::MotionError, cmd.motion_id, "unknown_curve_type"); return nullptr; @@ -49,9 +50,9 @@ static std::unique_ptr makeFadeMotion(const gme::signal::FadeCommand& c // Build lo_address lo_address addr = gme::osc::makeAddress(cmd.osc_host, cmd.osc_port); if (!addr) { - std::fprintf(stderr, "WARNING MotionFactory: lo_address_new failed for " - "%s:%d (motion_id=%s)\n", - cmd.osc_host.c_str(), cmd.osc_port, cmd.motion_id.c_str()); + GME_LOG_WARNING("MotionFactory: lo_address_new failed for " + cmd.osc_host + + ":" + std::to_string(cmd.osc_port) + + " (motion_id=" + cmd.motion_id + ")"); ctx.emitStatus(gme::signal::StatusKind::MotionError, cmd.motion_id, "osc_address_failed"); return nullptr; @@ -82,8 +83,8 @@ std::unique_ptr MotionFactory::fromCommand(const gme::signal::FadeComma case Type::START_CROSSFADE: // TODO Phase 7: return makeCrossfadePair(cmd, ctx); - std::fprintf(stderr, "INFO MotionFactory: START_CROSSFADE not yet " - "implemented (motion_id=%s)\n", cmd.motion_id.c_str()); + GME_LOG_INFO("MotionFactory: START_CROSSFADE not yet implemented (motion_id=" + + cmd.motion_id + ")"); return nullptr; default: diff --git a/src/motion/MotionRegistry.cpp b/src/motion/MotionRegistry.cpp index 88dd8fb..94b9956 100644 --- a/src/motion/MotionRegistry.cpp +++ b/src/motion/MotionRegistry.cpp @@ -17,6 +17,7 @@ #include #include #include +#include "daemon/logging.h" namespace gme { namespace motion { @@ -89,9 +90,8 @@ void MotionRegistry::apply(gme::signal::FadeCommand& cmd) { cancelAll(); break; case Type::START_CROSSFADE: - std::fprintf(stderr, "INFO MotionRegistry: START_CROSSFADE dropped " - "(deferred to future feature, motion_id=%s)\n", - cmd.motion_id.c_str()); + GME_LOG_INFO("MotionRegistry: START_CROSSFADE dropped (deferred to " + "future feature, motion_id=" + cmd.motion_id + ")"); break; } } @@ -137,8 +137,8 @@ void MotionRegistry::addMotion(std::unique_ptr m) { void MotionRegistry::cancelMotion(const std::string& motion_id, bool snap_to_end) { auto it = motions_.find(motion_id); if (it == motions_.end()) { - std::fprintf(stderr, "WARNING MotionRegistry: cancelMotion: motion_id '%s' " - "not found\n", motion_id.c_str()); + GME_LOG_WARNING("MotionRegistry: cancelMotion: motion_id '" + motion_id + + "' not found"); return; } @@ -182,6 +182,12 @@ void MotionRegistry::tick(long mtc_ms) { continue; } } else { + if (m.consecutive_osc_failures > 0) { + std::fprintf(stderr, + "DEBUG MotionRegistry: motion_id=%s osc send " + "recovered after %d failures\n", + m.motion_id.c_str(), m.consecutive_osc_failures); + } m.consecutive_osc_failures = 0; } diff --git a/src/motion/MotionRegistry.h b/src/motion/MotionRegistry.h index 543eaea..dc442d7 100644 --- a/src/motion/MotionRegistry.h +++ b/src/motion/MotionRegistry.h @@ -89,6 +89,10 @@ class MotionRegistry { * * Stored and forwarded to `MotionFactory::fromCommand` when a `START_FADE` * command is applied. Not called directly by the registry. + * + * Return convention matches liblo's `lo_send()`: `>= 0` (bytes sent) on + * success, `< 0` on failure. NEVER assume `0` == success — `lo_send` + * never returns exactly `0` for a non-empty message. */ using OscSendFn = std::function; diff --git a/src/signal/LockFreeQueue.h b/src/signal/LockFreeQueue.h index e721059..ed34af2 100644 --- a/src/signal/LockFreeQueue.h +++ b/src/signal/LockFreeQueue.h @@ -9,8 +9,9 @@ * @file LockFreeQueue.h * @brief Fixed-capacity single-producer / single-consumer ring buffer. * - * `LockFreeQueue` is the hand-off path from the NNG receive - * thread (producer) to the MTC tick thread (consumer). It satisfies + * `LockFreeQueue` is the hand-off path from the inbound command + * thread (producer — whichever transport is in use; currently the liblo + * OSC server thread) to the MTC tick thread (consumer). It satisfies * Principle IV (Real-Time Safety) of the project constitution: * * - **Zero heap allocation** after construction. Storage is @@ -22,7 +23,7 @@ * * ## SPSC contract * - * - **Exactly one thread calls `push`** (the NNG receive thread). + * - **Exactly one thread calls `push`** (the inbound command thread). * - **Exactly one site at a time calls `pop`** — either the MTC tick * callback or the 100 ms fallback drain timer, serialised via an * external `std::atomic_flag` owned by the queue's owner. @@ -46,7 +47,7 @@ * @code * gme::signal::LockFreeQueue q; * - * // Producer (NNG recv thread): + * // Producer (inbound command thread): * FadeCommand cmd = ...; * if (!q.push(std::move(cmd))) { * GME_LOG_WARNING("fade command queue overflow — oldest dropped"); diff --git a/src/time/MtcTickSource.cpp b/src/time/MtcTickSource.cpp index 35bb586..caf7263 100644 --- a/src/time/MtcTickSource.cpp +++ b/src/time/MtcTickSource.cpp @@ -74,6 +74,13 @@ MtcStartError MtcTickSource::start(const std::string& midiPort) { return MtcStartError::kPortNotFound; } + // Clear the >24h wrap accumulator before (re)starting. MtcReceiver keeps it + // in a process-global static, so a tick source restarted in-process after a + // >24h run would otherwise inherit a stale +86_400_000 ms offset and put the + // new session 24h ahead. The wire-driven reset can't catch a graceful + // restart (small backward delta), so clear it explicitly here. (Plan 3d) + MtcReceiver::resetWrapOffset(); + // Construct MtcReceiver — opens the port and starts the checker thread. // portIndex is the last constructor parameter (preserves positional compat). receiver_ = std::make_unique( diff --git a/tests/test_fade_motion.cpp b/tests/test_fade_motion.cpp index 8249a9d..0ef562c 100644 --- a/tests/test_fade_motion.cpp +++ b/tests/test_fade_motion.cpp @@ -486,6 +486,32 @@ static bool test_us4_transient_failure_recovery() { return true; } +static bool test_us4_real_lo_send_success_not_misclassified() { + // Regression for T069: liblo's lo_send() returns bytes-sent (a positive, + // non-zero int) on success, never 0. A mock that mirrors this — instead + // of the POSIX-style "0 = success" convention every other test in this + // file uses — must NOT trip the failure counter. + TestCtx ctx; + auto send = [](lo_address, const char*, float) -> int { return 20; }; + auto reg = ctx.makeRegWithSend(send); + + auto cmd = TestCtx::makeCmd("freal", "linear", 0.0f, 1.0f, 10000.0f, 0); + reg->apply(cmd); + + for (int i = 0; i < gme::motion::MotionRegistry::kOscFailureThreshold; ++i) { + reg->tick(i * 10); + } + + ASSERT_TRUE(reg->size() == 1, + "us4_real_lo_send: fade must survive N ticks of a real " + "liblo-style positive return value"); + + for (auto& r : ctx.emitted) + ASSERT_TRUE(r.reason != "osc_send_failed", + "us4_real_lo_send: no spurious osc_send_failed"); + return true; +} + // --------------------------------------------------------------------------- // US5 — MTC Pause, Resume, Rewind Behavior // --------------------------------------------------------------------------- @@ -557,6 +583,7 @@ int main() { {"us3_cancel_all_timing", test_us3_cancel_all_timing}, {"us4_failure_threshold", test_us4_osc_failure_threshold}, {"us4_transient_recovery", test_us4_transient_failure_recovery}, + {"us4_real_lo_send_not_misclassified", test_us4_real_lo_send_success_not_misclassified}, {"us5_pause_no_ticks", test_us5_pause_no_ticks}, {"us5_resume_correct_value", test_us5_resume_correct_value}, {"us5_rewind_before_start", test_us5_rewind_before_start},