hsaturn / TinyMqtt

ESP 8266 / 32 / WROOM Small footprint Mqtt Broker and Client
GNU General Public License v3.0
191 stars 40 forks source link

Bind a class function to setCallback function #54

Closed letmp closed 1 year ago

letmp commented 1 year ago

I would like to bind a class function to the setCallback function. Therefor I declared the callback function like that:

class MyClass
{

  private:
  void callbackFunction(MqttClient::CallBack cb);
  ...
}

and try to pass it like that:

client->setCallback(std::bind(&MyClass::callbackFunction, this, std::placeholders::_1));

I want to do that because I want to access dynamic variables of MyClass.

The problem: I cannot define my callbackFunction with MqttClient::Callback as function parameter because the type MqttClient::CallBack is inaccessible.

Is there any solution how I can access variables of MyClass within the function body of my callbackFunction?

hsaturn commented 1 year ago

Hello

You can use lambda for this:

class CallBackClass
{
   public:
      void onPublish(const MqttClient* srce, const Topic& topic, const char* payload, size_t len);
     ...
};

client->setCallback([&instance](const MqttClient* srce, const Topic& topic, const char* payload, size_t len)->void
{
   instance.onPublish(source, topic, payload, len);
});

Of course, you must take care that the instance should still exist when exiting the scope of setting a callback this way.

letmp commented 1 year ago

Thanks for your quick answer! I already tried that, but I always get this error:

no suitable conversion function from "lambda [](const MqttClient *source, const Topic &topic, const char *payload, size_t len)->void" to "MqttClient::CallBack" exists

Any ideas what could cause that? Thanks for your effort!

hsaturn commented 1 year ago

Strange, I tried this solution before sending it to you... I remember that I had tried it with tinytest. I'm going to write this again and send this to you. Are you sure that you have the latest release ?

hsaturn commented 1 year ago

What I've done:

1 - open TinyMqtt examples / tinymqtt-test 2 - at line 682 (just after client->setCallback(onPublish);

          client->setCallback([&x](const MqttClient *srce, const Topic &topic, const char* p, size_t len)->void
          {
            x.onPublish(srce, topic, p, len);
          });

3 - At the begining of the file

class X
{
  public:
    void onPublish(const MqttClient* srce, const Topic &topic, const char* p, size_t len) {}
};

X x;

4 - Compiles fine !

Not tested, but pretty sure this'll work. Version of IDE : 2 Version of TinyMqtt : 0.9.8

see https://pastebin.com/SytPfQMX

hsaturn commented 1 year ago

Sorry, 0.9.8 was only existing on my PC, and will never exist for some reasons. I've just released the 0.9.9.

Anyway the lambda should work with any release. Best regards

hsaturn commented 1 year ago

Hello Can I close this issue or is it still alive ?

letmp commented 1 year ago

I will test it tomorrow. Thanks

alranel commented 1 year ago

I had this problem as well. If I recall correctly, you can use a lambda here only if it does not capture. In fact, according to the C++ standard, only non-capturing lambdas can be converted to function pointers. Maybe turning MqttClient::CallBack to std::function could be the best solution, although it would introduce some overhead. In my case, I created my clients as global variables and created static callbacks.

hsaturn commented 1 year ago

Hello Alranel and Letmp

@Alranel, you're right. Memory on embedded devices is the main issue we are facing. This is why I've designed TinyMqtt with a function callback instead of any other design (inheritance, std::function ...). This is also the reason why both MqttClient and MqttBroker classes do not make use of virtual.

@letmp, As I said in my exemple with tinytest, the X instance is at the begining of the file. So it has to be global ! You cannot use a local instance of any class and put a callback on it for two reasons:

The destructor of MqttClient take care about point 2.

I may have a solution anyway (to be continued...)

hsaturn commented 1 year ago

@letmp -> (please read the previous answer)

In few words, I won't force other users of TinyMqtt to support the overload that would be needed to achieve binding to a class.

But there is a solution to move the overhead from TinyMqtt to your side: -> The idea is that we already have a pointer on the client.

So it is possible to use this pointer to route the message to another instance.

Here is the solution for your problem https://pastebin.com/SwaTGUyR

This solution is not fully satisfying because it can crash if the MqttClient instance is destroyed when MqttReceiver::onPublish makes use of it. Also only the last registered MqttReceiver on a given MqttClient will receive the message.

Maybe I'll add the binding class to the library (which requires to write unit tests and an example for this).

hsaturn commented 1 year ago

Hello again

I've released the version 0.9.11 that should take care of this problem. See the example 'advanced'.

letmp commented 1 year ago

Thank you very much for your effort!