next topic »

[on:]

Hi everyone,

I want to build a simple step sequencer. I want to store the sequence of notes in a pointer, which points to an array. I wrote this function

Code:

void ext(int n) {
  int newSeq[seqLength+1];
  for(int i=0; i < seqLength;i++)
    newSeq[i] = sequence[i];
  newSeq[seqLength] = n;
  Serial.println(n);
  free(sequence);
  sequence = newSeq; 
  seqLength++;
  for(int i=0; i < seqLength;i++) {
    Serial.println(sequence[i]);
    delay(10); 
  }
}

and have the global variables

Code:

int *sequence = 0;
int seqLength=0;

I added a button which calls the function with an random value. But only the newly added value is correctly printed, the others are wrong.
What is wrong here. Do I need to store newSeq globaly too? I would like t avoid that... for memory reasons.
Thanks for any help.
The complete code is here. A button is attached to digital pin 2.

Code:

int *sequence = 0;
int seqLength=0;

const byte button1 = 2;

byte b1State = LOW;

void setup() {
  pinMode(button1, INPUT);

  Serial.begin(9600);
  /*
  int a[2];
   
   Serial.println(sequence[0]);
   Serial.println(sequence[1]);
   Serial.println(sequence[2]);
   int b[3] = {
   5,6,7  };
   sequence = b;
   Serial.println(sequence[0]);
   Serial.println(sequence[1]);
   Serial.println(sequence[2]);
   */
}

void ext(int n) {
  int newSeq[seqLength+1];
  for(int i=0; i < seqLength;i++)
    newSeq[i] = sequence[i];
  newSeq[seqLength] = n;
  Serial.println(n);
  free(sequence);
  sequence = newSeq; 
  seqLength++;
  for(int i=0; i < seqLength;i++) {
    Serial.println(sequence[i]);
    delay(10); 
  }
}


void loop() {
  // button 1
  int reading = digitalRead(button1);

  if (reading == HIGH && !(b1State&2))  {// check pressed && last loop not: 2nd bit == 0
    b1State = b1State ^ 1; // set 1st bit = 1 > state = ON
    int z = (int)random(100);
    Serial.println("add");
    ext(z);
  }
  bitWrite(b1State, 1, reading); // set 2nd bit = reading (for next loop)

}

Sorry for the cryptic button thing. It's my way to check a simple button push in just one byte 

[Reply #1 on:]

Code:

void ext(int n) {
  int newSeq[seqLength+1];
  free(sequence);
  sequence = newSeq; 

a, b, and newSeq are "local variables" in C and C++, not dynamically allocated objects.  That means that they essentially disappear when the function where they were defined (setup() or ext()) returns; local variables are typically allocated on the stack, and that part of the stack is likely to be reused the next time you call a function.

This means that your pointer variable "sequence" is left pointing to memory that no longer contains the variables or values that you think it does.

Also, you cannot free() local variables, since they were not allocated from the dynamic memory pool.

You could probably do what you're trying to do by using malloc() to allocate the new sequence storage, but that's really unnecessarily complex and inefficient on a small microcontroller.  You might as well implement your array as a global variable/array with the maximum size you want to allow, and keep track of how many values are in it at the moment.  (This is essentially doing your own memory allocation, but in a very simplified form that is less prone to errors and easier to debug.)

[Reply #2 on:]

sequence is a global variable. I am calling free to deallocate the memory it was pointing on before to set it to let it point to the newSeq array beginning. mmh
I don't want to constrain the sequence length, tho if I would set it to a max so that the program almost fill the whole memory, it should be ok right. It can't get any further anyway...

[Reply #3 on:]

I am calling free to deallocate the memory it was pointing on before to set it to let it point to the newSeq array beginning. mmh

The free() function is ONLY used to free dynamically allocated memory.

I don't want to constrain the sequence length, tho if I would set it to a max so that the program almost fill the whole memory, it should be ok right.

As long as you leave enough room for the stack and heap, yes.

[Reply #4 on:]

if I would set it to a max so that the program almost fill the whole memory, it should be ok right.

By using a statically defined array, you'll be able to get a sequence about twice the length of the dynamically allocated version, since the latter requires two copies to exist at one time (the old sequence and the new sequence.)  If you make your static array half the total size of RAM, you should be relatively safe, AND you should have about the same length of sequence you would have had if your extension code could work the way you want it to...