Subscribing to messages using Homie.getMqttClient() is not reliable: topic is truncated

[peret2000]

Hello,

I am trying to use Homie as my base code for all the developments in my ESP8266. It is a great framework to run projects!
In my code, I am subscribing to messages from other Homie devices: let's say device 'A' subscribes to messages from device 'B' (homie/B/#).

It is done as this example:

_AsyncMqttClient& mqttClient = Homie.getMqttClient();                                                                                                                 
uint16_t packetIdSub = mqttClient.subscribe("homie/B/#", 2);
...
mqttClient.onMessage(onMqttMessage);
...
void onMqttMessage(char* topic, char* payload, AsyncMqttClientMessageProperties properties, size_t len, size_t index, size_t total)
{                                                                                                                                                                      
  Serial << "[MQTT onMqttMessage]: " << topic << ":" << ((String)payload).substring(0,len) << endl;
}

I have noticed that topic is truncated to 'homie/B'

The reason, as far as I have understood the code, is that BootNormal::_onMqttMessage(), that is the first event handler called by async-mqtt-client library when a subscribed message arrives, modifies 'topic'
This method calls to __splitTopic(), that calls to strtok(), to split the topic, and fill in _mqttTopicLevels.
That function exchanges '/' with a '\0' character after its first token (in my example 'homie/B').

Later, when you deal with 'topic', in onMqttMessage() , it has been truncated, because you are called after BootNormal::_onMqttMessage().

I see, at least, two solutions:

• One, more transparent, would be to make a copy of 'char* token' at the begining of BootNormal::_onMqttMessage(), and work with the copy. I need more experience in programing to do that, as I am not aware of all of the implications of allocating memory in a library that I do not fully know.

• And two: BootNormal::_onMqttMessage() could call first to the rest of functions that are subscribed to mqtt messages, and then, continue to do its job. This will probably imply more changes in code

Homie library would be much more powerful if we can use devices for generic purposes, based on Homie, not restricted to some 'limitations'. But that may need changes, like the ones proposed here.

I am sorry if all of this is wrong, because I am not an expert, maybe I am not coding as I should. In such case, please ignore this comment.

Regards

[timpur]

@peret2000, you make a good point. You should be able to tap into the mqtt lib and use it as expected it you really need to. I shall fix this/if you want to you can.

That said you shouldn't need to, for this kind of thing, sharing messages between devices directly over mqtt was thought of to be solved by broadcast messages.

http://marvinroger.github.io/homie-esp8266/docs/2.0.0/advanced-usage/broadcast/

Hopefully this will help you. That said it's quite basic, and if you think of improvements please share

Thanks

[peret2000]

@timpur, thanks a lot. I appreciate your interest and disposition to make the changes. I will try to propose something for you to evaluate if it is ok.
Having a thought, there may be a leak, each subscribed message implies this event to be invoked, so there is a call to several "new" calls to reserve memory and I do not see where it is freed.
So, I will propose a change to, on one hand, avoid the leak, and also allow to pass the topic to the next events to be called.

I see your point about broadcast messages. My idea is to allow subscriptions in general, without a particular pattern: they could come from ahomie

[peret2000]

Sorry, I sent the comment incomplete.
They could come from a homie device or from any mqtt sender.
Regards

[peret2000]

I was wrong. There is no memory leak at all. Memory keeps the same. Reading about std:unique_ptr smart pointers, I see there is no need to free memory explicitly.

So, I have added in my local copy of BootNotmal.hpp a new property (std::unique_ptr<char[]> _mqttTopicCopy) used in BootNormal::_onMqttMessage() as a copy of char* topic; so that char* topic remains unchanged.
I am testing the execution, and the memory remains constant (it seems), and the topic travels correctly to the events subscribed.

Thanks for your comments. I will implement these changes in my local deplyment of Homie. Let me know if you want them uploaded or sent to you. I am a newbie with github (I am learning)

[timpur]

All g.
Don't worry, only one way to learn.

Fork Homie, make your changes there, then create a pull request to homie here, with your changes. Google will help you along the way.

I'll review and decided if anything enhancements can be done :)

But sounds like you know what your doing, and have a viable solution.

Thanks :)