Invalid memory access in all callback examples

[martincizek]

According to the code, SerialTransfer requires the passed callback array to stay allocated for its whole lifetime.

1. This is not documented.
2. All the callback-related examples are actually accessing reusable stack memory.

Steps to reproduce: A modified uart_rx_with_callbacks.ino example is attached below - using LED blinking, as I have just one serial. I was able to reproduce it also by printing the memory addresses.

Expected behavior:

1. This should be documented.
2. Examples should have the callback config defined outside of any functions.
3. I actually noticed that a solution is already part of the "polishing" effort, where a dynamic linked list is used instead. So not filing a PR for now.

Example of invalid memory access:

#include <Arduino.h>
#include "SerialTransfer.h"

SerialTransfer myTransfer;

// A callback passed to SerialTransfer
void hi() {
  digitalWrite(LED_BUILTIN, HIGH);
  delay(500);
  digitalWrite(LED_BUILTIN, LOW);
  delay(500);
}

// My very private function
void hey() {
  for (int i = 0; i < 10; i++) {
    digitalWrite(LED_BUILTIN, HIGH);
    delay(50);
    digitalWrite(LED_BUILTIN, LOW);
    delay(50);
  }
}

// Prevent compiler from optimizing unused stack variables.
// Serial1.println(foo) would work too if you had Serial1 :-)
volatile uint16_t variableInUse1 = 0;
volatile uint16_t variableInUse2 = 0;

void setup() {
  Serial.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);
  functionPtr callbackArr[] = { hi };
  variableInUse1 = (uint16_t) &callbackArr[0];
  ///////////////////////////////////////////////////////////////// Config Parameters
  configST myConfig;
  myConfig.debug        = true;
  myConfig.callbacks    = callbackArr;
  myConfig.callbacksLen = sizeof(callbackArr) / sizeof(functionPtr);
  /////////////////////////////////////////////////////////////////
  myTransfer.begin(Serial, myConfig);

  // Dispatch a few messages with the setup() stack
  while (millis() < 10000LU) {
    myTransfer.tick(); // hi() is called
  }
}


void loop() {
  functionPtr myVeryPrivateArray[] = { hey };
  variableInUse2 = (uint16_t) &myVeryPrivateArray[0];

  // Dispatch messages within the loop() stack
  myTransfer.tick(); // oh my - hey() is called!
}

[chrisco484]

@martincizek

OMG - I just wasted hours on this bug and rewired the board a bazillion times thinking I had the wiring wrong.

Unfortunately as the crash is quite severe - as you would expect when you start executing code at a completely random address - there is no error logged - just silence - so you don't know what has happened.

I ended up single stepping all the way to the "crash point" i.e. where the callback is performed and then came to the same conclusion you did.

In the I2C sample I was using I simply moved the

functionPtr callbackArr[] = { hi };

from within the setup() method to just above it in global variable space so that the callbacks array stays valid and voila - everything works perfectly! (at last)

BTW - all my testing is on STM32 - recently moved away from the 8bit world - just kept hitting memory constraints: both RAM and Flash way too small and the CPUs are underpowered. Perhaps the SerialTransfer examples somehow manage to work on the 328p due to some weird coincidence with memory arrangement but they definitely don't work in STM32 world until the callbacks array is allocated in global space (or malloc'd somewhere).

[chrisco484]

@PowerBroker2

Do you think it would be better to fix the examples or fix the code so that it makes a deep copy of the callback array? If it made a deep copy then users (and the existing examples) could safely create the array on the stack - and the example source code could remain unchanged.

[PowerBroker2]

I'm not exactly sure how to do that - could you make a PR with all the changes you would like to see?

[chrisco484]

I'm not exactly sure how to do that - could you make a PR with all the changes you would like to see?

It might even be more elegant to define a IPacketReceiver interface class and Packet calls that when a packet has been received.

Hmmm, so many options.

[martincizek]

Hey guys, let me follow up on my comment :)

3. I actually noticed that a solution is already part of the "polishing" effort, where a dynamic linked list is used instead. So not filing a PR for now.

As it appears the "polish" effort (issue #35 and PR #37) will not finish in the immediate future, we should probably address this version.

And I'd vote for just fixing the docs and examples for two reasons:

1. I guess this library should stay lightweight and unopinionated, unless it is very well-advised. Making a dynamically allocated copy is sort of opinionated - users might want to e.g. share the callback table across instances. And it indeed introduces some memory footprint, which is often unnecessary.
2. It would be just a fix of this version, keeping the current behavior for users who took it into account (like me :-)). Introducing other behavior can be subject of another version.