adafruit / Adafruit_CircuitPython_MiniMQTT

MQTT Client Library for CircuitPython
Other
80 stars 49 forks source link

Wrong _sock_exact_recv implementation used when adafruit_esp32spi.adafruit_esp32spi_socket provided #165

Open cskwrd opened 1 year ago

cskwrd commented 1 year ago

As the title states this library uses the implementation meant for CPython/socket pools instead of the ESP32SPI implementation.

Steps to reporduce

  1. Upload the following script, and install adafruit_minimqtt and adafruit_esp32spi

    # SPDX-FileCopyrightText: 2021 ladyada for Adafruit Industries
    # SPDX-License-Identifier: MIT
    
    import time
    import board
    import busio
    from digitalio import DigitalInOut
    import neopixel
    from adafruit_esp32spi import adafruit_esp32spi
    from adafruit_esp32spi import adafruit_esp32spi_wifimanager
    import adafruit_esp32spi.adafruit_esp32spi_socket as socket
    
    import adafruit_minimqtt.adafruit_minimqtt as MQTT
    
    ### WiFi ###
    
    # Get wifi details and more from a secrets.py file
    try:
        from secrets import secrets
    except ImportError:
        print("WiFi secrets are kept in secrets.py, please add them there!")
        raise
    
    # If you are using a board with pre-defined ESP32 Pins:
    esp32_cs = DigitalInOut(board.ESP_CS)
    esp32_ready = DigitalInOut(board.ESP_BUSY)
    esp32_reset = DigitalInOut(board.ESP_RESET)
    
    # If you have an externally connected ESP32:
    # esp32_cs = DigitalInOut(board.D9)
    # esp32_ready = DigitalInOut(board.D10)
    # esp32_reset = DigitalInOut(board.D5)
    
    spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
    esp = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset, debug=False)
    """Use below for Most Boards"""
    status_light = neopixel.NeoPixel(
        board.NEOPIXEL, 1, brightness=0.2
    )  # Uncomment for Most Boards
    """Uncomment below for ItsyBitsy M4"""
    # status_light = dotstar.DotStar(board.APA102_SCK, board.APA102_MOSI, 1, brightness=0.2)
    # Uncomment below for an externally defined RGB LED
    # import adafruit_rgbled
    # from adafruit_esp32spi import PWMOut
    # RED_LED = PWMOut.PWMOut(esp, 26)
    # GREEN_LED = PWMOut.PWMOut(esp, 27)
    # BLUE_LED = PWMOut.PWMOut(esp, 25)
    # status_light = adafruit_rgbled.RGBLED(RED_LED, BLUE_LED, GREEN_LED)
    wifi = adafruit_esp32spi_wifimanager.ESPSPI_WiFiManager(esp, secrets, status_light)
    
    ### Adafruit IO Setup ###
    
    # Setup a feed named `testfeed` for publishing.
    default_topic = 'adafruit/circuitpython/minimqtt/esp32spi/test'
    
    ### Code ###
    
    # Define callback methods which are called when events occur
    # pylint: disable=unused-argument, redefined-outer-name
    def connected(client, userdata, flags, rc):
        # This function will be called when the client is connected
        # successfully to the broker.
        print("Connected to MQTT broker! Listening for topic changes on %s" % default_topic)
        # Subscribe to all changes on the default_topic feed.
        client.subscribe(default_topic)
    
    def disconnected(client, userdata, rc):
        # This method is called when the client is disconnected
        print("Disconnected from MQTT Broker!")
    
    def message(client, topic, message):
        """Method callled when a client's subscribed feed has a new
        value.
        :param str topic: The topic of the feed with a new value.
        :param str message: The new value
        """
        print("New message on topic {0}: {1}".format(topic, message))
    
    # Connect to WiFi
    print("Connecting to WiFi...")
    wifi.connect()
    print("Connected!")
    
    # Initialize MQTT interface with the esp interface
    MQTT.set_socket(socket, esp)
    
    # Set up a MiniMQTT Client
    mqtt_client = MQTT.MQTT(
        broker='test.mosquitto.org',
        port=1883,
        keep_alive=5,
    )
    
    # Setup the callback methods above
    mqtt_client.on_connect = connected
    mqtt_client.on_disconnect = disconnected
    mqtt_client.on_message = message
    
    # Connect the client to the MQTT broker.
    print("Connecting to MQTT broker...")
    mqtt_client.connect()
    
    # Start a blocking message loop...
    # NOTE: NO code below this loop will execute
    # NOTE: Network reconnection is handled within this loop
    while True:
        try:
            print('Checking for messages...')
            mqtt_client.loop(timeout=1)
            print('Done...')
        except (ValueError, RuntimeError) as e:
            print("Failed to get data, retrying\n", e)
            wifi.reset()
            mqtt_client.reconnect()
            continue
        time.sleep(1)

    The above script is a modified version of examples/esp32spi/minimqtt_pub_sub_blocking_esp32spi.py. Modified to reduce dependence on secrets.py. Shortened keep_alive to exhibit bug sooner. Added non-zero timeout to .loop() invocation to prevent infinite blocking loop (separate bug already mentioned in issues, #142).

  2. Add WiFi SSID and password to secrets.py.
  3. Reset device.

Actual Results

MMQTTException: Unable to receive 1 bytes within 5 seconds.

Expected Results

Checking for messages... followed by Done... repeatedly.

Root cause

It appears as though this library makes an assumption that is the socket has a recv_into implementation, that the socket is not an ESP32SPI socket. As of approximately 18 months ago, this is no longer the case. A recv_into implementation was added to adafruit_esp32spi. PR 151

Library versions

boot_out.txt

Adafruit CircuitPython 8.0.5 on 2023-03-31; Adafruit PyPortal with samd51j20
Board ID:pyportal

Related

cskwrd commented 1 year ago

My use case is to interact with a home assistant installation via MQTT using the PyPortal with an interface dependent on asyncio. As such the invocation of .loop() must be non-blocking. The workaround I implemented myself is to modify the check performed in the library's _sock_exact_recv function, like so:

        is_esp32spi_sock = self._sock.__module__.find('esp32spi')
        if not is_esp32spi_sock and not self._backwards_compatible_sock:

I generally shy away from python so I have no idea how poor this fix is. If it's suitable I can put it in a PR.