Ambika firmware with avr-g++ 9.1

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).

ambika_voicecard.hex.zip (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

-O2
=> ok: 72 Canada
=> bad: 84 bad taste
-O2 osc::RenderCz* with UPDATE_PHASE_MORE_REGISTERS
=> 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 oscillator.cc (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.
ambika_voicecard.hex.zip (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
END_SAMPLE_LOOP

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!
B

@machfour:
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/oscillator.cc -o build/ambika_voicecard/oscillator.o

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

@Bjarne:
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…

@Bjarne
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.
@stefanovic
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.

ambika_voicecard.bin.zip (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.

Update: I have the controller firmware also working now, and somehow down to 50908 bytes! I’m not sure what happened here, it seemed to just shrink magically after some refactoring :smile:

If anyone wants to test out my firmware, I will post .bin files below (and .elf, for the curious). It shouldn’t be functionally any different from the official v1.0, but just for fun I bumped the version number to 1.1, because I have spent way too many hours on this, and I feel like it’s worth celebrating a bit :stuck_out_tongue_closed_eyes: Of course, please let me know if you find any problems!

ambika-firmware-machfour.zip (146.1 KB)

3 Likes

Thats great news! Will you put this on GitHub? I still have some additions I’d like to make for integrating Ambika better with DAWs but its impossible working with a couple of bytes free. If this is glitch free, it would be just what the doctor ordered.