{{ message }}
Repository navigation
Tags: timtrautmann/sendspin-cpp
Tags
Add an opt-in plaintext message trace for conformance tooling (Sendsp… …in#205) Many sendspin-cpp client cases in the [conformance matrix](https://sendspin.github.io/conformance/implementations/sendspin-cpp.html) fail because the adapter cannot see the messages the library exchanges: received audio hashes, raw chunk frames, and the protocol-baseline trace. This adds an opt-in plaintext trace, mirroring sendspin-server-cpp's `ServerConfig::trace`. ## Change - `SENDSPIN_ENABLE_MESSAGE_TRACE`: a host-only CMake option, off by default, with no Kconfig entry. With it off, the build is unchanged, ESP included. - `SendspinClientConfig::message_trace` receives a `SendspinTracedMessage` for every message on the protocol task. Each one carries: - connection id, direction and stamp - the plaintext: inbound after decryption and reassembly, outbound before encryption (each fragment on its own), plus the cleartext handshake - `audio_bytes`: for a player audio chunk, the encoded audio it carries - The pairing secrets a client sends (`long_term_psk`, `wrapped_psk`, `wrapped_nonce_B`) read `"<redacted>"`. - `MessageTracer` (`src/message_trace.h`) is owned per connection. The trace points are gated inline in `connection.cpp` and in `NoiseTransport::encrypt_in_place()`. - The gate is listed in CLAUDE.md, `conventions.md`, `internals.md` and the house-patterns skill. `integration-guide.md` documents the option. - CI's test and TSan jobs build with the trace on; the build and no-opus jobs leave it off. ## Notes - Once this lands, the conformance adapter (`adapters/sendspin-cpp/client/main.cpp` in Sendspin/conformance) needs updating to turn the option on and read the trace. ## Tests - End-to-end against the fake encrypted server, read only through the trace. It asserts: - the exact opening sequence from `client/init` to `server/activate`, with the transport JSON readable - an audio chunk's `audio_bytes` - a single connection id, and stamps on the client clock - A redaction table whose secret rows are built by the real pairing formatters, with control rows: a message without secrets and a binary message reach the trace unchanged and uncopied. - Not covered by any test: inbound reassembly and outbound fragmentation.
Add an RMS line-sense option to the source client example (Sendspin#201) Addresses Sendspin#133. The spec has the server alone start a source stream (roles/source/v1.md "Default streaming behavior"); a client reports line sense as a hint. The library already supports that through `SourceRoleConfig::line_sense` and `SourceRole::set_signal()`, and detection stays in the application. - `docs/integration-guide.md`: says detection is the application's job and to call `set_signal()` on each change. - `examples/source_client`: `-s DBFS` meters each buffer's RMS level and reports present after 100 ms at or above the threshold, absent after 2 s below. Capture runs from startup in that mode; audio reaches `write_audio()` only while the stream is open. Without `-s` the example is unchanged. Testing: built and run on macOS; `-s -70` on the laptop mic reported absent then present, and an invalid threshold is rejected. No unit tests (example code).
Call the network provider on the main loop only (Sendspin#189) The protocol task polled `SendspinNetworkProvider::is_network_ready()` (`maybe_start_ws_server()`) while the WebSocket server was down. ESPHome's implementation reads plain main-loop fields (`WiFiComponent::connected_`, `EthernetComponent::state_`), so the poll was a data race, and any other platform would hit the same one. The persistence provider is already main-loop only. ## Change - The protocol task's poll was the only call off the main loop. Now `start()` and every `loop()` call `poll_network_provider()`, which stores the answer in `std::atomic<bool> network_ready_`. The protocol task reads that atomic before starting the server. - When the network comes up, `loop()` wakes the protocol task. The task no longer has a network timer, and `NETWORK_POLL_INTERVAL_MS` is removed. - Latency improves: the server starts within one `loop()` period (about 16 ms in ESPHome) instead of up to 1 s. - **Release note:** a consumer that never calls `loop()` never gets a server if the network was down at `start()`. ## Docs The `SendspinNetworkProvider` doc comment now says it is called on the main loop only. Also updated: the `start()` and `loop()` docs, the integration guide (provider section, threading summary), internals (tick timers, the drain, atomics), conventions (only the main loop calls the providers) and CLAUDE.md. ## Tests - `TheNetworkProviderIsCalledOnTheMainLoopOnly`: the provider reports the network down through `start()` and a run of `loop()` calls, then up. The server starts, and every call happened on the test thread. It fails on the old code, and removing the wake hangs it. - The `NextDeadline` network-poll row becomes a `NO_DEADLINE` control. 698/698 under ASan and TSan.
Deprecate the unuseful pairing_window_supported config flag (Sendspin… …#187) ## Summary `SendspinClientConfig::pairing_window_supported` only declared that the application implemented the pairing-window gesture callbacks, and every configuration with it off was a trap: - A `static_pairing_code` set without the flag was silently not advertised, although `start()` succeeded. Every static attempt is gesture-gated (pairing.md "Pairing Window"), so there is no valid static setup with the flag off. - A `dynamic_pairing_code` attempt held back by the 20-round limit (pairing.md "Rounds") is gated either way. The flag only suppressed the `on_open_pairing_window` prompt, leaving the attempt at `client/pair-pending` until the server gave up. ## Changes - Deprecate the field: it stays `[[deprecated]]` and ignored, with removal planned for v0.10.0. `static_pairing_code` is advertised whenever it is set and the dynamic code is not offered. - `SendspinClientConfig` is wrapped in a `-Wdeprecated-declarations` suppression so its implicit copy and move members do not warn under `-Werror`; a consumer's own assignment still warns (checked with Clang and GCC 16). - `on_open_pairing_window` always fires for a gesture-gated attempt. The callbacks already default to no-ops, so a device with no gesture UI behaves as it did with the flag off. - Integration guide: mark the config row deprecated and drop the flag-off paragraph; note that the gesture also clears the dynamic round limit, so a device offering either code method should prompt on `on_open_pairing_window` and call `confirm_pairing_window()`. - Examples and tests updated; `GatedAttemptWithoutWindowSupportSkipsPrompt` is deleted with the behavior it covered. ## Deprecation Not breaking, so this can ship as a point release. Code that sets `pairing_window_supported` still compiles, with a deprecation warning; delete the assignment before v0.10.0. One behavior change for flag-off configurations: a `static_pairing_code` set with the flag left `false` is now advertised (it was silently not advertised before), and `on_open_pairing_window` fires for a gated attempt (a no-op unless overridden). The release notes should name both. ## Testing - Host suite under ASan and under TSan: 694/694 each. - Host build with `-DENABLE_WERROR=ON` (library, examples, tests): clean, 694/694. - Host examples (basic_client, tui_client, source_client) build. - ESP-IDF not compiled; the change touches no platform code.
Allocate PSRAM thread stacks with 8-bit capable caps (Sendspin#184) ## Summary `platform_configure_thread()` set `stack_alloc_caps = MALLOC_CAP_SPIRAM` alone. `esp_pthread_set_cfg()` rejects caps without `MALLOC_CAP_8BIT` (`components/pthread/pthread.c`, same check in IDF 5.4, 5.5 and 6.1) and the return value was ignored, so every `psram_stack` option has been silently dropped since the first commit. The rejection drops the whole config, not just the caps: the thread started with the calling thread's previous pthread config (or the Kconfig defaults, 3072 bytes), so stack size, priority and name were wrong too. Affected: SsSync, SsArt, SsVis, SsSrc and SsProto when their PSRAM option is on. The httpd stack (`httpd_psram_stack`) was not affected, since `xTaskCreatePinnedToCoreWithCaps` accepts `MALLOC_CAP_SPIRAM` alone. ## Changes - Request `MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT` for PSRAM stacks. - Log an error under `sendspin.thread` if the config is ever rejected. ## Testing - ESP-IDF 5.5 build (ESP32-S3, SPIRAM) compiles. - Hardware: with `protocol_task_psram_stack` toggled, internal free heap now changes as expected.
Cache host CI compiles with ccache (Sendspin#183) The host CI jobs (Build, Unit tests, Build and test without Opus, ThreadSanitizer) now compile through ccache. After Sendspin#182 these compile-bound jobs set the overall CI time; most of their compile time is FetchContent dependencies that rarely change. - Each job installs ccache; the host CMake build picks it up automatically (Sendspin#181). - `actions/cache/restore` pulls the job's newest cache; `actions/cache/save` runs only on pushes to main, so PR runs reuse main's objects without filling the repository cache quota. Caches are capped at 300M per job. - `ccache --show-stats` after each build shows the hit rate in the log. This PR's own run starts cold since nothing is saved from a PR; the speedup shows on PRs after the first main push.
Remove the deprecated positional send_command() overload (Sendspin#125) `ControllerRole::send_command(SendspinControllerCommand, std::optional<uint8_t>, std::optional<bool>)` was deprecated in favor of the `ClientCommandControllerObject` form, which can also carry seek parameters. The deprecation notice scheduled removal for v0.8.0, which is the next release, so this removes it. - Drops the declaration from `include/sendspin/controller_role.h` and the definition from `src/controller_role.cpp` - Drops the migration note from `docs/integration-guide.md` No in-tree callers remain (examples and tests already use the struct form). 149/149 host tests pass. `SendspinClient::start_server()` is the only other deprecated symbol; its notice targets v0.9.0, so it is left in place.
Silence a GCC dangling-reference warning in controller parsing (Sends… …pin#106) Bind the `supported_commands` array to a named local before iterating it in `process_server_state_controller()`, so GCC stops emitting `-Wdangling-reference` there. ## Why The warning fires on every ESP-IDF build of the component (including via ESPHome), and is present in released v0.7.1 and on current `main`: ```text src/protocol.cpp:554:73: warning: possibly dangling reference to a temporary [-Wdangling-reference] 554 | controller_object["supported_commands"].as<JsonArrayConst>()) { | ^ src/protocol.cpp:554:52: note: 'MemberProxy<JsonObject, RamString>' temporary created here ``` The range initializer chained `.as<JsonArrayConst>()` off the `MemberProxy` temporary returned by `operator[]`. Only the final temporary in such a chain gets lifetime extension in C++20, so the proxy is destroyed at the end of the full-expression while the loop body is still running. **It was benign.** `JsonArrayConst` stores only a `const ArrayData*` and a `const ResourceManager*`, both pointing into the `JsonDocument`'s memory pool, and `JsonArrayConstIterator` holds an `ArrayData::iterator` plus that same resource pointer. Neither the array nor its iterators retain anything that points into the dead proxy, so the destroyed temporary was never read. GCC's heuristic cannot see through the proxy type, though, so this fixes the code rather than suppressing the diagnostic. ## How ```diff - for (JsonVariantConst command_var : - controller_object["supported_commands"].as<JsonArrayConst>()) { + JsonArrayConst commands_array = + controller_object["supported_commands"].as<JsonArrayConst>(); + for (JsonVariantConst command_var : commands_array) { ``` This is also what the rest of the file already does. The five other array loops (`active_roles`, artwork `channels`, visualizer `types`, and the `roles` lists in `stream/end` and `stream/clear`) each name a local first; the loop at line 264 ranges over a function parameter. This was the only chained range initializer left, so the change makes the file consistent rather than introducing a new idiom. No behavior change. ## Testing macOS builds with clang, which does not implement `-Wdangling-reference` and so never reproduced this. Verified with Homebrew `g++-15` driven at the host build's exact flags and include set: - Warning reproduces verbatim before the change, and is gone after it. - Swept every file in `src/` and `src/host/` with `-Wall -Wextra -Wdangling-reference -fsyntax-only`: no dangling-reference warnings remain anywhere. - Host clang build rebuilds clean; `ctest` 123/123 pass. - `clang-format --dry-run -Werror src/protocol.cpp` clean.
Cut stack use on the JSON receive path (Sendspin#104) The ESP httpd task runs with the ESP-IDF default 4 KB stack (`HTTPD_DEFAULT_CONFIG`), and every inbound JSON message is parsed and dispatched on it, underneath httpd's own call chain. Measured with `-fstack-usage` on xtensa-esp32s3 g++ 14.2 at `-Os`, a server/state message carrying metadata cost about 1250 bytes of library frames alone, before httpd's own usage below it and before any logging on top. That is a third of the whole stack spent in this library, and it is what the reported httpd stack overflow (issue Sendspin#102) was eating into. Almost all of it was avoidable duplication rather than real working set: - server/state was parsed into one `ServerStateMessage` aggregate (288 bytes), so every section's storage stayed live in the caller's frame for the whole parse while each section parser built a second copy of the same fields in its own. A `ServerMetadataStateDelta` is 200 bytes and was materialized twice at once, then a third time by the by-value handle_server_state parameter. - GCC inlined the section parsers into `process_server_state_message` and did not overlap their locals, so metadata, color, and controller storage all coexisted in a single 736 byte frame, the largest in the library. `server/state` is now parsed one section at a time. Each section is an out-of-line function that fills a caller-owned struct in place, the client scopes each section separately so the compiler can reuse the slots, and a section is only parsed when its role is present. The three `handle_server_state` overloads take an rvalue reference instead of a by-value copy. `stream/start` gets the same in-place treatment for its player, artwork, and visualizer sections. Measured frames, before -> after: process_json_message 512 -> 320 process_server_state_message 736 -> gone process_server_state_metadata 288 process_server_state_controller 224 process_server_state_color 192 process_stream_start_message 496 -> 448 Deepest `server/state` path: 1248 -> 608 bytes (51% less). Deepest `stream/start` path: 1008 -> 768. No behavior change: same fields, same validation, same tri-state delta semantics. tests/test_protocol.cpp moves to the per-section entry points. Closes Sendspin#102. It seems like this is a healthy enough margin without needing to increase the default task stack. The encryption work will require a small bump to the httpd task stack.
Artwork role: back-pressure, timing offset, and lateness reporting (S… …endspin#97) Adds three opt-in artwork role features aimed at smooth cross-fades on ESPHome displays, all backward-compatible (defaults preserve today's decode-and-display-every-frame behavior) beyond a function signature breaking change. ### Changes - **Per-slot `frame_done` acknowledgment (opt-in back-pressure):** Set `ImageSlotPreference::require_frame_done` and the role delivers at most one un-acked "delivery" (a frame or a clear) at a time for that slot; newer payloads are buffered latest-wins until the consumer calls `ArtworkRole::frame_done(slot)`. A clear supersedes any un-acked frame. Lets a consumer hold the gate open for the duration of a cross-fade animation instead of having buffers reused mid-fade. No timeout by design — the ack is the contract. - **Per-slot `display_offset_ms`:** Shifts a slot's display deadline (positive fires early, mirroring `PlayerRoleConfig::fixed_delay_us`) so a cross-fade can straddle the track boundary. - **Display lateness reporting:** `on_image_display()` now takes a `lateness_ms` argument reporting how far past the deadline it fired, so a consumer can shorten a fade to still end on schedule, or snap instantly when joining mid-track. `lateness_ms == 0` is reserved for the no-connection case; a connected on-time display never reports 0. New `tests/test_artwork_role.cpp` covers the gate state machine (hold/supersede/clear/restart/reentrant-ack), the deadline/offset arithmetic, and the lateness mapping. ### Breaking change `on_image_display(uint8_t)` signature is now `on_image_display(uint8_t, uint32_t lateness_ms)`.
PreviousNext
