Why doesn't my code work in a function, yet works inline?

Question

I'm trying to write a faster shiftOut function, which does not use the slow digitalWrite. In my code I have the original shiftOut, the new shiftOutFast, and the same as shiftOutFast, but inline. The three blocks are separated by delays to tell them apart on my scope. Here's the code:

#define CLR(x,y) (x &= (~(1 << y)) )
#define SET(x,y) (x |= (1 << y) )

void shiftOutFast(uint8_t dataPort, uint8_t dataBit, uint8_t clkPort, 
                  uint8_t clkBit, uint8_t bitOrder, uint8_t val) {
  for (uint8_t i = 0; i < 8; i++)  {
    if (bitOrder == LSBFIRST) {
      if (val & 0x01) {
        SET(dataPort, dataBit);
      } else {
        CLR(dataPort, dataBit);
      }
      val >>= 1;
    } else {    
      if (val & 0x80) {
        SET(dataPort, dataBit);
      } else {
        CLR(dataPort, dataBit);
      }
      val <<= 1;
    }   

    CLR(clkPort, clkBit);
    SET(clkPort, clkBit);
  }
}


#define dataPin 4
#define clockPin 5
#define dataPort PORTD
#define dataBit PORTD4
#define clockPort PORTD
#define clockBit PORTD5

void setup() {
  pinMode(dataPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
}

void loop() {
  shiftOut(dataPin, clockPin, LSBFIRST, 0x55);
  delay(1);

  shiftOutFast(dataPort, dataBit, clockPort, clockBit, LSBFIRST, 0x5C);
  delay(1);  

  bool bitOrder = LSBFIRST;
  uint8_t val = 0x5C; 

  for (uint8_t i = 0; i < 8; i++)  {
    if (bitOrder == LSBFIRST) {
      if (val & 0x01) {
        SET(dataPort, dataBit);
      } else {
        CLR(dataPort, dataBit);
      }
      val >>= 1;
    } else {    
      if (val & 0x80) {
        SET(dataPort, dataBit);
      } else {
        CLR(dataPort, dataBit);
      }
      val <<= 1;
    }   

    CLR(clockPort, clockBit);
    SET(clockPort, clockBit);
  }
  delay(1);  
}

The problem is that calling shiftOutFast doesn't do zilch. Absolutely nothing. It doesn't seem to be the logic, because when I do the same inline it works. Any ideas?

Answer 1

The problem is that you are passing the value of the ports to the function, not the address of the ports.

You need to either pass "by reference" or as a pointer, so that modifying the port variable modifies the actual port variable not the copy of the value that you pass.

The simplest way is to modify your function to be:

void shiftOutFast(volatile uint8_t &dataPort, uint8_t dataBit,
                  volatile uint8_t &clkPort, uint8_t clkBit, 
                  uint8_t bitOrder, uint8_t val) {
....
}

That way, instead of passing whatever is in the port at the time to the function (pass by value) it instead creates a new variable which is located at the same address as the variable that you are passing (pass by reference) so modifications of one variable modifies the other.

Note that only the "port" variables need to be passed by reference, all the others remain as pass by value. Also they should be, as Edgar says, flagged as volatile.

Answer 2

You should declare volatile the port parameters of your function. Otherwise, to the compiler, their write accesses seem useless, and they get optimized out.