A suggestion for the Arduino's shiftOut function

I encountered a minor issue with the Arduino’s shiftOut function, which took me a while to find out. I thought I should share it in case other people encounter the same problem. The current implementation of this function is as follows:

void shiftOut(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder, uint8_t val)
{
	uint8_t i;

	for (i = 0; i < 8; i++)  {
		if (bitOrder == LSBFIRST)
			digitalWrite(dataPin, !!(val & (1 << i)));
		else	
			digitalWrite(dataPin, !!(val & (1 << (7 - i))));
		digitalWrite(clockPin, HIGH);
		digitalWrite(clockPin, LOW);	
	}
}

Note that this assumes the clockPin is LOW when this function is called, because the shift register will only accept value on the dataPin on a rising edge of the clockPin. The problem I encountered is that my clockPin is shared with the LCD. Sharing pins itself should not be an issue since both the shift register and the LCD have their own ‘enable’ pins, so they won’t interfere with each other. However, with the current way the shiftOut function is implemented, upon calling this function, clockPin’s status is unknown due to the pin sharing, thus it will cause problems with a missing rising edge.

My suggestion is to add digitalWrite(clockPin, LOW); right before the for loop, thus asserting clockPin to be LOW before entering the loop. Alternatively, move this line to right before the if statement as follows:

void shiftOut(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder, uint8_t val)
{
	uint8_t i;

	for (i = 0; i < 8; i++)  {
		digitalWrite(clockPin, LOW);	
		if (bitOrder == LSBFIRST)
			digitalWrite(dataPin, !!(val & (1 << i)));
		else	
			digitalWrite(dataPin, !!(val & (1 << (7 - i))));
		digitalWrite(clockPin, HIGH);
	}
}

I think this is more robust anyways, since it’s not a good idea to assume that clockPin is unaltered outside this function. Hope my suggestion makes sense.

I agree that the code needs a tweak to ensure it always works. The proposed code fix corrects a missing clock when the data clocked on a rising edge. Not all devices clock on rising edges. LCDs and GLCDs for example usually clock on the falling edge of the E strobe.

The fix you proposed in the example code, fixes an initial missing rising edge but now creates a potential missing falling edge because it won't set the clock back to low after the last bit. It only sets the clock to high.

In order to fix it for both edges the clock state must be initially set to low and then strobed high to low for each bit.

To do that either it needs to do your first suggestion of setting the bit low before the for() loop or it needs to do the proposed example code and then set the bit low just after the for() loop.

It is probably cleansest to set the bit low before the for() loop.

BTW, note to moderator: This might be better placed in the Suggestions for the Arduino Project forum.

--- bill

including bperrybap’s remark results in :

void shiftOut(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder, uint8_t val)
{
	uint8_t i;
	digitalWrite(clockPin, LOW);	
	for (i = 0; i < 8; i++)  {
		if (bitOrder == LSBFIRST)
			digitalWrite(dataPin, !!(val & (1 << i)));
		else	
			digitalWrite(dataPin, !!(val & (1 << (7 - i))));
		digitalWrite(clockPin, HIGH);
		digitalWrite(clockPin, LOW);	
	}
}

Reported - http://code.google.com/p/arduino/issues/detail?id=696 -

Great. Thanks, Bill and Rob.

reaction on the bug report:

Won't this create problems for devices that accept a data bit on a falling edge? That is, if the clock pin is high when the function is called, the initial digitalWrite() call will generate a falling edge, clocking in an unknown bit (since the data pin hasn't been set yet). If so, it seems better to omit it, since people using devices that require rising edges can still include the digitalWrite(clock, LOW) call outside the call to shiftOut().

Think they have a valid point: If the state of the clockpin is not defined or wrong it is up to the user to make it right.

Very true, I agree. I was thinking about that a bit more after we proposed this.

Can this ever be made to work for both edges?

After thinking about this a bit more, shiftOut() can work for both edges as long as the clock is not used by anything else after a call to shiftOut().

Going back to the original code.

So for example, the user will set the initial clock level, once and only once. Set it to LOW for rising edge clocking and HIGH for falling edge clocking.

Then he can call shiftOut() as needed to clock out the bits. For rising edge the clock occurs when the pin is set to HIGH and for falling edge the clock occurs when the pin is set th LOW.

A situation like the original use case of sharing the clock line is a very special use case since it involves using enables which are outside the control of the shiftOut() function. This use cannot be known by the shiftOut() and therefore will take special setup by the users code because without enable lines, changing the clock line can clock garbage bits into hardware.

So I believe this boils down to a documentation issue and that no code changes should be made.

The shiftOut() function needs better documentation. The current documentation is quite lacking on its use, particularly about the initial state of the clock line. It works today because most people are using rising edge and the AVR pin states default to low.

It should indicate the function can work for rising or falling edge clocks and that the clock line must be set up to the proper level before calling shiftOut() and then that the clock line should not be altered between calls to shiftOut().

It should also indicate that if the clock is shared with other functions that it is the responsibility of the user code to re-intialize the clock line state to the prior level prior to calling shiftOut().

--- bill