Assigning Pointers to arrays - incorrect results?

Question

I'm trying to build a transmission frame for a workshop library for the Arduino Uno. I have the byte frame inside an array, give it to another function that builds a bit frame and then want to transmit that bit frame.

When I try to print out the arrays using the serial monitor, I get the correct results for the entire byte array and correct results for the bit array up to a certain point. However, since my output (that I'm checking using a logic analyzer) had also more incorrect values when trying to print the values, I suspect it might have to do with timing issues from my transmission and Serial.print.

I thus tried to make another pointer (in the code below frameCheck) to print that out after I'm done transmitting my frame, which still leaves me with results that are different from what my logic analyzer tells me from a certain point onwards.

My byte array is as follows:

10100101  // Preamble
10000000  // First line of my image
00100000
00010000
01000000
00000000
00000000
00000000
00000000  // Last (8th) line of my image
11110000  // Checksum

These values are also represented in the bit array (before assignment to what in my code be called _frame as well as after that). However, when I read the values from the logic analyzer, the checksum is transmitted as 11111111 instead of 11110000.

Here is my code in the .cpp file:

Transmitter::Transmitter()
{
  [...]
  _frame = new uint8_t[90];
  frameCheck = _frame;
  [...]
}

[...]

int Transmitter::prepFrame(LEDBitmap image) {
  if(_busy) return -1; //no Data can be sent right now since the transmitter is already busy
  if(!_pin) return -2;
  /** construct the frame that is to be transmitted
    *
    * frame structure is
    * 1 byte preamble (0xA5) | 8 byte payload | checksum
    *
    * the image will be transmitted in rows
    * with one row being one byte
    *  1  2  3  4  5  6  7  8   <-- 1st byte
    *  9 10 11 12 13 14 15 16   <-- 2nd byte
    * 17 18 19 20 21 22 23 24   <-- 3rd byte
    * ...                           ...
    */
  uint8_t size = 10;
  uint8_t frame[size];
  uint8_t checksum = 0x00;

    for(uint8_t h = 0; h < image.getHeight(); h++) {
      uint8_t value = image.getBitmap()[h];
      checksum = (checksum + value) % 255;
      frame[h+1] = value;
    }

    for(uint8_t h = image.getHeight(); h < 8; h++) {
      frame[h+1] = 0x00;
    }

  // building the frame
  frame[0] = PREAMBLE;
  frame[size-1] = checksum;


//The byte frame "frame" is correct at this point

  _frame = buildBitFrame(frame, size);

//The frame built by this function (see below) as well as the bit frame "_frame" have the correct checksum

  frameCheck = _frame;  // <-- This is the checkFrame which I print in my .ino which gives me false results.

  // set the transmitter into busy mode so the idle pattern will no longer
  // be transmitted but instead the frame will be sent out.
  _busy = true;
  _pos = 0;

  return 1; //preperation was successful
}

uint8_t* Transmitter::buildBitFrame(uint8_t* frame, uint16_t size) {
  _frameSize = size*8;
  uint8_t bitFrame[_frameSize];
  int pos = 0;
    for (size_t byte = 0; byte < size; byte++) {
      for (size_t bit = 0; bit < 8; bit++) {
        bitFrame[pos] = (frame[byte] >> (7 - bit)) & 0x01;
        pos++;
      }
    }

  return bitFrame; // <-- This frame is correct as well.
}


void Transmitter::transmitBit() {
  if(_active) {
    // Preperation
    // only change the state when the manchester bit is completely transmitted.
    if(!_manHalf) {
      if(_busy) {
        // if there is currently a transmission running
        // read the next bit from the frame
        _state = _frame[_pos];
        if(_pos >= _frameSize) {
          _busy = false;
          _state = 0;
          _pos = 0;
        }
        _pos++;
      } else { // else transmit the idle pattern
        _state = 0;
      }
    }
    // Transmission
    if (!_manHalf) {
      digitalWrite(_pin, !_state);
    } else {
      digitalWrite(_pin, _state);
    }
    _manHalf = !_manHalf;
  } else {
    digitalWrite(_pin, HIGH);
  }
}

[...]

The corresponding, relevant lines in the header file are as follows:

[...]

class Transmitter
{
  public:
    Transmitter();

  public: //methods
    int sendData(LEDBitmap image);
    void transmitBit();
    [...]
    uint8_t* frameCheck;

  private: //members
    uint8_t _state;
    uint8_t* _frame;
    [...]

    int prepFrame(LEDBitmap image);
    uint8_t* buildBitFrame(uint8_t* frame, uint16_t size);
    [...]

};

[...]

The function transmitBit() is called every 1ms via a ISR.

Last but not least, here is my arduino sketch:

[...]

Transmitter transmitter;

[...]

int smallData[] = {1, 0, 0, 0,
                   0, 0, 1, 0,
                   0, 0, 0, 1,
                   0, 1, 0, 0};

LEDBitmap small(4, 4, smallData);

void setup() {
  Serial.begin(2000000);
  transmitter.setPin(4);
  transmitter.start();
  delay(200);
  pinMode(5, OUTPUT);
  digitalWrite(5, HIGH);
  transmitter.sendData(small);
  delay(1000);
  for(int a = 0; a < 88; a++) {
    Serial.println(transmitter.frameCheck[a]);  
  }
}

The loop is currently empty.

Now onto the weird results I'm getting from printing checkFrame (which I have also already gotten when trying to print the full arrays inside my cpp). This is what it printed (already grouped in bytes):

1 0 1 0 0 1 0 1
1 0 0 0 0 0 0 0
0 0 1 0 0 0 0 0
0 0 0 1 0 0 0 0
0 1 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0        // Up to here everything is fine.
0 0 0 0 0 2 149 0      // On the logic analyzer this line reads 0x00 as it should
221 5 15 0 2 149 0 1   // On the logic analyzer this line reads 0xff instead
1 222 0 98 2 149 8 218 // This line is irrelevant as it is only a buffer I added to see if the problem lies within my array not having enough space

Now onto my questions!

Do you have an idea what could cause this? Did I do the array assignment wrong? Could this just be an issue with the timing of the Serial.prints and my transmitter going off every 1ms? But why does my logic analyzer say that the last line is 0xff then? Do you have any idea how to fix this?

I'd be super glad if you responded as quickly as possible. Thanks in advance!

EDIT: After increasing the initial value of a in my sketch to 64, I found out that the 8th byte is still 00000000 as it says on the logic analyzer. However, the checksum still was a mess. This did also not improve a lot after increasing it to 72. It then was 1 1 2 149 8 218 5 15. These last few values did also not change when I increased a again to 74.

Answer 1

uint8_t* Transmitter::buildBitFrame(uint8_t* frame, uint16_t size) {
  _frameSize = size*8;
  uint8_t bitFrame[_frameSize];
  int pos = 0;
    for (size_t byte = 0; byte < size; byte++) {
      for (size_t bit = 0; bit < 8; bit++) {
        bitFrame[pos] = (frame[byte] >> (7 - bit)) & 0x01;
        pos++;
      }
    }

  return bitFrame; // <-- This frame is correct as well.
}

You are returning a pointer to a function-local array which was allocated on the stack. After the function exists the stack for that function gets de-allocated and the array becomes invalid.

It may be valid for a short time after the function exists, but as soon as new functions are called and variables are allocated on the stack, this gets overwritten.

See https://www.fayewilliams.com/2015/06/30/a-challenge-discussion-returning-pointers-to-local-variables/

In cases like these (especially with a constant maximum frame length), you should re-write your memory management to use statically allocated buffers within your class.

For example:

uint8_t* _frame;

to

#define SOME_MAX_SIZE 128
uint8_t _frame[SOME_MAX_SIZE];

And then re-write functions to write into these buffers instead of allocating new memory every time.

Dynamic memory allocation in general should be avoided at all costs on a microcontroller due to heap fragmentation.

If you really want to keep doing it you could also allocate a new frame of the required length and make sure to delete it after usage!

uint8_t* Transmitter::buildBitFrame(uint8_t* frame, uint16_t size) {
  _frameSize = size*8;
  uint8_t* bitFrame = new uint8_t[_frameSize];
  int pos = 0;
    for (size_t byte = 0; byte < size; byte++) {
      for (size_t bit = 0; bit < 8; bit++) {
        bitFrame[pos] = (frame[byte] >> (7 - bit)) & 0x01;
        pos++;
      }
    }

  return bitFrame; //return pointer to malloced array, okay now.
}