add on_message callback with user data

[vladak]

This change introduces "on_message" callbacks with user_data. I chose to take a conservative approach in order not to break existing code, hence the new callback methods.

[FoamyGuy]

resolves #178

[FoamyGuy]

@vladak and @ehagerty

I think the root problem of accessing the user data object inside the on_message callback can be done with the current version of the library without any changes, although it may be desired to make a tweak to make public property in place of what is a private field today.

The def message(client, topic, message) callback receives the client object as the first parameter, and the client object is the one holding the user data, so inside of the message callabck it could access it from the client object that is already passed in.

I verified this with a modified version of the simpletest where I used this for the message callback:

def message(client, topic, message):
    # This method is called when a topic the client is subscribed to
    # has a new message.
    client._user_data['onoff'] = True if message == "1" else False
    print(f"New message on topic {topic}: {message}")
    print(client._user_data)

# ...
state = {
    "onoff": False
}

# Set up a MiniMQTT Client
mqtt_client = MQTT.MQTT(
    broker="io.adafruit.com",
    port=1883,
    username=aio_username,
    password=aio_key,
    socket_pool=pool,
    user_data=state,
    ssl_context=ssl_context,
)

This seems to work successfully with the currently released version of the library. Can you try that within your project(s) to see if this solution is workable for you?

This solution does rely on accessing client._user_data which is intended to be private denoted by it's leading underscore, but python doesn't enforce this so the code can work anyway.

However perhaps it would be worthwhile to change it so that the user_data isn't "private" with the leading underscore and instead is accessible at client.user_data. That would be only a very minor tweak and would mean that the above code would no longer be breaking the private access naming convention.

Please let me know if I've misunderstood the root problem or anything about it. If I do understand properly and if this solution works for you as it seems to for me I think I'm in favor of adopting only the name change of user_data property since it will provide the same functionality with slightly different syntax and keep the code less complex without the addition of a new type callback and add_callback function, and is inherently backwards compatible since the existing function signature can be used.

[vladak]

Reaching inside the MQTT object sounds more like a workaround, esp. with the (sort of) private member access. My primary interest was to be able to pass the data from/to the message callback, however upon seeing that all the other callbacks (connect, disconnect, publish, subscribe, unsubscribe) are being passed the user_data, it irks me to see this inconsistency.

If this was not the arguably most used callback, I'd propose to bite the bullet and add the user_data argument rather than introduce more callback types, maybe as a keyword argument (which I am not sure would work seamlesslish). Also, the inspect module comes to my mind when thinking about this further. Is it perhaps possible to get the callback method fingerprint prior to calling it and change the arguments dynamically based on that ? (not sure the inspect module is present in CP)

There is also the matter of documentation. The MQTT initializer Python doc says:

user_data: arbitrary data to pass as a second argument to the callbacks.

which does not match reality. The example code in the repo contains the usage matching current version of the library, however I am not sure how many peruse the examples for code construction.

If the reach-inside-MQTT approach prevails, it should be definitely accompanied with some pronounced documentation/examples to avoid (not so frequent ?) MiniMQTT user callbacks (sic!) to determine how to pass the user data around.

[ehagerty]

@FoamyGuy thank you much for investing so much time and thought in this. I am really grateful. I've started testing your suggestion and it certainly seems fine for our use case, although as you say, perhaps if it could "change ... so that the user_data isn't "private" with the leading underscore" that would certainly feel more 'tidy'. @vladak I apologise but I can't entirely understand if you are saying you don't accept this approach? Particularly if the 'offending' underscore were removed this would seem to be an entirely 'normal' bit of functionality for the module? I'm just glad either of you was willing to try to work on this :-)

[vladak]

Removing the underscore and providing sufficient amount of documentation is definitely practical solution within given constraints (i.e. not to break pre-existing users, particularly those of CP) and likely better than supporting bunch of callbacks for the same event with different signatures because that solution does not solve the inconsistency, however still makes me cringe a little bit. I will try to revert my changes within this PR and see what it would take to provide appropriate documentation for the callbacks.

[vladak]

Closing in favor of PR #188.