Weird problem with shift register reading

Go To Last Post
7 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AVR problem

 

Okay, so I have a weird problem using an ATMEGA328P. I have a project which involves the controller reading the 16 lines of an address bus by reading in from two daisy-chained shift registers. And that’s working fine.

 

I decided to upgrade it by also having it read from an 8-bit data bus (from a single shift register). There’s a switch connected to one of the AVR’s pins to decide which bus to read. And it sorta works except for one weird problem.

 

For testing purposes, I have some DIP switches set up to feed data to the AVR. The switches are set so that the address bus reads 0x8111 (a largely random choice). Instead, it’s reading 0x0222. It’s like it’s pulling one bit too many from the shift registers.

 

Here’s some code (there’s a setPin() method mentioned which I know is good and I’ve only included the relevant stuff):

 

// INPUT
#define BUS_SELECT PB0                      // which bus to read - HIGH = address, LOW = data
// Common to both buses and connected to both sets of shift registers
#define IN_CLK  PD4                               // clock pin - CLK pin 2 on shift reg chip
#define IN_SH_LD PD6                            // shift/load - SH//LD pin 1 on chip
// Address bus input shift register
#define IN_ADDR_DATA PD5                   // data pin - Qh pin 9 on chip
#define IN_ADDR_CLK_INH PD7             // clock inhibit    - CLK INH pin 15 on chip

// Data bus input shift register
#define IN_DATA_DATA PD2                   // data pin - Qh pin 9 on chip
#define IN_DATA_CLK_INH PD3             // clock inhibit    - CLK INH pin 15 on chip

#define ADDR_BUS 1
#define DATA_BUS 0

/* … other stuff … */

void readBus(volatile uint16_t * bus, const uint8_t busSel)
{
    *bus = 0;
    //busSel = ADDR_BUS;

    const uint8_t numBits = (busSel * 8) + 7; // 7 for data bus, 15 for addr bus
    uint8_t clockInhibit = IN_ADDR_CLK_INH;   // default to addr bus
    uint8_t inData = IN_ADDR_DATA;

    if (busSel == DATA_BUS) {
        clockInhibit = IN_DATA_CLK_INH;    // don't think this code is executing
        inData = IN_DATA_DATA;            // yet this line seems to be causing problem
    }

    setPin(&PORTD, IN_SH_LD, HIGH);
    setPin(&PORTD, IN_CLK, LOW);
    setPin(&PORTD, clockInhibit, LOW);
    for (int8_t b = numBits; b >= 0; b--) {       // shift bits in MSB first
        setPin(&PORTD, IN_CLK, HIGH);             // going high shifts data bit
        uint8_t bit = readPin(&PIND, inData);    // read data bit
        *bus |= (bit << b);                                   // add bit to our bus byte/word
         setPin(&PORTD, IN_CLK, LOW);             // going high shifts data
    }
    setPin(&PORTD, IN_CLK, HIGH);
    setPin(&PORTD, clockInhibit, HIGH);
    setPin(&PORTD, IN_SH_LD, LOW);

    // DUMMY DATA - FOR TESTING ONLY ---------------------------------------------------
    if (busSel == ADDR_BUS) {
        //*bus = *bus | 0xFF;
        //*bus = 0x0ADD;
    }
    if (busSel == DATA_BUS) {
        //*bus = 0xDA;
        //*bus = *bus & 0x00;
    }
}

int main(void) {
    uint16_t busVal = 0x0;
    uint8_t busSelect = 1;
    /* … */
    DDRB = 0b00001110;
    DDRC = 0b00101111;
    DDRD = 0b11011000;                            // 3,4,6,7 outputs, 2 & 5 inputs
    while 1 {
        busSelect = readPin(&PINB, BUS_SELECT);
        readBus(&busVal, busSelect);
    }
}

 

So here’s where it gets weird. Running with the address bus selected (and I know from other factors that it’s correctly selected) I get the incorrect value of 0x0222. But, see that if (busSel == DATA_BUS) block in the readBus() function? If I comment out the inData = IN_DATA_DATA line, then I get the correct value of 0x8111. This is in spite of the fact that the block should not be executing because busSel should be set to the value of ADDR_BUS.

 

So, I thought that maybe busSel isn’t getting set to the correct value. I tried not making busSel a const and manually setting it to busSel = ADDR_BUS within the function (you can see it commented out). This also solved the problem (but obviously isn’t what I want to do).

 

In the main loop, I also tried the following to check the validity of the comparison with ADDR_BUS:

 

if (busSelect == ADDR_BUS) {
    readBus(&busVal, busSelect);
}

 

The readBus() executed fine (albeit with the same value error).

 

But here’s another twist: let’s go back to the code as it is above (ie, producing incorrect output) and uncomment a couple of the lines in the DUMMY DATA block. Here, the if() tests work fine. In other words,  ‘if (busSel == ADDR_BUS)’ works as it should, suggesting that busSel *is* set correctly. (If I throw the switch, it correctly runs the ‘if (busSel == DATA_BUS)’ block instead.)

 

And finally another twist. I have IN_DATA_DATA assigned to PD2, a currently unused pin (I haven’t yet set up the address bus hardware, although I have tried tying this pin to ground to stop it floating). If I assign this to PD5 - the same as for the data bus - everything works fine. I can switch between address and data bus and get the results I expect. But using PD0, PD1 or PD2 produces the error.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just out of interest, why implement soft SPI when 328 has real SPI hardware? 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

  for (int8_t b = numBits; b >= 0; b--) {       // shift bits in MSB first
        setPin(&PORTD, IN_CLK, HIGH);             // going high shifts data bit
        uint8_t bit = readPin(&PIND, inData);    // read data bit
        *bus |= (bit << b);                                   // add bit to our bus byte/word
         setPin(&PORTD, IN_CLK, LOW);             // going high shifts data
    }
    
 

 

// I'm assuming a 165 shift register - as you didn't explicitly state this.
// when you load the 165, the msb is available before you clock
// here's how I'd do it:
  *bus = 0;
  for (uint8_t b = 0; b >= numBits; b++) 
  {
      // shift bits in MSB first
      if (PIND & PD5)
        {
        *bus |= 1;
        }
        setPin(&PORTD, IN_CLK, HIGH);             // going high shifts data bit
        setPin(&PORTD, IN_CLK, LOW);             // going high shifts data
        *bus <<=1;
    }
// we sample the bit first. 
// AVRs do not have a barrel shifter, so shifts by N are expensive
// so we set the lsb if the incoming bit is set, then shift the bit along -
// two 1 bit shifts is cheaper than N bit shifts.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
here's how I'd do it
The shift operation is in the wrong position.

So your code reproduces the error observed by the OP. ;-)

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The routine reading the shift register is working fine in the earlier version of the program. And, in fact, if I substitute the macros for the variables it also works - ie:

 

    setPin(&PORTD, IN_SH_LD, HIGH);
    setPin(&PORTD, IN_CLK, LOW);
    setPin(&PORTD, IN_ADDR_CLK_INH, LOW);
    for (int8_t b = numBits; b >= 0; b--) {       // shift bits in MSB first
        setPin(&PORTD, IN_CLK, HIGH);             // going high shifts data bit
        uint8_t bit = readPin(&PIND, IN_ADDR_DATA);    // read data bit
        *bus |= (bit << b);                                   // add bit to our bus byte/word
         setPin(&PORTD, IN_CLK, LOW);             // going high shifts data
    }
    setPin(&PORTD, IN_CLK, HIGH);
    setPin(&PORTD, clockInhibit, HIGH);
    setPin(&PORTD, IN_SH_LD, LOW);

This now works perfectly. So I'm thinking there's something weird going on with the inData variable. It's a uint8_t, which is what the readPin() function expects.

 

I still can't work out why 

if (busSel == DATA_BUS)

seems to be matching (because commenting out 'inData = IN_DATA_DATA' removes the bug) even though, later in the function, 'if (busSel == ADR_BUS)' matches and 'if (busSel -- DATA_BUS)' doesn't.

 

EDIT: My attention is now on the inData variable. If I amend the shifting process above so it reads as below (ie, clockInhibit remains in place but inData is replaced by IN_ADDR_DATA), then it works.

 

setPin(&PORTD, IN_SH_LD, HIGH);
    setPin(&PORTD, IN_CLK, LOW);
    setPin(&PORTD, clockInhibit, LOW);
    for (int8_t b = numBits; b >= 0; b--) {       // shift bits in MSB first
        setPin(&PORTD, IN_CLK, HIGH);             // going high shifts data bit
        uint8_t bit = readPin(&PIND, IN_ADDR_DATA);    // read data bit
        *bus |= (bit << b);                                   // add bit to our bus byte/word
         setPin(&PORTD, IN_CLK, LOW);             // going high shifts data
    }
    setPin(&PORTD, IN_CLK, HIGH);
    setPin(&PORTD, clockInhibit, HIGH);
    setPin(&PORTD, IN_SH_LD, LOW);

 

Last Edited: Sun. Oct 29, 2017 - 03:38 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Curioser and curioser. I decided I needed to see a little more as to what's happening in that readBus() function. So the function now looks like this:

 

void readBus(volatile uint16_t * bus, const uint8_t busSel)
{
	*bus = 0;
	//busSel = ADDR_BUS;

	const uint8_t numBits = (busSel * 8) + 7;
	uint8_t clockInhibit = IN_ADDR_CLK_INH;
	uint8_t inData = IN_ADDR_DATA;

	uint8_t tmp = 0;

	if (busSel == DATA_BUS) {
		clockInhibit = IN_DATA_CLK_INH;		// don't think this code is executing
		inData = IN_DATA_DATA;			// yet this line seems to be causing problem
		tmp = 0xF0;
	}

	setPin(&PORTD, IN_SH_LD, HIGH);
	setPin(&PORTD, IN_CLK, LOW);
	setPin(&PORTD, clockInhibit, LOW);
	for (int8_t b = numBits; b >= 0; b--) {		// shift bits in MSB first
		setPin(&PORTD, IN_CLK, HIGH);			// going high shifts data bit
		uint8_t bit = readPin(&PIND, inData);	// read data bit
		*bus |= (bit << b);						// add bit to our bus byte/word
		setPin(&PORTD, IN_CLK, LOW);			// going high shifts data
	}
	setPin(&PORTD, IN_CLK, HIGH);
	setPin(&PORTD, clockInhibit, HIGH);
	setPin(&PORTD, IN_SH_LD, LOW);

	// DUMMY DATA - FOR TESTING ONLY ---------------------------------------------------
	if (busSel == ADDR_BUS) {
		*bus &= 0xFF00;	// clear last byte
		*bus |= busSel;
		*bus |= tmp;
		//*bus = 0x0ADD;
	}
	if (busSel == DATA_BUS) {
		//*bus = 0xDA;
		*bus = *bus & 0x00;
	}
	// ---------------------------------------------------------------------------------
}

I've added the tmp variable. This gets set to 0xF0 inside the 'if (busSel == DATA_BUS)' block. At the end of the function I clear the lower byte of the result and then OR the result with tmp and busSel. The result is that the lower byte displays as 01 telling me that:

  • the 'if (busSel == DATA_BUS)' block isn't being executed.
  • The value of busSel is, indeed, 1, which is what it should be.

And yet, if I comment out 'inData = IN_DATA_DATA;' in the block that isn't being executed, the code works.

 

I then changed the code to OR the end result with inData, instead of busSel, and this gives the value 5 - which is correct because the PD5 macro is set to 5 in the standard libraries. So inData is correct (regardless of whether the line mentioned above is commented out or not).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Okay, I took Kartman's point about the 74HC165's MSB being available before shifting. I swapped two lines and it works:

	for (int8_t b = numBits; b >= 0; b--) {		// shift bits in MSB first
		uint8_t bit = readPin(&PIND, inData);	// read data bit
		setPin(&PORTD, IN_CLK, HIGH);			// going high shifts data bit
		*bus |= (bit << b);						// add bit to our bus byte/word
		setPin(&PORTD, IN_CLK, LOW);			// going high shifts data
	}

But I'm still intrigued as to why the commenting out of a line that wasn't being executed made a difference!