I'm porting some AVR code from PROGMEM/PGM_P to __flash, and I want to reduce the amount of conditional compilation I need to do in the code.Here's all the code (but keep in mind that only the parts in the conditionals should need changing):#define F_CPU 12000000#include <avr/io.h>//#undef __FLASH#ifndef __FLASH#include <avr/pgmspace.h>#define FLASH(x) const x PROGMEM#else#define FLASH(x) const __flash x#endif#include <avr/interrupt.h>#include <avr/sleep.h>#define M_CPORT PORTC#define M_CDIR DDRC#define M_C1 PC7#define M_C2 PC6#define M_C3 PC5#define M_C4 PC4#define M_C5 PC3#define numCols 5#define M_RPORT PORTA#define M_RDIR DDRA#define M_R1 PA0#define M_R2 PA1#define M_R3 PA2#define M_R4 PA3#define M_R5 PA4#define M_R6 PA5#define M_R7 PA6#define numRows 7// bit values for each rowFLASH(unsigned char) rows[] = {_BV(M_R1), _BV(M_R2), _BV(M_R3), _BV(M_R4), _BV(M_R5),    _BV(M_R6), _BV(M_R7)};// byte values for each row of each frameFLASH(unsigned char) letterI[7] = {0x1f, 0x4, 0x4, 0x4, 0x4, 0x4, 0x1f}; ...#ifdef __FLASHconst __flash unsigned char const * const __flash sequence[] = {#elseunsigned PGM_P const PROGMEM sequence[] = {#endif  letterI, ...};// how long to sustain each row fow, based on the number of dots in the rowconst unsigned int rowDur[6] = {0, 140, 160, 180, 200, 220};unsigned char currFrame, currSlice;unsigned char frameBuffer[7];// optimized routine to count number of 1s in a frame rowunsigned char count5Bits(unsigned char v){  asm volatile(clr __tmp_reg__\n\t               ror %[val]\n\t               adc __tmp_reg__,__zero_reg__\n\t               ror %[val]\n\t               adc __tmp_reg__,__zero_reg__\n\t               ror %[val]\n\t               adc __tmp_reg__,__zero_reg__\n\t               ror %[val]\n\t               adc __tmp_reg__,__zero_reg__\n\t               ror %[val]\n\t               adc __tmp_reg__,__zero_reg__\n\t               mov %[val], __tmp_reg__                : [val] =&r (v)                :);  return v;}// advance the frame, oring the dots from the next image frame in turnISR(TIMER1_COMPA_vect){  int r;  ++currSlice;#ifdef __FLASH  const __flash unsigned char *fPtr = sequence[currFrame];#else  PGM_P fPtr = (PGM_P)pgm_read_word(&(sequence[currFrame]));#endif  for (r = numRows; r > -1; --r)  {    frameBuffer[r] = ((frameBuffer[r] << 1) |#ifdef __FLASH      fPtr[r]#else      pgm_read_byte(&(fPtr[r]))#endif       >> ((numCols + 1) - currSlice));  }  currSlice %= (numCols + 1);  if (!currSlice)  {    ++currFrame;    currFrame %= sizeof(sequence) / sizeof(sequence[0]);  }}unsigned char currRow;unsigned char rowByte;unsigned char frameRow;// turn on the dots for the current rowISR(TIMER0_OVF_vect){#ifdef __FLASH  rowByte = rows[currRow];#else  rowByte = pgm_read_byte(&(rows[currRow]));#endif  frameRow = frameBuffer[currRow];  M_CPORT |= ~(frameRow << 3);  M_RPORT |= rowByte;  ++currRow;  currRow %= numRows;  OCR0A = rowDur[count5Bits(frameBuffer[currRow])];}// turn off the dots for the current rowISR(TIMER0_COMPA_vect){  M_RPORT = 0;  M_CPORT = 0;}int main(){  register unsigned char newMCUCR = MCUCR | _BV(JTD);  MCUCR = newMCUCR;  MCUCR = newMCUCR;  // timer 1 for frame advance, CTC mode  // OCRA for advance  TCCR1A = 0;  TCCR1B = (_BV(WGM12) | _BV(CS12) | _BV(CS10));  TIMSK1 = _BV(OCIE1A);  OCR1AH = 0x3;  OCR1AL = 0x0;  //timer 0 for row advance, PWM mode  // OVF for advance, OCRA for shutoff  TCCR0A = (_BV(WGM01) | _BV(WGM00));  TCCR0B = _BV(CS01);  TIMSK0 = (_BV(OCIE0A) | _BV(TOIE0));  OCR0A = 200;  M_CDIR |= (_BV(M_C1) | _BV(M_C2) | _BV(M_C3) | _BV(M_C4) | _BV(M_C5));  M_RDIR |= (_BV(M_R1) | _BV(M_R2) | _BV(M_R3) | _BV(M_R4)    | _BV(M_R5) | _BV(M_R6) | _BV(M_R7));  set_sleep_mode(SLEEP_MODE_IDLE);  sei();  while (1)  {    sleep_enable();    sleep_cpu();    sleep_disable();  }}Both code paths currently work perfectly, but having a single code path would likely reduce maintenance. I can replace pgm_read_byte with *, but I'm befuddled by both the declaration and access of sequence and the declaration of fPtr, and don't know where I should start with them.EDIT:After a bit of mucking around, here's what I came up with:#ifndef __FLASH#include <avr/pgmspace.h>#define FLASH(x) const x PROGMEM#define FLASH_P(x) const x * const PROGMEM#define FLASH_PR(x, y) (x *)pgm_read_word(&(y))#else#define FLASH(x) const __flash x#define FLASH_P(x) const __flash x * const __flash#define FLASH_PR(x, y) (y)#define pgm_read_byte(x) *(x)#endif ...FLASH_P(unsigned char) sequence[] = { ...FLASH(unsigned char *) fPtr = FLASH_PR(unsigned char, sequence[currFrame]);Wouldn't mind a sanity check on this though, in case I missed anything.
Embedded conditional code compaction
c;assembly;type safety;embedded
A few notes:Your included libraries and definitions at the beginning of your code is not very organized.#include <avr/io.h>//#undef __FLASH#ifndef __FLASH#include <avr/pgmspace.h>#define FLASH(x) const x PROGMEM#define FLASH_P(x) const x * const PROGMEM#define FLASH_PR(x, y) (x *)pgm_read_word(&(y))#else#define FLASH(x) const __flash x#define FLASH_P(x) const __flash x * const __flash#define FLASH_PR(x, y) (y)#define pgm_read_byte(x) *(x)#endif#include <avr/interrupt.h>#include <avr/sleep.h>I would sort it where all of the #includes are at the beginning, then a space, and then all of your preprocessor definitions.  Also, I don't like to #include stuff in an #ifndef, just include it.  Your compiler will make the proper optimizations if it's not used anyways.#include <avr/io.h>#include <avr/interrupt.h>#include <avr/sleep.h>#include <avr/pgmspace.h>#ifndef __FLASH#define FLASH(x) const x PROGMEM#define FLASH_P(x) const x * const PROGMEM#define FLASH_PR(x, y) (x *)pgm_read_word(&(y))#else#define FLASH(x) const __flash x#define FLASH_P(x) const __flash x * const __flash#define FLASH_PR(x, y) (y)#define pgm_read_byte(x) *(x)#endifYou have a lot of preprocessor conditionals spread throughout your program.#ifdef __FLASHconst __flash unsigned char const * const __flash sequence[] = {#elseunsigned PGM_P const PROGMEM sequence[] = {#endifAnother option to this is to have two separate files for the different implementations.  This would mean having version-specific implementations of some classes, and switch entire implementations rather than just a few lines here and there.  It would clean up your code of all these preprocessor conditionals.You have some preprocessor definitions that aren't capitalized.#define numRows 7You should always capitalize all preprocessor definitions.#define NUMROWS 7You use some magic numbers here and there.const unsigned int rowDur[6] = {0, 140, 160, 180, 200, 220};You already defined NUMROWS, so you should use it here.const unsigned int rowDur[NUMROWS - 1] = {0, 140, 160, 180, 200, 220};Define variables in your for loops.(C99)for (int r = numRows; r > -1; --r)Use more comments.  If another developer was to take a look at your code, they would have to use a lot of reason and logic to derive what you are trying to do in some places.  For longer explanations, put your comments above the tricky statement.M_CPORT |= ~(frameRow << 3);  //  <insert comment here>