is_connected raises MMQTTException when not connected, rather that returning False.

[kaelfischer]

I would expect is_connected to return True or False, rather than raise an exception in the False case:

>>> mqtt_client.is_connected()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_minimqtt/adafruit_minimqtt.py", line 978, in is_connected
MMQTTException: MiniMQTT is not connected.

To recreate:

import socketpool
import wifi
import adafruit_minimqtt.adafruit_minimqtt as MQTT
from secrets import secrets
pool = socketpool.SocketPool(wifi.radio)
mqtt_client = MQTT.MQTT(
    broker=secrets["broker"],
    port=secrets["port"],
    socket_pool=pool,
)
mqtt_client.is_connected()

[2bndy5]

I too find this annoying. I'm writing a library that needs to set the will/testament before making a connection (without knowing the prior state of connection).

The fact that is_connected() can only return True or raise an exception makes it more like an assert statement. Meaning every time I use this function, I have to wrap it in a try ... except clause:

try:
    if self.client.is_connected():
        self.client.disconnect()
except MMQTTException:
    pass  # this means the client was/is disconnected
self.client.will_set(topic, msg)
self.client.connect(**mqtt_settings)

I could write this without is_connected():

try:
    self.client.disconnect()
except MMQTTException:
    pass  # this means the client was/is disconnected
self.client.will_set(topic, msg)
self.client.connect(**mqtt_settings)

But at that point, what is the actual utility of is_connected()?

[2bndy5]

git blames 41bc399 which seems to prefer internally using the method as an assertion rather than a getter.

[BiffoBear]

I would like to have this return a boolean instead of raising an error so that I can use it in a loop. Since there is already a breaking change pull request in for making <init> key word only, perhaps this is a good opportunity to change this behaviour.

[BiffoBear]

The is_connected() method is poorly named. It only checks to see if there is a socket and if the variable `self._is_connected' is true. It does not reflect the state of the connection at the moment it is called.

self._is_connected is initialised False then set to True when connect() is called and a connection to the broker is successful. self._is_connected is set to False when `disconnect() is called.

The code calls is_connected() at the beginning of methods like disconnect(), publish() and will_set().

In will_set, for example:

    if self._is_connected:
        raise MMQTTException("Last Will should only be called before connect().")

From reading through the code, I conclude that the is_connected() method has nothing to say about whether there is connection to the MQTT broker at the moment when it is called. The method is used by the author to decide whether it is appropriate to call methods based on whether a call to connect() or disconnect() have previously been made.

Because the method is named is_connected I and others incorrectly assumed that the method will tell us whether there is a current, live connection to the MQTT server and think that the method should return True instead of raising an exception. We're wrong.

Therefore I think that changing is_connected() to return True instead of raising an exception will cause more problems for users than leaving it alone. The ping() method is the intended way to see if the broker connection is live or not.

Hope this helps!

[2bndy5]

Being that the intent of this library is to have a minimal footprint ("mini" is in the name), I think it's fine to let is_connected() return a private flag describing if dis/connected() was called. This at least provides other libraries with a way of understanding the context in which it needs to use minimqtt without slowing things down with a ping [then wait for] pong approach. Remember, the original intention was to provide a minimal alternative to paho_mqtt on microcontrollers, so there may not have been a lot of options to pick a function name from when first writing this lib.

As you say, users can call ping() to test for a live connection. But, if this happens in the main loop(), then it could be a very costly approach (for a microcontroller).

While I value your observation, the purpose of this thread is to allow using is_connected() as a getter instead of an assertion. Personally, I think it would be clearer in the lib src if the internal usage of is_connected() as an assertion was called _assert_connected_called().

[BiffoBear]

While I value your observation, the purpose of this thread is to allow using is_connected() as a getter instead of an assertion. Personally, I think it would be clearer in the lib src if the internal usage of is_connected() as an assertion was called _assert_connected_called().

I agree with you. Perhaps also renaming is_connected() to connect_called() and having it return a bool would help to remove the ambiguity around the function name, since having the function return bool is a breaking change anyway?

[2bndy5]

Well I'm not in favor of renaming the function in the public API because that would be even more breaking than limiting the return value to a bool. That suggestion was mainly for the private API. The name is_connected is borrowed from paho_mqtt, so it goes against the lib's primary goal if you rename the function in the public API.

Personally, I think you had it right the first time with #125. But you did what most of us coders do, and you thought (or over-thought) about it too much.

[BiffoBear]

Personally, I think you had it right the first time with #125. But you did what most of us coders do, and you thought (or over-thought) about it too much.

Guilty as charged. I was looking at the Paho source code when you replied. I'll reopen the pull request with 'is_connectedreturning abool`.

Thanks!

[2bndy5]

Remember that you can use the keep_alive param in connect(), and then loop() should automatically do a ping to the broker when appropriate.

[dhalbert]

@brentru Could you weigh in on this?

[brentru]

@dhalbert @BiffoBear I agree with @2bndy5 - this library's function calls should be similar to Paho MQTT Python's API.

Let's go with #125