on_message_received called with data_lenght instead of topic_length

[kamplom]

From the examples it is clear that it should be called with data_length and not topic_length.

For further improvements, in my opinion it should be called with both arguments, data_length and topic_length. That will require edits to networking/mqtt.h and will maybe break some user plugins in case they exists and may require edits to the mqtt.c files on other platorms different from esp32. I can give a look into it if you want me to.

[terjeio]

The topic should be a null teminated string, I do not know if it is in the callback data. This is a way to ensure it will be:

            if(mqtt_events.on_message_received) {
                char *topic;
                if((topic = malloc(event->topic_len + 1))) {
                    strncpy(topic, event->topic, event->topic_len);
                    topic[event->topic_len] = '\0';
                    mqtt_events.on_message_received(topic, (void *)event->data, (size_t)event->data_len);
                    free(topic);
                }
            }

There is more to this, it seems the payload may arrive in chunks in the event handler as well and if so it has to be reassembled like I do in the generic mqtt code.

It would be nice if you could look into this.

[kamplom]

Sure thing, I will do it when I get some free time for proper testing. In the meantime if someone gets interested, the struct esp_mqtt_event_t from the ESP-MQTT API has the members total_data_len and current_data_offset.

I am not using the topic and missed that it was not a pointer! I think the null termination should be done on the plugin side, since you may not want to know from which topic a message comes from. This avoids two memory copies; the one to null terminate the string and the one on the function call. An MQTT topic can be quite long, and then probably someone will use long topics.

A call would look like: mqtt_events.on_message_received((void *)event->topic, (void *)event->data, (size_t)event->topic_len, (size_t)event->data_len);.

That will require modifications to already existing code, and at that point we could just return a pointer to a esp_mqtt_event_t see for the members, so if desired you could access other message info.

That would be implemented into the generic code as well, and the calls would look like: mqtt_events.on_message_received((void *)event);

IMO it is a good time to make this changes as it seems not a lot of things are going on with MQTT, let me know your preferred solution.

[terjeio]

This avoids two memory copies; the one to null terminate the string and the one on the function call.

I do not see that as a disadvantage since the topic is freed immediately after the event callback.

An MQTT topic can be quite long

More than say a few hundred bytes? And will treating it as as a null-terminated string make the plugin code more complex?

and at that point we could just return a pointer to a esp_mqtt_event_t

The point of mqtt.c was to provide an API that wraps the underlying driver code in a minimal way so the user code becomes simple and can be shared across platforms. IMO more advanced plugins should interface with the underlying API directly. And if esp_mqtt_event_t is to be returned it has to be added (along with supporting code) to the generic mqqt.c implementation that sits on top of the lwIP app.

Have you added, or plan to add, plugin code using the API and have run into issues other than described in the OP?

[kamplom]

I do not see that as a disadvantage since the topic is freed immediately after the event callback.

I was thinking more in terms of saving execution time rather than RAM usage. I admittedly do not know if this is justifies making the change to call with the topic as a vector.

More than say a few hundred bytes? And will treating it as as a null-terminated string make the plugin code more complex?

Topic length can be represented by two bytes, so as per the MQTT spec ~65 kilobytes. No, treating it as a null terminated string will make plugin code simpler if anything.

The point of mqtt.c was to provide an API that wraps the underlying driver code in a minimal way so the user code becomes simple and can be shared across platforms. IMO more advanced plugins should interface with the underlying API directly. And if esp_mqtt_event_t is to be returned it has to be added (along with supporting code) to the generic mqqt.c implementation that sits on top of the lwIP app.

I get your point. Yes my idea was to add it to the generic implementation but keeping things simple may facilitate people attempting to create new plugins.

Have you added, or plan to add, plugin code using the API and have run into issues other than described in the OP?

Yes I have added some plugin code but it is rather basic on the mqtt side, recieving and sendig messages with just tens of bytes and with topic lenghts of around 10 bytes. No other issues found with the current API.

[terjeio]

I was thinking more in terms of saving execution time rather than RAM usage. I admittedly do not know if this is justifies making the change to call with the topic as a vector.

IMO the execution time wasted is insignificant in the grand scheme of things.

... recieving and sendig messages with just tens of bytes and with topic lenghts of around 10 bytes.

So my fix above is likely ok. My only worry is about potentially chunked payloads - which the generic implementation handles but this may not do.

[kamplom]

Ok will handle the chunked payloads and pass the topic as a string. I will get back to you, but I do not have a lot of time the next 1-2 weeks, so it will take a while

[terjeio]

it will take a while

That is ok as I believe nobody else is using this.