Volatile Variable breaks code, will not compile

volatile long led[64][4]; //we have an ISR
void setLED(byte x, byte y, byte z, byte r, byte g, byte b)
{
  int loc = (y << 3) + z;
  
  led[loc][0] = ((        (r & 1L) << (x * 3)) + (        (g & 1L) << (x * 3 + 1)) + (        (b & 1L) << (x * 3 + 2))) | (led[loc][0] & ~(7L << (x * 3)));
  led[loc][1] = (((long)!!(r & 2L) << (x * 3)) + ((long)!!(g & 2L) << (x * 3 + 1)) + ((long)!!(b & 2L) << (x * 3 + 2))) | (long)(led[loc][1] & ~(7L << (x * 3)));
  led[loc][2] = (((long)!!(r & 4L) << (x * 3)) + ((long)!!(g & 4L) << (x * 3 + 1)) + ((long)!!(b & 4L) << (x * 3 + 2))) | (long)(led[loc][2] & ~(7L << (x * 3)));
  led[loc][3] = (((long)!!(r & 8L) << (x * 3)) + ((long)!!(g & 8L) << (x * 3 + 1)) + ((long)!!(b & 8L) << (x * 3 + 2))) | (long)(led[loc][3] & ~(7L << (x * 3)));
}

Under completely random conditions, I get this error

  This report would have more information with
  "Show verbose output during compilation"
  enabled in File > Preferences.
Arduino: 1.0.5 (Mac OS X), Board: "Arduino Pro or Pro Mini (5V, 16 MHz) w/ ATmega328"
LED_Cube.ino: In function 'void setLED(byte, byte, byte, byte, byte, byte)':
LED_Cube:139: error: unable to find a register to spill in class 'POINTER_REGS'
LED_Cube:139: error: this is the insn:
(insn 107 106 108 2 LED_Cube.ino:127 (set (reg:SI 52 [ D.3738 ])
        (ashift:SI (reg:SI 52 [ D.3738 ])
            (subreg:QI (reg:HI 51 [ D.3740 ]) 0))) 55 {ashlsi3} (expr_list:REG_DEAD (reg:HI 51 [ D.3740 ])
        (nil)))
LED_Cube.ino:139: confused by earlier errors, bailing out

Removing random lines from the function will fix it, removing volatile will fix it, or even changing the size of the array to certain sizes will fix it such as led[64][5] will work while led[65][4] will not...

Is there any solution to this?

Unable to recreate problem here. You are missing a setup and loop function at the very least.

I also notice you're making use of a NOT NOT operator. Does this even work?

//includes
#include <SPI.h>

//constants
#define ANODE_LEVELS 8 //currently only supported up to 8 anode layers, untested anything lower than 8 but should technically work...maybe with a tweak or two
#define REFRESH_RATE 500

//instance variables
//Timer anodeTimer;
//THESE PIN NUMBERS CANNOT BE CHANGED, THERE ARE HARDWARE DEDICATED TO SPI
#define dataPin 11
#define clockPin 13
#define latchPin 10 //this one can be any pin
#define LATCH (1<<2) //SS (RCK), pin 10 is port B2

//Anode Layers begin at:
//brightness bit 0: 0:[0][0]    1:[24][0]   2:[48][0]   3:[72][0]   4:[96][0]   5:[120][0]  6:[144][0]  7:[168][0]
//brightness bit 1: 0:[0][1]    1:[24][1]   2:[48][1]   3:[72][1]   4:[96][1]   5:[120][1]  6:[144][1]  7:[168][1]
//brightness bit 2: 0:[0][2]    1:[24][2]   2:[48][2]   3:[72][2]   4:[96][2]   5:[120][2]  6:[144][2]  7:[168][2]
//brightness bit 3: 0:[0][3]    1:[24][3]   2:[48][3]   3:[72][3]   4:[96][3]   5:[120][3]  6:[144][3]  7:[168][3]
//  0: GRBGRBGR  22111000
//  1: RBGRBGRB  54443332
//  2: BGRBGRBG  77766655
//...: ........  ........
volatile byte anodeLevel = 0;
volatile byte bamCounter = 0;
volatile byte bamBit = 0;
volatile unsigned long count = 0;
volatile long led[64][4];
#define clearMatrix() memset(led, 0, sizeof(led));

//functions
void setup()
{
  Serial.begin(115200);
  Serial.println('\n');

  SPI.setBitOrder(MSBFIRST);//Most Significant Bit First
  SPI.setDataMode(SPI_MODE0);// Mode 0 Rising edge of data, keep clock low
  SPI.setClockDivider(SPI_CLOCK_DIV2);//Run the data in at 16MHz/2 = 8MHz

  //noInterrupts();// kill interrupts until everybody is set up
  //We use Timer 1 to refresh the cube, its the only 16 bit timer
  TCCR1A = B00000000;//Register A all 0's since we're not toggling any pins
  TCCR1B = B00001011;//bit 3 set for CTC mode, will call interrupt on counter match, bits 0 and 1 set to divide clock by 64, so 16MHz/64=250kHz
  //TIMSK1 = B00000010;//bit 1 set to call the interrupt on an OCR1A match
  OCR1A  = (unsigned int)((250000UL / (REFRESH_RATE * ANODE_LEVELS)) - 1UL);//our clock runs at 250kHz, which is 1/250kHz = 4us, with OCR1A set to 30, the interrupt will be called every (30+1)x4us=124us (8kHz)

  pinMode(dataPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
  pinMode(latchPin, OUTPUT);
  
  SPI.begin();//start up the SPI library
  interrupts();//let the show begin, this lets the multiplexing start
}

//volatile byte x2 = 7, y2 = 7, z2 = 7, r2 = 15, g2 = 15, b2 = 15;

void loop()
{
  count = micros();
  //derp();
  //setLED(x2, y2, z2, r2, g2, b2);
  setLED(0, 0, 0, 0, 0, 0);
  count = micros() - count;
  Serial.println(count);
  while(1);
}

//a background running interrupt when the clock matches
//ISR(TIMER1_COMPA_vect) //Interrupt Service Routine, Timer/Counter1 Compare Match A
void derp()
{
  volatile signed char x, z;
  
  //each output cycle takes ANODE_LEVELS executions and each brightness bit's numerical representation is the length of time it remains on
  //then the 1st bit stays on for 1 cycle (ANODE_LEVELS executions), the 2nd bit stays on for 2 cycles (ANODE_LEVELS * 2 executions), then 4 cycles, and so forth
  //but since we never* reset bamCounter, we must check for the previous bit's cycles, ie: check the 4th bit for (ANODE_LEVELS + ANODE_LEVELS * 2 + ANODE_LEVELS * 4)
  //which is why you see the (x0 default implied), x1, x3, x7, higher resolution would continue x15, x31, x63, x127, or (compareTo = ANODE_LEVELS * ((1 << (bit - 1)) - 1))
  if(bamCounter == ANODE_LEVELS || bamCounter == (ANODE_LEVELS * 3) || bamCounter == (ANODE_LEVELS * 7)) //hardcoded a 4 bit resolution
  {
    bamBit++;
  }
  bamCounter++;
  
  //Anode Shift Register
  SPI.transfer(0xFF - (1 << anodeLevel));
  
  //The other 24 Shift Registers
  for(z = ANODE_LEVELS - 1; z >= 0 ; z--)
  {
    for(x = ANODE_LEVELS - 1; x >= 0 ; x--)
    {
      
    }
  }
  
  //Toggle latch to copy data to the storage register
  PORTB |= LATCH;//Latch pin (pin 10, port B2) HIGH
  PORTB &= ~LATCH;//Latch pin (pin 10, port B2) LOW
  
  if(bamCounter >= ANODE_LEVELS * 15) //4 bit resolution
  {
    bamCounter = 0;
    bamBit = 0;
  }
  anodeLevel = (anodeLevel + 1) % ANODE_LEVELS;
}

      //     0      1      2      3      4      5      6      7
      //0  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //1  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //2  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //3  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //4  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //5  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //6  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B
      //7  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B  R,G,B

void setLED(byte x, byte y, byte z, byte r, byte g, byte b)
{
  int loc = (y << 3) + z;
  
  led[loc][0] = ((        (r & 1L) << (x * 3)) + (        (g & 1L) << (x * 3 + 1)) + (        (b & 1L) << (x * 3 + 2))) | (led[loc][0] & ~(7L << (x * 3)));
  led[loc][1] = (((long)!!(r & 2L) << (x * 3)) + ((long)!!(g & 2L) << (x * 3 + 1)) + ((long)!!(b & 2L) << (x * 3 + 2))) | (long)(led[loc][1] & ~(7L << (x * 3)));
  led[loc][2] = (((long)!!(r & 4L) << (x * 3)) + ((long)!!(g & 4L) << (x * 3 + 1)) + ((long)!!(b & 4L) << (x * 3 + 2))) | (long)(led[loc][2] & ~(7L << (x * 3)));
  led[loc][3] = (((long)!!(r & 8L) << (x * 3)) + ((long)!!(g & 8L) << (x * 3 + 1)) + ((long)!!(b & 8L) << (x * 3 + 2))) | (long)(led[loc][3] & ~(7L << (x * 3)));
  
  /*Serial.print(loc);
  Serial.println(" = ");
  for(int i = 31; i >= 0; i--){if((i + 1) % 8 == 0)Serial.print(" "); Serial.print(!!(led[loc][0] & (1L << i)));}
  Serial.println();
  for(int i = 31; i >= 0; i--){if((i + 1) % 8 == 0)Serial.print(" "); Serial.print(!!(led[loc][1] & (1L << i)));}
  Serial.println();
  for(int i = 31; i >= 0; i--){if((i + 1) % 8 == 0)Serial.print(" "); Serial.print(!!(led[loc][2] & (1L << i)));}
  Serial.println();
  for(int i = 31; i >= 0; i--){if((i + 1) % 8 == 0)Serial.print(" "); Serial.print(!!(led[loc][3] & (1L << i)));}
  Serial.println();*/
}

What is the double exclamation mark notation all about? ( !! )

!!x is equivalent to (bool)x, and constrains a number to either 1 or 0.

When using the silly boolean data type ( Arduino addin, not C++ std bool ), using !! or a cast to bool is is required to assign a number as a boolean correctly (constrain a number greater than or less than zero to one ). Otherwise only tests comparing a range will succeed (x>0), x==true will only be correct if x is 1 or 0, anything else returns false ).

I posted about this problem here: Drop the boolean data type, its broken. (or fix it) - Website and Forum - Arduino Forum

pYro_65:
!!x is equivalent to (bool)x, and constrains a number to either 1 or 0.

When using the silly boolean data type ( Arduino addin, not C++ std bool ), using !! or a cast to bool is is required to assign a number as a boolean correctly (constrain a number greater than or less than zero to one ). Otherwise only tests using comparing a range will succeed (x>0), x==true will only be correct if x is 1 or 0, anything else returns false ).

I posted about this problem here: Drop the boolean data type, its broken. (or fix it) - Website and Forum - Arduino Forum

BUT then cast as a long?

KenF:
BUT then cast as a long?

Its needed if the expression x*3 results in a value greater than 15 (bits to shift ).

Even though a literal long has been used in the expression r & 1L, the !! returns a bool (1 byte) which is promoted to int by the shift operators (usual arithmetic conversions are applied).

So, I would be questioning the literal longs, not the cast, as they only need to be a byte.

But does any of this issue around Arduino's boolean variable type belong in the OPs setLED function?

I don't see any boolean variable types being used here. Just shifting of bits in regular bytes and long type variables.

That setLed() function is a terrible piece of coding.

You are calculating the same expressions over and over again. If the compiler is smart enough to optimise it, it is probably running out of registers to do it.

Furthermore, I don't think you have given enough thought to the actual consequences of using the volatile keyword.

"error: unable to find a register to spill in class 'POINTER_REGS'"

I would think that this is your problem, right there.

Is there any solution to this?

Post the full code that fails please. Not just say that "some" code fails with this error.

[quote author=Nick Gammon link=msg=1982618 date=1417411012]
Post the full code that fails please. Not just say that "some" code fails with this error.
[/quote]Actually Nick, he's already done that in answer #2

I've attempted to compile it in IDE v 1.0.6 and it fails with exactly the errors he mentions. But I suspect it's the convoluted expressions being used in the code he has highlighted.

Oh, sorry.

If you are going to cast them to long madly, why not just make them long?

void setLED(byte x, byte y, byte z, long r, long g, long b)
{
  int loc = (y << 3) + z;
  
  led[loc][0] = ((  (r & 1) << (x * 3)) + (  (g & 1) << (x * 3 + 1)) + (  (b & 1) << (x * 3 + 2))) | (led[loc][0] & ~(7L << (x * 3)));
  led[loc][1] = ((!!(r & 2) << (x * 3)) + (!!(g & 2) << (x * 3 + 1)) + (!!(b & 2) << (x * 3 + 2))) | (led[loc][1] & ~(7L << (x * 3)));
  led[loc][2] = ((!!(r & 4) << (x * 3)) + (!!(g & 4) << (x * 3 + 1)) + (!!(b & 4) << (x * 3 + 2))) | (led[loc][2] & ~(7L << (x * 3)));
  led[loc][3] = ((!!(r & 8) << (x * 3)) + (!!(g & 8) << (x * 3 + 1)) + (!!(b & 8) << (x * 3 + 2))) | (led[loc][3] & ~(7L << (x * 3)));
  
}

That compiles. I also took out the L suffix on most of the literals.

The reason that programming looks so bad is because there are 2 processes that need to be considered.
1)Storing the information into the array
2)Decoding the information into shift register format compliant with how the cube is wired up

I tried storing the information in a simple way, which meant it stored information very fast. But this had the negative effect of making it very slow to reorient the information in a way I need it shifted out.

So Im trying a new method of storing information slowly but shifting it out very efficiently. This is what you see here. It is a test, and just seeing my options.

I totally forgot about the (bool) typecast, I have been very used to !!.

The reason you see L and (long) so many times is because after adding them it SEEMED to fix 1 of the lines in the function, but after applying the same fix to the rest and uncommenting them out, it still didnt work, the math behind it works, I have debugged the results with a non-volatile.

IGNORE
I may probably have to scrap this method totally and just reposition 192 wires on my cube for more efficient data storage and retrieval...this is what I was trying to avoid...

Although... "If the compiler is smart enough to optimise it, it is probably running out of registers to do it." sounds promising...
IGNORE

Nick Gammon...Thank you, it actually does compile!
HOWEVER, using !! and not (long)!! does not work, it treats it as a 16bit number, so I will re-add those in, im curious if it breaks again...

It works :smiley:

Yes, but if r is declared long, as I had it, the expression should be long.

Like, if r is long then:

r & 1

should also be long.

Writing:

r & 1L

is over-egging the pudding, and we know that should be avoided. :wink:

I tried it, it did not work.

Your code (copied and pasted)

63 = 
 00000000 11100000 00000000 00000000
 00000000 00000000 00000000 00000000
 00000000 00000000 00000000 00000000
 00000000 00000000 00000000 00000000

Adding (long) before the !!

63 = 
 00000000 11100000 00000000 00000000
 00000000 11100000 00000000 00000000
 00000000 11100000 00000000 00000000
 00000000 11100000 00000000 00000000

Which is correct
Mental Note To Self: Unplugging the MPU from your USB port can cause your $1100 computer to crash...twice

Let me show you another approach. Taking your original code, and just using temporary variables, like this:

void setLED(byte x, byte y, byte z, byte r, byte g, byte b)
{
  int loc = (y << 3) + z;
  
  long L1 = ((        (r & 1L) << (x * 3)) + (        (g & 1L) << (x * 3 + 1)) + (        (b & 1L) << (x * 3 + 2))) | (led[loc][0] & ~(7L << (x * 3)));
  long L2 = (((long)!!(r & 2L) << (x * 3)) + ((long)!!(g & 2L) << (x * 3 + 1)) + ((long)!!(b & 2L) << (x * 3 + 2))) | (long)(led[loc][1] & ~(7L << (x * 3)));
  long L3 = (((long)!!(r & 4L) << (x * 3)) + ((long)!!(g & 4L) << (x * 3 + 1)) + ((long)!!(b & 4L) << (x * 3 + 2))) | (long)(led[loc][2] & ~(7L << (x * 3)));
  long L4 = (((long)!!(r & 8L) << (x * 3)) + ((long)!!(g & 8L) << (x * 3 + 1)) + ((long)!!(b & 8L) << (x * 3 + 2))) | (long)(led[loc][3] & ~(7L << (x * 3)));

  led[loc][0] = L1;
  led[loc][1] = L2;
  led[loc][2] = L3;
  led[loc][3] = L4;
}

That also compiles. We are not forcing (here) the compiler to consider the volatileness* for the intermediate expression.

  • I made that word up. The spell-check suggests volatility.

I tried it, it did not work.

Try making x, y and z long as well, then.

This guy...clever ducky

But still the x,y,z as long still does not work, (long)!! is required for the output to be correct

That is true, however the expression !!(r&1) is an int after promotion. Regardless of weather r is 8 or 64 bits.

There is nothing wrong with a cast to long there. The shift expression is evaluated before it becomes part of the LHS of another expression containing x,y,z (as long), therefore the 16bit shift result is implicitly converted to long, not the inputs to the shift.