Repository navigation
Hand pairing text, tokens and server information to consumers without copies - #261
Merged
Merged
Conversation
… copies on_pairing_attempt(), on_paired(), on_pairing_aborted() and on_pairing_code() deliver their text as std::string_view into the pairing note being dispatched; client_pairing_token() and pairing_token_for() return the 107-character token as a std::array by value; and server_information() returns views into a main-loop copy of the primary connection's server, refreshed through the Inbox only when the primary changes. The std::string forms (on_pairing_started(), on_pairing_succeeded(), on_pairing_failed(), on_display_pairing_code(), pairing_token(), format_pairing_token(), get_server_information() and ServerInformationObject) are deprecated and go in v0.11.0. The library calls each new callback first and then its deprecated form, skipping it after a re-entrant stop(), and the deprecated pairing callbacks now fill a string reserved at construction rather than building one per call.
kahrendt
marked this pull request as ready for review
November 11, 2026 18:40
kahrendt
marked this pull request as draft
November 11, 2026 18:47
server_information() and is_connected() now say the copy can trail the connected flag by one loop(), so it is read on each loop() rather than on the rising edge, and the drain comment says what its callbacks see. The protocol task hands the server to its slot in place through update(), with a test for two primary changes between drains. The pairing tests drop a pointer check no contract needs and the TearDown cross-check that duplicated PairingCallbacksReachBothForms.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Teardown callbacks can observe stale primary-server state, and stopping inside deprecated pairing callbacks lacks regression coverage.
4 open issues
What changed in this PR
Continues #260’s allocation-free public API migration in sendspin-cpp. Pairing callbacks and server information gain view-based APIs, while pairing tokens use fixed-size arrays; deprecated string APIs remain available until v0.11.0.
Changes:
- Adds pairing text views, token arrays, and an Inbox-backed server-information snapshot for the main loop.
- Retains compatibility callbacks with reserved string storage and re-entrant
stop()handling. - Migrates examples and documentation, and expands lifecycle, compatibility, and allocation checks.
| File | Description |
|---|---|
tests/test_record_store.cpp |
Checks new and deprecated token getters. |
tests/test_pairing_state_machine.cpp |
Checks callback order and matching content. |
tests/test_encrypted_lifecycle.cpp |
Uses new callbacks and server snapshots. |
tests/test_connection_lifecycle.cpp |
Checks snapshots through admission, handoff, and drops. |
tests/test_client_lifecycle.cpp |
Adds callback-form and re-entrant stop cases. |
tests/test_allocations.cpp |
Pins pairing allocations at zero and checks snapshot refreshes. |
tests/README.md |
Documents pairing callback coverage. |
tests/lifecycle_test_fixtures.h |
Adds a snapshot-refresh and compatibility helper. |
tests/CMakeLists.txt |
Documents additional private accesses used by tests. |
src/record_store.cpp |
Updates the token-getter reference in a comment. |
src/inbox.h |
Adds the server-information topic. |
src/connection.h |
Updates the pairing callback reference in a comment. |
src/connection_manager.h |
Makes inline published server data available internally. |
src/connection_manager.cpp |
Publishes server changes to the Inbox. |
src/client.cpp |
Implements new getters, snapshots, and compatibility dispatch. |
src/client_dispatch.cpp |
Updates the persistence-ordering callback comment. |
include/sendspin/types.h |
Adds ServerInformation and deprecates its string counterpart. |
include/sendspin/config.h |
Updates callback and token-getter documentation. |
include/sendspin/client.h |
Declares new APIs, lifetimes, and deprecations. |
examples/tui_client/main.cpp |
Migrates pairing callbacks and token storage. |
examples/source_client/main.cpp |
Uses new pairing APIs and length-aware printing. |
examples/basic_client/main.cpp |
Uses new pairing APIs and length-aware printing. |
docs/internals.md |
Describes snapshot delivery and compatibility storage. |
docs/integration-guide.md |
Updates migration guidance, usage, and threading contracts. |
docs/conventions.md |
Extends view-storage conventions to pairing and server text. |
Review effort: Max
drain_inbox() takes the server information before the catch-up fires a teardown's clears, so a clear reads the server the same tick left, with a test that fails the other way round. The migration notes say that overriding a deprecated callback draws no warning, and the token test reads the client's key from client_id() rather than a private member.
kahrendt
added a commit
that referenced
this pull request
Nov 11, 2026
## Summary No listener, role or provider callback may call `start()` or `stop()`, the same rule #259 set for `loop()` and enforced the same way (documentation, no runtime check). With all three off-limits, no drain ever runs beneath another and no role's main-loop teardown half runs while one of its callbacks is on the stack. That removes the only reason for the re-entrancy guards in the drain, so they go. A consumer that wants to stop or restart in response to a callback records the request and acts on it between two `loop()` calls, which is what the ESPHome hub already does: it calls `stop()`/`start()` only from its own `loop()` and `on_shutdown()`. ## Change Removed, each of which only served a callback that calls `stop()` (and maybe `start()`) beneath the drain that fired it: - `EventState::drain_generation`, its bump in `stop()`, and every check in `drain_inbox()`: the event-ring skip and per-event abort, the pairing-note checks, the gates before each role drain and the group drain, and the check before the deprecated `on_group_update()`. - The pairing notes' move-out-and-back plus the `reserve()` a nested drain needed. The drain uses its buffer in place. The internals Memory row for the extra buffer goes too. - `start()`'s STOPPING branch and its warning, which only a callback inside `stop()` could reach. The STOPPING state stays: other threads, and `connect_to()`/`disconnect()` from callbacks inside `stop()`, still need `is_started()` to read false. - The `accepts(generation)` checks after player, artwork and controller callbacks and at the head of their drains and `drain_scheduled_state()`, then `RoleTeardown::accepts()` itself. - Between a drain's catch-up and those head checks only the clear callback runs, so they only caught a callback calling `stop()`. - A protocol-task teardown in that window now gives "state, then clear on the next drain". That is the outcome it already gave when it landed just after the check. - The player's indexed loop, `teardown_reentered` and the erase clamp. The awaiting events change only from the ring drain and the catch-up, neither of which can run inside the loop. - Metadata's `TeardownTracker::completed()` check before the deprecated `on_metadata()`, and `completed()` itself. The protocol task's teardowns stay caught by the generation stamps (`keep_current_payload()`, `event_is_current()`, `ScheduledStateSlot`'s sequence check), which are unchanged. One take did depend on the removed checks: the player's STREAM_START took whatever stream parameters were pending and dropped them on a stamp mismatch. A teardown during an earlier callback in the same drain, plus the next connection's `stream/start`, left the next START reading the previous stream's parameters. The new `GenerationSlot::take_stamped()` takes a payload only under the given stamp and leaves a later one pending. The player takes its parameters under the START's stamp, and the visualizer its config the same way. Docs: `client.h` (`start()`, `stop()`, `LifecycleState`), integration guide (Stopping and Restarting, Thread Safety Summary, string_view section), conventions.md, the house-patterns checklist, and internals.md (the re-entrant section becomes "Callbacks During Dispatch", plus `stop()` steps 0 and 6). Also removed, from what #261 added for pairing: the skip before each deprecated pairing callback and the nested-drain comments on its text buffer. ## Tests - Deleted `StopFromInsideADrainCallbackIsSafe`, `StopFromInsideAPairingCallbackDismissesThePromptOnce` and `StopFromTheStoppedCallbackFindsTheHoldReleased`. - Dropped the stop/start rows from `MetadataSlot`, the "stop() from ..." rows from the metadata and group listener-form tables, and the "stop() from the first view form" row from `PairingCallbacksReachBothForms`. - `CallbackDuringStopCannotRecurse` becomes `CallbackDuringStopSeesTheStoppedClient`: it keeps the stopped-state reads and now checks the request slot, so a `connect_to()` or `disconnect()` the callback posted fails it (mutant: drop `connect_to()`'s running check). - New `TeardownReorder.AStreamStartTakesOnlyItsOwnStreamParams`: a teardown plus the next stream's `stream/start` during `on_volume_changed()`, with a control row. A mutant restoring the unconditional take makes the next stream's `on_stream_start()` read no sample rate. - The metadata-form row where the protocol task tears down during `on_track_metadata()` stays as the control: the deprecated form still follows. - ASan, TSan and the allocation tests run clean, and an ESP-IDF 5.5 esp32s3 build of `tests/esp_idf` compiles the library without warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second step of moving the public API from
std::stringtostd::string_view(after #260). The pairing callbacks built astd::stringper call, and the pairing token and server information getters built new strings on every call, all throughoperator new, which aborts on ESPHome. The new forms hand over views or a fixed-size array; the string forms are deprecated and go in v0.11.0. Part of Sendspin/backlog#92.Migration:
on_pairing_started(const std::string&)on_pairing_attempt(std::string_view)on_pairing_succeeded(const std::string&)on_paired(std::string_view)on_pairing_failed(const std::string&, reason)on_pairing_aborted(std::string_view, reason)on_display_pairing_code(const std::string&, format)on_pairing_code(std::string_view, format)pairing_token(),format_pairing_token(psk)client_pairing_token(),pairing_token_for(psk):std::optional<std::array<char, PAIRING_TOKEN_LENGTH>>get_server_information()(ServerInformationObject)server_information()(ServerInformation), main loop onlyCallback views are valid for the callback;
server_information()'s until control next returns toloop()orstop(), likegroup_state(). It can trailis_connected()by oneloop(), so read it on eachloop()rather than only whenis_connected()turns true. Code off the main loop has no replacement forget_server_information()and keeps calling it until v0.11.0. The token array carries no NUL. Overriding a deprecated pairing callback draws no deprecation warning (their parameter types are not deprecated), so search for the four old names, and mark overridesoverrideso their removal in v0.11.0 fails to compile.Change
stop(). The deprecated forms fill a string reserved at construction rather than building one per call, so they no longer allocate either.ConnectionManagerpublishes the primary's server through a new Inbox slot only when the primary changes (about 115 bytes then, nothing perloop());drain_inbox()takes it before any callback, andstop()clears the copy.PAIRING_TOKEN_LENGTH(107) is a public constant, checked against the internal encoder's size.client.hwraps the deprecatedget_server_information()declaration: GCC otherwise warned in every file including the header, because the declaration names the deprecated type.Tests
ClientLifecycle.PairingCallbacksReachBothForms: only new, only deprecated, both (new first) and astop()from the new form, across every abort reason and both code formats; the pairing state-machine suite checks every text callback is followed by its deprecated form with the same content.nulloptbeforestart(), equal to the encoder over the client's identity (configured and stored PSK) and to the deprecated getters.server_information()matches the deprecated getter across restart, re-handshake, pairing, outbound, a liveness drop and a two-server handoff, and is cleared bystop().pairing_pskand code-pairing windows (digits and QR) drop from 2, 2 and 3operator newcalls to 0 with listeners overriding both forms; admission and the playback handoff cover the server-information refresh at 0. Mutants caught include a skipped or reordered deprecated call, thestop()skip removed, views into the deprecated string, an unreserved compat string and a copy never refreshed or cleared.-DENABLE_WERROR=ONwith examples, and an ESP-IDF esp32s3 build with no deprecation warnings. Protocol task stack bound unchanged (6,512 B at -Os).