SynthstromAudible / DelugeFirmware

https://synthstromaudible.github.io/DelugeFirmware/
GNU General Public License v3.0
551 stars 88 forks source link

Clarify Performance View Documentation #2147

Closed acook closed 1 week ago

acook commented 2 weeks ago

If this change is considered "too small", then it can be aggregated with other doc changes later.

github-actions[bot] commented 2 weeks ago

Test Results

46 tests  ±0   46 :white_check_mark: ±0   0s :stopwatch: ±0s  8 suites ±0    0 :zzz: ±0   8 files   ±0    0 :x: ±0 

Results for commit a0ad60bb. ± Comparison against base commit 0855cf9f.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. ``` ValueScalingTest ‑ arpMidiCvRatchetAmountValueScaling ``` ``` ValueScalingTest ‑ arpMidiCvRatchetValueScaling ```

:recycle: This comment has been updated with latest results.

seangoodvibes commented 2 weeks ago

Hey @acook thanks for the contribution!

I think it might also be worth mentioning that this "time" for a long / short press is also configurable in the default menu Hold Press Time (I think that's the name, don't have my deluge right in front of me)

This same behaviour can also be found with the sticky shift feature as well as the keyboard sidebar controls feature.

acook commented 2 weeks ago

Ah I saw that kHoldTime was removed in ece66d1afaf7d8bbc991e705b31c98cdbe8008e9 and for some reason I assumed that it was replaced with kShortPressTime directly, but it was replaced with HoldTime.readCurrentValue() by way of FlashStorage::holdTime.

The default in FlashStorage is set to 2, but the math used to implement it is:

holdTime = (defaultHoldTime * kSampleRate) / 20

src/deluge/storage/flash_storage.cpp#311 src/deluge/storage/flash_storage.cpp#676 src/deluge/gui/menu_item/defaults/hold_time.h#31

Which, if kSampleRate is equivalent to 1 second worth of samples, then we're looking at 2 * 1 / 20 seconds, which is 0.1 seconds (100ms).

The division by 20 is consistent across the 3 different places it is used (why is it done in multiple places instead of just via a single setter?) but seems weird to me to cancel out the 2s this way.

The Deluge does display the default Hold Press Time as 100 MS, so the translation between units is seamless. I'll update the docs, thanks for pointing it out!

m-m-adams commented 2 weeks ago

It's just divided by 20 so that the menu goes in 50ms increments (1 is 1/20th of a second etc). It felt good when we were testing

Feel free to refactor if you see a way to simplify that doesn't lose readability! The audio clock is used to measure the length of a press so it needs to be in units of samples at 44.1khz

acook commented 2 weeks ago

Hmm. Yeah I can see what you mean, it's basically a 50ms "unit" so 2 of them is 100ms. It's not really worth configuring by single milliseconds, so a step function makes it more responsive.

The sample rate part makes sense to me, since everything needs to happen at audio rate. Maybe if I survive this weekend I'll look at adding a setter to reduce code duplication!

m-m-adams commented 2 weeks ago

Yeah there's a couple other clocks but the audio clock is wrapped for use as a non-interrupting timer already

seangoodvibes commented 1 week ago

My bad @acook I totally fucked up the commit history on this PR trying to rebase it to include latest community changes. I'm going to cherry pick your stuff to a new PR and merge your changes from there.

seangoodvibes commented 1 week ago

Re-created as https://github.com/SynthstromAudible/DelugeFirmware/pull/2176