fix: stabilize CoreS3 streamed audio playback - #650
Conversation
📝 WalkthroughWalkthroughChangesWi-Fi power-save control
Realtime audio prebuffering
Chat-state face motion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkService
participant Modules
participant wifi_power_save
participant ESP32Native
NetworkService->>Modules: load wifi-power-save
Modules->>wifi_power_save: invoke disableWiFiPowerSave
wifi_power_save->>ESP32Native: disable Wi-Fi power saving
sequenceDiagram
participant ServerChatWebSocketWorker
participant ServerOpenAIRealtimeModel
participant AudioConsumer
ServerChatWebSocketWorker->>ServerOpenAIRealtimeModel: provide audio offset and size
ServerOpenAIRealtimeModel->>ServerOpenAIRealtimeModel: buffer until 48000 bytes
ServerOpenAIRealtimeModel->>AudioConsumer: deliver audio
ServerOpenAIRealtimeModel->>AudioConsumer: flush on response.done
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cloudflare PR previewOpen the latest preview for commit Immutable deployment: https://c04c91b1.stack-chan-pr-preview.pages.dev Warning Pull request previews contain untrusted web and firmware code. Review the changes before granting WebSerial/Bluetooth permissions or flashing a device. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
firmware/host/modules/conversation/__tests__/server-realtime-model/server-realtime-model.test.ts (1)
112-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert PCM coverage and terminal ordering, not only message count.
The threshold assertion passes when the implementation drops the first buffered chunks and forwards only the last chunk. A correct implementation may emit several queued ranges or one contiguous range, so
receivedAudioCount() === 1is not a valid correctness check.Assert that emitted ranges cover
0..48000in order for the threshold case and48000..60000for the short response. Also assert thatreceiveAudioappears beforespeakafterresponse.done.As per path instructions, tests must verify observable behavior or relational invariants; message count alone does not verify audio coverage or event ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firmware/host/modules/conversation/__tests__/server-realtime-model/server-realtime-model.test.ts` around lines 112 - 128, Strengthen the test around the response.created/onBase64 threshold flow by inspecting each receiveAudio payload’s PCM range and asserting ordered coverage of 0..48000, rather than only counting messages. For the short response after response.done, assert coverage of 48000..60000 and verify the receiveAudio event precedes the speak event; update the test fixtures or helpers as needed to observe payload ranges and event order.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/calm-streaming-audio.md:
- Line 5: Update the changeset entry to state that disabling CoreS3 Wi-Fi power
saving increases CoreS3 power consumption, and document the tested target and
playback verification, including the one-second audio prebuffer and preserved
lip-sync/autonomous-motion behavior.
In
`@firmware/host/modules/conversation/chat-audioio/server-openai-realtime-model.js`:
- Around line 195-200: Update the prebuffering logic in the response event
handlers, including response.done and the threshold path around
outputPrebufferOffset/outputPrebufferSize, to queue every { offset, size } range
rather than overwrite prior chunks. When the threshold is reached or
response.done flushes the buffer, replay all queued ranges in arrival order,
preserving one receiveAudio delivery per range; only merge ranges when the ring
buffer guarantees they are contiguous and retained.
---
Nitpick comments:
In
`@firmware/host/modules/conversation/__tests__/server-realtime-model/server-realtime-model.test.ts`:
- Around line 112-128: Strengthen the test around the response.created/onBase64
threshold flow by inspecting each receiveAudio payload’s PCM range and asserting
ordered coverage of 0..48000, rather than only counting messages. For the short
response after response.done, assert coverage of 48000..60000 and verify the
receiveAudio event precedes the speak event; update the test fixtures or helpers
as needed to observe payload ranges and event order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ce11e26-655b-47bc-b601-645ca86b75f0
📒 Files selected for processing (13)
.changeset/calm-streaming-audio.mdfirmware/host/modules/connectivity/__tests__/network-manager.test.tsfirmware/host/modules/connectivity/__tests__/network-service.test.tsfirmware/host/modules/connectivity/__tests__/network-service/manifest.jsonfirmware/host/modules/connectivity/esp32/wifi-power-save.cfirmware/host/modules/connectivity/esp32/wifi-power-save.jsfirmware/host/modules/connectivity/manifest.jsonfirmware/host/modules/connectivity/network-service.tsfirmware/host/modules/conversation/__tests__/server-realtime-model/server-chat-websocket-worker.jsfirmware/host/modules/conversation/__tests__/server-realtime-model/server-realtime-model.test.tsfirmware/host/modules/conversation/chat-audioio/server-openai-realtime-model.jsfirmware/host/modules/ui/views/main/__tests__/face-view-state/face-view-state.test.tsfirmware/host/modules/ui/views/main/face-view.ts
| "stack-chan": patch | ||
| --- | ||
|
|
||
| CoreS3のWi-Fi省電力を無効化し、stack-chan-aiの音声を1秒プリバッファします。再生中は口パクを維持したまま自律顔モーションを抑制します。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the tested target and CoreS3 power impact.
The patch classification is present, but Line 5 does not state that disabling Wi-Fi power saving increases CoreS3 power consumption or identify the tested target and playback verification. Add those details.
Suggested release-note update
CoreS3のWi-Fi省電力を無効化し、stack-chan-aiの音声を1秒プリバッファします。再生中は口パクを維持したまま自律顔モーションを抑制します。
+CoreS3ではWi-Fi消費電力が増加します。検証対象はCoreS3で、debug buildと90秒連続再生試験を2回実施しました。As per path instructions: “Since this PR changes user-visible firmware behavior, retain the changeset and document tested targets and hardware-specific CoreS3 impact.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CoreS3のWi-Fi省電力を無効化し、stack-chan-aiの音声を1秒プリバッファします。再生中は口パクを維持したまま自律顔モーションを抑制します。 | |
| CoreS3のWi-Fi省電力を無効化し、stack-chan-aiの音声を1秒プリバッファします。再生中は口パクを維持したまま自律顔モーションを抑制します。 | |
| CoreS3ではWi-Fi消費電力が増加します。検証対象はCoreS3で、debug buildと90秒連続再生試験を2回実施しました。 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/calm-streaming-audio.md at line 5, Update the changeset entry to
state that disabling CoreS3 Wi-Fi power saving increases CoreS3 power
consumption, and document the tested target and playback verification, including
the one-second audio prebuffer and preserved lip-sync/autonomous-motion
behavior.
Source: Path instructions
| 'response.done'() { | ||
| if (this.outputPrebuffering) { | ||
| this.outputPrebuffering = false | ||
| if (this.outputPrebufferSize) { | ||
| super.onBase64(this.outputPrebufferOffset, this.outputPrebufferSize) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Preserve every prebuffered audio range.
When multiple chunks arrive before the threshold, Lines 212-214 overwrite the previous range. Line 217 then forwards only the last chunk. With the supplied test inputs, the first 36,000 bytes are lost. Lines 198-200 have the same defect for short responses.
Queue each { offset, size } range and replay all queued ranges in order when the threshold is reached or response.done flushes the buffer. Combine ranges only when the shared ring buffer guarantees contiguous, retained data.
The delivery contract in firmware/host/modules/conversation/__tests__/server-realtime-model/server-chat-websocket-worker.js forwards one receiveAudio message per range, so downstream code cannot recover omitted PCM.
Proposed fix
- this.outputPrebufferOffset = 0
- this.outputPrebufferSize = 0
+ this.outputPrebuffer = []
- if (this.outputPrebufferSize) {
- super.onBase64(this.outputPrebufferOffset, this.outputPrebufferSize)
+ for (const { offset, size } of this.outputPrebuffer) {
+ super.onBase64(offset, size)
}
+ this.outputPrebuffer = []
- this.outputPrebufferOffset = offset
- this.outputPrebufferSize = size
+ this.outputPrebuffer.push({ offset, size })
- super.onBase64(offset, size)
+ for (const chunk of this.outputPrebuffer) {
+ super.onBase64(chunk.offset, chunk.size)
+ }
+ this.outputPrebuffer = []Also applies to: 212-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@firmware/host/modules/conversation/chat-audioio/server-openai-realtime-model.js`
around lines 195 - 200, Update the prebuffering logic in the response event
handlers, including response.done and the threshold path around
outputPrebufferOffset/outputPrebufferSize, to queue every { offset, size } range
rather than overwrite prior chunks. When the threshold is reached or
response.done flushes the buffer, replay all queued ranges in arrival order,
preserving one receiveAudio delivery per range; only merge ranges when the ring
buffer guarantees they are contiguous and retained.
1 participant
Summary
patch;.changeset/calm-streaming-audio.mdis included.Root Cause
Receive-only device tests showed that the audio data already arrived in bursts before
AudioOut: with default power save the Worker notification gap reached 1,117 ms, whileWIFI_PS_NONEreduced gaps over 100 ms by about 86% and gaps over 200 ms by about 95%. The remaining jitter is absorbed by the one-second prebuffer without allocating or copying another audio buffer.ESP-IDF documents
WIFI_PS_MIN_MODEMas the default. Moddable 8.3.1 and 9.0.0 both initialize and start Wi-Fi without overriding that default, so the missing override predates Moddable 9.0.0 / ESP-IDF 6.0.2. ESP-IDF 6.0.2 updates Wi-Fi/PHY binary submodules, so the combination may have changed how visible the latency is, but the exact version-dependent severity has not been isolated to a single upstream commit.What Changed
esp_wifi_set_ps(WIFI_PS_NONE); no Moddable source change is required.FaceViewinterpretsonChatStateand suppresses autonomous face motion duringLISTENINGand buffered drain (WAITING); lip sync remains driven byonOutputLevelChanged.Power consumption increases while Wi-Fi is active on CoreS3. Other targets retain their current behavior.
Verification
cd firmware && npm run formatcd firmware && npm run lint(passes; one pre-existing informational warning inconnectivity/__tests__/fakes/crypt.ts)cd firmware && npm run test:unit(387 passed)cd firmware && npm run check:legacy-namesserver-realtime-modelonlinchat-status-barandface-view-stateonlin/m5stackesp_wifi_set_psreference in Moddable'swifi-esp32.c.o; the reference comes from stack-chan'swifi-power-save.c.o/dev/ttyACM0M5StackChan, static 24 kHz PCM16 1 kHz stream, volume 0.2, 90 seconds × 2Affected Areas
Breaking Changes
Related Issues
Summary by CodeRabbit
Improvements
Tests