Add the source role and task with a PCM streaming pipeline - #114
Draft
chrisuthe wants to merge 12 commits into
Draft
Add the source role and task with a PCM streaming pipeline#114chrisuthe wants to merge 12 commits into
chrisuthe wants to merge 12 commits into
Conversation
chrisuthe
force-pushed
the
source-role/03-role-pcm
branch
from
September 1, 2026 02:32
68cfcc4 to
34c194a
Compare
Names the source config's default sample rate and makes the pass-through SourceRole::Impl::write_audio const, matching the other Impl methods that only write through pointers.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Trust enforcement, command concurrency, send ordering, lifecycle accuracy, documentation, and active-pipeline test coverage remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the source@v1 PCM capture pipeline, including public APIs, background streaming, protocol integration, build configuration, and tests.
Changes:
- Adds configurable PCM capture and timestamped source streaming.
- Integrates source lifecycle, commands, state, and Inbox events.
- Adds conditional builds and source-role tests.
File summaries
| File | Description |
|---|---|
CMakeLists.txt |
Adds source build option and sources. |
Kconfig |
Adds ESP source-role option. |
cmake/sources.cmake |
Defines source and shared audio sources. |
include/sendspin/client.h |
Exposes source-role registration and access. |
include/sendspin/config.h |
Adds source capture configuration. |
include/sendspin/source_role.h |
Defines the public source API. |
src/audio_ring_buffer.cpp |
Supports configurable memory placement. |
src/audio_ring_buffer.h |
Generalizes ring-buffer documentation and creation. |
src/client.cpp |
Integrates source lifecycle and message routing. |
src/constants.h |
Shares the time-sync wait interval. |
src/inbox.h |
Adds source command and lifecycle events. |
src/protocol_messages.h |
Uses the public source signal type. |
src/source_encoder.h |
Adds the PCM encoder abstraction. |
src/source_role.cpp |
Implements source configuration and lifecycle. |
src/source_role_impl.h |
Defines private source-role state. |
src/source_task.cpp |
Implements the capture-to-server pipeline. |
src/source_task.h |
Declares source task state and helpers. |
src/sync_task.cpp |
Removes the duplicated sync wait constant. |
tests/CMakeLists.txt |
Conditionally registers source tests. |
tests/test_source_role.cpp |
Tests source configuration and bookkeeping. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Gate server/command source writes to the current connection so a concurrently delivering displaced connection cannot overwrite the current one's command before the drain, close the write_audio gate before cleanup's synthetic STOPPED so the stopped callback and the gate can never disagree, and wait for the client-stream/start send confirmation before accepting audio (queued is not sent on the async ESP-server transport). The default chunk drops to 20 ms so a codec-only switch to Opus keeps a valid config, and the stack-size and write_audio docs now state their measured basis and per-platform locking.
is_initialized() reports the event flags, so creating them before the fallible ring and staging allocations let a failed init leave a half-built task that a retried start would accept; ordering them last makes initialized mean fully initialized.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Stream cancellation and completion races can corrupt lifecycle behavior, while unchecked size arithmetic can bypass configuration limits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 3
- Review effort level: Balanced
Fold the desired-streaming flag and the write_audio gate into one atomic: opening the gate is now a compare-exchange requiring the desired bit, so a stop or cleanup that lands between start confirmation and gate open wins the race, the gate stays shut, and STREAMING_STARTED is never emitted after the stop's synthetic STOPPED. The client-stream/ start send gets its own completion channel so a late start completion cannot release a later chunk-send wait. Buffer sizing checks the frame count against the byte cap before multiplying so hostile uint32 configs fail closed instead of wrapping.
An ESP httpd_queue_work that reports ESP_OK but silently drops its control message (full mailbox) never runs the worker, so the previous unbounded wait would hang the source task forever. The wait is now bounded; on timeout the chunk is dropped and the task resumes from live capture, without releasing the still-in-flight slot (preserving the no-reuse-after-timeout invariant) -- the connection degrades to NOT_FINISHED until teardown frees the slot and a reconnect re-arms it. The completion bit is cleared before each send so a late completion cannot satisfy the next wait.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Late asynchronous completions can access destroyed task state, and timed-out starts can remain open remotely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines
+127
to
+131
| this->send_complete_cb_ = [this](bool ok) { | ||
| this->last_send_ok_.store(ok, std::memory_order_release); | ||
| this->event_flags_.set(SourceTaskBits::SOURCE_SEND_COMPLETE); | ||
| }; | ||
| this->start_complete_cb_ = [this](bool ok) { |
Comment on lines
+325
to
+329
| if ((bits & SourceTaskBits::SOURCE_START_COMPLETE) == 0U || | ||
| !this->start_send_ok_.load(std::memory_order_acquire)) { | ||
| SS_LOGW(TAG, "client-stream/start not confirmed; stream not opened"); | ||
| return; | ||
| } |
Comment on lines
+210
to
+211
| SS_LOGW(TAG, "write_audio() rejected: %u bytes is not a whole number of %u-byte frames", | ||
| static_cast<unsigned>(len), static_cast<unsigned>(this->bytes_per_frame_)); |
Comment on lines
+63
to
+66
| // 64-bit contract: the ms x rate intermediate here is 4.8e9 > 2^32, which 32-bit | ||
| // arithmetic (size_t on ESP32) would wrap to 505032 frames; init() additionally caps the | ||
| // derived byte sizes before narrowing. | ||
| EXPECT_EQ(source_ms_to_frames(100000, 48000), 4800000ULL); |
| EXPECT_FALSE(advertises_source(*make_with_chunk(151))); | ||
| // Control: the bound values themselves and the default are accepted. | ||
| EXPECT_TRUE(advertises_source(*make_with_chunk(5))); | ||
| EXPECT_TRUE(advertises_source(*make_with_chunk(25))); |
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.
Part 3/6 of the source@v1 stack (tracker: #95).
What it adds: the source role itself —
SendspinClient::add_source(),SourceRoleConfig, and a dedicated task that assembles captured audio into timestamped PCM chunks and streams them to the server, gated byserver/commandstart/stop with per-connection permission.How it's used: the consumer adds the role with its capture format and feeds
write_audio()from its capture thread; the server starts and stops the stream. After this part the library is a complete PCM source. End-to-end use against Music Assistant additionally needs the encryption/pairing work (servers only activate source@v1 on paired connections) and aiosendspin's pendingclient-stream/*rename.Validation: exercised end-to-end against Music Assistant's
sendspin_sourceprovider on a dev MA server (with the two server-side interop shims aiosendspin needs anyway: the pairing-gate lift for legacy transport and theclient-stream/*rename aliases): mDNS discovery, source-typed registration, server-gated start/stop lifecycle, and decoded PCM ingest through MA's clock bridge to a live player.