Fix documentation drift in the visualizer and artwork docs - #109
Merged
Conversation
The VisualizerDataType enum table was missing PEAK and described F_PEAK as carrying only a frequency; it also carries the amplitude of that frequency. VisualizerRole::request_format() is public API with no coverage in the guide, so document the runtime format change alongside the role setup. The internals "Other Primitives" list described SlotBuffer::write_idx and drain_active as atomics; every SlotBuffer field is guarded by DrainTask::slot_mutex.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Changes are documentation-only and the updated statements align with the current public headers and implementation comments for the referenced APIs and synchronization behavior.
Pull request overview
Fixes documentation drift around the Visualizer and Artwork roles so the guides accurately reflect the current public API and synchronization model.
Changes:
- Corrected
VisualizerDataTypereference table to includePEAKand to describeF_PEAKas frequency + amplitude, matchinginclude/sendspin/config.h. - Documented
VisualizerRole::request_format()usage and clarified stream restart behavior when changing visualizer format at runtime. - Fixed
docs/internals.mdto describeArtworkRole::Impl::SlotBuffersynchronization as mutex-guarded (not atomics), consistent withsrc/artwork_role_impl.h.
File summaries
| File | Description |
|---|---|
| docs/internals.md | Updates the “Other Primitives” section to accurately describe SlotBuffer synchronization via DrainTask::slot_mutex. |
| docs/integration-guide.md | Aligns visualizer terminology, documents request_format(), and corrects the VisualizerDataType enum table to match the public headers. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kahrendt
enabled auto-merge (squash)
August 31, 2026 12:34
kahrendt
disabled auto-merge
August 31, 2026 12:35
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.
Three documentation defect fixes.
What changed
VisualizerDataTypeenum table (docs/integration-guide.md)The table was missing the
PEAKrow entirely, and describedF_PEAKas "Peak frequency" when it carries the dominant frequency and its amplitude (seeVisualizerRoleListener::on_f_peak). Row descriptions now match the enum comments ininclude/sendspin/config.h, and the row order matches declaration order.VisualizerRole::request_format()(docs/integration-guide.md)request_format()and itsVisualizerFormatRequeststruct are public API with no mention anywhere in the guide. Added a snippet in the Visualizer Role section showing a rate-only change and a types change, plus the note that an active stream gets a freshstream/start(soon_visualizer_stream_start()fires again with the updated config) while an idle server remembers the request for the next stream.SlotBuffersynchronization (docs/internals.md, "Other Primitives")The list claimed
ArtworkRole::Impl::SlotBuffer::write_idxanddrain_activearestd::atomic. They are plainuint8_t/bool; everySlotBufferfield is guarded byDrainTask::slot_mutex(see theSlotBufferdoc comment insrc/artwork_role_impl.h). Replaced the two false bullets with one mutex bullet covering all the fields it guards.Notes for reviewers
-std=c++20against the realVisualizerFormatRequestlayout..types = {{...}}needs the double braces (outer for theoptional, inner for thevector); that is what the snippet uses.F_PEAKdescription.ArtworkRole::Impl::stream_activebullet was checked and left alone; it is a realstd::atomic<bool>.