Only one callback function is available for each of on*Callback types

[vortigont]

Hi! 1st to say - this is a nice lib to have, I like the code style.

To issue, comparing to origin Marvinroger's async-mqtt-client this lib allows to bind only single one callback function to each of the event types available, i.e. if one callback is assigned for onConnect(), the second assigned callback will simply override previous one. Marvinroger's lib has a vector container that allows assigning multiple callbacks that are executed one by one for the specified event. std::vector<AsyncMqttClientInternals::OnConnectUserCallback> _onConnectUserCallbacks; For sure such a container could be created in a user code and has a wrapper that will iterate an execute it, but that incompatibility is a bit disappointing. I could also have come up with a derived class for this functionality, but MqttClientSetup is a template and it does not allow any simple inheritance tricks here. It that an intentional behavior or just not implemented (yet)?

[bertmelis]

This is intentional behaviour. I, can only assess my own applications but I haven't encountered a use case for multiple callbacks. I indeed thought a user could just use an intermediate function or whatever std::function object.

I'm always open for a discussion.

[vortigont]

Well, first I hit this when tried your lib as a drop-in replacement to Marvinroger's code. And one of the use cases could be like this: I'm developing a library that uses MQTT as one of the possible transport layers, it also could work with WebSocket and HTTP REST. So it has AsyncMqttClient instance as a public member, it uses this instance for it's own purposes and at the same time allows end user to interact with AsyncMqttClient instance directly to utilize it's full features, assign own callbacks, subscribe, publish, etc... If I have to hide callbacks behind my own implementation of some container then I have to hide an instance of AsyncMqttClient too, so to prevent end user to override callbacks on his own. But then it forces me to reimplement a full set of wrapper methods of the AsyncMqttClient too to allow user to have it's full flexibility. That is not a nice design concept for sure. Another example that comes to my mind is ESP32's event message API. It allows to subscribe any number of consumers for i.e. WiFi Events, and all those callbacks would be executed one after the other. It allows to create a very flexible apps, where any component could subscribe to a group of events independently of the others without the need to create a "broker owner" and share it between user and libs code.

[bertmelis]

I'll see what I can do. I might be able to make it configurable with preprocessor: something like #ifndef SINGLE_CALLBACKS so the multiple callback option is the default one.

On the top of my head I think I could create intermediate functions in the MqttClientSetup class and dispatch from there using std::bind (to pass extra this into the intermediate callback).

[bertmelis]

I drafted some code in this branch: https://github.com/bertmelis/espMqttClient/tree/multiple_callbacks

I have to rework the testing code though. It uses temporary std::functions (with temporary variables) and this messes in runtime. An alternative solution would be to add a method to erase all the registered callbacks.

[vortigont]

I like the idea! Thanks! Will try it this week. Just throwing more ideas here - you may consider using lambda's instead of std::bind, could be more readable. i.e. I use it like this with Mangrover's code to pass another object's member function.

mqttClient->onMessage( [this](char* topic, char* payload, AsyncMqttClientMessageProperties properties, size_t len, size_t index, size_t total){_onMqttMessage(topic, payload, properties, len, index, total);} );

Also I use std::list instead of std::vector for callback container, it might be less memory hungry for default use case with a single callback, vector pre-allocates some place in advance.

And I use structs like this to keep callback handlers in a list

struct HndlrChain {
    int id;
    std::unique_ptr<FrameSend> handler;
    HndlrChain() = delete;
    HndlrChain(const HndlrChain& rhs) = delete;
    HndlrChain(std::unique_ptr<FrameSend>&& rhs) noexcept : id(std::rand()), handler(std::move(rhs)) {};
};

do not look into std::unique_ptr, the idea is on addition each callback receives a random id in a list, and could be removed later by that id if required. Got this idea from ESP32's event handlers, although they use pure C const char*'s ad ID.

Thanks for prompt feedback on this anyway!

[bertmelis]

I like the idea of being able to remove callback handlers. Only being able to add seems 'lacking basic features' to me. Deleting handlers would also minimize the impact of this change to automatic testing.

[bertmelis]

Wrt deleting:

• add handlers to a std::forward_list<uint32_t, callback>
• add an optional uint32_t id to the onX-methods. The user has to supply the ID
• create a deleteOnX(uint32_t id) method to remove a handler. When IDs collide, only the first one is deleted. (nothing is deleted if no matching id is found)

what do you think?

[vortigont]

Like it! That would be a full-featured functionality allowing adding/removing callbacks on-demand or object create/destruct.

[bertmelis]

Well, I suppose the change is good to go. Would you mind if I turn off multiple callbacks by default?

[vortigont]

I'll be totally fine with default off configuration :) Enabling it on-demand is not a problem at all. Thanks for your time and support on this! Appreciate it!

[bertmelis]

implemented with release 1.5.0