8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The known alternative to #conditionals is to make a variable and use normal control flow statements. So, I don't understand what you are asking here? The advantage of #conditionals is your code is smaller (important in embedded stuff) - but the disadvantage is that your #conditionals create "versions" of your program and you may have unexpected combinations. In this particular case, using normal control flow is going to cause both memory and code size to increase. \$\endgroup\$ – Jasmine Nov 5 '13 at 21:08
  • \$\begingroup\$ @Jasmine: I'm not looking for an alternative to the conditionals, I'm looking for a way to unify the code in the lower conditionals by using a few macros in the first conditional. \$\endgroup\$ – Ignacio Vazquez-Abrams Nov 5 '13 at 21:15
  • 1
    \$\begingroup\$ Really hard to say much about that, given so much of the code is missing. You could reduce some of this by #define things instead of creating actual types, but that would be super tricky with these alternatives. I do not envy your situation :) \$\endgroup\$ – Jasmine Nov 5 '13 at 21:25
  • \$\begingroup\$ I agree with Jasmine: you haven't given us enough context to be able to help you. \$\endgroup\$ – Gareth Rees Nov 5 '13 at 21:32
  • 1
    \$\begingroup\$ @GarethRees: Enjoy. \$\endgroup\$ – Ignacio Vazquez-Abrams Nov 5 '13 at 21:43
11
\$\begingroup\$

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>
    
\$\endgroup\$
  • \$\begingroup\$ The spread conditionals will fortunately all disappear; that was the point of the exercise in the first place. \$\endgroup\$ – Ignacio Vazquez-Abrams Jul 19 '15 at 19:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.