Single compilation unit / In-line configuration #224
Merged
Conversation
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
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
2 times, most recently
from
7eef203 to
47d385f
Compare
(This is expected to increase flash size, marginally, but the effect will mostly go away once implementation and usage are once again in the same compilation unit.)
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
47d385f to
2fa17a4
Compare
While at it, use explict precision types.
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
49af0d5 to
74d2814
Compare
When int is not an alias to intXY_t, compilers may be too stubborn to understand, which overload to use. Conversely, having both an int16_t and and int overload is not possible on AVR (where the two _are_ an alias).
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
c5fe44c to
1856897
Compare
|
Memory usage change @ 1856897
Click for full report table
Click for full report CSV |
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
2 times, most recently
from
0d63ad6 to
88303c1
Compare
…tant step for single compilation unit) This required introduction of a private namespace, so that internals do not leak out and clash with user code. That, in turn, meant changes all over the place.
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
88303c1 to
51ae09a
Compare
|
Memory usage change @ 51ae09a
Click for full report table
Click for full report CSV |
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
cfaa748 to
ee4efe3
Compare
…e time (but continued to occupy flash)
|
Memory usage change @ 832cf3a
Click for full report table
Click for full report CSV |
Adjusted two of the stereo examples for starters
tfry-git
force-pushed
the
devel/single_compilation_unit
branch
from
6a0e001 to
f0bdae2
Compare
|
Memory usage change @ 6a0e001
Click for full report table
Click for full report CSV |
tfry-git
changed the title
|
Memory usage change @ 9d64122
Click for full report table
Click for full report CSV |
Collaborator
Author
|
I promised to explain a bit, what I have done (not in any particular order).
TLDR:
|
Collaborator
|
Without looking at the diffs yet (I tried to kept up to date, but that went a bit fast in the end…), some preliminary answers:
Completely, this actually makes more sense to me, name-wise.
Seems like a clean solution to me, isn't that what namespaces are for basically?
Will try to do soon, alongside a more in-depth review! |
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
| #undef MOZZI__LEGACY_AUDIO_INPUT_IMPL | ||
|
|
||
| // "export" publicly accessible functions defined in this file | ||
| // NOTE: unfortunately, we cannot just write using MozziPrivate::mozziMicros(), and that will conflict with, rather than define mozziMicros() |
Collaborator
There was a problem hiding this comment.
I might be a bit slow, but for the sake of learning could you explain me why MozziPrivate::startMozzi() would not work for instance?
Collaborator
Author
There was a problem hiding this comment.
In a vain attempt at keeping the source somewhat readable, I trying not to "enter" and "leave" the MozziPrivate-namespace too often. I.e. the basic idea was to write
namespace MozziPrivate {
void privateFunction1() {}
void privateFunction2() {}
void publicFunction1() {}
void privateFunction3() {}
}
void publicFuction1() { MozziPrivate::publicFunction1(); }
rather than:
namespace MozziPrivate {
void privateFunction1() {}
}
namespace MozziPrivate {
void privateFunction2() {}
}
void publicFunction1() {}
namespace MozziPrivate {
void privateFunction3() {}
}
Maybe the second variant is preferable after all? I'm not sure at all (too bad we cannot just write void MozziPrivate::privateFunction1() { [definition] }).
Collaborator
Author
There was a problem hiding this comment.
Or perhaps it would help to simply ignore conventional code-formatting rules?
namespace MozziPrivate { void privateFunction1() {
[definition]
}}```
Collaborator
There was a problem hiding this comment.
But in that case, users would need to use the Mozzi namespace right? Or would the "export" part would still be there?
A global namespace (mozzi::) would be neat for all functions in my opinion but:
- it might be intimidating to some users
- this would break existing sketches badly (we are already there, but not that badly).
Collaborator
Author
There was a problem hiding this comment.
Two somewhat separate but related topics, here:
As for the different variants of defining the MozziPrivate-namespace: The latter two variants would not need the "export" trick, because the "public" functions would not be placed inside MozziPrivate, in the first place. In contrast, the first variant (== current state of the PR) puts everything into MozziPrivate, and then "exports" the public ones.
I have indeed also been thinking a bit about adding a mozzi-namespace. Note that we could make it such that a default setup and/or examples could simply include a using namespace mozzi; line, which could again be turned off by some config switch, if desired. I have already added a MOZZI_COMPATIBILITY_LEVEL config define (mostly unused for now), which could also help with such a transition (while the compatibility level is set to < 2.0, the using line would be included by default, to keep old sketches working, but for >= 2.0, the default could be reversed).
In other words, the impact on users could be kept quite minimal, but without a doubt, we'd need to add a whole bunch of "namespace" statements, everywhere.
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
tomcombriat
reviewed
Jan 7, 2024
Collaborator
tomcombriat
left a comment
tomcombriat
left a comment
There was a problem hiding this comment.
Okay, I scanned through and left some minor comments here and there, principally to get a good grasp of what is going on.
Will try to do some testing soon!
Collaborator
TestingI will update here what tests I do and there results as I go.
Quick thought, before I forget. I know I was the author of the Not sure anyone would need to change them, but while this is still fresh maybe worth deciding how much is hidden and not editable and how much should be protected but still editable? That does not concern only the RP2040, but probably other platforms where fine hardware tuning were needed. To be honest, I think this is probably easier for users as it is right now, and probably easy to change afterward anyway… |
tomcombriat
approved these changes
Jan 8, 2024
Collaborator
Author
|
Ok, in order to keep everything from diverging too far, I think I'll merge this (into devel/Mozzi2), later tonight. As far as I understand, the FixMath2 branch is currently stalled waiting on this, because you are still going to touch the examples, right? I'll try to get that in sync, too. We can still address the question of how to best set up the MozziPrivate-namespace, later. (Even after Mozzi 2.0, as it won't be user visible). Regarding your question about additional options in config_example_rp2040_i2s_pt8211.h, I concur, we should not make too much detail too visible. As a rule of thumb, I'd say, anything that isn't immediately expected to make a useful difference "outside the box" should be considered an implementation detail and remain hidden. (E.g. the number of separate buffers). |
Collaborator
|
Hi,
Well, not really… It is stalled because I do not manage to put enough time on it these days… But it starts to come to an end,
I agree. I did not have to work on that file yet (but that might come with the next port I have in mind for the ATTiny), so I honestly do not have a super clear idea of what is best… |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Notes:
Regarding flash (and RAM) usage changes:
I left only the reports at a few steps that I think are meaningful, to get a clearer picture, and removed the other ones. What we see is: