Average readouts from multiple IR sensors with Arduino

Question

I've written an Arduino sketch to continuously read values from multiple analog IR distance sensors, average the last 7 values and output in millimeters. Being pretty new to both Arduino and embedded programming, I'm sure there's a lot of room for improvement. Any critique would be highly appreciated, including idioms and built-in functions I could have used.

My plan is to extend this program to run LED strips based on the sensor values.

#define LOOP_PERIOD_MS 10 // run loop every 10 ms
#define IR_WIN_LENGTH 7 // IR sensor moving-average window
#define IR_NUM_SENSORS 4 // Number of IR sensors, must be 1-6 and connected to the first analog pins
// Declare IR sensor functions
void ir_read_new_vals();
uint16_t ir_get_cur_mm( uint8_t sensor_i );
uint16_t ir_get_avg_mm( uint8_t sensor_i );
// Variables
unsigned long loop_start_ms = 0; // When the last loop iteration started
// Setup
void setup() {
 // Initialize serial communication
 Serial.begin(115200);
}
// Main loop: Read and print IR sensor data continuously
void loop() {
 // Limit loop frequency
 while ( millis() - loop_start_ms < LOOP_PERIOD_MS )
 delayMicroseconds(10);
 loop_start_ms = millis();
 // Print timestamp 
 Serial.print( loop_start_ms );
 // Read and print IR sensor values
 ir_read_new_vals();
 for ( uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
 Serial.print(",");
 Serial.print(ir_get_avg_mm(sensor_i));
 }
 // print new line
 Serial.println("");
}
// IR sensor functions
uint8_t ir_i = IR_WIN_LENGTH; // array index for the last read value
uint16_t ir_vals_mm[IR_NUM_SENSORS][IR_WIN_LENGTH]; // "rolling" stored values for sensors
bool ir_avg_is_valid = false; // whether moving-average window is filled
// Read, scale and store values from analog sensors
// The four sensors are connected to analog pins 0, 1, 2, 3
void ir_read_new_vals() {
 // Increment "rolling array" index
 ir_i = ( ir_i + 1 ) % IR_WIN_LENGTH;
 // Read each sensor
 for ( uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
 // Dummy sensor read (suggested when switching ADC pin)
 analogRead(sensor_i);
 // Read sensor value (values 0..1023 corresponds to 0..5V)
 uint32_t raw = analogRead(sensor_i);
 // Clamp values to 5-80cm 
 if ( raw > 600 )
 raw = 600;
 if ( raw < 80 )
 raw = 80;
 // Rescale value to mm according to datasheet curve and store
 ir_vals_mm[sensor_i][ir_i] = (uint16_t)( 67870 / ( raw - 3 ) - 40 );
 }
 // Check if the moving-average window was filled
 if ( ! ir_avg_is_valid && ir_i == IR_WIN_LENGTH - 1 )
 ir_avg_is_valid = true;
}
// Return last read sensor value in mm
uint16_t ir_get_cur_mm( uint8_t sensor_i ) {
 return ir_vals_mm[sensor_i][ir_i];
}
// Return moving average of sensor values
uint16_t ir_get_avg_mm( uint8_t sensor_i ) {
 // Return 0 if not enough values
 if ( ! ir_avg_is_valid )
 return 0;
 // Sum sensor values
 uint32_t accum_mm = 0;
 for ( uint8_t i = 0; i < IR_WIN_LENGTH; i++ )
 accum_mm += ir_vals_mm[sensor_i][i];
 // Divide and return
 return accum_mm / IR_WIN_LENGTH;
}

Answer 1

• The code feels overcommented. The only comment I see having some value is

  // Dummy sensor read (suggested when switching ADC pin)
  

  but even that shall refer to the proper place in the datasheet.

  Everything else could (and should) be taken care of by using proper names. Don't be shy factoring blocks of code into functions, even just for the sake of giving them names.

• The code does not know (or care about) the nature of the sensor connected to the pins. Does it matter that it is IR? Would anything be changed if it were RF, or sonar, or anything else? So the ir_ prefix seems like a pure noise. I strongly advise of dropping it.

• The code does care on the signal postprocessing (clamping and rescaling). This operation is highly specific on the part used. If by any chance you'd opt for another part, you'd need to fix the heart of the program. I strongly advise to factor this functionality into the function, to be called via a pointer.

• I do not endorse variables like ir_avg_is_valid. It is toggled once, but consumes cycles for the entire program lifetime. Consider filling up first IR_WIN_LENGTH samples outside the loop.

All that said, the code should look along the lines of

 void read_new_values(uint32_t (*postproc)(uint32_t raw))
 {
 for (uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
 uint32_t raw = read_sensor(sensor_i);
 uint32_t cooked = postproc(raw);
 values[sensor_i][win_ix] = cooked;
 win_ix = (win_ix + 1) % WIN_LENGTH;
 }
 }
 uint32_t read_sensor(uint8_t pin) {
 analogRead(pin); // Dummy read, see datasheet, chapter and verse
 return analogRead(pin);
 }
 uint32_t whatever_part_number_postproc(uint32_t value) {
 // See datasheet, chapter and verse
 clamp
 return rescale
 }
 void prefill_window(uint8_t win_size) {
 for (win_ix = 0; win_ix < win_size; win_ix++) {
 read_new_values(whatever_part_number_postproc);
 }
 }
 void setup() {
 ....
 prefill_window(WIN_LENGTH);
 }
 void loop() {
 ....
 read_new_values(whatever_part_number_postproc);
 ....
 }

• Anna: Thank you so much! That all makes a lot of sense. I've just got a question about making read_sensor more general. I'm used to You Ain't Gonna Need It and not generalizing unnecessarily. What are your thoughts about that? No other sensors will be used in this project.