Struggling with the output of ADC on a PIC18F252

Question

I've recently finished code for a PIC18F252 which uses the built in ADC to convert analogue inputs from 3 sensors into digital outputs that can be sent to motors. The code builds and uploads to the PIC perfectly fine, but unfortunately just doesn't work, and I'm struggling to see where the issue is. After testing the output ports for the motors, I've found that they are all outputting the same signal, when they shouldn't be, and it doesn't change depending on the sensors. My first thought is that there's something wrong between the output of the ADC (loading the result from ADRESH), and the if statements used to assign a digital value to these outputs. I can't upload the entire code as this is coursework, but I've added the relevant parts. I've only included my function for AD conversion using one channel, but the functions for the other two channels are the same, just changing variables.

If anyone is able to spot an issue with this code (presumably around the if statements) I would be grateful!

#include <p18F252.h> // PIC specific definitions
#include <xc.h> // Xc8 compiler specifics
#include <stdio.h> // Standard C I/O library
#include <stdlib.h> // Standard C library

// Function declarations
void Config_ADC(void);
void delay(void);
void Motor_Output(unsigned char x, unsigned char y, unsigned char z);
void channel3(void);

unsigned char leftSens = 0b00000000; //Initialise left sensor variable

void Config_ADC(void){
    // ADC Setup
    TRISA = 0xFF; // configure port A as inputs
    ADCON1bits.ADFM =0; // left justified
    ADCON1bits.PCFG3=0; 
    ADCON1bits.PCFG2=1;
    ADCON1bits.PCFG1=0; 
    ADCON1bits.PCFG0=0; 
    ADCON1bits.ADCS2=0;
    ADCON0bits.ADCS1=1;
    ADCON0bits.ADCS0=0; 
    ADCON0bits.ADON =1; 
}

// Function for a delay of 1ms
void delay(void){ 
    T2CON = 0x49; // start counting from 73
    PR2 = 0x7C; // stop count at 124
    T2CONbits.TMR2ON = 1; // activate timer 2
    while(!PIR1bits.TMR2IF); // wait for timer flag
    T2CONbits.TMR2ON = 0; // stop timer 2
    PIR1bits.TMR2IF = 0; // clear flag
}

void channel3(void){
    ADCON0bits.CHS2 = 0; //Channel selection
    ADCON0bits.CHS1 = 1;
    ADCON0bits.CHS0 = 1;
    delay(); // Acquisition time to charge hold capacitor
    ADCON0bits.GO_DONE = 1; // Start Conversion
    while(ADCON0bits.GO_DONE); // Wait for A/D Conversion to complete
    leftSens = ADRESH; // Return result
}

void main(void){
    TRISB = 0x00; // configure Port B as output
    Config_ADC(); // load ADC
    while(1) { //loop forever/
        channel3(); // Call channel 3 conversion
        Motor_Output(leftSens, middleSens, rightSens);
    }
}

// Function to send instructions to motors
void Motor_Output(unsigned char x, unsigned char y, unsigned char z){
    int left, middle, right;
    if(x >= 173){left = 1;}   //173 chosen as threshold 8 bit value
    else if(x < 173){left = 0;}
    else if(y >= 173){middle = 1;}
    else if(y < 173){middle = 0;}
    else if(z >= 173){right = 1;}
    else if(z < 173){right = 0;}

    unsigned char output;
    output = (left << 2) | (middle << 1) | right;  //Bit shifted variables into one value
    switch(output){
        case 0x0:
            PORTBbits.RB7 = 0; //Test for each output and activate motors accordingly
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1;
        case 0x1:
            PORTBbits.RB7 = 0; 
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 1;
            PORTBbits.RB4 = 0;
        case 0x2:
            PORTBbits.RB7 = 0; 
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1;
        case 0x3:
            PORTBbits.RB7 = 0; 
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 1;
            PORTBbits.RB4 = 0;
        case 0x4:
            PORTBbits.RB7 = 1; 
            PORTBbits.RB6 = 0;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1;
        case 0x5:
            PORTBbits.RB7 = 0;
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1;
        case 0x6:
            PORTBbits.RB7 = 1; 
            PORTBbits.RB6 = 0;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1;
        case 0x7:
            PORTBbits.RB7 = 0; 
            PORTBbits.RB6 = 1;
            PORTBbits.RB5 = 0;
            PORTBbits.RB4 = 1; 
    }
    return;
}

Answer 1

Without going any deeper in analysis there are two major problems:

1. The if-else if-chain will do just one assigment to left. To make that obvious I have indented your source this way without any other changes:

   if (x >= 173) {     // Either THIS...
       left = 1;
   } else
       if (x < 173) {  // Or THAT will be true.
           left = 0;
       } else          // None of the following is executed therefore.
           if (y >= 173) {
               middle = 1;
           } else
               if (y < 173) {
                   middle = 0;
               } else
                   if (z >= 173) {
                       right = 1;
                   } else
                       if (z < 173) {
                           right = 0;
                       }
   

   So middle and right will have undefined values.

2. All your cases in the switch are missing a break. So only the last case or none at all will be executed. Why "none"? Because middle and right have undefined values, resulting in a random value in output.

   Note: "Random" might be always the same value, depending on the bits set or reset at the locations of middle and right, respectively.

Answer 2

Quick question: Is it by design that you are falling through your switch-cases?

void Motor_Output(unsigned char x, unsigned char y, unsigned char z){
 // ...
switch(output){
 case 0x0:
  ... <-- no break, will continue to execute next case
 case 0x1:
  ... <-- no break, will continue to execute next case
 case 0x2:
  ... <-- no break, will continue to execute next case
 case 0x3:
  ... <-- no break, will continue to execute next case
 case 0x4:
  ... <-- no break, will continue to execute next case
 case 0x5:
  ... <-- no break, will continue to execute next case
 case 0x6:
  ... <-- no break, will continue to execute next case
 case 0x7:
  ... <-- no break, will continue to execute next case
 }
return;
}