Skip to content

dac_additive: Decouple the buffer length from the waveform length #22276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

Nebuleon
Copy link
Contributor

Description

In the additive DAC driver, I saw these two pieces of code:

static const dacsample_t dac_buffer_sine[AUDIO_DAC_BUFFER_SIZE] = {
// 256 values, max 4095

static dacsample_t dac_buffer[AUDIO_DAC_BUFFER_SIZE];

To me, it made no sense to have the runtime audio buffer in that driver sized according to the number of entries in the sine wave table or require all waveform tables to be sized like the runtime audio buffer, especially because the runtime audio buffer gets filled from generated samples that come from a waveform table indirectly after calculations.

This PR makes the buffer size for both DAC drivers configurable, documents it and adds sane defaults depending on the sample rate. In dac_additive, I've also changed the waveform tables to remove the explicit size in the array definitions, and while I was at it, I also changed the square wave and the commented-out staircase wave to give them only 2 and 4 steps, respectively. In the future, the waveform tables may also be made more or less precise, as required, by adding or removing entries within them.

For users of the dac_additive driver, on default settings, this change reduces RAM usage by 384 bytes; for those who choose the square wave, the flash usage also gets reduced by 508 bytes.

For users of the dac_basic driver, on default settings, this change reduces flash usage by 768 bytes, because the square waves, which are also the runtime buffers, are stored in flash.

In either case, on default settings, this PR also reduces the latency to stop a note after the relevant input stops being applied (music mode, melody, MIDI, etc.) from a maximum of 15.6 ms to a maximum of 3.9 ms.

Tested on moonlander using dac_additive. Needs testing on keyboards using dac_basic.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).
    I do not have hardware to test all of the things I changed.

@Nebuleon
Copy link
Contributor Author

Currently lint fails on the trapezoid wave, but I formatted it like the existing sine wave. Should I add a comment like in the sine wave array, or format it like the linter says?

@sigprof
Copy link
Contributor

sigprof commented Oct 15, 2023

You may also add a trailing comma to make the formatting less horrible (the line length remains huge though).

@Nebuleon
Copy link
Contributor Author

Adding a comma and rerunning qmk format-c did make the code look much better, in that it moved }; to the next line.

As for the huge lines, they are undeniably giving the least vertical space and visual attention possible to static data of this nature, i.e. just two lines (but give way for some gnarly horizontal scroll bars if not wrapping!). Since I'm in that code anyway, I might as well either reformat the arrays in rows of 8 entries, ending at column 60, or in rows of 16 entries, ending at column 116. But that fails linting, and qmk format-c brings either of those back to two lines of 128 entries, ending at column 900. I'll revert that.

@drashna
Copy link
Member

drashna commented Oct 17, 2023

Compiles and works properly, for me.

@Nebuleon
Copy link
Contributor Author

Compiles and works properly, for me.

On what driver, dac_additive or dac_basic? Or both?

@drashna
Copy link
Member

drashna commented Oct 22, 2023

Compiles and works properly, for me.

On what driver, dac_additive or dac_basic? Or both?

dac_additive specifically.

@drashna drashna requested a review from a team December 12, 2023 09:06
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@KarlK90 KarlK90 merged commit 229a169 into qmk:develop Dec 12, 2023
itsjonny96 pushed a commit to itsjonny96/qmk_firmware that referenced this pull request Jan 7, 2024
…k#22276)

* dac_additive: Decouple the buffer length from the waveform length

* Formatting changes for the previous commit

* Reformat waveform tables with rows of 16 entries, ending at column 116

* Revert "Reformat waveform tables with rows of 16 entries, ending at column 116"

This reverts commit 6f2d379.
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jan 17, 2024
…k#22276)

* dac_additive: Decouple the buffer length from the waveform length

* Formatting changes for the previous commit

* Reformat waveform tables with rows of 16 entries, ending at column 116

* Revert "Reformat waveform tables with rows of 16 entries, ending at column 116"

This reverts commit 6f2d379.
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jan 19, 2024
…k#22276)

* dac_additive: Decouple the buffer length from the waveform length

* Formatting changes for the previous commit

* Reformat waveform tables with rows of 16 entries, ending at column 116

* Revert "Reformat waveform tables with rows of 16 entries, ending at column 116"

This reverts commit 6f2d379.
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
…k#22276)

* dac_additive: Decouple the buffer length from the waveform length

* Formatting changes for the previous commit

* Reformat waveform tables with rows of 16 entries, ending at column 116

* Revert "Reformat waveform tables with rows of 16 entries, ending at column 116"

This reverts commit 6f2d379.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants