Skip to content

Conversation

@gamblor21
Copy link
Member

Fix for #10286 and #10200.

There may still be more locations where fixed point could be handled better in synthio but this fix hits two big ones that I discovered so worth fixing those now.

Also refactored sat16 that will shift a fixed point result back into 16 bits out of audiofreeverb and into synthio to be used by any audio module that needs it. Potentially worth moving this to a generic "audiohelper" type module in the future in case someone builds without synthio but still wants audio effects.

@gamblor21
Copy link
Member Author

I updated the tests as this change breaks most of them. I looked and as far as I could tell it is small rounding differences due to how I fixed this issue. I also listened but would appreciate it if someone else can confirm that things sound right. The rounding changes were about 1 (for ints) or less then 0.0001 for floats so not large for audio.

@relic-se
Copy link

Potentially worth moving this to a generic "audiohelper" type module in the future in case someone builds without synthio but still wants audio effects.

Not that it's necessary for this PR, but would audiocore be a reasonable location in the future?

Copy link

@relic-se relic-se left a comment

Choose a reason for hiding this comment

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

Looks clean so far. I haven't yet tested it, but I will extensively soon.

@relic-se
Copy link

Just finished running tests. No more distortion across the board!

Now that we can push Q up to much higher values, I'm a bit disappointed that it doesn't appear to want to self-oscillate like you'd expect from an analog filter. After a bit of research, I was able to find some anecdotes about biquads not being able to self-oscillate without the help of additional processing (using an envelope follower controlling gain). For our purposes, I think we can live without it.

@gamblor21
Copy link
Member Author

Todbot has tested this (comment on #10200) and it fixes the issue he was having as well.

@gamblor21 gamblor21 requested a review from tannewt April 27, 2025 14:33
}
int16_t idx = accum >> SYNTHIO_FREQUENCY_SHIFT;
int16_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768;
int16_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768; // consider for synthio_sat16 but had a weird artificat
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int16_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768; // consider for synthio_sat16 but had a weird artificat
int16_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768; // consider for synthio_sat16 but had a weird artifact

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I'll catch it my next time. It mostly is a note to remind myself to look at it more in the future anyways.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

One typo you may want to fix first. I don't really care so I approved.

@gamblor21 gamblor21 merged commit 1e4d766 into adafruit:main Apr 28, 2025
474 checks passed
@jepler
Copy link

jepler commented Apr 29, 2025

thank you!

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.

4 participants