AVR Freaks

The code corresponds to a part of an LFO of an electric guitar effect. It uses an infinite loop in the main function and updates the values ​​(sound effect) of frequency, wave type and others when there are modifications in the potentiometers (I assume that this is why it uses ISR).

Originally, the code changed the wave type and the time divisions according to the received voltage values ​​ADC2 and ADC3, respectively, in this function. It uses other ISRs to update other parameters, but I understand that this function is what I need to work on for the following: now I want to add a third function using a free channel, which will send a voltage signal through ADC0 that is unused. The received voltage values ​​are between 0 and 5v for each channel.

To make the tests I have a device that turns on an LED according to the type of wave and the division chosen, with voltage variators. Although it is not "sound", with the way of turning the LED on and off it is enough to disign if it is a sine wave, a square wave, etc. In that sense, the original code works perfectly. However, when I modify this function to try to read a third channel, even when it enters that case "do nothing", the behavior is no longer correct.

ADMUX = ADC_VREF_TYPE + input_channel; // where input_channel would be 0, 2 or 3 (ADC0, ADC2 and ADC3)

If so, it would be necessary to define 

#define ADC_VREF_TYPE 0x40;

And it should be modified at the beginning:

sample_value = ADCW;

And at the end of the switch:

ADCSRA |= (1 << ADSC);

Or am I wrong?

ADMUX = (1 << MUX1) | (1 << MUX0);

// ..
case (0x00):   // ADC0 MUX[1:0] == 00
  // Do something here
  // Update ADMUX to ADC2
  ADMUX |= (0 << REFS1) | (0 << REFS0) | (0 << ADLAR) | (0 << REFS2) | (0 << MUX3) | (0 << MUX2) | (1 << MUX1) | (0 << MUX0);
  break;
// ...

I'm not sure if it should be assigned with | = or directly with =.

ISR(ADC_vect)
{
  static bool first_sampling = true;
  uint8_t sample_value;
  sample_value = ADCH;
  
  if (first_sampling)
  {
    first_sampling = false;
  }
  else
  {
    switch (ADMUX & 0x0F)
    {
      case (0x00):   // ADC0 MUX[1:0] == 00
        // Do nothing here...
        // Update ADMUX to ADC2
        ADMUX = (1 << ADLAR) | 2;
        break;
      case (0x02):  // ADC2 MUX[1:0] == 10
        // SubdivisionFunction();
        // Update ADMUX to ADC3
        ADMUX = (1 << ADLAR) | 3;
        break;
      case (0x03):  // ADC3 MUX[1:0] == 11
        // WaveFormFunction();
        // Update ADMUX to ADC0
        ADMUX = (1 << ADLAR) | 0;
        break;
      default:
        break;
    }
  }
}

It improved, but it still doesn't work as it should when the voltage values of ADC2 and ADC3 are modified. I tried removing the first part of the switch (case 0x00), so it should respond as it did originally, but it did not work either. What could we be overlooking?

Sorry for the delay in responding. Thank you very much everyone for the help!

I was doing some tests, but first I would like to mention some considerations. In relation to what was mentioned by Simonetta and joeymorin abot ADC0, it seemed to me that the most important thing would be to first ensure that ADC2 and ADC3 work correctly, as they did originally. So for a moment I "discarded" the inclusion of ADC0, until the original and the modified code work the same.

For that, I took the original code and rewrote it with all the suggestions proposed here (forgetting that I really want to include ADC0). If we do not mess up, they should work the same way.

Original code:

int main()
{
  // ...
  ADMUX = (1 << ADLAR) | (1 << MUX1) | (1 << MUX0);
  // ...
  
  while (true)
  {
  }
}


ISR(ADC_vect)
{
  static bool_t first_sampling = false;
  uint8_t sample_value;
  sample_value = ADCH;

  if (first_sampling == true)
  {
    first_sampling = false;
  }
  else
  {
    if (ADMUX & 0x01)   // MUX[0:0] == 1
    {
      SetWaveform(sample_value);
    }
    else    // MUX[0:0] == 0
    {
      SetMultiplier(sample_value);
    }
    // Switch the ADC input channel
    ADMUX ^= (1 << MUX0);   // toggle between MUX[1:0] = 11 and MUX[1:0] = 10
    first_sampling = true;
  }
}

New code A:

#define ADMUX_SETUP ((0 << REFS2) | (0 << REFS1) | (0 << REFS0) | (1 << ADLAR))
// ...

int main()
{
  //...
  ADMUX = ADMUX_SETUP | 2;
  //...
  
  while(true)
  {
  }
}

ISR(ADC_vect)
{
  static bool first_sampling = true;
  uint8_t sample_value;
  sample_value = ADCH;
    
  if (first_sampling)
  {
    first_sampling = false;
  }
  else
  {
    switch (ADMUX & 0x0F)
    {
      case (0x02):  // ADC2 MUX[1:0] == 10
        SetMultiplier(sample_value);
        // Update ADMUX to ADC3
        ADMUX = ADMUX_SETUP | 3;
        break;
      case (0x03):  // ADC3 MUX[1:0] == 11
        SetWaveform(sample_value);
        // Update ADMUX to ADC0
        ADMUX = ADMUX_SETUP | 2;
        break;
      default:
        break;
    }
  }
}

On impacting both codes, I noticed the following: in the new code, when modifying the voltage values of ADC2 and ADC3 they were "inverted", that is, when modifying ADC2 it behaved as if modifying ADC3 and vice versa. Therefore, it occurred to me to make the following modification and prove what happened:

New code B:

// the rest equal to the new code A above...

ISR(ADC_vect)
{
  // the rest equal to the new code A above...

    switch (ADMUX & 0x0F)
    {
      case (0x02):  // ADC2 MUX[1:0] == 10
        SetWaveform(sample_value);
        // Update ADMUX to ADC3
        ADMUX = ADMUX_SETUP | 3;
        break;
      case (0x03):  // ADC3 MUX[1:0] == 11
        SetMultiplier(sample_value);
        // Update ADMUX to ADC0
        ADMUX = ADMUX_SETUP | 2;
        break;
      default:
        break;
    }
  }
}

That is, I inverted the places where the functions that handle the wave type and the subdivision are called. Therefore, it was to be expected that the new A code will now work "upside down" by modifying the voltage values in ADC2 or ADC3 (and, therefore, the same as the original code). However, the new A and B codes work exactly the same. In other words, for the new code it does not seem to matter where in the two available I call the multiplier and wave type functions (and that's definitely not right).

From this I could determine that there is a problem with the new way of writing the ISR function (ADC_vect). I could not distinguish which one it is, but it seems to me that the most sensible thing is to be able to correct this first and then turn to add the ADC0 ...

If you think it is useful to include all the main function code, I can put it. I omitted it so that it would not be too long. Soon I will upload an schematic to make it easier to follow.  Meanwhile, I think the following summary will be useful:

- ADC2 = PB4: Regulates the tempo subdivisions (SetMultiplier Function) -> 0 to 5v
- ADC3 = PB3: Regulate the wave type (SetWaveForm Function) -> 0 to 5v