Tone module: awkward name, unused private vars

upbeat27 commented 4 years ago

First of all, cool lib and looking forward to trying out Daisy hardware!

To me the one pole filter being called "Tone" is awkward due to this being an audio library, and a tone is a musical note. "onepole" is better, or variations thereof depending on preference. Just a suggestion!

The module also has some unused/unnecessary private variables.

stephenhensley commented 4 years ago

Agreed tone definitely seems awkward in context. There is a trivial onepole in dsp.h already, but I think a more specific appropriate name would be fitting (i.e. OnePoleIir, or similar)

stephenhensley commented 4 years ago

Also worth mentioning, good catch on the unused vars! I just noticed them (along with a few others) myself while testing something on my end. They're fixed on #5, though I'll probably move the minor maintenance stuff I caught to a separate PR to merge sooner.

upbeat27 commented 4 years ago

I'd leave iir out of the name since it is implied.

The one pole in dsp.h looks like it has a bug with 'out' not being dereferenced. That one has a bit awkward naming, with the preceding f (for float?) It could be a macro instead and work for floats or doubles (this is coming from a c guy).

For example: https://github.com/pichenettes/stmlib/blob/master/dsp/dsp.h

stephenhensley commented 4 years ago

After some deliberation, the name 'Tone' is going to stay. This is what it's called in Csound/soundpipe where this was ported from (albeit a simple filter, the credit should stay).

csounder commented 4 years ago

excellent decision.

for sure there are some confusing and counterintuitive names in the collection of almost 2000 Csound opcodes; but they also have historical significance and are a standard. There was a day when we "all" wished that tone or atone had more poles and so, we added other filters to Csound that did. Now, there are a lot more interesting, powerful, and amazing filters in soundpipe (from Csound) and in Csound and in the Csound sources that Daisy can draw upon, port and feature. And I am sure that you will. And when all you need is a little, fast, tone filter for a little damping of your reverb algorithm, you will only have to use tone and nothing more computationally intensive.

Dr. Richard Boulanger Professor of Electronic Production and Design Professional Writing and Music Technology Division Berklee College of Music facebook: https://www.facebook.com/richard.boulanger.58

On Mon, May 18, 2020 at 10:03 PM Stephen Hensley notifications@github.com wrote:

After some deliberation, the name 'Tone' is going to stay. This is what it's called in Csound/soundpipe where this was ported from (albeit a simple filter, the credit should stay).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/electro-smith/DaisySP/issues/4#issuecomment-630532212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALWYFXYQXQUZMJM7N3NC6LRSHSHTANCNFSM4LDMIT7A .

electro-smith / DaisySP

Tone module: awkward name, unused private vars #4