0

So I'm trying to read 4 different analog values and store each one to a variable. I imagine doing this by first initializing the ADC registers and each time it enters the ADC ISR checking which port it read from and saving it to the corresponding variable. Then setting the ADMUX to the next ADC port and repeating the whole process. Unfortunately something goes wonky and I just can't figure out what it is. If I get rid of the switch case and just read from one port it all works fine, but if I put the switch case back in and try to cycle through them it stops working. Please ignore the rest of code as this is not complete. I just included everything I could. Eventually this will take in 4 values. Then use them for error checking and a PID loop.

Thanks for any help!

/*
 * Throttle_by_Wire.c
 *
 * Basic throttle by wire controller
 * for the 2015 Formula Hybrid car
 *
 *  Created on: Nov 24, 2014
 *  Author: Austin R. Bartz
 *
 *  Pin Connections:
 *      PB2 - Motor Speed (Enable pin) (PWM)
 *      PB4 - Direction control (Counter-Clockwise)
 *      PB5 - Direction control (Clockwise)
 *      PC0 - Throttle Pedal Sensor A (TPA) (Shallower Slope)
 *      PC1 - Throttle Pedal Sensor B (TPB) (Steeper Slope)
 *      PC2 - Throttle Valve Sensor A (TPSA) (Negative Slope)
 *      PC3 - Throttle Valve Sensor B (TPSB) (Positive Slope)
 */

#include <avr/io.h>
#include <avr/interrupt.h>
#include <inttypes.h>
#include <stdlib.h>

uint16_t TPA = 0;
uint16_t TPB = 0;
uint16_t TPSA = 0;
uint16_t TPSB = 0;
uint16_t analogpin = 0;

long map(long x, long in_min, long in_max, long out_min, long out_max);

int main(void)
{
    //Configure ADC for single shot conversion
    ADMUX |= (1<<REFS0);
    ADCSRA = (1<<ADEN)|(1<<ADSC)|(1<<ADIE)|(1<<ADPS2)|(1<<ADPS1)|(1<<ADPS0);
    DIDR0 = (1<<ADC4D)|(1<<ADC3D)|(1<<ADC2D)|(1<<ADC1D)|(1<<ADC0D);
    //Configure timer/counter1 for FastPWM
    TCCR1A = (1<<COM1B1)|(1<<WGM11)|(1<<WGM10);
    TCCR1B = (1<<WGM13)|(1<<WGM12)|(1<<CS10);
    OCR1A = 800;    //20kHz
    //Set motor control pins as outputs
    DDRB = (1<<PB2)|(1<<PB4)|(1<<PB5);
    PORTB |= (1<<PB5);
    //Enable global variables and interrupts
    sei();

    while(1)
    {
        //Control PWM % to motor
        OCR1B = map(TPA, 0, 1023, 0, OCR1A);
    }
}

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

ISR(ADC_vect)
{
    //Save current conversion's ADC value
    analogpin = ADMUX & 0b00001111;
    switch (analogpin)
    {
    case 0:
        TPA = ADCW;
        //Increment ADC port to ADC1
        ADMUX |= (1<<MUX0);
        break;
    case 1:
        TPB = ADCW;
        //Increment ADC port ADC2
        ADMUX &= ~0b00001111;
        ADMUX |= (1<<MUX1);
        break;
    case 2:
        TPSA = ADCW;
        //Increment ADC port to ADC3
        ADMUX &= 0b00001111;
        ADMUX = (1<<MUX0)|(1<<MUX1);
        break;
    case 3:
        TPSB = ADCW;
        //Reset ADC Port to ADC0
        ADMUX &= ~0b00001111;
        break;
    }
    //Start next conversion
    ADCSRA |= (1<<ADSC);
}
1

Case 2 is missing the ~ and does not clear off the low 4 bits

ADMUX &= 0b00001111;

should be this to clear off the MUX selection bits:

ADMUX &= ~0b00001111;

Also, you have no default case. I would agree that by design the bits can only contain the values 0-3, but what happens if it ends up 4?

The bit manipulation is hard to follow. I would implement like this:

ISR(ADC_vect) {
  uint8_t nextMux;

  //Save current conversion's ADC value
  analogpin = ADMUX & 0b00001111;
  switch (analogpin) {
    case 0:
      TPA = ADCW;
      nextMux = (1<<MUX0);  
      break;
    case 1:
      TPB = ADCW;
      nextMux = (1<<MUX1);
      break;
    case 2:
      TPSA = ADCW;
      nextMux = (1<<MUX0)|(1<<MUX1);
      break;
    case 3:
      TPSB = ADCW;
    default:
      nextMux = 0;
  }
  // set mux for next conversion
  ADMUX &= 0b11110000;
  ADMUX |= nextMux;

  //Start next conversion
  ADCSRA |= (1<<ADSC);

  return;
}

I like the consistent pattern of this because it makes errors easy to see. With respect to code space, a few bytes should be saved. Execution time wise, it adds one |= to the case 3.

  • I looked at the original, and gave up because frankly it is too hard to follow the code. If you followed better programming practice e.g. using a #define for ~0b00001111 it would be easier to follow (and less error prone). – Milliways Nov 25 '14 at 9:44
  • @jdr5ca Thank you! I changed the code to your suggested segment and it works perfectly now! I apologize for the unclear bit manipulation. I just started learning how to do all this last quarter (I'm a second year electrical engineering student) so it's still all relatively new to me. – Austin Richard Bartz Nov 25 '14 at 12:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.