- 
                Notifications
    You must be signed in to change notification settings 
- Fork 193
Specializations of Mozzi classes for U/SFix #236
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
Changes from all commits
285795e
              59b1778
              ebca5ab
              9e1c4ad
              8d69afc
              5f2f36e
              b088666
              cfe7973
              05ef0f1
              dbed60a
              672ce21
              4b483dd
              a52b44e
              1730643
              5a121db
              75ad03d
              be74b3b
              7b2b1d3
              8b760a6
              57f94f4
              be0a9b0
              f7a40f2
              74ef271
              273f2c0
              e8376db
              574fb90
              4f00936
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -159,9 +159,9 @@ class Oscil | |
| each direction. This fixed point math number is interpreted as a SFix<15,16> internally. | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimization question (maybe for later): Aren't those 15 NI bits fully ineffective, 16 fractional bits alone would fully suffice? (Sign might be a problem, I imagine) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I thought of that too. Some examples (for instance FMSynth) do wander further than pure fractional, but as it rolls back, only fractional should be enough (and further is taking care of smoothly by the overflow). Indeed the sign is a problem, but maybe an extra function taking SFix<0,15>, so sacrificing one bit of precision, could be interesting. Might not be straightforward to make the compiler choose between the two though. I would suggest taking that a bit further down the pipeline as this is more optimisation of legacy code that directly related to FixMath. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Maybe collection things like this somewhere would help keeping track, though. Perhaps in an issue, or by marking them as "TODO: optimize" in the code. | ||
| @return a sample from the table. | ||
| */ | ||
| template <byte NI, byte NF> | ||
| template <int8_t NI, int8_t NF, uint8_t RANGE> | ||
| inline | ||
| int8_t phMod(SFix<NI,NF> phmod_proportion) | ||
| int8_t phMod(SFix<NI,NF,RANGE> phmod_proportion) | ||
| { | ||
| return phMod(SFix<15,16>(phmod_proportion).asRaw()); | ||
| } | ||
|  | @@ -216,11 +216,11 @@ class Oscil | |
| @note This didn't run faster than float last time it was tested, after 2014 code changes. Need to see if 2014 changes improved or worsened performance. | ||
| @param frequency in UFix<NI,NF> fixed-point number format. | ||
| */ | ||
| template <int8_t NI, int8_t NF> | ||
| template <int8_t NI, int8_t NF, uint64_t RANGE> | ||
| inline | ||
| void setFreq(UFix<NI,NF> frequency) | ||
| void setFreq(UFix<NI,NF,RANGE> frequency) | ||
| { | ||
| setFreq_Q16n16(UFix<16,16>(frequency).asRaw()); | ||
| setFreq_Q16n16(UFix<16,16>(frequency).asRaw()); | ||
| } | ||
|  | ||
|  | ||
|  | @@ -256,8 +256,9 @@ class Oscil | |
| less than 64 Hz. | ||
| @param frequency in UFix<24,8> fixed-point number format. | ||
| */ | ||
| template <uint64_t RANGE> | ||
| inline | ||
| void setFreq(UFix<24,8> frequency) | ||
| void setFreq(UFix<24,8,RANGE> frequency) | ||
| { | ||
| setFreq_Q24n8(frequency.asRaw()); | ||
| } | ||
|  | @@ -295,8 +296,9 @@ class Oscil | |
| @note This didn't run faster than float last time it was tested, after 2014 code changes. Need to see if 2014 changes improved or worsened performance. | ||
| @param frequency in UFix<16,16> fixed-point number format. | ||
| */ | ||
| template <uint64_t RANGE> | ||
| inline | ||
| void setFreq(UFix<16,16> frequency) | ||
| void setFreq(UFix<16,16,RANGE> frequency) | ||
| { | ||
| setFreq_Q16n16(frequency.asRaw()); | ||
| } | ||
|  | @@ -309,9 +311,9 @@ class Oscil | |
| @note This didn't run faster than float last time it was tested, after 2014 code changes. Need to see if 2014 changes improved or worsened performance. | ||
| @param frequency in SFix<16,16> fixed-point number format. | ||
| */ | ||
| template <byte NI, byte NF> | ||
| template <int8_t NI, int8_t NF, uint64_t RANGE> | ||
| inline | ||
| void setFreq(SFix<NI,NF> frequency) | ||
| void setFreq(SFix<NI,NF,RANGE> frequency) | ||
| { | ||
| setFreq_Q16n16(UFix<16,16>(frequency).asRaw()); | ||
| } | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a somewhat general note, I wonder, whether we can still avoid a hard dependency on the - now external - FixMath. Sure, installing it via the library manager should be a breeze, but at least when installing from git, that may be a small bonus.
The trick would be to never have a full specialization. Due to the RANGE parameter, I think that requirement is actually easy to meet. Pseudocode:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to clarify, FixMath.h would then have to be included, manually, in sketches that actually use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, I did not think of it. My only concern would be that the dependency is less clear (ie, the error message would be something like:
SFix is declared but never definedand notFile FixMath.h not found). But at the same time, the error message for the examples would be clear. Will try to see how that goes!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be overlooking something, but I am not sure this is always simple. Let's take this line from Oscil.h:
with the forward declaration
setFreq(UFix<NI,NF,RANGE> frequency)is correctly resolved by the compiler butUFix<16,16>(frequency)is not as it does not have the correct number of arguments. Adding another argument, say_RANGE, is not straightforward because we would need to add in Mozzi the macros to compute the default range here (hence negating a bit the advantage of having it with a default value in FixMath.Did I overlooked something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the fact that I tend to comment before testing... It may or may not be possible to weasle our way out of this particular problem using something like
But I'm not sure, where the next problem of this type pops up, and I'll admit, I'm not totally convinced, myself.