What can I do to optimise this code?

Question

I have optimised it as much as I feel I can and want to know what suggestions you have that could make it more efficient etc.

Note: I know the array goes on forever :)

volatile uint16_t IRCode[]={340,171,21,21,21,21,21,21,21,64,21,21,21,21,21,21,21,21,21,64,21,21,21,64,21,21,21,64,21,64,21,64,21,64,21,64,21,21,21,21,21,21,21,64,21,21,21,21,21,21,21,21,21,64,21,64,21,64,21,21,21,64,21,64,21,64,21,1556,340,86,21,3654}; // ProntoHex to uint16_t delete first 4 array numbers.
volatile uint16_t MicrosPassed;
volatile uint8_t Array_Seq;
uint8_t State;
void setup() {
  Config(); // Config sketch will set working settings for different microcontrollers.
  SendIRCode();
}
void loop() {
}
void SendIRCode() {
  switch(State) {
    case 1:
      TCCR1A&=63; // Opposite of case 0 -193 = 63
      State=0;
      break;
case 0:
  TCCR1A|=192;
  State=1;
  break;

}
}
void Config() {
  DDRB|=10;
  ICR1=416;
  OCR1A=9983;
  TCCR1A=2;
  TCCR1B=24;
  TCCR1B|=1;
  TIMSK1=1;
}
ISR(TIMER1_OVF_vect) {
  MicrosPassed++;
  if(MicrosPassed==IRCode[Array_Seq]) {
    SendIRCode();
    MicrosPassed=0;
    Array_Seq++;
       }
}

score 3 · Answer 1 · answered Nov 06 '20 at 20:17

As hinted by chrisl in a comment, there are various criteria you may want to optimize for: speed, size, readability... Judging from the current state of this sketch, I would say that your first goal at this point should be to optimize for readability.

The hardest think to make sense of are most of the plain numbers. When setting registers that are bit fields, do not use decimal values: use either hexadecimal or binary. Or, better yet, use the names of the bits. The avr-libc defines those names, as well as the macro _BV(bit_position), which expands to (1 << bit_position) and is meant to be used with those names:

void Config() {
  DDRB   |= _BV(PB1) | _BV(PB3);  // PB1 and PB3 as output
  ICR1    =  416;        // timer period = 417 cycles
  OCR1A   = 9983;        // ???
  TCCR1A  = _BV(WGM11);  // mode 14: fast PWM, TOP = ICR1
  TCCR1B  = _BV(WGM12) | _BV(WGM13);  // ...ditto
  TCCR1B |= _BV(CS10);   // clock @ F_CPU
  TIMSK1  = _BV(TOIE1);  // enable TIMER1_OVF interrupt
}

Granted, the bit names are somewhat obscure, but they do make the program easier to read if you have the datasheet nearby. I do like to have comments that make the code understandable even without reading the datasheet.

Note that evaluating those expressions does not make the program any slower, as they are evaluated at compile time.

When a variable is used in a single function, it is better to define it in that function. Keeping the definition close to where the variable is used makes the code more “local” and thus easier to read. Since you want some of those variables to remember their values across invocations of the function, give them the static qualifier.

You may not need to optimize this program for speed. There is, however, a low hanging fruit here: remove the volatile keywords. This keyword is meant to explicitly disable some compiler optimizations that are unsafe when a variable is shared between the interrupt and non-interrupt contexts. None of the variable you use needs this.

If I understood correctly, the function SendIRCode() just toggles a couple of bits in TCCR1A. You can simplify it to this:

void SendIRCode() {
  TCCR1A ^= _BV(COM1A0) | _BV(COM1A1);  // toggle PWM output on OC1A
}

Finally, there is I believe a bug in this sketch. The variable Array_Seq is continuously incremented and never reset. The program will then end up reading past the end of the IRCode array.

What can I do to optimise this code?

1 Answers1