Skip to content

Adds support for the VIBrato command to SampleInstrument#1234

Open
n1LS wants to merge 30 commits intoxiphonics:masterfrom
n1LS:627-vibrato
Open

Adds support for the VIBrato command to SampleInstrument#1234
n1LS wants to merge 30 commits intoxiphonics:masterfrom
n1LS:627-vibrato

Conversation

@n1LS
Copy link
Copy Markdown
Contributor

@n1LS n1LS commented Feb 4, 2026

  • adds a 16bit, 64 step sine LUT with fast linear interpolation
  • implements the Vibrato SRPUpdater class
  • adds the InstrumentCommandVibrato definitions and help text
  • vibrato depth is max. 4 semitones (0x00-0x40 is roughly the range for a soft musical vibrato above that it goes more into fx territory)

@n1LS n1LS mentioned this pull request Feb 4, 2026
Copy link
Copy Markdown
Collaborator

@maks maks left a comment

Choose a reason for hiding this comment

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

@n1LS given the pico is already cpu limited, have you profiled how much extra work this command causes, especially for worst case of across 4 or more channels simultaneously?

result[1] = (char *)("send rel notes:+a,+b,+c,+d");
break;
case FourCC::InstrumentCommandVibrato:
result[0] = (char *)("VIBrato:aabb");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

its not enough for this to be added to the onscreen help, it needs to be properly documented in both editions of the user manual.

@n1LS
Copy link
Copy Markdown
Contributor Author

n1LS commented Mar 20, 2026

@n1LS given the pico is already cpu limited, have you profiled how much extra work this command causes, especially for worst case of across 4 or more channels simultaneously?

In VIbrato::Trigger(bool tableTick) there's a few additions, one inlined call to the table interpolation, a mutliplication and a bit shift. The table interpolation costs roughly 20 cycles, so the entire call ends up costing roughly 50 cycles.

Trigger gets called from doKrateUpdate @ 441 Hz and from doTickUpdate depending on the bpm, at 120 bpm roughly 50 times -> 160k cycles per second.

Vibrato::UpdateSRP(struct RUParams &rup) is a branch to exit and a single fixed point multiplication, so around 15 cycles. Called the same as above thats 60k cycles per second.

In total that's 220k cycles/second so roughly 0.18% CPU per voice.

@maks
Copy link
Copy Markdown
Collaborator

maks commented Mar 20, 2026

@n1LS ok that seems reasonable but what I'd want is actual profiling numbers vs theoretical instruction usage, have a look at the existing timing code and let me know if you need help using it to time this.

@n1LS
Copy link
Copy Markdown
Contributor Author

n1LS commented Mar 23, 2026

@maks Sure.

This is the typical output.

[PROFILER] Vibrato::Trigger              : avg=   0 us, calls=  505, total=   390 us
[PROFILER] Vibrato::UpdateSRP            : avg=   1 us, calls=  505, total=   788 us
[PROFILER] Vibrato::SetData              : avg=   2 us, calls=    2, total=     4 us

Averaged over 2-3 screens worth of collected data:

Trigger: 0.77597695 µs

@ 441 Hz → 342 µs / 1s
→ 0.0342% CPU load per voice
0.27% for 8 voices

UpdateSRP: 1.63623267 µs

@ 441 Hz → 722 µs / 1s
→ 0.0722% CPU per voice
0.58% for 8 voices

SetData: 2.2375 µs

only gets called on note on, negligible

@maks maks self-requested a review April 7, 2026 08:01
Copy link
Copy Markdown
Collaborator

@maks maks left a comment

Choose a reason for hiding this comment

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

Apart from the code comments, we are still missing user manual documentation for the command.

Comment thread sources/Foundation/Constants/SineTable.h Outdated
uint8_t depth = value & 0xFF;
// setup the vibrato
rp->vibrato_.SetData(rate, depth);
if (!rp->vibrato_.Enabled()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is likely not going to work well with VIB cmds in tables combined with phrases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to cause any issues when applying a bunch of Vibrato commands from both tables and phrases simultaneously. What issue are you referring to?

rate_ = 1 + (rate << 4);

// reset output and phase
phase_ = 0;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

having reset here is likely to cause issues with cmd usage in a table, so we really want phase reset to be separate and do the phase reset only when a vibrato cmd first starts

Copy link
Copy Markdown
Contributor Author

@n1LS n1LS Apr 9, 2026

Choose a reason for hiding this comment

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

@maks I moved the vibrato reset to Start() so that a vibrato always starts at 0 without any jump in frequency. A second reset might helpful when calling ARP 0000 so that a later arp command cannot start with a jump in the frequency. What are your thoughts on that?

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