Memory alloc problem in example code / SevSegShift::begin

[foxblock]

Hi there,

I think there is a bug in all examples of SevSegShift which use the following (or similar) setup function:

void setup() {
  byte numDigits = 4;
  byte digitPins[] = {8+2, 8+5, 8+6, 2}; // of ShiftRegister(s) | 8+x (2nd Register)
  byte segmentPins[] = {8+3, 8+7, 4, 6, 7, 8+4, 3,  5}; // of Shiftregister(s) | 8+x (2nd Register)
  bool resistorsOnSegments = false; // 'false' means resistors are on digit pins
  byte hardwareConfig = COMMON_ANODE; // See README.md for options
  bool updateWithDelays = false; // Default 'false' is Recommended
  bool leadingZeros = false; // Use 'true' if you'd like to keep the leading zeros
  bool disableDecPoint = false; // Use 'true' if your decimal point doesn't exist or isn't connected

  sevseg.begin(hardwareConfig, numDigits, digitPins, segmentPins, resistorsOnSegments,
  updateWithDelays, leadingZeros, disableDecPoint);
  sevseg.setBrightness(90);
}

The problem lies herein ( https://github.com/bridystone/SevSegShift/blob/ShiftRegister/SevSegShift.cpp#L51 ):

void SevSegShift::begin(
  byte hardwareConfig, 
  byte numDigitsIn,
  byte shiftRegisterMapDigits[],
  byte shiftRegisterMapSegments[],
  bool resOnSegmentsIn, 
  bool updateWithDelaysIn, 
  bool leadingZerosIn,
  bool disableDecPoint
) {
  // here the digit"pins" of the Shift register is stored
  // pins are here the OUTPUT PINs of the Shift Register
  _shiftRegisterMapDigits = shiftRegisterMapDigits;
  // here the segment"pins" of the Shift register is stored
  // pins are here the OUTPUT PINs of the Shift Register
  _shiftRegisterMapSegments = shiftRegisterMapSegments;

SevSegShift::begin assigns the pointers to the array of pins passed as digitPins and segmentPins in the above examples to the corresponding internal variables. Since digitPins and segmentPins are declared locally in setup() however, they might get deallocated as soon as the program leaves the scope (i.e. the setup function ends) - in any case this probably is undefined behaviour according to the C standard. At least this is what happens on the ESP32.

Compare this to how SevSeg copies the values explicitly: https://github.com/DeanIsMe/SevSeg/blob/master/SevSeg.cpp#L170

  // Save the input pin numbers to library variables
  for (uint8_t segmentNum = 0 ; segmentNum < numSegments ; segmentNum++) {
    segmentPins[segmentNum] = segmentPinsIn[segmentNum];
  }

  for (uint8_t digitNum = 0 ; digitNum < numDigits ; digitNum++) {
    digitPins[digitNum] = digitPinsIn[digitNum];
  }

A quick fix would just be moving the declaration of digitPins and segmentPins in the examples to the global scope. However I would advise against doing so, since the difference in behaviour compared to SevSeg is certainly unexpected and makes switching between the libraries harder.

A better solution would be to just malloc the member variables (_shiftRegisterMap*) in SegSevShift::begin and memcpy the passed values to them. Or copy the values by hand as SevSeg does (which probably is a tad slower).

Cheers!

[bridystone]

Hi @foxblock,

Sorry for not reacting... I was not looking into this for quite some time.

It is an interesting point. I'm not aware of this, since I'm not a "natural" C Developer.

Would you be so kind, to provide a Pull-Request? I would merge it into the Main Branch, if this fixes the issue.

[foxblock]

Pull-request is up. Sorry it took a while. I had no access to hardware to test this until today. Tested the code on an ESP32 (using local variables works fine now), but it should work on an esp8266 as well.