Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce the use of MMQTTException #240

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vladak
Copy link
Contributor

@vladak vladak commented Jan 28, 2025

This changes some of the exceptions raised. Namely it reduces the use of MMQTTException to non-programming errors. For these, I converted the code to use ValueError/NotImplementedError or the newly introduced MMQTTStateError (which can be caught via MMQTTException).

use it for protocol/network/system level errors only

fixes adafruit#201
@dhalbert dhalbert requested a review from brentru January 28, 2025 21:52
dhalbert
dhalbert previously approved these changes Jan 28, 2025
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These make sense to me, thanks! Do you see any exception catching in examples here or in https://github.com/adafruit/Adafruit_Learning_System_Guides that might break due to these changes?

@dhalbert
Copy link
Contributor

@brentru Look good to you?

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhalbert Rather than throwing a generic exception, the library now throws a specific Exception or Error. In theory, this looks good to me as it lets a user catch a more specific error rather than a generic.

I do worry about handling these errors, as try/except in a number of examples may rely on MMQTTException. It would generate additional support load.

@vladak @dhalbert Do you mind holding off on this until tomorrow PM? I'd like to check through examples that use MiniMQTT on the Learning System Guide repository, Libraries, and in the Adafruit Learning System to see if any Except: cases would fail due to the change in exception.

@dhalbert dhalbert dismissed their stale review January 28, 2025 22:37

waiting for @brentru review

@dhalbert
Copy link
Contributor

@brentru Thanks - I removed my approval to prevent merging until/if you approve.

@brentru
Copy link
Member

brentru commented Jan 30, 2025

@vladak For something like this, where MQTTException is used to catch a generic network error and later reconnect, do you expect it to change?

https://github.com/brentru/Adafruit_Learning_System_Guides/blob/400269bdaf387789ddb46472ee7d70cf7a93143f/MEMENTO/Memento_itsaSNAP_Capture_Cam/code.py#L137

@brentru
Copy link
Member

brentru commented Jan 30, 2025

@vladak I think the correct change here is MMQTTException -> MMQTTStateError as the MQTT clientis_connected call now throws a MQTTStateError.

Does this look correct to you?

@brentru
Copy link
Member

brentru commented Jan 30, 2025

This is for my notes, to change, and unrelated to the questions above for @vladak.

Similar network components that import MiniMQTT and use this exception:

CircuitPython AWS IoT Library. Though the library is much older and likely incompatible with the current AWS IoT API, we should probably update it for this PR.

@vladak
Copy link
Contributor Author

vladak commented Feb 3, 2025

@vladak I think the correct change here is MMQTTException -> MMQTTStateError as the MQTT clientis_connected call now throws a MQTTStateError.

Does this look correct to you?

MMQTTStateError extends the MMQTTException class so the code will keep working as is because the except will match the parent class. At least this is the way it works in CPython.

This code:

#!/usr/bin/env python3

import json
import socket
import ssl
import sys

import adafruit_minimqtt.adafruit_minimqtt as MQTT

import logging


def main():
    logging.basicConfig()
    logger = logging.getLogger(__name__)
    logger.setLevel(logging.DEBUG)

    logger.info("Creating MQTT instance")

    broker = "172.40.0.3"
    port = 1883
    mqtt_client = MQTT.MQTT(
        broker=broker,
        port=port,
        socket_pool=socket,
        ssl_context=ssl.create_default_context(),
        connect_retries=3,
    )

    logger.info("Looping before connect()")
    try:
        mqtt_client.loop()
    except MQTT.MMQTTException as e:
        logger.error(e.__class__)
        sys.exit(1)

    logger.info("Connecting to MQTT broker")
    mqtt_client.connect()


if __name__ == "__main__":
    main()

yields the following output on CPython:

INFO:__main__:Creating MQTT instance
INFO:__main__:Looping before connect()
ERROR:__main__:<class 'adafruit_minimqtt.adafruit_minimqtt.MMQTTStateError'>

If someone wants to catch the MMQTTStateError specifically, this can be done (and has to be done in pytest). The code can be changed to:

    logger.info("Looping before connect()")
    try:
        mqtt_client.loop()
    except MQTT.MMQTTStateError as e:
        print("wrong state")
        sys.exit(1)
    except MQTT.MMQTTException as e:
        logger.error(e.__class__)
        sys.exit(1)

which will use the except MQTT.MMQTTStateError block in this case (the order matters - if I moved the except MQTT.MMQTTException before except MQTT.MMQTTStateError, it would match because the matching is done on first fit basis).

@vladak
Copy link
Contributor Author

vladak commented Feb 11, 2025

Tested on Adafruit CircuitPython 9.2.4 on 2025-01-29; Adafruit QT Py ESP32S2 with ESP32S2 with the above code slightly adapted to run on CP:

#!/usr/bin/env python3

import ssl
import sys

import adafruit_logging as logging
import socketpool
import wifi

import adafruit_minimqtt.adafruit_minimqtt as MQTT


try:
    from secrets import secrets
except ImportError:
    print(
        "WiFi and Adafruit IO credentials are kept in secrets.py, please add them there!"
    )
    raise


def main():
    logger = logging.getLogger(__name__)
    logger.setLevel(logging.DEBUG)

    logger.info("Connecting to wifi")
    wifi.radio.connect(secrets["ssid"], secrets["password"], timeout=10)
    logger.debug(f"IP: {wifi.radio.ipv4_address}")

    pool = socketpool.SocketPool(wifi.radio)  # pylint: disable=no-member

    logger.info("Creating MQTT instance")

    broker = "172.40.0.3"
    port = 1883
    mqtt_client = MQTT.MQTT(
        broker=broker,
        port=port,
        socket_pool=pool,
        ssl_context=ssl.create_default_context(),
    )

    logger.info("Looping before connect()")
    try:
        mqtt_client.loop()
    except MQTT.MMQTTException as e:
        logger.error(e.__class__)
        sys.exit(1)

    logger.info("Connecting to MQTT broker")
    mqtt_client.connect()


if __name__ == "__main__":
    main()

It produced the following output:

Auto-reload is on. Simply save files over USB to run them or enter REPL to disable.
code.py output:
217.475: INFO - Connecting to wifi
217.478: DEBUG - IP: 172.40.0.20
217.479: INFO - Creating MQTT instance
217.484: INFO - Looping before connect()
217.486: ERROR - <class 'MMQTTStateError'>

Code done running.

@tyeth tyeth mentioned this pull request Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants