Ambika build has noisey SPI bus [SOLVED]

I was aware of there sometimes being a problem with size of code, but it actually fits. So if it’s a avrgcc version problem, it’s because it somehow introduced a bug.

I’m going to use the hexs tonight to test the theory. It would be nice if it was easy to manage multiple specific versions of avr-gcc.

Just a note: I believe it may be perfectly normal that the mobo is constantly talking to (selecting) each voicecard to update lfos etc so this in itself is probably not the issue.

Cheers

Im going to double check tonight to confirm it’s also bringing the SD reader SS high constantly too, because that’s not expected as far as I understand.

Can confirm, its still making weird noises even with the precompiled hex’s

Okay. Then it’s probably a soldering defect somewhere on the motherboard

i wish i could figure out for sure where, because ive not found any yet lol.

reflowed all the areas that as far as i can tell could possibly be involved in this (considering everything else appears to work (display, menu diving, buttons etc) No luck.

Last night i got sysprogs analzyer2go working with my cypress fx3 dev board (it works amazingly btw). It looks like the voicecards are replying with FF, a whole lot, and the master is shooting out complete madness to the voicecards. Lots of data that isnt even listed in the tech notes as actual SPI commands used by the ambika, with a few real commands like 00x50/51/52 laced throughout.

im going to replace the atmega644p and recompile and re-flash everything tonight.

Fixed it last night! it wasnt the hardware at all. I had massaged some minor things in the firmware code to get it to compile and fit with newer version of GCC-4. Last night i hunted down the OG 4.3.3 version it was originally written for.

I powered it up, tested tweaking knobs, everything appaears to be working now! huzzah!

You managed to get it compile fine using a brew version of avr 4.3.3 ? I’d like to compile the ambika firmware on my Mac using a brew solution or virtual machine/environment…

you can get specific versions here. indispensable

1 Like

that looks awesome, never found it while searching the interwebs. Will definitely check it out later today :slight_smile:

So I managed to compile it downloading a 2010 version. I did have to change the source code to

// #include "voicecard\resources.h"
#include "resources.h"

on both voice card and controller.

Just out of interest, what was the buggy ‘minor change’ you made that broke the firmware for newer GCC?

I’m currently trying to compile MIDIPal firmware using GCC 8.2, but it also required ‘massaging’. But I’ve been spending ages trying to diagnose some lingering bugs. It’s definitely not a size thing, I’ve checked both the static/flash size and the RAM usage during runtime, so at this point I’m out of ideas.

I would suggest getting the old crosspack. The changes i had made were to due to some of the assembly code having registers appearing to be assigned wrong, I had thought i had it working but so many weird issues with timing were happening (probably because thats what a lot of the assembly code does in the ambika source).

Yeah, getting crosspack is my current plan of attack. Then if I have the time and inclination I might compare the output .elf files. Unfortunately I am on linux so I’ll also have to find a way to run it…
Thanks for the advice!

i spent more time poking around than i had hoped, but i found something that should work for getting 4.3.3 running in linux. (when i give my work macbook back, ill be glad i spent this time!)

Yeah, after some pain trying to compile and use a Darwin/Mach-O emulation layer for linux (which didn’t work) I managed to compile AVR-gcc 4.3.3 (which was also painful)

In an interesting turn of events, I found that if I use avr-g++ 4.3.3 just to compile the code into assembly, but keep using the existing toolchain (avr-as, avr-ld 8.2, and corresponding versions of avr-binutils and avr-libc), then the bugs go away. So it’s not my changes that cause the problem! So I suppose it really is some different behaviour introduced by newer versions of GCC, which I still am yet to pinpoint.

if you determine what it is I would love for you to share your discovery. the idea of avoiding having to track down old avrlib and gcc code to compile the firmware in the future sounds great

I believe I’ve found the problem: once again it’s undefined behaviour that obviously worked in GCC 4.3 but doesn’t under more recent compiler versions and possibly also because of recent C++ standards. @pichenettes you might find this interesting :wink:

Here’s an example, from the Midipal CC knob app:

This one took much longer to find because there is no warning about the UB. I came across it several days ago while looking around the codebase and was a little wary, but I guess because similar lines are used in many places around the code base, I assumed it wasn’t a problem. I was wrong.

What’s going on here is that value_ is the first of several static variables belonging to the CcKnob class, which hold its current state. In order to manipulate the variables with a function (that can be called via a function pointer), the variables are assumed to lie in a contiguous block of memory, like an array, and hence the cast to uint8_t*. Unfortunately, the C++ standard makes no guarantee about the placement of static class variables in memory, as discussed in this StackOverflow question, and so the resulting pointer from this line of code is ill-defined. (Note: the C++11 standard does say that the order of non-static member variables in memory is guaranteed to be the same as that in which they appear in the source, but they may not necessarily be placed in adjacent memory locations.

Take a look at a diff of the output of avr-nm -n, where the left-hand side has the midipal firmware compiled with GCC 8.2 and the right hand side has everything compiled with GCC 8.2, except for cc_knob.cc which was compiled to assembly with GCC 4.3.3, and then assembled and linked with GCC 8.2:

00800790 _ZN7midipal4apps6CcKnob4max_E ----- 00800790 _ZN7midipal4apps6CcKnob6value_E
00800791 _ZN7midipal4apps6CcKnob4min_E ----- 00800791 _ZN7midipal4apps6CcKnob8channel_E
00800792 _ZN7midipal4apps6CcKnob7number_E -- 00800792 _ZN7midipal4apps6CcKnob5type_E
00800793 _ZN7midipal4apps6CcKnob5type_E ---- 00800793 _ZN7midipal4apps6CcKnob7number_E
00800794 _ZN7midipal4apps6CcKnob8channel_E - 00800794 _ZN7midipal4apps6CcKnob4min_E
00800795 _ZN7midipal4apps6CcKnob6value_E --- 00800795 _ZN7midipal4apps6CcKnob4max_E

It just so happens that GCC 4.3.3 places all the static variables together in order of declaration in the source file. On the other hand, GCC 8.2.0 places them in reverse order. Hence incrementing the pointer to value_ in the SetParameter() function causes other memory to be overwritten!

Since there are many similar instances of this undefined behaviour in the Midipal code (and from a cursory search, also in the ambika code), this may explain other strange misbehaviours.

It seems like a good way removing this UB is to rewrite the affected logic by explicitly using arrays rather than class variables. I have tested this with the CC Knob app in the Midipal firmware (compiled with GCC 8.2.0 with -std=c++11), and it does indeed solve the observed problems on the actual device.

Here are snippets of the new code

cc_knob.h:

... 
  static void SetParameter(uint8_t key, uint8_t value);
  static uint8_t GetParameter(uint8_t key);
  
  static const AppInfo app_info_ PROGMEM;
  // holds all the settings in an array to enable pointer arithmetic
  static uint8_t settings[Parameter::COUNT];

  enum Parameter : uint8_t {
      cc_value_,
      channel_,
      type_,
      number_,
      min_,
      max_,
      COUNT
  };

 private:
  // returning a reference allows modifying settings using these functions too
  static inline uint8_t& settingValue(uint8_t key) {
      return settings[key];
  }
  static inline uint8_t& cc_value() { return settings[Parameter::cc_value_]; }
  static inline uint8_t& channel() { return settings[Parameter::channel_]; }
  static inline uint8_t& type() { return settings[Parameter::type_]; }
  static inline uint8_t& number() { return settings[Parameter::number_]; }
  static inline uint8_t& min() { return settings[Parameter::min_]; }
  static inline uint8_t& max() { return settings[Parameter::max_]; }
  ...
void CcKnob::SetParameter(uint8_t key, uint8_t value) {
  //uint8_t previous_value = cc_value_;
  uint8_t previous_cc_value = cc_value();
  //static_cast<uint8_t*>(&value_)[key] = value;
  settingValue(key) = value;
  ...

So, it will take a bit of code rewriting, but I hope that this will finally let us use the latest version of GCC to compile Ambika and midipal firmware! (Though, recent versions of GCC do probably produce slightly larger binaries than earlier versions.)

3 Likes

:fearful: