OverflowError small int overflow on QT Py SAMD21

[carlfj]

When testing QT Py SAMD21 with the NeoKey 1x4 QT I2C Breakout, I got the error OverflowError: small int overflow, using the example neokey1x4_simpletest.py from the library / learn.adafruit.com:

code.py output:
Adafruit NeoKey simple test
Traceback (most recent call last):
  File "code.py", line 17, in <module>
  File "adafruit_neokey/neokey1x4.py", line 73, in __getitem__
  File "adafruit_seesaw/seesaw.py", line 211, in digital_read
  File "adafruit_seesaw/seesaw.py", line 217, in digital_read_bulk
OverflowError: small int overflow

The program is identical to the linked, but is embedded below for reference. Running the latest bootloader 3.14 and CircuitPython 7.3.2.

I added the line buf[0] = buf[0] & 0x3F before line 217 with the error, compiling it with mpy-cross, and the error is gone. I copied the line from the get_temp function, line 361 with a similar context. That was inspired by a adafruit forum post about the same error, mentioning to keep a value at 8 bits with a bitwise &= 0xff. In my setup it only works when keeping it at 6 bits as the original code, not 8 bits.

It turns out the pull request https://github.com/adafruit/Adafruit_CircuitPython_seesaw/pull/98 on 2022-04-04 had removed this exact line to get PA30 and PA31 values: Do not blank out the value read for pins PA30 and PA31. The commit: https://github.com/adafruit/Adafruit_CircuitPython_seesaw/pull/98/commits/d92217cf68cb97a4627a661322af6cb1ca403ef5

For reference the function digital_read_bulk looks like this, with the reverted edit to work with QT Py SAMD21:

    def digital_read_bulk(self, pins, delay=0.008):
        """Get the values of all the pins on the 'A' port as a bitmask"""
        buf = bytearray(4)
        self.read(_GPIO_BASE, _GPIO_BULK, buf, delay=delay)
        buf[0] = buf[0] & 0x3F
        ret = struct.unpack(">I", buf)[0]
        return ret & pins

The commit seems to be the catalyst to the OverflowError: small int overflow, but my solution just cancels this commit. I haven't read into what the ret = struct.unpack(">I", buf)[0] line does, so I don't know what should be done to keep adafruit_seesaw library compatible with QT Py SAMD21 / M0. Any suggestions?

Originally posted on forums.adafruit.com

For reference, the copy of example code neokey1x4_simpletest.py that ran on the QT Py SAMD21:

# SPDX-FileCopyrightText: 2021 ladyada for Adafruit Industries
# SPDX-License-Identifier: MIT
"""NeoKey simpletest."""
import board
from adafruit_neokey.neokey1x4 import NeoKey1x4

# use default I2C bus
i2c_bus = board.I2C()

# Create a NeoKey object
neokey = NeoKey1x4(i2c_bus, addr=0x30)

print("Adafruit NeoKey simple test")

# Check each button, if pressed, light up the matching neopixel!
while True:
    if neokey[0]:
        print("Button A")
        neokey.pixels[0] = 0xFF0000
    else:
        neokey.pixels[0] = 0x0

    if neokey[1]:
        print("Button B")
        neokey.pixels[1] = 0xFFFF00
    else:
        neokey.pixels[1] = 0x0

    if neokey[2]:
        print("Button C")
        neokey.pixels[2] = 0x00FF00
    else:
        neokey.pixels[2] = 0x0

    if neokey[3]:
        print("Button D")
        neokey.pixels[3] = 0x00FFFF
    else:
        neokey.pixels[3] = 0x0

[Neradoc]

I briefly looked at it, it happens on builds without long ints, because it's trying to unpack a 4 bytes buffer into an int, and small ints in Circuitpython are limited to 30 bits, so the maximum is 0x3FFFFFFF.

If we don't want to unconditionally reverse #98 we could add a try/except for OverflowError.

[carlfj]

Since the most prominent product photo/gif of the NeoKey 1x4 QT I2C Breakout shows it connected to QT Py SAMD21, I think it's rather urgent that the standard adafruit_seesaw library works for SAMD21. Getting support for the last 2 bits / 2 pins on builds without long ints can wait until a compromise has been found.

Instead of just reversing #98, adding a try/except for the special case where the 2 bits are lost, seems like a good compromise for now. My initial effort is along the lines of changing line 217 of seesaw.py into this try/except:

        try:
            ret = struct.unpack(">I", buf)[0]
        except OverflowError:
            buf[0] = buf[0] & 0x3F
            ret = struct.unpack(">I", buf)[0]

It works on QT Py SAMD21 with NeoKey 1x4. I am not near a M4 SAMD51 or RP2040 today to test the successful try clause.

I am curious if the user will ever see the error? If a print statement were to be added, the serial console would be filled with warnings. Is there a way to warn the user, without causing too much noise? An unaffected user doesn't want a warning on every boot.

To give context to #98, as far as I understand it was added as a special case, to access the SWDCLK / PA30 and SWDIO / PA31 pins as inputs on the ATSAMD09 Breakout with Seesaw and on a third party project with ATSAMD10. With the caveat that the SWDCLK / PA30 needs an external pull-up. So it deserves to be accessible, we just need a solution for the builds without long ints. I really like the the modular approach to Seesaw, so I have been following the releases of new breakouts with it. Though I was not too familiar with the technical design of the protocol, which I got the chance to dig into a bit this time.

[jepler]

That sounds like a good compromise. @carlfj can you open a Pull Request with the change?

[jepler]

.. adding a comment to state that it's for compatibility with boards that don't support arbitrary integers may help prevent it being removed in the future by a well-meaning contributor.

[carlfj]

Thank you for the help. Lovely so see a fix be released quickly, all the way to the CircuitPython bundle.

I don't have any real idea about the performance impact with this try/exception error handling of struct.unpack(">I", buf). It mostly impacts builds without long ints when using ATSAMD09 based Seesaw products, as they will run both try and except scopes. These boards all have an external pull-up on SWDCLK / PA30, so normally bit 31 of buf is always set. That would be most Adafruit Seesaw products, except the ATtiny817 Breakout with Seesaw, NeoSlider and LED Arcade Button 1x4.

After reading @mkende's comment on Pull request #98, I became aware that the last 2 bits of buf causing the error aren't used anyway, if the bits aren't set in the pins parameter, e.g. when pins <= 0x3FFFFFFF . So as long as the pins parameter of digital_read_bulk is an int type, it cannot be larger than 0x3FFFFFFF on builds without long ints. Instead of the try/except case in Pull request #108, the issue could be solved in a similar way by simply ignoring the last to bits of buf[0] when pins <= 0x3FFFFFFF:

        # Only unpack bits 31 and 32 of buf, when they are also set in pins,
        # to avoid OverflowError on boards that don't support arbitrary integers
        if pins <= 0x3FFFFFFF:
            buf[0] = buf[0] & 0x3F
        ret = struct.unpack(">I", buf)[0]

I have tested this on QT Py SAMD21 with NeoKey 1x4 QT I2C Breakout. I included a comment to make sure it is not removed once again as suggested above, but I am not sure if this is what was meant.

This feels like a lot more elegant solution, without the code duplication, and runs the same code on both builds types. A bit like simply walking around the obstacle rather than handling it head on. The performance penalty seems smaller with the if <= comparison and bitwise-&. I'd love to get a second opinion on the difference in performance with this approach.

[tekktrik]

I'm torn about it, honestly. I agree that this second solution is more elegant in a way, but I also like the "Easier To Ask for Forgiveness Than Permission" (EAFP) approach as more Pythonic. But I'm also not sure what happens behind the scenes. The way it's written now, I'd be surprised if it ever got refactored accidentally by someone with good intentions as @jepler mentioned, and I like that it's clear because of the exception why that code is written that way. But I don't feel strongly either way. Curious what you think, @jepler and @Neradoc.