Wacky unknown pointer issues.

This method runs smoothly for about 30 calls or so. However, after that is suddenly starts giving out odd numbers. I am new to pointers, but I don’t really see what could cause this. Chances are I’m doing something wrong though.

Here is the method:

byte *layerPointer;

void CubeControl::writeCube(cubeStep thisCube)
{
    int startTime = millis();
    layerPointer = 0;
    
    while(millis()-startTime <= thisCube.stepDuration)
    {
        for (int thisLayer = 0; thisLayer < numLayers; thisLayer++)
        {
            layerPointer = &thisCube.layer[thisLayer][0];
            writeLayer(layerPointer, thisLayer, thisCube.layerDuration);
        }
    }
    
    Serial.println(*layerPointer);
    
    return;
}

thisCube is a struct datatype containing a 2 dimensional byte array and 2 ints.

The serial output is good but then changes suddenly. I think the pointer is getting attached to the wrong thing, but I don’t see how. Here is the output when it starts going wacky. 4 is normal:

4
4
4
128
128
128

Any ideas?

I don’t think your pointer is pointing at anything.

Wouldn’t it be more like below, based on what http://www.codeproject.com/Articles/627/A-Beginner-s-Guide-to-Pointers says

byte *layerPointer;

void CubeControl::writeCube(cubeStep thisCube)
{
    int startTime = millis();

    layerPointer = new byte;
    *layerPointer = 0;
    
    while(millis()-startTime <= thisCube.stepDuration)
    {
        for (int thisLayer = 0; thisLayer < numLayers; thisLayer++)
        {
            *layerPointer = &thisCube.layer[thisLayer][0];
            writeLayer(*layerPointer, thisLayer, thisCube.layerDuration);
        }
    }
    
    Serial.println(*layerPointer);
    
    return;
}

The code you suggested gives a compiler error.

But the pointer definitely points to something. The code starts off and runs fine at the start. The correct value is displayed for a while. However, after some time, the value suddenly changes. I am not sure if the Arduino itself is moving memory locations or if passing a pointer into a function asking for an array is causing something to go crazy. Here is the function I am passing the pointer into:

void CubeControl::writeLayer(byte layer[], byte col, unsigned int duration)
{
    //Set reset pin to high, to allow writing. Low is active reset in shift registers.
    for (int chips = 0; chips < numRegisters; chips++) 
    {
        digitalWrite(registerResetPin[chips], HIGH);
    }
    
    //Turn layer pin to LOW to allow current flow
    digitalWrite(layerPin[col], LOW);
    
    for (int chips = 0; chips < numRegisters; chips++) 
    {
        shiftOut(registerDataPin[chips], registerClockPin[chips], LSBFIRST, layer[chips]);
    }
    
    //Delay the output so that you can actually see the output.
    delay(duration);
    
    //Reset the shift registers. Helps avoid bad data.
    for (int chips = 0; chips < numRegisters; chips++) 
    {
        digitalWrite(registerResetPin[chips], LOW);
    }
    
    //Turns off the layer.
    digitalWrite(layerPin[col], HIGH);
    
    return;
}

Turns out it’s not the pointers. This seizes up as well.

void CubeControl::writeCube(cubeStep thisCube)
{
    int startTime = millis();
    byte arrayHolder[numRegisters];

    while(millis()-startTime <= thisCube.stepDuration)
    {
        for (int thisLayer = 0; thisLayer < numLayers; thisLayer++)
        {
            for (int regnum = 0; regnum < numRegisters; regnum++) 
            {
                arrayHolder[regnum] = thisCube.layer[thisLayer][regnum];
            }
            
            writeLayer(arrayHolder, thisLayer, thisCube.layerDuration);
            
            Serial.println();
            Serial.println(arrayHolder[0]);        
        }
    }
    
    return;
}

Any clues? I’m at a loss.

It’s for an LED cube. It blinks in the correct order, stops blinking, and either quits going though the loop like in the above example or starts throwing out garbage data.

Are you using the String class? Or other dynamic memory (like new/malloc)?

Wouldn't it make more sense to pass cubeStep as a pointer as well? Passing by value may make you run out of memory. Also, can't layerPointer be a local variable within the for loop?

I do use new, but only in the constructor once. And I never get rid of it.

Here, I’ll post all the code. I think Nick is right, something is devouring memory.

Started to realize that it is pervasive throughout the code.
Getting late. Most likely won’t reply till tomorrow. Thanks in advance for the help.

I would look at the constructor in the CubeControl class. Maybe I am doing something wrong there.

Arduino Code:

#include <CubeControl.h>

byte numLayers = 4;
byte numRegisters = 2;

byte BottomRow = 0;
byte BottomMid = 1;
byte TopMid = 2;
byte TopRow = 3;

byte chipOneData = 4;
byte chipTwoData = 5;
byte chipOneClock = 6;
byte chipTwoClock = 7;
byte chipOneReset = 8;
byte chipTwoReset = 9;

byte layerPinData[4] = {0,1,2,3};
byte chipDataPinData[2] = {chipOneData, chipTwoData};
byte chipClockPinData[2] = {chipOneClock, chipTwoClock};
byte chipResetPinData[2] = {chipOneReset, chipTwoReset};

byte layerOff[2] = {0,0};
byte layerTest[2] = {12, 255};
byte layerTestDu[2] = {6, 200};
byte layerTestTres[2] = {19, 170};
byte layerTestQuaf[2] = {37, 90};

unsigned int oneSec = 1000;

cubeStep awesomeOne = {{{1,10},{2,20},{3,30},{4,40}},2,100};

CubeControl controller(numLayers, numRegisters,layerPinData, chipDataPinData, chipClockPinData, chipResetPinData);

void setup()
{
   pinMode(BottomRow, OUTPUT);
   pinMode(BottomMid, OUTPUT);
   pinMode(TopMid, OUTPUT);
   pinMode(TopRow, OUTPUT);
   pinMode(chipOneData, OUTPUT);
   pinMode(chipTwoData, OUTPUT);
   pinMode(chipOneClock, OUTPUT);
   pinMode(chipTwoClock, OUTPUT);
   pinMode(chipOneReset, OUTPUT);
   pinMode(chipTwoReset, OUTPUT);
  
  digitalWrite(chipOneReset, HIGH);
  digitalWrite(chipTwoReset, HIGH);
  
  digitalWrite(BottomRow, HIGH);
  digitalWrite(BottomMid, HIGH);
  digitalWrite(TopMid, HIGH);
  digitalWrite(TopRow, HIGH);
  
  Serial.begin(9600);
}

void loop()
{
  controller.writeCube(awesomeOne);
  //controller.makeItRain(10000, 1000, 16);
  delay(100);
}

The rest is attached. I can’t put it all in. Lol.

CubeControl.h (1.85 KB)

CubeControl.cpp (6.33 KB)

And I believe the default for passing a struct into a function is pass by reference. So unless I am wrong, that shouldn't be the problem.

It must have something to do with this code.

CubeControl::CubeControl(byte howManyLayers, byte howManyRegisters, byte layerPinNumbers[], byte registerDataPinNums[], byte registerClockPinNums[], byte registerResetPinNums[])
{
    //byte data type
    numLayers = howManyLayers;

    numRegisters = howManyRegisters;
    
    //byte pointers
    layerPin = layerPinNumbers;
    registerDataPin = registerDataPinNums;
    registerClockPin = registerClockPinNums;
    registerResetPin = registerResetPinNums;
}

The data types holding all the pin numbers are all pointers. I only call this function once in the Arduino code, but I do create it outside of the loop function. Besides, the pointers only reference the arrays in their original locations. Unless I am missing something.

I thought when you passed an array you also passed it's size.

CubeControl::CubeControl(... byte layerPinNumbers[4] ...)

just my guess.

Trying to make the code so that you could pass any size array into it so that anyone could make code for any size cube they wanted. So I'm using pointers. I could hardcode all the values in and originally had it that way, but I want it to be more flexible. The weird thing is that sometimes the code will start working again. So it's not getting stuck in an infinite loop somewhere, however somehow it does temporarily seize up.

Some questions. You’ve handed off control of a bunch of pins to the CubeControl class. Yet, you diddle with them in setup(). Why?

The CubeControl class should have a begin() method, and that method should be called in setup(), and that method should do the pin diddling.

Does you computer lack an enter key?

    CubeControl(byte howManyLayers, byte howManyRegisters, byte layerPinNumbers[], byte registerDataPinNums[], byte registerClockPinNums[], byte registerResetPinNums[]);

would, in my opinion, be easier to read as:

    CubeControl(byte howManyLayers, 
               byte howManyRegisters,
               byte layerPinNumbers[],
               byte registerDataPinNums[],
               byte registerClockPinNums[],
               byte registerResetPinNums[]);

This is a picky thing, I’m sure, but I like singular names for singular things and plural names for plural things. howManyLayers is a singular thing (layerCount would be a better name, in my opinion, to reflect this). layerPinNumbers is a plural thing - a list of pin numbers.

In the cpp file, then, you use singular names, layerPin, registerDataPin, etc. to point to plural things.

Again, in the picky department, a name like layerPointer says to me that you really don’t know what this variable is for. If it is a pointer (and it is by definition, it points to something. The name should reflect what it points to.

As I said, these are picky things, but when reading the code, it really helps to spot incorrect logic when one does not have to keep flipping back to the header file to see if a variable is a singular thing being used as though it was plural or is it a plural thing being used as though it was a singular thing.

You have stuff commented out in the destructor, and it is a good thing that it is. The things pointed to be the members of the class are not owned by the class, so the destructor should not be trying to destroy them. That code should not just be commented out, it should be deleted.

            writeLayer(arrayHolder, thisLayer, thisCube.layerDuration);
void CubeControl::writeLayer(byte layer[], byte col, unsigned int duration)

The value in thisLayer is referred to in the called method by the name col. Now, this is very confusing. Is the value a row number, a column number, or a layer number, or the temperature last Tuesday in Singapore?

    for (int chips = 0; chips < numRegisters; chips++)

Again with the mismatch in plural name/singular intent. registerNum would be a better name, in my opinion. Or, just r or c.

            for (int regnum = 0; regnum < numRegisters; regnum++) 
            {
                arrayHolder[regnum] = thisCube.layer[thisLayer][regnum];
            }
            
            writeLayer(arrayHolder, thisLayer, thisCube.layerDuration);
            
            Serial.println();
            Serial.println(arrayHolder[0]);

I can’t see anything in writeLayer that affects the value in arrayHolder[0]. If you suspect that something there IS affecting arrayHolder[0], you should print the value before and after the call. If you suspect that arrayHolder[n] is being populated incorrectly, you should be printing the value in the loop where the assignment occurs.

Might I also suggest that output like

4
4

is less meaningful than output like

arrayHolder[0] = 4
arrayHolder[1] = 4

Thanks for the advice Paul. A lot of the stuff I have been writing, rewriting, and slamming debug code into it so it’s messy and I do need to clean it up.

You are right about the setup() area. I’ll write a method to fix that.

The destructor was left from another way I was approaching passing in arrays earlier. Again, just need to be cleaned up.

Although I did figure out what was wrong. Had nothing to do with pointers or arrays. It has to do with the millis() function. When it hit about 33000, it flipped to negative. Although I thought it could run for days without this problem, apparently it can’t. When it flips, it skips the while loop from that point on since the test fails, even though it shouldn’t. I still don’t know why it breaks, but I did fix it.

void CubeControl::writeCube(cubeStep thisCube)
{
    int startTime = millis();
    byte arrayHolder[numRegisters];
    
    delay(1);
    
    int nowTime = millis();
    Serial.print("Time difference: ");
    Serial.println((nowTime - startTime));
    Serial.print("Current time: ");
    Serial.println(nowTime);
    Serial.print("Start Time: ");
    Serial.println(startTime);
    Serial.println();
    
    int testTime = nowTime - startTime;

    while(testTime <= thisCube.stepDuration)
    {
        Serial.println("In while loop. \n");
        
        for (int thisLayer = 0; thisLayer < numLayers; thisLayer++)
        {
            for (int regnum = 0; regnum < numRegisters; regnum++) 
            {
                arrayHolder[regnum] = thisCube.layer[thisLayer][regnum];
            }
            
            writeLayer(arrayHolder, thisLayer, thisCube.layerDuration);      
        }
        
        testTime = (millis() - startTime);
    }
        
    return;
}

Any clue why when calling millis() in the while loop test causes the test to fail?

It has to do with the millis() function. When it hit about 33000, it flipped to negative.

It absolutely, positively, does not do that.

If you save the value of returned by millis() in an int, the value in the int will do that. Completely separate issue, though, and a clear indication that you did not look at the documentation of the millis() function.

I seriously completely misread the millis() function. Realized it right after the last post. Changed int to unsigned long. All good now.

Thanks for all the help. Can't believe I completely missed that.

Can't believe I completely missed that.

Your not the first to make that mistake. Likely not to be the last, either.

So, post your completed project in the Exhibition section, when/if it is complete.

Will do. Was writing this as a library to maybe help with people making LED cubes since there are so many people making them. Trying to add as much functionality as possible but I only have 1 cube to test it on so it's getting hard to keep of whether or not it will work on any size cube.

BizarrePsycho: And I believe the default for passing a struct into a function is pass by reference.

No, it isn't. Try this:

struct foo {
    int a;
    int b;
};

void bar (struct foo x)
  {
  x.a = 42;
  }
  
void setup () 
{
  struct foo y;
  y.a = 1;
  y.b = 2;

  Serial.begin (115200);
  Serial.println (y.a);
  bar (y);
  Serial.println (y.a);
  exit (0);  // little jest there
}
void loop () {}

That prints:

1
1

Now change bar to be:

void bar (struct foo & x)

That prints:

1
42

So the default, as with all other arguments, is call by value.

Alright. Changed it. No discernible difference, but can't hurt to be safe. Thanks for the tip. I'll work on it a bit more then maybe re-post in a different thread since it has nothing to do pointers anymore.

Depends what you mean by "safe". If you want the original to be changed, it is safe to pass by reference. If you want a copy, it isn't.

Safer in the fact that I know that the original will be changed if I change a value and not just hope it changes. There is a function I need to retool that I want to change all the values.