MQTT receive: Bug fix and avoid payload copy

[eikel]

See the individual commits for details.

Program size has increased by 488 bytes in my setup (only MQTT_ENABLE enabled in settings-override.h).

Building the dev branch before:

RAM:   [==        ]  18.1% (used 59308 bytes from 327680 bytes)
Flash: [===       ]  31.9% (used 2090733 bytes from 6553600 bytes)

Building with my changes:

RAM:   [==        ]  18.1% (used 59308 bytes from 327680 bytes)
Flash: [===       ]  31.9% (used 2091221 bytes from 6553600 bytes)

I have not yet tested my changes on a real device.

[biologist79]

@eikel Along with d89bc7c you introduced std::string_view() for Mqtt.cpp. I'd like to know how this works :-)
In the past we've copied payload to heap because I recognized *payload gets orphaned somehow fast. Now when I unterstand string_view() right, there's no copy done. This having said, you have to make somehow sure, that payload out-lives the string_view object.
My guess: it is just luck that the problem didn't re-occur. Maybe because of the movement to ESP32-Arduino2.

Any thoughts on this?

[eikel]

You are right, no copy is done, as this is the purpose of a std::string_view. In Mqtt_ClientCallback, receivedString provides a C++ interface for working with the string given as payload and length. More or less, it is only syntactic sugar. I do not see why this extra heap copy was needed in the first place. The payload is only accessed inside Mqtt_ClientCallback and not stored elsewhere. I assume the MQTT framework in use should guarantee that the payload is available until Mqtt_ClientCallback returns, therefore it is safe to use it in that function.

[biologist79]

As it turns out: your assumption is wrong :-)
https://github.com/knolleary/pubsubclient/blob/master/examples/mqtt_publish_in_callback/mqtt_publish_in_callback.ino#L34

Wasn't 100 % sure about it but just gave the example a view. Even the lib author recommends to make a string copy.

[eikel]

Okay, but the problem is the publish, as this modifies the string that you give it. So probably the cases where publishMqtt is called would need to make that copy, or maybe one can change it to something like this (to also use the parameter types to signal what is expected here):

/* Wrapper-functions for MQTT-publish
   The payload buffer will be overwritten whilst constructing the PUBLISH packet,
   therefore accept a std::string parameter by copy here.
*/
bool publishMqtt(const std::string_view topic, std::string payload, bool retained) {
#ifdef MQTT_ENABLE
	if (topic != "" && Mqtt_PubSubClient.connected()) {                        
		Mqtt_PubSubClient.publish(topic.data(), payload.c_str(), retained);
		// delay(100);
		return true;
	}
#endif

	return false;
}

I have not compiled it, but I hope you get the idea.