displayio arg validation tweaking

[RetiredWizard]

The Magtag Google Calendar from the learning guide uses some Arial PCF fonts that apparently contain bitmap elements with a width and height of 0. The DisplayIO argument validations introduced in #7548 cause the example code to fail.

Traceback (most recent call last):
  File "code.py", line 233, in <module>
  File "adafruit_magtag/magtag.py", line 170, in set_text
  File "adafruit_portalbase/__init__.py", line 291, in set_text
  File "adafruit_display_text/bitmap_label.py", line 113, in __init__
  File "adafruit_display_text/bitmap_label.py", line 176, in _reset_text
  File "adafruit_display_text/bitmap_label.py", line 306, in _text_bounding_box
  File "/lib/adafruit_bitmap_font/glyph_cache.py", line 54, in get_glyph
  File "/lib/adafruit_bitmap_font/pcf.py", line 374, in load_glyphs
ValueError: width must be 1-32767

This PR increases the valid range for width and height from 1-32767 to 0-32767 and increases the maximum valid value for x, y, x1 and y1 in the displayio_bitmap_obj_blit function by 1 (I believe this was how it was validated prior to #7548). This prevents a bitmap object of width or height of zero from being validated against a range of 0 to -1.

In looking through the original issue #7540, it looks like the displayio_bitmap_obj_blit function change might have been introduced to cause a crash with a useful error message rather than incorrectly wrapping the display, but I didn't know if it was worth throwing in the extra code to identify the 0 width and/or height case and validating it separately from the potential wrap case. I also have not actually tested if the border wrap occurs.

[RetiredWizard]

I'm realizing the only reason I hesitated adding the test for the 0 width/height object for the border wrap issue is that I don't know how to call a micropython max() function. Is there a way to call something like mp_MAX(0, source->width - 1) rather than adding an if statement and two assignment statements?

i.e.

int16_t x = mp_arg_validate_int_range(args[ARG_x].u_int, 0, mp_MAX(0, source->width - 1), MP_QSTR_x);

rather than:

if (source->width == 0) {
    int16_t x = mp_arg_validate_int_range(args[ARG_x].u_int, 0, 0, MP_QSTR_x);
} else {
    int16_t x = mp_arg_validate_int_range(args[ARG_x].u_int, 0, mp_MAX(0, source->width - 1), MP_QSTR_x);
}

[dhalbert]

MIN() and MAX() macros are defined in py/misc.h. This may already be included by something you're including, so just try it first.

[RetiredWizard]

Thanks @dhalbert! I'm pretty sure you already pointed me at those macros a while back, sorry for may poor memory 😢. I really appreciate all the support you've given me over the past couple years 😁

[tannewt]

I don't think 0 width or 0 height blitting should be valid. It should always have some pixels to blit. The client code shouldn't try it.

[dhalbert]

I don't think 0 width or 0 height blitting should be valid. It should always have some pixels to blit. The client code shouldn't try it.

I kind of disagree, since it makes some client-side algorithms easier for the edge cases. Similar to how drawing things off the edges are OK, [3] * 0 is OK, zero-byte transfers in other cases are OK, etc.

[RetiredWizard]

The decision about blitting zero width or height bitmaps is above my pay grade 😁 but in case it helps I poked around the libraries for awhile.

I believe the MagTag issue stems from this code fragment in the adafruit_bitmap_font.pcf libarary:

                width = metrics.right_side_bearing - metrics.left_side_bearing
                height = metrics.character_ascent + metrics.character_descent
                bitmap = bitmaps[i] = self.bitmap_class(width, height, 2)
                self._glyphs[code_points[i]] = Glyph(
                    bitmap,
                    0,
                    width,
                    height,
                    metrics.left_side_bearing,
                    -metrics.character_descent,
                    metrics.character_width,
                    0,
                )

It seems that PCF font space characters have a calculated width and height of zero but the character_width is non-zero.

I was able to modify the adafruit_bitmap_font and adafruit_display_text libraries so that they didn't attempt to blit the space character but in order to keep track of the spacing I ended up storing a "space holder" glyph with the bitmap set to None. This solution felt like a hack and I'm sure there's a better way to handle it.

I don't know if there are other library modules that blit 0 by 0 bitmaps when they want to move the current position forward though.

The changes I made were:

ADAFRUIT_BITMAP_FONT.PCF

        for i in range(len(all_metrics)):  # pylint: disable=consider-using-enumerate
            metrics = all_metrics[i]
            if metrics is not None:
                width = metrics.right_side_bearing - metrics.left_side_bearing
                height = metrics.character_ascent + metrics.character_descent
++              if width == 0 or height == 0:
++                  bitmap = None
++              else
                    bitmap = bitmaps[i] = self.bitmap_class(width, height, 2)
                self._glyphs[code_points[i]] = Glyph(
                    bitmap,
                    0,
                    width,
                    height,
                    metrics.left_side_bearing,
                    -metrics.character_descent,
                    metrics.character_width,
                    0,
                )

        for i, code_point in enumerate(code_points):
            metrics = all_metrics[i]
            if metrics is None:
                continue
            self.file.seek(first_bitmap_offset + bitmap_offsets[i])
            width = metrics.right_side_bearing - metrics.left_side_bearing
            height = metrics.character_ascent + metrics.character_descent
++          if width == 0 or height == 0:
++              continue

            bitmap = bitmaps[i]

            if _bitmap_readinto:
                _bitmap_readinto(
                    bitmap,
                    self.file,
                    bits_per_pixel=1,
                    element_size=4,
                    reverse_pixels_in_element=True,
                )
            else:
                words_per_row = (width + 31) // 32
                buf = bytearray(4 * words_per_row)
                start = 0
                for _ in range(height):
                    self.file.readinto(buf)
                    for k in range(width):
                        if buf[k // 8] & (128 >> (k % 8)):
                            bitmap[start + k] = 1
                    start += width

ADAFRUIT_DISPLAY_TEXT.BITMAP_LABEL

++                  if my_glyph.width != 0 and my_glyph.height != 0 and bitmap is not None:
                        self._blit(
                            bitmap,
                            xposition + my_glyph.dx,
                            y_blit_target,
                            my_glyph.bitmap,
                            x_1=glyph_offset_x,
                            y_1=y_clip,
                            x_2=glyph_offset_x + my_glyph.width,
                            y_2=my_glyph.height,
                            skip_index=skip_index,  # do not copy over any 0 background pixels
                        )

                    xposition = xposition + my_glyph.shift_x

[tannewt]

Ok, makes sense. 0 width or height is fine with me.

[dhalbert]

Thanks!