Skip to content

WavWriter: fix SaveFile final flushing + add comprehensive unit test coverage#696

Merged
stephenhensley merged 8 commits intoelectro-smith:masterfrom
chattob:fix/wavwriter-savefile-tail
Apr 2, 2026
Merged

WavWriter: fix SaveFile final flushing + add comprehensive unit test coverage#696
stephenhensley merged 8 commits intoelectro-smith:masterfrom
chattob:fix/wavwriter-savefile-tail

Conversation

@chattob
Copy link
Copy Markdown
Contributor

@chattob chattob commented Mar 17, 2026

Summary

This PR fixes WavWriter finalization bugs and adds comprehensive unit tests.

What Was Fixed

SaveFile() final flush logic at buffer boundaries

On master, SaveFile() does not flush any pending full half-buffer before finalizing. Instead, it writes wptr_ * bytes_per_sample bytes starting at the beginning of the transfer buffer.

That is only correct while recording is still in the first half of the buffer. Once recording has advanced into the second half the final WAV tail repeats earlier samples from the first half.

The fix changes finalization to:

  • flush any pending full half-buffer first
  • then write only the true remaining tail
  • select that tail from the correct half of the buffer
  • handle the exact-half-boundary case without writing the same half twice

Uninitialized writer state

Init() and OpenFile() now reset the writer’s internal state and clear the transfer buffer before a new recording starts.
This is more a preventive measure.

Missing WAV type dependency in header

src/util/WavWriter.h now includes util/wav_format.h, so WAV_FormatTypeDef and the WAV constants are always available.

Test Coverage Added

  • Header/size and empty-file behavior
  • Length accessor consistency with actual saved payload

Save-path state transitions

  • partial first-half tail
  • pending FLUSH0
  • FLUSH0 + second-half tail
  • pending FLUSH1
  • FLUSH1 + new first-half tail

Representative format cases

  • 32-bit mono + stereo
  • 16-bit mono
  • 16-bit stereo crossing from the first half into the second half

Validation

  • Built and ran util_WavWriter tests via CMake/CTest. The new tests expose the old finalization regressions and pass with the fix.
  • Tested on a real application. Did not encounter bugs anymore.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 19, 2026

Test Results

163 tests  +13   163 ✅ +13   0s ⏱️ ±0s
  1 suites ± 0     0 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 4bdab6d. ± Comparison against base commit 9498417.

♻️ This comment has been updated with latest results.

@stephenhensley
Copy link
Copy Markdown
Collaborator

This looks great!

While minimal, the FatFS mock is pretty clean, and allows for a good foundation for confirming the logic of the classes using it.

I'll be reviewing/merging a few other PRs this week, and should be able to merge this at the same time. If I notice anything while going through typical pre-merge testing, I'll let you know. Otherwise, this looks good to me!

Thanks for the contribution!

@stephenhensley
Copy link
Copy Markdown
Collaborator

Alright, test coverage seems good, and I did a bit of testing on hardware to check for any regressions/behavioral issues. Seems good to me!

I also wrote a more real-time oriented example program since the current example in the repo only does an offline render, which isn't typically what users would be trying to do. I'll add that to the examples after merge.

@stephenhensley
Copy link
Copy Markdown
Collaborator

Thanks again for the contribution, @chattob !!

@stephenhensley stephenhensley merged commit b1bf1ef into electro-smith:master Apr 2, 2026
13 checks passed
@chattob
Copy link
Copy Markdown
Contributor Author

chattob commented Apr 3, 2026

I'm really glad I could contribute! Thanks for your work on daisy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants