fix(firmware): audio logic does not use all of the available buffers - #7689
Open
philmoz wants to merge 2 commits into
Open
fix(firmware): audio logic does not use all of the available buffers#7689philmoz wants to merge 2 commits into
philmoz wants to merge 2 commits into
Conversation
Member
|
@richardclli Can you follow this one up, as you did mention in #7472 you were going to check main also ;) |
Member
|
This PR reclaims the wasted slot by adding I would strongly prefer an implementation that preserves the property of textbook SPSC: single writer for each variable. The same technique can be used slightly generalised for any N by computing the modulo on index usage rather than only counter advances. Something like this: private:
volatile uint8_t readIdx; // free-running in [0, 2*AUDIO_BUFFER_COUNT)
volatile uint8_t writeIdx;
inline uint8_t nextIndex(uint8_t idx) const {
return (idx >= 2 * AUDIO_BUFFER_COUNT - 1 ? 0 : idx + 1);
}
inline uint8_t slot(uint8_t idx) const {
return idx >= AUDIO_BUFFER_COUNT ? idx - AUDIO_BUFFER_COUNT : idx;
}
uint8_t used() const {
return writeIdx >= readIdx ? writeIdx - readIdx
: writeIdx + 2 * AUDIO_BUFFER_COUNT - readIdx;
}
bool full() const { return used() == AUDIO_BUFFER_COUNT; }
bool empty() const { return readIdx == writeIdx; }
public:
AudioBuffer* getEmptyBuffer() const {
return full() ? nullptr : &audioBuffers[slot(writeIdx)];
}
void audioPushBuffer() { writeIdx = nextIndex(writeIdx); }
void freeNextFilledBuffer() { readIdx = nextIndex(readIdx); }
const AudioBuffer* getNextFilledBuffer() {
return empty() ? nullptr : &audioBuffers[slot(readIdx)];
} |
…IFOs raphaelcoeffic pointed out in review that the bufferFull flag added here is written by both producer (audioPushBuffer) and consumer (freeNextFilledBuffer), breaking the textbook SPSC property that each mutable field has exactly one writer (writeIdx only by the producer, readIdx only by the consumer). That property is what makes this queue obviously correct without reasoning about cross-context timing. Switch AudioBufferFifo to his suggested fix: free-running counters over [0, 2*AUDIO_BUFFER_COUNT), folded down to the actual buffer slot -- the standard generalization of the power-of-2-mask ring buffer trick to arbitrary N. No extra state, no wasted slot, no dual-writer field. Apply the same technique to AudioFragmentFifo, which had the identical wasted-slot sentinel issue and already used the power-of-2-mask pattern (AUDIO_QUEUE_LENGTH), so the fix generalizes it rather than introducing a new idiom. A couple of hardening follow-ups from an independent review pass: - used() now snapshots each volatile into a local before computing, so the result is provably self-consistent instead of relying on a race analysis of the compiler's re-reads across the ternary. - static_asserts guard the two assumptions the free-running-counter math depends on: AUDIO_QUEUE_LENGTH being a power of 2, and 2*AUDIO_BUFFER_COUNT fitting in uint8_t. - The DEBUG_AUDIO printAudioVars() output now also prints the folded slot alongside each raw free-running index, for easier debugging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
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.
The audio logic uses a circular buffer array to handle writing and reading data to the audio hardware.
The code would not allow all of the available buffers to be filled as it marked the buffers as full when there was still one empty buffer available.
A fix for this was applied to 2.11 in #7472 (plus #7685).
This PR applies to fixed logic for 2.12 and 3.0.
It also removes some unused code.