Finding the Minimum and Maximum Values of a Subset of Numbers in C

Question

I am working on an embedded systems project, specifically a line follower car. It has an emitter, a left detector and a right detector. I am working on an algorithm to find the maximum white value, so when the detector gets down to that value it will trigger white, and a minimum black value that will trigger black when it gets up to that value.

Right now, the detector reads values in the 70-80 range for white but could go as low as 40 and 400-500 for black but could go over 800. It is not required to write this algorithm, but I am trying to be efficient.

The microcontroller has two switches that we can use. My plan is to press switch 1 to begin calibrating the sensors with this algorithm I am writing. My idea is to place the car on a white surface and press the switch. After moving the car all around the white surface for 10 seconds, the code will store the maximum white value it encountered.

Then I'll move the car to calibrate black in the same manner: move it around the black line for 10 seconds and have the code store the minimum black value it encountered. After it has its maximum white value and minimum black value, I want it to display it to the screen.

I don't expect help with the display process, as that is beyond the scope of this question. I just need some feedback on my min/max process and if there is a more efficient way to go about it then I am all ears. I have posted the code that I wrote a couple of days ago.

// Function Used to Calibrate Detectors
void calibration(void){
 // These Variables are Declared as unsigned ints
 // My Ideas Is if It is Below 400 => It Will Be Trying to Detect White
 // And if It is Above 400 => It Will Be Trying to Detect Black
 minWhite_L = 400;
 minWhite_R = 400; 
 maxBlack_L = 400;
 maxBlack_R = 400;
 
 // Minimum White Value
 // ADC_Left_Detect Variable Holds the Value Read By the Left Detector
 // ADC_Right_Detect Variable Holds the Value Read By the Right Detector
 if((ADC_Left_Detect < 400) && (ADC_Right_Detect < 400)){
 if(ADC_Left_Detect < minWhite_L) { minWhite_L = ADC_Left_Detect; }
 if(ADC_Right_Detect < minWhite_R) { minWhite_R = ADC_Right_Detect; }
 }
 // Maximum Black Value
 if((ADC_Left_Detect >= 400) && (ADC_Right_Detect >= 400)){
 if(ADC_Left_Detect > maxBlack_L) { maxBlack_L = ADC_Left_Detect; }
 if(ADC_Right_Detect > maxBlack_R) { maxBlack_R = ADC_Right_Detect; }
 }
 // The Idea Was that First I Would Need the Absolute Minima and Maxima
 if(time_change){ // This is the Code to Wait for 10 Seconds for My Car
 time_change = 0;
 if(delay_start++ >= (10 * SEC_MULTIPLIER)){
 delay_start = 0;
 // Send Found Values to Another Function to Determine the Maximum White
 // And the Minimum Black Values => Which is What I am Really Looking For
 maxW_minB(minWhite_L,minWhite_R,maxBlack_L,maxBlack_R); 
 }
 }
}
void maxW_minB(unsigned int mwL,unsigned int mwR,unsigned int MBL,unsigned int MBR){
 int x; // Temporary Local Variable to Hold the Left White Value
 int y; // Temporary Local Variable to Hold the Right White Value
 
 // Maximum White Value
 if((ADC_Left_Detect < 400) && (ADC_Right_Detect < 400)){
 if(ADC_Left_Detect > mwL) { maxWhite_L = ADC_Left_Detect; }
 if(ADC_Right_Detect > mwR) { maxWhite_R = ADC_Right_Detect; }
 }
 // Minimum Black Value
 if((ADC_Left_Detect >= 400) && (ADC_Right_Detect >= 400)){
 if(ADC_Left_Detect < MBL) { minBlack_L = ADC_Left_Detect; }
 if(ADC_Right_Detect < MBR) { minBlack_R = ADC_Right_Detect; }
 }
 
 if(time_change){ // This is the Code to Wait for 10 Seconds for My Car
 time_change = 0;
 if(delay_start++ >= (10 * SEC_MULTIPLIER)){
 delay_start = 0;
 // Just One Value for the Maximum White
 // 'HextoBCD' Takes an int Argument, so I typecast
 // 'HextoBCD' Converts an int to a Hex Value to Display on the LCD
 if(maxWhite_L > maxWhite_R) { x = (int)maxWhite_L; HEXtoBCD(x); }
 else{ y = (int)maxWhite_R; HEXtoBCD(y); }
 }
 }
 
 if(P6IN & IR_LED) { strcpy(display_line[0], "Emit: ON "); }
 else { strcpy(display_line[0], "Emit: OFF "); }
 strcpy(display_line[1], "MaxW: "); // Display Max White Value
 strcpy(display_line[2], "MinB: "); // Display Min Black Value
 strcpy(display_line[3], " ");
 adc_line(1,6); // This Will Start Printing My Value on the First Row, Sixth Column
 adc_line(2,6); // This Will Start Printing My Value on the Second Row, Sixth Column
 display_changed = TRUE;
 
 if(time_change){ // The Idea is to Display the End Result for 10 Seconds
 time_change = 0;
 if(delay_start++ >= (10 * SEC_MULTIPLIER)){
 delay_start = 0;
 event = NONE;
 }
 }
}

• Toby Speight: There are quite a lot of definitions missing from this code; it would be helpful to see where ADC_Left_Detect etc are declared, as well as SEC_MULTIPLIER, HEXtoBCD etc. - or restructure the code so that all these external identifiers are separate from the algorithm function, so it can be tested in isolation.

Answer 1

Avoid hardcoded assumptions

You are already hardcoding the assumption that anything below 400 is white, and anything above it is black. Ideally, the calibration procedure does not depend on that assumption. If indeed you first put the car onto a white surface and press the button to calibrate what white is, you just need a function to get the maximum value over a period of ten seconds. Then you have another function that calibrates black by getting the minimum value over the same period. Finally, you can do something like calculating the midpoint between the maximum white value and the minimum black value, and use that as the crossover point for deciding whether you are on a white or a black surface.

Consider recording a histogram

If you want to have a one-shot calibration routine, where the car will drive over both white and black regions, and you don't know in advance where the crossover point will be, then consider just recording the measured values into a histogram. For example, if you have a 10-bit ADC and you have enough RAM to store 2 * 1024 integers, then you could write something like:

unsigned int histogram_L[1024] = {0};
unsigned int histogram_R[1024] = {0};
for (int i = 0; i < 10 * SEC_MULTIPLIER; i++) {
 histogram_L[ADC_Left_Detect]++;
 histogram_R[ADC_Right_Detect]++;
 /* delay here */
}

The advantage of this is that you don't have to do any calculations while you are taking the data. You can process the histogram afterwards. For example, you could use \$k\$-means clustering with \$k = 2\$, or look at some other ways to find the peaks in a bimodal histogram.

If you don't have enough RAM, then you could reduce the resolution of your histogram (for example, divide all measured values by 8 to reduce RAM usage by 8) at the cost of a bit less precision in the measured peaks, but if the expected difference between black and white is about 400, that should not be a problem.

Reduce code duplication

There is a lot of duplication in your code; you want to find both the minimum and maximum of each of white and black, and you have both left and right sensors. Try to find some ways to avoid having to repeat yourself. For example, create a struct that holds the state for a single sensor, and write a function that updates the state of one sensor, using a pointer to the struct and the value of the sensor as parameters:

struct MinMax {
 unsigned int min;
 unsigned int max;
};
struct SensorRange {
 struct MinMax white;
 struct MinMax black;
};
static const unsigned int crossOver = 400;
static void updateMinMax(struct MinMax *minMax, unsigned int value) {
 if (value < minMax->min) minMax->min = value;
 if (value > minMax->max) minMax->max = value;
}
static void updateRange(struct SensorRange *range, unsigned int value) {
 updateMinMax(value < crossOver ? &range->white : &range->black, value);
}
void calibrate() {
 struct SensorRange leftRange = {{1023, 0}, {1023, 0}};
 struct SensorRange rightRange = {{1023, 0}, {1023, 0}};
 ...
 updateRange(&leftRange, ADC_Left_Detect); 
 updateRange(&rightRange, ADC_Right_Detect);
 ...
}