LED Cube interupts

Hi. I've got a problem with my function update() that updates the LEDs of my 4x4x4 cube. I wrote it for the loop() function and it works fine. Then I learnt about timer interrupts from here but when I move update() from loop() to my interrupt function ISR(TIMER1_OVF_vect), 3 layers are only intermittently on and the other is on fine. My code is below.

So what do I have to do to make the update() function work in a interrupt function? From the guide it talks about volatile global variables but is that it? What about #define variables?

Thanks

#include <avr/interrupt.h>

#define TIME 0

#define CUBE_WIDTH 4
#define BITS_PER_BYTE 8
#define NUM_BYTES  CUBE_WIDTH*CUBE_WIDTH*CUBE_WIDTH/(BITS_PER_BYTE*sizeof(uint8_t))
#define NUM_DATA_PINS sizeof(dataPin)/sizeof(int)

volatile uint8_t  cube[NUM_BYTES] = {
  0};
volatile int previous_layer = CUBE_WIDTH;

volatile int dataPin[] = {
  2, 3};
volatile int latchPin = 4;
volatile int clockPin = 5;
volatile int PlanePin[] = {
  13,12,11,10};

void setup() {
  int i;
  for (i=0; i<NUM_DATA_PINS; i++) {
    pinMode(dataPin[i], OUTPUT);
  }
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
  for (i=0; i<CUBE_WIDTH; i++) {
    pinMode(PlanePin[i], OUTPUT);
    digitalWrite(PlanePin[i], HIGH);
  }

  // initialize Timer1
  cli();          // disable global interrupts
  TCCR1A = 0;     // set entire TCCR1A register to 0
  TCCR1B = 0;     // same for TCCR1B
  TCCR1B |= (1 << CS11); // Set CS11 & CS10 bits for 1024 prescaler:
  TCCR1B |= (1 << CS10);
  TIMSK1 = (1 << TOIE1); // enable Timer1 overflow interrupt:
  sei();          // enable global interrupts:

  cube[0] = 249; // This produces a wire frame cube
  cube[1] = 159;
  cube[2] = 144;
  cube[3] = 9;
  cube[4] = 144;
  cube[5] = 9;
  cube[6] = 249;
  cube[7] = 159; 
}

void shiftin(uint8_t data1, uint8_t data2) {
  int b, i;
  for (b=0; b<8; b++) {
    digitalWrite(clockPin, LOW);
    digitalWrite(dataPin[0], data1 & (1<<b));
    digitalWrite(dataPin[1], data2 & (1<<b));
    digitalWrite(clockPin, HIGH);
  }
}

void update(void) {
  int current_layer;
  for (current_layer=0; current_layer<CUBE_WIDTH; current_layer++) {
    digitalWrite(latchPin, LOW); // reset latches
    delay(TIME);
    shiftin(cube[current_layer*NUM_DATA_PINS], cube[current_layer*NUM_DATA_PINS+1]);
    // shift in bits into both 74hc595N
    delay(TIME);
    digitalWrite(PlanePin[previous_layer], HIGH); // turn off previous layer
    // all layers off
    delay(TIME);
    digitalWrite(latchPin, HIGH); // latch shifted bits
    delay(TIME);
    digitalWrite(PlanePin[current_layer], LOW); // turn on current layer
    delay(TIME); 
    previous_layer = current_layer; // record previous layer
  }
}

void loop() {
  update(); //works fine here
}


ISR(TIMER1_OVF_vect)  {
  // update(); //doesn't work properly here
}

You can't call delay() in an ISR. You really have no clue about interrupts, do you?

Yes this is my first time using interrupts, and also my first decent project with Arduino. Edit:If I comment out all the delay() in my update function the problem still persists. But I guess you can't have delays in interrupts because they use timer 0 so you get interrupts in interrupts which is a bit messy.

But I guess you can't have delays in interrupts because they use timer 0 so you get interrupts in interrupts which is a bit messy.

No, that is not why you can't use delay() in an ISR. It doesn't matter which timer is used. The fact is that in an ISR, interrupts are disabled, so you don't get interrupted while you are interrupting.

Since you can't be interrupted by silly things like the clock ticking, time stands still in an ISR.

Therefore, since you've said "wake me up after so much time has passed", and time does not pass in an ISR, you never get waken up.

what do I have to do to make the update() function work in a interrupt function?

Fairly easy:

  1. write the update function so that on each invokation, it updates one layer (or side) of the cube, depending on how your hardware is organized. Like this:
void update(void) {
  static unsigned char layer_index=0; //layer to be updated
  update_current_layer(); //put your code here that updates the current layer indicated by layer_index;
  layer_index=(layer_index==MAX_LAYER - 1)?0:(layer_index+1); //increment to the next layer
}

So each time update() is called from within the isr, it updates a layer.

  1. get rid of those delays.

A couple more suggestions:

  1. the shiftin() isn't well coded. Consider using masks;
  2. you don't want to call another function from within the isr. I would put shiftin() in update() and potentially unroll it too (at least make it inline).
  3. digitalWrite() is done via function calls -> slow and risky. Since you know the precise pins, use them directly.

The key is to make your isr small, fast, and robost.

PaulS:
No, that is not why you can't use delay() in an ISR. It doesn't matter which timer is used. The fact is that in an ISR, interrupts are disabled, so you don't get interrupted while you are interrupting.

Thanks for clearing that.

Following your first suggestions dhenry, I rewrote my code and the cube is now cycling through the layers fine using the ISR but funnily enough it's like it delays on the current layer for a few hundred ms, thus not cycling through the layers fast enough for the POV effect, even though I removed all the delays. Although I don't understand what has changed to make the layers run at full brightness. It seems to me that overall it's doing the same operations but arranged differently.

This is my current code that cycles slowly. I will try later on to streamline it by using bitmasks and the PORTs, all in the one ISR function as like your further tips.

#include <avr/interrupt.h>

#define CUBE_WIDTH 4
#define BITS_PER_BYTE 8
#define NUM_BYTES  CUBE_WIDTH*CUBE_WIDTH*CUBE_WIDTH/(BITS_PER_BYTE*sizeof(uint8_t))
#define NUM_DATA_PINS sizeof(dataPin)/sizeof(int)

volatile uint8_t  cube[NUM_BYTES] = {
  0};
volatile int previous_layer = CUBE_WIDTH;

volatile int dataPin[] = {
  2, 3};
volatile int latchPin = 4;
volatile int clockPin = 5;
volatile int PlanePin[] = {
  13,12,11,10};

void setup() {
  int i;
  for (i=0; i<NUM_DATA_PINS; i++) {
    pinMode(dataPin[i], OUTPUT);
  }
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
  for (i=0; i<CUBE_WIDTH; i++) {
    pinMode(PlanePin[i], OUTPUT);
    digitalWrite(PlanePin[i], HIGH);
  }

  // initialize Timer1
  cli();          // disable global interrupts
  TCCR1A = 0;     // set entire TCCR1A register to 0
  TCCR1B = 0;     // same for TCCR1B
  TCCR1B |= (1 << CS11); // Set CS11 & CS10 bits for 1024 prescaler:
  TCCR1B |= (1 << CS10);
  TIMSK1 = (1 << TOIE1); // enable Timer1 overflow interrupt:
  sei();          // enable global interrupts:

  cube[0] = 249; // This produces a wire frame cube
  cube[1] = 159;
  cube[2] = 144;
  cube[3] = 9;
  cube[4] = 144;
  cube[5] = 9;
  cube[6] = 249;
  cube[7] = 159; 
}

void shiftin(uint8_t data1, uint8_t data2) {
  int b, i;
  for (b=0; b<8; b++) {
    digitalWrite(clockPin, LOW);
    digitalWrite(dataPin[0], data1 & (1<<b));
    digitalWrite(dataPin[1], data2 & (1<<b));
    digitalWrite(clockPin, HIGH);
  }
}

void layers_off(void) {
  int i;
  for (i=0; i<CUBE_WIDTH; i++) {
    digitalWrite(PlanePin[i], HIGH); // turn off layer
  }
}

void update(void) {
  static uint8_t layer_index=0; //layer to be updated
  layers_off();
  update_current_layer(layer_index); //put your code here that updates the current layer indicated by layer_index;
  layer_index=(layer_index==CUBE_WIDTH - 1)?0:(layer_index+1); //increment to the next layer
}

void update_current_layer(uint8_t layer_index) {
  digitalWrite(latchPin, LOW);                  // reset latches
  shiftin(cube[layer_index*NUM_DATA_PINS], cube[layer_index*NUM_DATA_PINS+1]);// shift in bits into both 74hc595N
  digitalWrite(latchPin, HIGH);                 // latch shifted bits
  digitalWrite(PlanePin[layer_index], LOW);     // turn on current layer
}
    

void loop() {
}

ISR(TIMER1_OVF_vect)  {
  update(); 
}

funnily enough it's like it delays on the current layer for a few hundred ms

Try fix this line:

  layer_index=(layer_index==(CUBE_WIDTH - 1))?0:(layer_index+1); //increment to the next layer

Careless me.

I still think your can greatly simplify your update() routine. Way to generic.

Thanks for the help dhenry, I'm pretty happy where I'm at now. I have removed the digitalWrite functions from my ISR function. This is my current code:

#include <avr/interrupt.h>

#define CUBE_WIDTH 4
#define BITS_PER_BYTE 8
#define NUM_BYTES  CUBE_WIDTH*CUBE_WIDTH*CUBE_WIDTH/(BITS_PER_BYTE*sizeof(uint8_t))
#define NUM_DATA_PINS sizeof(dataPin)/sizeof(int)

volatile uint8_t  cube[NUM_BYTES] = {
  0};
volatile int dataPin[] = {
  2, 3};
volatile int latchPin = 12;
volatile int clockPin = 5;
volatile int PlanePin[] = {
  11,10,9,8};

void setup() {
  int i;
  for (i=0; i<NUM_DATA_PINS; i++) {
    pinMode(dataPin[i], OUTPUT);
  }
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
  for (i=0; i<CUBE_WIDTH; i++) {
    pinMode(PlanePin[i], OUTPUT);
    digitalWrite(PlanePin[i], HIGH);
  }

  // initialize Timer1
  cli();                  // disable global interrupts
  TCCR1A = 0;             // set entire TCCR1A register to 0
  TCCR1B = 0;             // same for TCCR1B
//  TCCR1B |= (1 << CS11);  // Set CS11 & CS10 bits for 64 prescaler:
  TCCR1B |= (1 << CS10);
  TIMSK1 = (1 << TOIE1);  // enable Timer1 overflow interrupt:
  sei();                  // enable global interrupts:

  cube[0] = 249;          // This produces a wire frame cube
  cube[1] = 159;
  cube[2] = 144;
  cube[3] = 9;
  cube[4] = 144;
  cube[5] = 9;
  cube[6] = 249;
  cube[7] = 159; 
}

void loop() {
}

ISR(TIMER1_OVF_vect)  {
  static uint8_t layer_index=0; //layer to be updated   
  int b;
  uint8_t bitmask = 1<<layer_index; 
  for (b=0; b<8; b++) {
    PORTD = PORTD | ((cube[layer_index*NUM_DATA_PINS] & (1<<b))>>b)<<2; // DATA0 = (cube[layer_index*NUM_DATA_PINS] & (1<<b)
    PORTD = PORTD | ((cube[layer_index*NUM_DATA_PINS+1] & (1<<b))>>b)<<3; // DATA1 = (cube[layer_index*NUM_DATA_PINS+1] & (1<<b)
    PORTD = PORTD | B00100000; // CLK = 1
    PORTD = PORTD & B11010011; // CLK = 0, DATA0 = 0, DATA1 = 0
  }
  bitmask = (~bitmask) & B00011111;
  PORTB = PORTB & B11100000; // LATCH = 0, All layers on
  PORTB = PORTB | bitmask; // LATCH = 1, layer on
  layer_index=(layer_index==CUBE_WIDTH - 1)?0:(layer_index+1); //increment to the next layer
}

It refreshes the cube at about the same apparent speed as before with all the digtalWrite functions. Then I changed the timer1 prescaler from 64 to 1. At 64 it refreshed the cube every 0.262s, not fast enough, 8 - 0.032s, still slow, then to 1, 0.00041s, which is fast enough.

dhenry:

funnily enough it's like it delays on the current layer for a few hundred ms

Try fix this line:

  layer_index=(layer_index==(CUBE_WIDTH - 1))?0:(layer_index+1); //increment to the next layer

Careless me.

I can't tell what needs fixing? One thing that's weird to me is that if I comment your layer_index increment line and replace it for a while or for loop in my ISR function, I get the same issues as at the very beginning, one layer is bright and the other 3 a flashing dull.

But then I think about your layer_index line and wonder why it works, because to me, it updates only layer_index 0 then increments layer_index to 1, then drops out of the ISR loop, ISR is called again, then layer_index is defined again as 0, updates layer 0, increments layer_index to 1, then drops out of the ISR loop and so on... only ever updating layer 0. How does layer_index ever get to 1, 2 and 3?

layer_index is defined again as 0

layer_index is a static variable.

I can confirm that the increment for the layer index works as expected.

When I get time, I will check the reset of your code.

dhenry:
I can confirm that the increment for the layer index works as expected.

When I get time, I will check the reset of your code.

Sorry, I should have made myself a bit clearer. My code works fine and I've got a few basic animations working, I'm quite pleased with the results. I was just puzzled before because I didn't know how your layer_index code works, but I Googled static variables (which I should have done earlier), and now I see how it works as layer_index is only initialised once. I'm looking forward to moving onto an 8x8x8 now!! Thanks dhenry!