[SOLVED] Bits moved one place to the left...

Projects Programming
1

Hi everyone,

I'm building an S88 decoder for my model railroad. The S88 protocol is based on the 4014 shift registers.
My code works almost... The first 16 bits are OK but the last 16 not, I get on my command station all the bits moved one place to the right on my command station screen.
So the zero on S88data[16] disappeared... Anyone who can find my mistake?

// S88 pins setup
#define S88DATAIN 5
#define S88DATAOUT 4
#define S88CLOCK 2
#define S88LOAD 3 

volatile uint16_t S88DataOutShiftRegister1; // Shift register 1 
volatile uint16_t S88DataOutShiftRegister2; // Shift register 2
volatile uint32_t S88data[33] = {0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,   1, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1}; // S88 data, when this works completely I want to add the wireless stuff to manipulate this data array.

void setup() {
  attachInterrupt(0, CLOCK, CHANGE); // Interrupt stuff for the CLOCK and LOAD
  attachInterrupt(1, LOAD, RISING);
  pinMode(S88DATAOUT, OUTPUT);
}

void loop() { // No loop stuff because it's all interrupt driven.
}

void CLOCK()
{
  detachInterrupt(0); // Disable interrupt while doing some stuff with it.
  if (((PIND & (1 << S88CLOCK)) == (1 << S88CLOCK))) {   // If rising edge, write S88DATAOUT
                if ((S88DataOutShiftRegister1 & 1) == 0) 
			PORTD &= ~(1 << S88DATAOUT);
		else {
			PORTD |= (1 << S88DATAOUT);
                      }

		// Shift register and add value (Make place & move on)
		S88DataOutShiftRegister1 = (S88DataOutShiftRegister1 >> 1);
                S88DataOutShiftRegister2 = (S88DataOutShiftRegister2 >> 1);

	} else {   // If falling edge, read S88DATAIN -> Put the data from the 2nd shift register into the 1st shift register.
		S88DataOutShiftRegister1 |= (S88DataOutShiftRegister2 << 15); // Add it at the last place in the S88DataOut.
 
	};
  attachInterrupt(0, CLOCK, CHANGE);
}

void LOAD() // Setup the S88data arrays / shift registers... Now it's ready to be send!
{
  detachInterrupt(1); // Disable interrupt while doing some stuff with it.
  S88DataOutShiftRegister1 = S88data[0] | (S88data[1]<<1) | (S88data[2]<<2) | (S88data[3]<<3) | (S88data[4]<<4) | (S88data[5]<<5) | (S88data[6]<<6) | (S88data[7]<<7) | (S88data[8]<<8) | (S88data[9]<<9) | (S88data[10]<<10) | (S88data[11]<<11) | (S88data[12]<<12) | (S88data[13]<<13) | (S88data[14]<<14) | (S88data[15]<<15);   
  S88DataOutShiftRegister2 = S88data[16] | (S88data[17]<<1) | (S88data[18]<<2) | (S88data[19]<<3) | (S88data[20]<<4) | (S88data[21]<<5) | (S88data[22]<<6) | (S88data[23]<<7) | (S88data[24]<<8) | (S88data[25]<<9) | (S88data[26]<<10) | (S88data[27]<<11) | (S88data[28]<<12) | (S88data[29]<<13) | (S88data[30]<<14) | (S88data[31]<<15);   
  attachInterrupt(1, LOAD, RISING);
}

Thanks!

Cheers,
Dylan

2

I don't really see the problem. Your array can hold 33 elements (0 - 32) and you have 33 elements in the array. If you try to add more when the array is full, it will overwrite the elements starting at index 0.

Or if the arduino is smart enough, it will stop storing the elements when the array is full. But I don't think it is.

3

C, and C++, have no bounds checking. The arduino will blindly overwrite things before and after your array if you have the indexing wrong.

4

Found it!

// S88 pins setup
#define S88DATAIN 5
#define S88DATAOUT 4
#define S88CLOCK 2
#define S88LOAD 3 

volatile uint16_t S88DataOutShiftRegister1; // Shift register 1 
volatile uint16_t S88DataOutShiftRegister2; // Shift register 2
volatile uint32_t S88data[32] = {0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,   0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1}; // S88 data, when this works completely I want to add the wireless stuff to manipulate this data array.

void setup() {
  attachInterrupt(0, CLOCK, CHANGE); // Interrupt stuff for the CLOCK and LOAD
  attachInterrupt(1, LOAD, RISING);
  pinMode(S88DATAOUT, OUTPUT);
}

void loop() { // No loop stuff because it's all interrupt driven.
}

void CLOCK()
{
  detachInterrupt(0); // Disable interrupt while doing some stuff with it.
  if (((PIND & (1 << S88CLOCK)) == (1 << S88CLOCK))) {   // If rising edge, write S88DATAOUT
                if ((S88DataOutShiftRegister1 & 1) == 0) 
			PORTD &= ~(1 << S88DATAOUT);
		else {
			PORTD |= (1 << S88DATAOUT);
                      }

		// Shift register and add value (Make place & move on)
		S88DataOutShiftRegister1 = (S88DataOutShiftRegister1 >> 1);
                

	} else {   // If falling edge, read S88DATAIN -> Put the data from the 2nd shift register into the 1st shift register.
		S88DataOutShiftRegister1 |= (S88DataOutShiftRegister2 << 15); // Add it at the last place in the S88DataOut.
                S88DataOutShiftRegister2 = (S88DataOutShiftRegister2 >> 1);
	};
  attachInterrupt(0, CLOCK, CHANGE);
}

void LOAD() // Setup the S88data arrays / shift registers... Now it's ready to be send!
{
  detachInterrupt(1); // Disable interrupt while doing some stuff with it.
  S88DataOutShiftRegister1 = S88data[0] | (S88data[1]<<1) | (S88data[2]<<2) | (S88data[3]<<3) | (S88data[4]<<4) | (S88data[5]<<5) | (S88data[6]<<6) | (S88data[7]<<7) | (S88data[8]<<8) | (S88data[9]<<9) | (S88data[10]<<10) | (S88data[11]<<11) | (S88data[12]<<12) | (S88data[13]<<13) | (S88data[14]<<14) | (S88data[15]<<15);   
  S88DataOutShiftRegister2 = S88data[16] | (S88data[17]<<1) | (S88data[18]<<2) | (S88data[19]<<3) | (S88data[20]<<4) | (S88data[21]<<5) | (S88data[22]<<6) | (S88data[23]<<7) | (S88data[24]<<8) | (S88data[25]<<9) | (S88data[26]<<10) | (S88data[27]<<11) | (S88data[28]<<12) | (S88data[29]<<13) | (S88data[30]<<14) | (S88data[31]<<15);   
  attachInterrupt(1, LOAD, RISING);
}

I moved this from the INT 0 HIGH
S88DataOutShiftRegister2 = (S88DataOutShiftRegister2 >> 1);
to INT 0 LOW
It was that bit that always felt off.

Cheers,
Dylan

5

why not using shiftOut() and shiftIn() to read those registers?

6

Oh I didn't saw those 2 functions... Thanks!

Cheers,
Dylan

7

I thought it was solved but it isn't. When I extend the code with a 4th shift register, all the bits move one place to the left.
I was wrong with my title it should be left in stead of right.
With a 5th shift register they also move one place and so on...

I think there's missing something in my code but I can't find it...

// S88 pins setup
#define S88DATAIN 5
#define S88DATAOUT 4
#define S88CLOCK 2
#define S88LOAD 3

volatile uint16_t S88DataOutShiftRegister1; // Shift register 1
volatile uint16_t S88DataOutShiftRegister2; // Shift register 2
volatile uint16_t S88DataOutShiftRegister3; // Shift register 3
volatile uint16_t S88DataOutShiftRegister4; // Shift register 4
volatile uint64_t S88data[64] = {0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,   0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1,   0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,    0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1}; // S88 data, when this works completely I want to add the wireless stuff to manipulate this data array.

void setup() {
  attachInterrupt(0, CLOCK, CHANGE); // Interrupt stuff for the CLOCK and LOAD
  attachInterrupt(1, LOAD, RISING);
  pinMode(S88DATAOUT, OUTPUT);
}

void loop() { // No loop stuff because it's all interrupt driven.
}

void CLOCK()
{
  detachInterrupt(0); // Disable interrupt while doing some stuff with it.
  if (((PIND & (1 << S88CLOCK)) == (1 << S88CLOCK))) {   // If rising edge, write S88DATAOUT
    if ((S88DataOutShiftRegister1 & 1) == 0)
      PORTD &= ~(1 << S88DATAOUT);
    else {
      PORTD |= (1 << S88DATAOUT);
    }

  } else {   // If falling edge, read S88DATAIN -> Put the data from the 2nd shift register into the 1st shift register.
    S88DataOutShiftRegister1 = (S88DataOutShiftRegister1 >> 1); // Make place

    S88DataOutShiftRegister1 |= (S88DataOutShiftRegister2 << 15); // Add it at the last place in the S88DataOutRegister1.
    S88DataOutShiftRegister2 = (S88DataOutShiftRegister2 >> 1);   // Make place
    
    S88DataOutShiftRegister2 |= (S88DataOutShiftRegister3 << 15); // Add it at the last place in the S88DataOutRegister2.
    S88DataOutShiftRegister3 = (S88DataOutShiftRegister3 >> 1);   // Make place
    
    S88DataOutShiftRegister3 |= (S88DataOutShiftRegister4 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister4 = (S88DataOutShiftRegister4 >> 1);   // Make place
  };
  attachInterrupt(0, CLOCK, CHANGE);
}

void LOAD() // Setup the S88data arrays / shift registers... Now it's ready to be send!
{
  detachInterrupt(1); // Disable interrupt while doing some stuff with it.
  S88DataOutShiftRegister1 = S88data[0]  | (S88data[1] << 1)  | (S88data[2] << 2)  | (S88data[3] << 3)  | (S88data[4] << 4)  | (S88data[5] << 5)  | (S88data[6] << 6)  | (S88data[7] << 7)  | (S88data[8] << 8)  | (S88data[9] << 9)  | (S88data[10] << 10) | (S88data[11] << 11) | (S88data[12] << 12) | (S88data[13] << 13) | (S88data[14] << 14) | (S88data[15] << 15);
  S88DataOutShiftRegister2 = S88data[16] | (S88data[17] << 1) | (S88data[18] << 2) | (S88data[19] << 3) | (S88data[20] << 4) | (S88data[21] << 5) | (S88data[22] << 6) | (S88data[23] << 7) | (S88data[24] << 8) | (S88data[25] << 9) | (S88data[26] << 10) | (S88data[27] << 11) | (S88data[28] << 12) | (S88data[29] << 13) | (S88data[30] << 14) | (S88data[31] << 15);
  S88DataOutShiftRegister3 = S88data[32] | (S88data[33] << 1) | (S88data[34] << 2) | (S88data[35] << 3) | (S88data[36] << 4) | (S88data[37] << 5) | (S88data[38] << 6) | (S88data[39] << 7) | (S88data[40] << 8) | (S88data[41] << 9) | (S88data[42] << 10) | (S88data[43] << 11) | (S88data[44] << 12) | (S88data[45] << 13) | (S88data[46] << 14) | (S88data[47] << 15);
  S88DataOutShiftRegister4 = S88data[48] | (S88data[49] << 1) | (S88data[50] << 2) | (S88data[51] << 3) | (S88data[52] << 4) | (S88data[53] << 5) | (S88data[54] << 6) | (S88data[55] << 7) | (S88data[56] << 8) | (S88data[57] << 9) | (S88data[58] << 10) | (S88data[59] << 11) | (S88data[60] << 12) | (S88data[61] << 13) | (S88data[62] << 14) | (S88data[63] << 15);
  attachInterrupt(1, LOAD, RISING);
}

Cheers,
Dylan

8

Thanks to the HackerSpace email talk it's fixed:

// S88 pins setup
#define S88DATAIN 5
#define S88DATAOUT 4
#define S88CLOCK 2
#define S88LOAD 3

uint16_t S88DataOutShiftRegister1; // Shift register 1
uint16_t S88DataOutShiftRegister2; // Shift register 2
uint16_t S88DataOutShiftRegister3; // Shift register 3
uint16_t S88DataOutShiftRegister4; // Shift register 4
uint16_t S88DataOutShiftRegister5; // Shift register 5
uint16_t S88DataOutShiftRegister6; // Shift register 6
uint16_t S88DataOutShiftRegister7; // Shift register 7
uint16_t S88DataOutShiftRegister8; // Shift register 8
uint16_t S88DataOutShiftRegister9; // Shift register 9
uint16_t S88DataOutShiftRegister10; // Shift register 10
uint8_t S88data[64] = {0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,   0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1,   0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1,    0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1}; // S88 data, when this works completely I want to add the wireless stuff to manipulate this data array.

void setup() {
  attachInterrupt(0, CLOCK, CHANGE); // Interrupt stuff for the CLOCK and LOAD
  attachInterrupt(1, LOAD, RISING);
  pinMode(S88DATAOUT, OUTPUT);
}

void loop() { // No loop stuff because it's all interrupt driven.
}

void CLOCK()
{
  detachInterrupt(0); // Disable interrupt while doing some stuff with it.
  if (((PIND & (1 << S88CLOCK)) == (1 << S88CLOCK))) {   // If rising edge, write S88DATAOUT
    if ((S88DataOutShiftRegister1 & 1) == 0)
      PORTD &= ~(1 << S88DATAOUT);
    else {
      PORTD |= (1 << S88DATAOUT);
    }

  } else {   // If falling edge, read S88DATAIN -> Put the data from the 2nd shift register into the 1st shift register.
    S88DataOutShiftRegister1 = (S88DataOutShiftRegister1 >> 1); // Make place

    S88DataOutShiftRegister1 |= (S88DataOutShiftRegister2 << 15); // Add it at the last place in the S88DataOutRegister1.
    S88DataOutShiftRegister2 = (S88DataOutShiftRegister2 >> 1);   // Make place
    
    S88DataOutShiftRegister2 |= (S88DataOutShiftRegister3 << 15); // Add it at the last place in the S88DataOutRegister2.
    S88DataOutShiftRegister3 = (S88DataOutShiftRegister3 >> 1);   // Make place
    
    S88DataOutShiftRegister3 |= (S88DataOutShiftRegister4 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister4 = (S88DataOutShiftRegister4 >> 1);   // Make place
    
    S88DataOutShiftRegister4 |= (S88DataOutShiftRegister5 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister5 = (S88DataOutShiftRegister5 >> 1);   // Make place
   
    S88DataOutShiftRegister5 |= (S88DataOutShiftRegister6 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister6 = (S88DataOutShiftRegister6 >> 1);   // Make place
    
    S88DataOutShiftRegister6 |= (S88DataOutShiftRegister7 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister7 = (S88DataOutShiftRegister7 >> 1);   // Make place
    
    S88DataOutShiftRegister7 |= (S88DataOutShiftRegister8 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister8 = (S88DataOutShiftRegister8 >> 1);   // Make place
    
    S88DataOutShiftRegister8 |= (S88DataOutShiftRegister9 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister9 = (S88DataOutShiftRegister9 >> 1);   // Make place
    
    S88DataOutShiftRegister9 |= (S88DataOutShiftRegister10 << 15); // Add it at the last place in the S88DataOutRegister3.
    S88DataOutShiftRegister10 = (S88DataOutShiftRegister10 >> 1);   // Make place
  };
  attachInterrupt(0, CLOCK, CHANGE);
}

void LOAD() // Setup the S88data arrays / shift registers... Now it's ready to be send!
{
  detachInterrupt(1); // Disable interrupt while doing some stuff with it.
  S88DataOutShiftRegister1 = S88data[0]  | (S88data[1] << 1)  | (S88data[2] << 2)  | (S88data[3] << 3)  | (S88data[4] << 4)  | (S88data[5] << 5)  | (S88data[6] << 6)  | (S88data[7] << 7)  | (S88data[8] << 8)  | (S88data[9] << 9)  | (S88data[10] << 10) | (S88data[11] << 11) | (S88data[12] << 12) | (S88data[13] << 13) | (S88data[14] << 14) | (S88data[15] << 15);
  S88DataOutShiftRegister2 = S88data[16] | (S88data[17] << 1) | (S88data[18] << 2) | (S88data[19] << 3) | (S88data[20] << 4) | (S88data[21] << 5) | (S88data[22] << 6) | (S88data[23] << 7) | (S88data[24] << 8) | (S88data[25] << 9) | (S88data[26] << 10) | (S88data[27] << 11) | (S88data[28] << 12) | (S88data[29] << 13) | (S88data[30] << 14) | (S88data[31] << 15);
  S88DataOutShiftRegister3 = S88data[32] | (S88data[33] << 1) | (S88data[34] << 2) | (S88data[35] << 3) | (S88data[36] << 4) | (S88data[37] << 5) | (S88data[38] << 6) | (S88data[39] << 7) | (S88data[40] << 8) | (S88data[41] << 9) | (S88data[42] << 10) | (S88data[43] << 11) | (S88data[44] << 12) | (S88data[45] << 13) | (S88data[46] << 14) | (S88data[47] << 15);
  S88DataOutShiftRegister4 = S88data[48] | (S88data[49] << 1) | (S88data[50] << 2) | (S88data[51] << 3) | (S88data[52] << 4) | (S88data[53] << 5) | (S88data[54] << 6) | (S88data[55] << 7) | (S88data[56] << 8) | (S88data[57] << 9) | (S88data[58] << 10) | (S88data[59] << 11) | (S88data[60] << 12) | (S88data[61] << 13) | (S88data[62] << 14) | (S88data[63] << 15);
  S88DataOutShiftRegister5 = S88data[0]  | (S88data[1] << 1)  | (S88data[2] << 2)  | (S88data[3] << 3)  | (S88data[4] << 4)  | (S88data[5] << 5)  | (S88data[6] << 6)  | (S88data[7] << 7)  | (S88data[8] << 8)  | (S88data[9] << 9)  | (S88data[10] << 10) | (S88data[11] << 11) | (S88data[12] << 12) | (S88data[13] << 13) | (S88data[14] << 14) | (S88data[15] << 15);
  S88DataOutShiftRegister6 = S88data[16] | (S88data[17] << 1) | (S88data[18] << 2) | (S88data[19] << 3) | (S88data[20] << 4) | (S88data[21] << 5) | (S88data[22] << 6) | (S88data[23] << 7) | (S88data[24] << 8) | (S88data[25] << 9) | (S88data[26] << 10) | (S88data[27] << 11) | (S88data[28] << 12) | (S88data[29] << 13) | (S88data[30] << 14) | (S88data[31] << 15);
  S88DataOutShiftRegister7 = S88data[32] | (S88data[33] << 1) | (S88data[34] << 2) | (S88data[35] << 3) | (S88data[36] << 4) | (S88data[37] << 5) | (S88data[38] << 6) | (S88data[39] << 7) | (S88data[40] << 8) | (S88data[41] << 9) | (S88data[42] << 10) | (S88data[43] << 11) | (S88data[44] << 12) | (S88data[45] << 13) | (S88data[46] << 14) | (S88data[47] << 15);
  S88DataOutShiftRegister8 = S88data[0]  | (S88data[1] << 1)  | (S88data[2] << 2)  | (S88data[3] << 3)  | (S88data[4] << 4)  | (S88data[5] << 5)  | (S88data[6] << 6)  | (S88data[7] << 7)  | (S88data[8] << 8)  | (S88data[9] << 9)  | (S88data[10] << 10) | (S88data[11] << 11) | (S88data[12] << 12) | (S88data[13] << 13) | (S88data[14] << 14) | (S88data[15] << 15);
  S88DataOutShiftRegister9 = S88data[16] | (S88data[17] << 1) | (S88data[18] << 2) | (S88data[19] << 3) | (S88data[20] << 4) | (S88data[21] << 5) | (S88data[22] << 6) | (S88data[23] << 7) | (S88data[24] << 8) | (S88data[25] << 9) | (S88data[26] << 10) | (S88data[27] << 11) | (S88data[28] << 12) | (S88data[29] << 13) | (S88data[30] << 14) | (S88data[31] << 15);
  S88DataOutShiftRegister10 = S88data[32] | (S88data[33] << 1) | (S88data[34] << 2) | (S88data[35] << 3) | (S88data[36] << 4) | (S88data[37] << 5) | (S88data[38] << 6) | (S88data[39] << 7) | (S88data[40] << 8) | (S88data[41] << 9) | (S88data[42] << 10) | (S88data[43] << 11) | (S88data[44] << 12) | (S88data[45] << 13) | (S88data[46] << 14) | (S88data[47] << 15);
  attachInterrupt(1, LOAD, RISING);
}

CHANGES: 'volatile' removed and 64bit array changed to 8bit array.

Cheers,
Dylan