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++;
 }
}

• ocrdu: IRCode[], MicrosPassed, and Array_Seq are only used in the ISR; you could declare them as static in the ISR, IRCode[] could also be const, and no need to declare them as volatile because only the ISR reads/writes them. No idea if this will help the compiler optimising for speed, though.
• chrisl: For what to do you want to optimize? Clean code? Or some meaning of "speed" (which speed)?
• jsotola: convert array to actual bits .. 0 is 21, 21 ... 1 is 21, 64 ... s is 340, 17 - start ... and so on .... the array would be { s, 0, 0, 0, 1, 0 ......} ... you could even store bytes, instead of separate bits
• Gerben: Maybe fix the problem @chrisl mentioned before optimizing your code.

Answer 1

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.