Specializations of Mozzi classes for U/SFix - #236
Merged
Conversation
Memory usage change @ 285795e
Click for full report table
Click for full report CSV |
Memory usage change @ 59b1778
Click for full report table
Click for full report CSV |
tfry-git
commented
Feb 18, 2024
Collaborator
I suppose you tried some stuff (while I did not). But first question: Is a full specialization needed at all, won't the default implementation cover most of this? (Side note: Those Runtime polymorphism would surely impose a performance hit, indeed (going quite counter to the whole idea of UFix/SFix). Toying with the idea, however, maybe there could be a common template: And after than, UFix and SFix would simply be given as template aliases: I admit, that looks a bit daunting... |
tomcombriat
commented
Feb 18, 2024
CollaboratorAuthor
Thanks for your comment!
Indeed, only some functions might need to be specialized (I copied the way the specialization is done), but they are the interesting/complex ones (but implementation is the same for U/SFix). To be fair, this only concerns
Will do. I was actually wondering about them. |
Memory usage change @ ebca5ab
Click for full report table
Click for full report CSV |
Memory usage change @ 9e1c4ad
Click for full report table
Click for full report CSV |
CollaboratorAuthor
Small note while I am here: I am now in the process to convert the examples to Note that I sometimes keep the original ones as well, for now for historical reasons, we can decide later if we want to replace everything or keep some of them. (I have a slight sentimental attachment to the FM_synth example…). Note also that, in the |
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
February 24, 2024 21:09
3bba32e to
7e1cf70CompareMemory usage change @ 7e1cf70
Click for full report table
Click for full report CSV |
Added documentation
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
February 25, 2024 11:10
7e1cf70 to
5f2f36eCompareMemory usage change @ 5f2f36e
Click for full report table
Click for full report CSV |
Memory usage change @ cfe7973
Click for full report table
Click for full report CSV |
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
February 26, 2024 19:52
f460ba5 to
05ef0f1CompareTypo in AudioOutput
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
February 26, 2024 20:00
0687042 to
dbed60aComparetomcombriat
commented
Feb 26, 2024
| toSFraction(aCos5.next()) + toSFraction(aCos5b.next()) + | ||
| toSFraction(aCos6.next()) + toSFraction(aCos6b.next()); /* + | ||
| toSFraction(aCos7.next()) + toSFraction(aCos7b.next()) +*/ | ||
| return MonoOutput::fromSFix(asig); |
CollaboratorAuthor
There was a problem hiding this comment.
@tfry-git Here comes the automatic output scaling :)
Memory usage change @ dbed60a
Click for full report table
Click for full report CSV |
CollaboratorAuthor
There seems to be a slight cost of using |
This comment was marked as outdated.
This comment was marked as outdated.
CollaboratorAuthor
There are still a few of the "legacy" mozzi_fixmath here and there but I also have the feeling that having the two paradigms is also useful…
Manual flash / RAM changes (on Uno, according to my IDE)
|
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
March 4, 2024 20:37
c11e5d9 to
f5a433eCompareFix FixMath Removed comment
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
March 4, 2024 20:39
f5a433e to
8b760a6CompareRemoved comment
tomcombriatforce-pushed
the
devel/mozziClass_FixMath
branch
from
March 4, 2024 22:01
e1960f9 to
57f94f4CompareMemory usage change @ be0a9b0
Click for full report table
Click for full report CSV |
tomcombriat
marked this pull request as ready for review
March 5, 2024 20:11
Memory usage change @ f7a40f2
Click for full report table
Click for full report CSV |
Memory usage change @ 74ef271
Click for full report table
Click for full report CSV |
tfry-git
left a comment
Collaborator
There was a problem hiding this comment.
A quick review so as not to keep you waiting too much longer. I did not look super-closely at the examples, and importantly, I did not test even a single one...
| #ifndef AUDIOOUTPUT_H | ||
| #define AUDIOOUTPUT_H | ||
| #include <FixMath.h> |
Collaborator
There was a problem hiding this comment.
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:
//#include <FixMath.h> -> removed
template<int8_z, int8_t, uint64_t> class SFix; // forward declaration, only
// This partial specialization is only compiled when actually used
// therefore it's ok that SFix is an incomplete type, so far.
template<int8_t NF, uint64_t RANGE> void someFunction(SFix<16,NF,RANGE> x) { [...] };
Collaborator
There was a problem hiding this comment.
And to clarify, FixMath.h would then have to be included, manually, in sketches that actually use it.
CollaboratorAuthor
There was a problem hiding this comment.
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 defined and not File 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!
CollaboratorAuthor
There was a problem hiding this comment.
I might be overlooking something, but I am not sure this is always simple. Let's take this line from Oscil.h:
template <int8_t NI, int8_t NF, uint64_t RANGE>
inline
void setFreq(UFix<NI,NF,RANGE> frequency)
{
setFreq_Q16n16(UFix<16,16>(frequency).asRaw());
}
with the forward declaration setFreq(UFix<NI,NF,RANGE> frequency) is correctly resolved by the compiler but UFix<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?
Collaborator
There was a problem hiding this comment.
Did I overlooked something?
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
setFreq_Q16n16((int32_t) frequency.sL(16-NF).asRaw());
But I'm not sure, where the next problem of this type pops up, and I'll admit, I'm not totally convinced, myself.
| @@ -157,9 +157,9 @@ class Oscil | |||
| each direction. This fixed point math number is interpreted as a SFix<15,16> internally. | |||
Collaborator
There was a problem hiding this comment.
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)
CollaboratorAuthor
There was a problem hiding this comment.
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.
Collaborator
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tomcombriat
commented
Mar 10, 2024
CollaboratorAuthor
Thanks for the comments! I think the optimizations can also be taken care of further down the line (I though the same when working on mtof) but I will start thinking on it. If I come up with a nice solution (might be very easy) before we want to merge I will it here. |
This comment was marked as outdated.
This comment was marked as outdated.
Memory usage change @ e8376db
Click for full report table
Click for full report CSV |
Memory usage change @ 574fb90
Click for full report table
Click for full report CSV |
tomcombriat
commented
Mar 28, 2024
CollaboratorAuthor
tfry-git
commented
Mar 28, 2024
Collaborator
tomcombriat
commented
Mar 28, 2024
CollaboratorAuthor
Top! I won't be able to work on it in the next days but will try to get it up to date when I come back for merging! |
Memory usage change @ 4f00936
Click for full report table
Click for full report CSV |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Still work in progress. So far only
LineandSmoothare done.I am a bit sad that there is so much code duplication between
Line<UFix>andLine<SFix>but up to now, I did not figure a way to avoid that. Maybe a virtual mother classFixType, which bothUFixandSFixwould inherit from would be the solution? (I read here and there that the performances of such dynamic polymorphism are not as good, except by doing Curiously recurring template pattern maybe?)