Modify an array inside a function

Question

Can someone explain this to me? I want to pass an array to a function to modify its values. But I faced some difficulties. First it looks like I get back some trash from memory.

void CustomEthernetClientClass::readMessages(size_t &messageCount, Message *messages) {
  digitalWrite(ETHERNET_SS, LOW);
  if(_client.available()) {
    uint8_t msgBuffer[ETHERNET_RX_BUFF];
    uint16_t bufferSize = 0;
    while(_client.available() && (bufferSize < ETHERNET_RX_BUFF)) {
      msgBuffer[bufferSize] = _client.read();
      bufferSize++;
    }
    Serial.println("converting...");
    messages = reinterpret_cast<Message *>(msgBuffer);
    messageCount = ETHERNET_FRAME_LENGTH * sizeof(uint8_t) / sizeof(Message);
  } 
  digitalWrite(ETHERNET_SS, HIGH);
}

void loop() {
  Message messageBuffer[10];
  size_t messageCount = 0;
  CustomEthernetClient.readMessages(messageCount,messageBuffer);
  Serial.println(messageCount);
  if(messageCount > 0) {
    for(int i=0; i<messageCount; i++) {
      Serial.print("H1: ");
      Serial.print(messageBuffer[i].header1 );
      Serial.print(", ");
      Serial.print(messageBuffer[i].header2 );
      Serial.print(", ");
      Serial.print(messageBuffer[i].data1 );
      Serial.print(", ");
      Serial.print(messageBuffer[i].data1 );
      Serial.println();
    }
  }
  delay(1000);
}

I get this result (not what I expected...):

converting...
1
H1: 255, 255, 4294967295, 4294967295

But If I modify the readMessages() function like this:

void CustomEthernetClientClass::readMessages(size_t &messageCount, Message *messages) {
  digitalWrite(ETHERNET_SS, LOW);
  if(_client.available()) {
    uint8_t msgBuffer[ETHERNET_RX_BUFF];
    uint16_t bufferSize = 0;
    while(_client.available() && (bufferSize < ETHERNET_RX_BUFF)) {
      msgBuffer[bufferSize] = _client.read();
      bufferSize++;
    }
    Serial.println("converting...");
    messages = reinterpret_cast<Message *>(msgBuffer);
    messageCount = ETHERNET_FRAME_LENGTH * sizeof(uint8_t) / sizeof(Message);

    //This is what I have added
    for(int i=0; i<messageCount; i++){
      Serial.print("H1: ");
      Serial.print(messages[i].header1 );
      Serial.print(", ");
      Serial.print(messages[i].header2 );
      Serial.print(", ");
      Serial.print(messages[i].data1 );
      Serial.print(", ");
      Serial.print(messages[i].data1 );
      Serial.println();
    }
  } 
  digitalWrite(ETHERNET_SS, HIGH);
}

But after I add some print lines to the code for the purpose of debugging, it started to work. I get what I expected:

converting...
H1: 1, 0, 0, 0
1
H1: 1, 0, 0, 0

Message is a simple struct with fields.

Anywhere you have a reinterpret_cast, your code is almost certainly wrong. — user2100815
– user2100815, Commented Aug 21, 2017 at 20:40
messageCount calculation is also wrong - your buffer length is ETHERNET_RX_BUFF not ETHERNET_FRAME_LENGTH moreover actual read bytes count could be even less than that - bufferSize is used to monitor actual number. — Artemy Vysotsky
– Artemy Vysotsky, Commented Aug 21, 2017 at 21:05
@NeilButterworth, sweeping and wrong statement. There is no shortage of valid reinterpret_cast cases. — SergeyA
– SergeyA, Commented Aug 21, 2017 at 21:17
@NeilButterworth while OP obviously is doing it incorrectly here, in general, in code like this (reading network packets) reinterpret_cast is the customary thing to do. — SergeyA
– SergeyA, Commented Aug 21, 2017 at 21:46

Oskari Teirilä · Accepted Answer · 2017-08-21 20:58:38Z

1

Arguments are mutable in C++, but they are copied on call, which means that you are assigning to your local copy instead of the variable the caller passed. You probably knew this, as you are passing messageCount as a reference, as you should. However,

messages = reinterpret_cast<Message *>(msgBuffer);

stores the pointer to your local array to messages, which is not declared as a reference, and is, therefore, a local variable. Try making your messages argument a reference as well:

void CustomEthernetClientClass::readMessages(size_t &messageCount, Message *&messages)

You also need to allocate your buffers from the heap instead of stack if you do this; I don't know it that works on Arduino. Another way is to replace the assignment with a memcpy or similar, which will overwrite the data the pointer points to instead of the pointer itself.

edited Aug 21, 2017 at 20:58

answered Aug 21, 2017 at 20:53

Oskari Teirilä

1467 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

eszik.k Over a year ago

My only question about this As I wrote at the second scenario when I just added some Serial.print(messages[i].header1); it started to work at both side. Do you have any idea why?

Oskari Teirilä Over a year ago

My best guess is that the optimizer is simply using a part of the stack that is known not to be uninitialized, so it is allowed to put anything there, and as you have a large enough uninitialized variable, the compiler shares its address with msgBuffer. That it happens to be exactly where you want your data is a mere coincidence. This doesn't happen, though, if you don't have the debug output inside your method, because the compiler sees that msgBuffer is never read, and the writes to it get optimized away.

user7860670 · Accepted Answer · 2017-08-21 20:50:23Z

0

You pass a messages pointer, however you don't write to the location it points to. Instead you are overwriting its value:

messages = reinterpret_cast<Message *>(msgBuffer);

To avoid errors of such nature you should write const-correct code:

readMessages(size_t & messageCount, Message * const messages) {

edited Aug 21, 2017 at 20:50

answered Aug 21, 2017 at 20:40

user7860670

37.9k4 gold badges65 silver badges96 bronze badges

Collectives™ on Stack Overflow

Modify an array inside a function

2 Answers 2

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related