Max72xxPanel size limit solution

[rob040]

A compiler warning (gcc -Wall -Wextra ) pointed me to this bug:

.pio\libdeps\default\arduino-Max72xxPanel\Max72xxPanel.cpp: In member function 'virtual void Max72xxPanel::drawPixel(int16_t, int16_t, uint16_t)':
.pio\libdeps\default\arduino-Max72xxPanel\Max72xxPanel.cpp:126:32: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  126 |  if ( x < 0 || x >= WIDTH || y < 0 || y >= HEIGHT ) {
      |                              ~~^~~

Cause: y is unsigned byte and cannot be smaller than 0.

In function drawPixel, where the arguments are signed 16-bit integers, however they are converted internally to unsigned bytes.
With PR #14 there was a fix for the 16-tile panel limit. That fix is basically OK, but it is incomplete, when taking panel rotation into account.

Currently the drawPixel function starts like:

void Max72xxPanel::drawPixel(int16_t xx, int16_t yy, uint16_t color) {
  // Operating in bytes is faster and takes less code to run. We don't
  // need values above 200, so switch from 16 bit ints to 8 bit unsigned
  // ints (bytes).
  // Keep xx as int16_t so fix 16 panel limit
  int16_t x = xx;
  byte y = yy;
  byte tmp;

a bit later with x and y are exchanged:

  if ( rotation & 1 ) {  // rotation == 1 || rotation == 3
  	tmp = x; x = y; y = tmp;
  }

Obviously, this can only succeed when all three variables are of the same type/width, and that is not the case!

Solution

So to get this properly solved, change the type of y and tmp to int16_t.

void Max72xxPanel::drawPixel(int16_t xx, int16_t yy, uint16_t color) {
  // Operating in bytes is faster and takes less code to run. We don't
  // need values above 200, so switch from 16 bit ints to 8 bit unsigned
  // ints (bytes).
  // Keep xx as int16_t so fix 16 panel limit
  int16_t x = xx;
  int16_t y = yy;
  int16_t tmp;

AND
change bitmapSize type from byte to int16_t in Max72xxPanel.h

other references

That is probably the cause of other issue reports like #13, #19

[markruys]

As far as I know, the current (undocumented) limitation is 31 modules. In that case we don't need uint16_t but can use byte. I wonder, should we support 32 and more panels at the price of a bigger code base? To give you an idea, the Mad Fly example would go up from 4120 bytes to 4234 bytes (on a ATmega328P MCU). In some projects program storage can be a significant constraint.

I'm rewriting the library into two classes: one with the Adafruit GFX dependency, and a base class without. Like #7, but then without the need to modify the library with #defines.

[rob040]

Hi Mark,
Those extra 114 bytes doesn't appear that much, especially for removing an undocumented and unseen limitation.
If your library would explicitly state the limitation and would not compile beyond that limitation (e.g. with #error), then go ahead and go for the smallest and fastest code on 8-bit (AVR) architectures.

If you included an Arduino library.properties file, you could state which architectures are supported (and version numbering this library and more). Read https://arduino.github.io/arduino-cli/1.2/library-specification/ and/or https://docs.platformio.org/en/latest/librarymanager/creating.html

You could also create an 8-bit architecture and 32-bit architecture optimized version, and perhaps do some performance measurement to find and solve performance bottlenecks.