Keeping clean code for differenct hardware revs.

Forum 2005-2010 (read only) Software Syntax & Programs
1

I'm having a slight problem with this.

While pursuing to improve the hardware of a project of mine, I'm trying to keep the code functional on all previous boards. Why? Because I still have many prototypes that work just fine and shouldn't go to waste.

Now it seems that I've successfully created my private little hell, which consists of lots of

#ifdef <version> 
  // do this
#else
  // do that
#endif

spread across my code. It all works, but readability is converging to zero. This is especially annoying inside functions. Of course I could just #ifdef ... #endif whole functions, but then why not create completely separate files, which I don't want.

Is there a better way for this? Or should I just try to get rid of the prototypes (and maybe make a kid or some hacker happy) and forget about it?

2

You might look into a software versioning system that allow code branches; set up properly (not an easy task, for certain), this could make it easier to keep the code in sync with changes, while at the same time providing versioning. The "main players" you typically see mentioned are subversion, cvs, and of course, Visual Source Safe - though there are others, just like any software...

3

How "object oriented" is your Sketch?

I've been able to solve similar problem by making classes more "fine grained" (a larger number of simpler / smaller classes with work divided into smaller bits).

4

Ah, the good old divide and conquer. I still hate that :wink: Nobody was able to properly explain why some algorithms scale as N*log(N) when using that approach (instead of say N²). Most of the time recursive programming was used as well. Shudder. And of course most forgot to set the condition when to terminate. BOOM :wink:

Classes... object orientation... currently I use none of this. And honestly I'm not a fan of having to solve inheritance issues and similar headaches :wink:

And I already use GIT to keep my head from spinning too much and at least keep a 'safe' revision history.

But my code already looks as messy as the 'arduino bootloader'... I've already abstracted away the bare hardware interaction, so at least the 'end-user' code in main/loop doesn't have to deal with it directly. I could change some options to be run-time configurable and remove their #ifdef-s, but for others that option just doesn't make any sense.

I think I must meditate about this problem a bit.

5

In my case, I could help more if I know some of the details. If you post an example or two of the most troublesome routines I'm willing to spend a bit of time looking them over.

6

OK :wink:

It's not difficult per se, it just looks ugly. Mainly because all the preprocessor stuff can't be indented.

ISR (TIMER1_OVF_vect)
{                        /* Framebuffer interrupt routine */
  TCNT1 = 0xFFFF - timer1_cnt;
  uint8_t led;

  switch (rgb_mode)
    {
    case 0:

      uint8_t cycle;

      for (cycle = 0; cycle < max_brightness; cycle++)
      {
        uint8_t led;
        for (led = 0; led <= (8 - 1); led++)
          {

            PORTB = 0xFF;      // all cathodes HIGH --> OFF
#ifdef V122
            PORTD &= ~((1 << RED_A2) | (1 << GREEN_A2) | (1 << BLUE_A2));
#endif
            PORTD &= ~((1 << RED_A) | (1 << GREEN_A) | (1 << BLUE_A));      // all relevant anodes LOW --> OFF
            PORTB &= ~(1 << fix_led_numbering[led]);      // only turn on the LED that we deal with right now (current sink, on when zero)

            if (cycle < brightness_red[led])
            {
#ifdef V122
              PORTD |= ((1 << RED_A) | (1 << RED_A2));
#else
              PORTD |= (1 << RED_A);
#endif
            }

            if (cycle < brightness_green[led])
            {
#ifdef V122
              PORTD |= ((1 << GREEN_A) | (1 << GREEN_A2));
#else
              PORTD |= (1 << GREEN_A);
#endif
            }

            if (cycle < brightness_blue[led])
            {
#ifdef V122
              PORTD |= ((1 << BLUE_A) | (1 << BLUE_A2));
#else
              PORTD |= (1 << BLUE_A);
#endif
            }
          }
      }

      break;
    case 1:
      for (led = 0; led <= (8 - 1); led++)
      {

        PORTB = 0xFF;            // all cathodes HIGH --> OFF
#ifdef V122
        PORTD &= ~((1 << RED_A2) | (1 << GREEN_A2) | (1 << BLUE_A2));
#endif
        PORTD &= ~((1 << RED_A) | (1 << GREEN_A) | (1 << BLUE_A));      // all relevant anodes LOW --> OFF
        PORTB &= ~(1 << fix_led_numbering[led]);      // only turn on the LED that we deal with right now (current sink, on when zero)

        if (brightness_red[led] > 0)
          {
#ifdef V122
            PORTD |= ((1 << RED_A) | (1 << RED_A2));
#else
            PORTD |= (1 << RED_A);
#endif
          }

        if (brightness_green[led] > 0)
          {
#ifdef V122
            PORTD |= ((1 << GREEN_A) | (1 << GREEN_A2));
#else
            PORTD |= (1 << GREEN_A);
#endif
          }

        if (brightness_blue[led] > 0)
          {
#ifdef V122
            PORTD |= ((1 << BLUE_A) | (1 << BLUE_A2));
#else
            PORTD |= (1 << BLUE_A);
#endif
          }
        _delay_us (200);      // pov delay
      }
      break;
    default:
      break;
    }
  PORTB = 0xFF;                  // all cathodes HIGH --> OFF
}

In the 2nd example 2 boards have two roles, MASTER and SLAVE. As you will see 2 commands have to be reversed to make it work properly.

void
wobble2 (uint8_t * wobble_pattern_ptr, uint8_t pattern_length, enum COLOR_t led_color, enum DIRECTION_t direction, uint8_t times, uint16_t delay_time)
{
  uint8_t ctr1;
  uint8_t ctr2;
  color_on (led_color);
  switch (direction)
    {
    case CW:
      for (ctr1 = 0; ctr1 < times; ctr1++)
      {
        for (ctr2 = 0; ctr2 < pattern_length; ctr2++)
          {
            set_byte (wobble_pattern_ptr[ctr2]);
#ifdef MASTER
            __delay_ms (delay_time);
            sync ();
#else
            sync ();
            __delay_ms (delay_time);
#endif
          }
      }
      break;
    case CCW:
      for (ctr1 = 0; ctr1 < times; ctr1++)
      {
        for (ctr2 = 0; ctr2 < pattern_length; ctr2++)
          {
            set_byte (rotate_byte (wobble_pattern_ptr[ctr2], 4, CW));
#ifdef MASTER
            __delay_ms (delay_time);
            sync ();
#else
            sync ();
            __delay_ms (delay_time);
#endif
          }
      }
      break;
    default:
      break;
    }
  color_off (led_color);
}
7

Mainly because all the preprocessor stuff can't be indented.

Whoever told you that needs to be slapped silly.

TIMER1_OVF_vect is a tough call. Is this more readable?

#ifdef V122
  const uint8_t RED_Ax = ((1 << RED_A) | (1 << RED_A2));
  const uint8_t GREEN_Ax = ((1 << GREEN_A) | (1 << GREEN_A2));
  ...
#else
  const uint8_t RED_Ax = (1 << RED_A);
  const uint8_t GREEN_Ax = (1 << GREEN_A);
#endif
...
  PORTD &= ~(RED_Ax | GREEN_Ax);
...
  PORTD |= RED_Ax;

The code in wobble2 cries out for a simple inline function...

#ifdef MASTER
inline void SyncAndDelay( uint16_t delay_time )
{
  __delay_ms (delay_time);
  sync ();
}
#else
inline void SyncAndDelay( uint16_t delay_time )
{
  sync ();
  __delay_ms (delay_time);
}
#endif
8

Madworm,

Assuming that you don't need to update the code for previous hardware versions, I would just keep separate files for each hardware version. It may be crude, but it sure makes the code easier to read.

Regards,

-Mike

9

@Coding Badly:

Yes, I like that substitution. Now all I need to add is a few comments with 'RTFHF' :wink:

But I doubt that inlining the SyncAndDelay function gives any noticeable improvements, as I'm dealing with delays in the ms range here. Nevertheless it works.

10

Yes, I like that substitution.

Excellent.

Now all I need to add is a few comments with 'RTFHF'

;D

But I doubt that inlining the SyncAndDelay function gives any noticeable improvements, as I'm dealing with delays in the ms range here.

The intent with inline wasn't to improve performance but to make it possible, if you'd like, to put the whole thing in a header file (I think that works).

Any more?