Embedded conditional code compaction

Question

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 row
FLASH(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 frame
FLASH(unsigned char) letterI[7] = {0x1f, 0x4, 0x4, 0x4, 0x4, 0x4, 0x1f};
 ...

#ifdef __FLASH
const __flash unsigned char const * const __flash sequence[] = {
#else
unsigned PGM_P const PROGMEM sequence[] = {
#endif
  letterI, ...
};

// how long to sustain each row fow, based on the number of dots in the row
const 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 row
unsigned 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 turn
ISR(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 row
ISR(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 row
ISR(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.

Answer 1

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)
  #endif
  

• You have a lot of preprocessor conditionals spread throughout your program.

  #ifdef __FLASH
  const __flash unsigned char const * const __flash sequence[] = {
  #else
  unsigned PGM_P const PROGMEM sequence[] = {
  #endif
  

  Another 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 7
  

  You should always capitalize all preprocessor definitions.

  #define NUMROWS 7
  

• You 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>