Closed tomcombriat closed 6 months ago
Memory usage change @ fdf873cdbafa424a40a5dbab1c0c94d58ed53a05
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8 |
:grey_question: -7492 - +20 | -11.43 - +0.03 | :green_heart: -16 - 0 | -0.08 - 0.0 |
arduino:avr:uno |
0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_giga:giga |
:green_heart: -3424 - 0 | -0.17 - 0.0 | :green_heart: -8 - 0 | -0.0 - 0.0 |
arduino:samd:adafruit_circuitplayground_m0 |
:grey_question: -14492 - +12 | -5.53 - 0.0 | :green_heart: -4 - 0 | -0.01 - 0.0 |
esp8266:esp8266:huzzah |
N/A | N/A | N/A | N/A |
rp2040:rp2040:rpipico |
:grey_question: -2464 - +16 | -0.12 - 0.0 | :green_heart: -48 - 0 | -0.02 - 0.0 |
In the end, I made the table private. Reading a bit more about static variables in headers, it seems fairly clear that this table might end up duplicated otherwise. Good on my side!
Memory usage change @ 9c8e363cb1e3819ed5d6e621114dde4ac1b38080
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8 |
:grey_question: -7492 - +20 | -11.43 - +0.03 | :green_heart: -16 - 0 | -0.08 - 0.0 |
arduino:avr:uno |
0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_giga:giga |
:green_heart: -3424 - 0 | -0.17 - 0.0 | :green_heart: -8 - 0 | -0.0 - 0.0 |
arduino:samd:adafruit_circuitplayground_m0 |
:grey_question: -14492 - +12 | -5.53 - 0.0 | :green_heart: -4 - 0 | -0.01 - 0.0 |
esp8266:esp8266:huzzah |
N/A | N/A | N/A | N/A |
rp2040:rp2040:rpipico |
:grey_question: -2464 - +16 | -0.12 - 0.0 | :green_heart: -48 - 0 | -0.02 - 0.0 |
Looks good to me. It also seems to enable some massive flash savings in some scenarios (probably, only where the compiler can infer which midi notes are used though?).
It also seems to enable some massive flash savings in some scenarios
Ah yes, indeed, I only noticed the RAM… Would not have expected the compiler to make to much optimization, but if it fancy…
Will merge soon!
Hm, experimenting some more, it turns out, when including mozzi_midi.h from more than one .cpp-file (in the user sketch), like this, we will be violating the "one definition rule". Contrary to class member functions, global functions may be defined only once, even if their definitions match. This restriction can be lifted by declaring these functions (and the midiToFreq-table, too) "inline". That in turn is generally ignored as an optimization hint, these days (I tested, and avr-gcc ignores it, too), but lifts the strict one definition rule.
In short: Prefix all definitions with "inline".
As a side-note, I believe to have understood the reason why AVR (contrary to the other platforms) does not show these massive flash improvements: It's because the table is forced into PROGMEM-space, there, and the compiler is not able to deduce that the full table isn't still needed there. (Not something we can do anything about, I believe, and also unlikely to matter in real world scenarios.)
Hm, experimenting some more, it turns out, when including mozzi_midi.h from more than one .cpp-file (in the user sketch), like this, we will be violating the "one definition rule".
Ah, good that you figured out because I had the feeling it worked when I tested (but I might have included in several header, not source file). Will do!
Okay, indeed I have the same problem when I include mozzi_midi.h
in another user source file. I guess this also concerns FixMath2.0 (#212).
and the midiToFreq-table, too
Unfortunately, it seems that C++-17 is needed for that (complain from the compiler: "inline variables are only available with '-std=c++17' or '-std=gnu++17". It also creates a conflict with the Oscillators tables that I did not investigated…
My current solution is to move the table to its own source file, but maybe you have a better solution? This kills a bit the idea that everything in the header but at least all functions definitions are at the same place.
Memory usage change @ 946258225f5db2b0c13d3a7b19ca8d46f24b9586
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8 |
:grey_question: -7492 - +20 | -11.43 - +0.03 | :green_heart: -16 - 0 | -0.08 - 0.0 |
arduino:avr:uno |
0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_giga:giga |
:green_heart: -3424 - 0 | -0.17 - 0.0 | :green_heart: -8 - 0 | -0.0 - 0.0 |
arduino:samd:adafruit_circuitplayground_m0 |
:grey_question: -14492 - +12 | -5.53 - 0.0 | :green_heart: -4 - 0 | -0.01 - 0.0 |
esp8266:esp8266:huzzah |
N/A | N/A | N/A | N/A |
rp2040:rp2040:rpipico |
:grey_question: -2464 - +16 | -0.12 - 0.0 | :green_heart: -48 - 0 | -0.02 - 0.0 |
guess this also concerns FixMath2.0 (https://github.com/sensorium/Mozzi/pull/212).
Actually it does not… But I do not really why. I know that the functions defined inside the class are considered inline
by default, hence not affected, but all the reverse overloads should be from my understanding…
Edit: I now found out that it is because all the functions defined in there are function templates which are not affected by the "one definition rule". Again learned something today ;)!
I must admit, I'm still learning about the one definition rule, too, while working on the "single compilation unit" thing. Fortunately, no true road block(*) in sight, but a bunch of small complications that I had not foreseen.
Anyway, I believe, what may work is this:
class MozziMidiPrivate {
private:
friend function int mtof(uint8_t); // add other functions using this, here
static uint32_t miditoFreq(uint8_t which) {
static CONSTTABLE_STORAGE(uint32_t) lot[128] = { ... };
return FLASH_OR_RAM_READ<const uint32_t>(lot + which);
}
};
I.e. move the definition into a function body, instead, where it ought to be acceptable.
() Besides, strictly speaking, we won't really be needing a single* compilation unit, but only a single compilation unit that is affected by configuration defines.
Okay, the compiler seems to be happy this trick! I just have to test on some hardware and do a bit of cleanup.
Actually, as you said, some things will probably do not need to be in the "main" translation unit. Hence I wonder if keeping the table out in its own cpp file is actually cleaner in order to keep that header clean?
EDIT:
From the workflow (that's quite useful), some platforms do not seem to like it…
error: lot causes a section type conflict with COS2048_DATA
Learning a new error message every day... This is what the internet has to say about it: https://stackoverflow.com/questions/35091862/inline-static-data-causes-a-section-type-conflict . An interesting read, but to me it boils down to: Too difficult to bother with. So, yes, let's indeed keep the table in a separate .cpp file, then (while waiting for availability of C++17).
Memory usage change @ 946258225f5db2b0c13d3a7b19ca8d46f24b9586
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8 |
:grey_question: -7492 - +20 | -11.43 - +0.03 | :green_heart: -16 - 0 | -0.08 - 0.0 |
arduino:avr:uno |
0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_giga:giga |
:green_heart: -3424 - 0 | -0.17 - 0.0 | :green_heart: -8 - 0 | -0.0 - 0.0 |
arduino:samd:adafruit_circuitplayground_m0 |
:grey_question: -14492 - +12 | -5.53 - 0.0 | :green_heart: -4 - 0 | -0.01 - 0.0 |
esp8266:esp8266:huzzah |
N/A | N/A | N/A | N/A |
rp2040:rp2040:rpipico |
:grey_question: -2464 - +16 | -0.12 - 0.0 | :green_heart: -48 - 0 | -0.02 - 0.0 |
Memory usage change @ 17770c0b36502c29bb06fd391b33e6908bc21c44
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8 |
:grey_question: -7492 - +20 | -11.43 - +0.03 | :green_heart: -16 - 0 | -0.08 - 0.0 |
arduino:avr:uno |
0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_giga:giga |
:green_heart: -3424 - 0 | -0.17 - 0.0 | :green_heart: -8 - 0 | -0.0 - 0.0 |
arduino:renesas_uno:minima |
:grey_question: -48 - +16 | -0.02 - +0.01 | 0 - 0 | 0.0 - 0.0 |
arduino:samd:adafruit_circuitplayground_m0 |
:grey_question: -14492 - +12 | -5.53 - 0.0 | :green_heart: -4 - 0 | -0.01 - 0.0 |
esp8266:esp8266:huzzah |
N/A | N/A | N/A | N/A |
rp2040:rp2040:rpipico |
:grey_question: -2464 - +16 | -0.12 - 0.0 | :green_heart: -48 - 0 | -0.02 - 0.0 |
Merging.
I did not try very hard to make the table private (gave up at the first error), but trying with several files importing it does not cause any problem. Will try more intensively if deemed necessary!
Best