DeanIsMe / SevSeg

Seven segment display controller library for Arduino
MIT License
325 stars 130 forks source link

Unpredictable behavior in a specific case of unused decimal point pins caused by an array out of bounds condition #109

Open 6v6gt-duino opened 7 months ago

6v6gt-duino commented 7 months ago

Unpredictable behavior occurs if only seven segments pins (that is no decimal point pins) are defined in the array segmentPins[] in the call to the begin() method but the parameter disableDecPoint is omitted.

This could be the case when the decimal point pins of the seven segment displays are not used.

The problem is that the default value of disableDecPoint is false. This leads to the library treating the number of elements in segmentPins[] as 8 even though only 7 elements are present. When the array is later scanned an attempt is made to read the non-existent element number 8 leading to this unpredictable behavior [array out of bounds].

In the specific case recorded at https://forum.arduino.cc/t/2-digit-display-with-sevseg-library-a-segment-staying-on/1230896 the effect of this error was that an unwanted segment on a display remained lit.

DeanIsMe commented 7 months ago

That makes sense. With this setup, SevSeg reads in 8 pin numbers, where the 8th pin number is not defined. I could and probably should make the interface more strict using std::array or variadic templates or a setup struct, or a sizeof(segmentPins) argument... But I likely won't get the chance soon. Unfortunately most of these would be breaking changes to the begin() function. The way that I set it up is not too flexible.

6v6gt-duino commented 7 months ago

Many thanks for looking at this so quickly. In retrospect, It is probably best to treat this case as a user error and maybe something to be addressed in a future release by enhanced input validation. I could imagine adding a new format begin() method signature but retaining the old format one for compatibility with old code. Anyway, if the user's starting point had been one of the up to date examples in the library, this should not have happened. It is, of course, not optimal that inconsistent user input gives rise to internal corruption errors (array overflow etc.) but then again as long as the rules are followed, everything should work.

There are, unfortunately, code samples circulating which are misleading, for example this implying that all works when only seven segment pins are defined and without reference to a disableDecPoint parameter : https://forum.arduino.cc/t/how-to-use-a-2-digit-7-segment-display-with-sevseg-h-library/644014. I'll deal with that one specifically by requesting that the tread is reopened and adding a comment.

If you wish you can mark this as closed.

DeanIsMe commented 7 months ago

Yep that's exactly right. Your idea of maintaining the old begin function for backwards compatibility is a great one.

I think that it needs a new begin function that takes in a 'struct SevSegSettings'. This would allow more flexibility for input arguments.