Ambika firmware with avr-g++ 9.1

So I’ve spent almost a week on this now. I’m still not quite there with the voicecards, there were a few bugs that I fixed (that I had introduced myself while refactoring), but it still doesn’t sound good.

If I play a fixed note, set the oscaillator mix to 0 (all OSC1), and set OSC1 waveform to be a wavetable synthesis algorithm (or some others, but notably not Wavequence or the cz ones), the pitch of OSC1 is incorrect, and actually wavers if I change the selected waveform for OSC2, or the crossmod function…

But I can’t think of anything else I can do except for optimisation to reduce cycles, since compiling with -O2 sounds better than -Os. I think I need a break for the moment, may come back to it in a few weeks.

Anyway my code is up there at I’ve been a bit unapologetic with refactoring so I don’t recommend trying to patch my commits on top of anyone else’s forks.

@machfour: I know how frustrating this activity is sometimes, and I fully sympathise.

Still, I decided to give it another try.

…and I think I almost nailed it (until I’m proven wrong of course).

First I enabled all warning flags: -Wall -Wextra -pedantic and I (painfully) suppressed all benign warnings.

But the most useful one is - Winline, which warns when inlining has not occurred for whatever reason.

It turns out that with -Os avg-gcc 4.3 inlines most sensible parts, while with -Os avr-gcc 9.3 decides to not inline some important asm code.

avr-gcc 9.3 -O2 does succeed in inlining all inlined-tagged functions, and produces a 30192 bytes firmware which is smaller than the limit (with a few tweaks though).

Still, some patches sounded weird, especially FM-based ones.

…until I discovered potentials slips in the code. When fixed, all patches sound right to my ears, except “voxelito”, and vowel-based sound in general.

A voicecard firmware should be joined to this message: it would be nice if someone confirms or refutes that it sounds right (again, except voxelito).

One thing is, if it’s right, then the sounds with this firmware might be different, as I changed (fixed?) the code.

Hopefully tomorrow I’ll find the last fix (I still haven’t figure out the vowel code), and I’ll send a PR to Emilie. (24.5 KB)

1 Like

Hi Stefanovic,

I actually returned to it this morning hehe, and found some silly mistakes I had made in my refactoring. In particular, I found I had caused a bug last year in the S16ClipU14 function from the Avril library which would have affected you … I apologise for any time you wasted on that! :disappointed_relieved:
(Having said this, I have now learned even more to be incredibly careful when refactoring code sprinkled with things like a = x & 0x0f; b = x & 0xf0; and coexisting variable names like phase_ and phase, which can lead to incredible amounts of time wasted trying to find bugs).

I am now up to the same stage as you: my new firmware, compiled with -O2 sounds almost identical to the official fimware, except for the ‘voxelito’ patch . I tried to add -Winline however no warnings were generated by my gcc.

I have listened to your firmware, and it sounds good! I hear the same kind of artifact on the Voxelito patch as with my firmware. I’ve uploaded mine here, and the source is updated on Github.

Update: I believe that the problem with the Voxelito patch is somewhere in the modulation code (LoadSources / ProcessModulationMatrix / UpdateDestinations) because if you play with the mod matrix when Voxelito is loaded, you can hear weird things with the modulations. (I have one voicecard with my custom firmware, and the other ones loaded with the stock firmware, so I can do like A/B testing). (36.5 KB)

1 Like

yes fixing macros like phase_ makes FM sound correctly.
-O2 does inline, it’s -Os that does not. I suspect some compiler parameter tweaking might inline code even in -Os mode.
as for the S16ClipU14 bug: I don’t understand, is it in your code? or in the avril github code? It’s used in the modulation matrix…

Using your suggestion avr-gcc -Os -Q --help=optimizer seems to show that all the inlining options are actually enabled with -Os, but with -O2 they are actually not enabled (-finline-functions is disabled which disables the others I believe). But you are right in practice, when I looked at the disassembly, there were functions not inlined when I thought they should have been… strange.

As for the bug, it was in the version of Avril that I pushed to my own github account back when I did work on the Midipal. So if you used my avril code then you would have the bug. It’s easy to check: just make sure you don’t have a line like uint8_t msb = lowByte(x) in the function. (Side note: when you use a properly named function rather than x & 0xff the bug becomes immediately obvious… unfortunately I did not think to check for bugs in code that I wrote a year ago :sweat_smile:)

I did not use your base, fortunately :wink:

ok, so here are the results of my last investigations.
I started with a base that has practically no modification compared to master code, except some statements to silence “unused variables” warnings.
In order to make gcc 9.3 actually inline all code with -Os, we must set --param max-inline-insns-size to at least 31 (more will not harm that much I guess).

-Os :
=> bad: 01 juno
-Os --param max-inline-insns-size=31
=> ok: 01 juno, 17 flat eric
=> bad: 41 FmFmFmFm
-Os --param max-inline-insns-size=31 osc::RenderFm with UPDATE_PHASE_MORE_REGISTERS
=> ok: 41 FmFmFmFm good
=> bad: 72 Canada

=> ok: 72 Canada
=> bad: 84 bad taste
=> ok: 84 bad taste
=> bad: 110 Voxelito

I could not make Voxelito work unfortunately. I had a look to the generated assembly code, to no avail. There are some differences (more registers pushed on the stack, could not get rid of them with -fomit-frame, or some code moved around), but nothing obvious to my untrained eyes.

I have some hypotheses in mind, but they might be highly worthless…
So it’s better, but we are not quite there yet. I think I’ll pause for a while, and see if you @machfour or someone else come with fresh ideas and skills.

Hello again! I have made some discoveries:

  • I believe I have found the problem with the Voxelito patch, and it is actually from the RenderVowel() function in (so any use of the vowel wave should sound bad). The problematic line is
    uint8_t x = S16ClipS8(4 * result) + 128;
    Specifically, the (optimised) S16ClipS8() function in op.h doesn’t work properly. You should replace it with the unoptimised one: return x < -128 ? -128 : (x > 127 ? 127 : x);. The RenderVowel() function is the only time ever in the Ambika code that the S16ClipS8 function is called, which I think is why it was hard to pick this one up.
    My Voxelito patch sounds good now, but could you try this change out in your code and see if it fixes the problem?
  • As for the RenderCz* functions: interestingly, they have all in common that they use ReadSample(), which is used nowhere else in the code. I wonder if manually inlining the ReadSample() function by hand - i.e. replacing calls to it with the corresponding ResourcesManager::Lookup call might improve things? Anyway, I used the UPDATE_PHASE_MORE_REGISTERS trick that you suggested, and the ‘Badtaste’ patch sounds good, so no real worries here.

So at this point I’d say that the new voicecard firmware is sounding just like the stock one! I’ll upload a hex file here in case anyone wants to test. (36.3 KB)

Hi @machfour, nice you’re still on it!

I replaced S16ClipS8 with the unoptimized version, but unfortunately I still get a garble sound :thinking:, with -O2…

[EDIT] I tried without the clipping function: uint8_t x = result + 128; The sound has a lower level, as expected, but is still garbled. S16ClipS8 might be faulty, but there might be another culprit…

I actually replaced the entire call because it seemed simpler to calculate the value directly. Here is how the end of that function looks in my code now:

// (-32, 32) -> (0, 255)
//uint8_t x = S16ClipS8(4 * result) + 128;
uint8_t x;
if (result <= -32) {
    x = 0;
} else if (result >= 32) {
    x = 255;
} else {
    x = U8(result + 32) << 2u; // U8(x) is just static_cast<uint8_t>(x)
*buffer++ = x;
*buffer++ = x;
size--; // second decrement

Let me know if this works or doesn’t work. By the way, I am compiling with -O2 as well.

If the sound is “crackling” it may simply be a matter of cpu overload due to insufficient optimization since vowel is one of the most computationally demanding algorithms. (Sorry for pointing out the “bleeding obvious”.)

Great work in this thread!

still no luck.
Now that’s strange, if you succeeded, there is no reason I cannot. Here are the g++ options I have:

/usr/local/bin/avr-g++ -c -mmcu=atmega328p -I. -DF_CPU=20000000 -D__PROG_TYPES_COMPAT__ -fdata-sections -ffunction-sections -fshort-enums -fno-move-loop-invariants -DDISABLE_DEFAULT_UART_RX_ISR -DDISABLE_WAVETABLE_LFOS -Wall -Wextra -Winline -pedantic -g -Wno-deprecated-declarations -Wno-attributes -O2 --param max-inline-insns-size=31    -DATMEGA328P -DSERIAL_RX_0 -mcall-prologues -fno-exceptions voicecard/ -o build/ambika_voicecard/oscillator.o

with this avr-g++:
gcc version 9.3.0 (Homebrew AVR GCC 9.3.0)

yes, the question is which optimization that was applied with 4.3, is not anymore with 9.x?
The fix with UPDATE_PHASE_MORE_REGISTERS suggests that the use of registers(?) instead of object fields is improving things. Vowel also seems to heavily use tables. So, it may be related to the way the code accesses memory with 9.x, or with recent AVR-related headers (pgm_read_word maybe?).
These are all speculations of course.
First I should find out why machfour’s code works for him and not for me…

It was not really ‘crackling’, which I believe I did hear from the CZ* functions, before I added UPDATE_PHASE_MORE_REGISTERS. It was more like noticeable distortion, which varied in quality when the oscillator parameter was changed.
Hm, it after some testing, it seems I wrongly accused the S16ClipS8 function of not working as intended. But I think it does rely on implementation defined behaviour… Just for a sanity check, could you please check that the firmware I uploaded before sounds fine on the Voxelito patch?

Here are my compilation flags, I have changed them a bit. By the way, I am also using 9.3.0.

/usr/bin/avr-g++ -c -g -O2 -std=c++2a -I. -mmcu=atmega328p -Wall -Wextra -pedantic -Wno-narrowing -Winline  -ffreestanding -fno-common -fshort-enums -fno-move-loop-invariants -nostdlib -fverbose-asm -fdata-sections -ffunction-sections -flto -mrelax -mcall-prologues -DF_CPU=20000000 -DDISABLE_DEFAULT_UART_RX_ISR -DDISABLE_WAVETABLE_LFOS  -DATMEGA328P -DSERIAL_RX_0  -fno-exceptions -fno-non-call-exceptions -fno-use-cxa-atexit -fno-rtti 

Removing the irrelevant flags, we get

/usr/bin/avr-g++ -O2 -std=c++2a -Winline -ffreestanding -fno-common -fshort-enums -fno-move-loop-invariants -nostdlib -fverbose-asm -fdata-sections -ffunction-sections -flto -mrelax -mcall-prologues -fno-exceptions -fno-non-call-exceptions -fno-use-cxa-atexit -fno-rtti

The most significant differences I can see are that i used -mrelax and -flto which may improve things for you. See if that helps. Otherwise, you are welcome to dig around / ask questions about my (significantly refactored) code to see if anything else works for you.

(unfortunately,) my ambika is not dismantled anymore, which means my only way to upload a voicecard firmware is to use the SD card method with a .bin: can you send the .bin instead please?

I don’t have a bin file in my build directory… it’s just made by running avr-objcopy -I ihex -O binary ambika_voicecard.hex ambika_voicecard.bin… right?

the .hex and .bin file are made from .elf.
can you try make bin maybe?

The command was make build/ambika_voicecard/ambika_voicecard.bin, but I checked using diff and it’s exactly the same output as if you run the command I gave before. Anyway, here’s the bin file. (24.4 KB)

sorry I misread your previous answer :man_facepalming: you were right about the command.
and I confirm that your firmware is working! :grinning:

1 Like

Awesome! Well as you said, at least we now know that it’s possible to get it working :laughing:

I’m currently working on some refactoring of the controller. Hopefully I haven’t introduced any bugs…

now the question is: what exactly made your firmware work?

Also, I find it too bad that we need to resort to UPDATE_PHASE_MORE_REGISTERS: either the genuine firmware was very close to the max capacity of the proc and any change would have exhibit garbled sound, or avr-gcc 9.x produces less efficient code, which is a disappointment… Without better debug tools (and better skills for me), it is difficult to understand what happens here.

(Other related news: the gcc crew has obsoleted support for avr in the next version. There is a bounty to help port the avr backend to the new gcc backend engine, but the current amount seems far from enough…)

Yeah, the fact that we’re working in an embedded system context makes it really hard to get a good understanding of what’s going on at runtime. I guess it’s possible to use a simulator by developing software dummies for the necessary hardware components… but that’s a lot more work than it’s worth now.

Are they really deprecating AVR support in GCC 10??? That is so sad :disappointed: I knew there was a lack of GCC developers from the AVR backend but I hoped it would pick up somehow.