search

espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
591 stars 254 forks source link

fix: set TCP transport every time when setting the config (IDFGH-13175) #281

Open huming2207 opened 5 days ago

huming2207 commented 5 days ago

Hi @gabsuren @euripedesrocha

We just realised that the TCP transport is not set every time. When the incoming config::network::transport handle is null, it will not be set, and the MQTT client will keep using the previous old (invalid) TCP transport handle.

This is a bit problematic for us. One of our products have a few network interfaces. For example, when using WiFi, the MQTT (over WebSocket) client requires our own TCP transport handle with HTTP proxy, but 4G modem doesn't. When a switchover is needed, we would call esp_mqtt_client_disconnect to disconnect, and then call esp_mqtt_set_config() to reconfigure the client to let it start using or stop using a the TCP transport with proxy.

But now in current implementation, since you have a if (config->network.transport) null check, this will prevent us from clearing the TCP transport setting. Therefore I think we need to remove this unnecessary null check.

Regards, Jackson

euripedesrocha commented 5 days ago

Hi @huming2207, thanks for the update. From my reading of the code, we are checking if there is valid transport to set. What you want is a way to set it to NULL and use the transport initialized by the mqtt client. Am I correct? Could you also please add a small note to the field documentation in the header?

huming2207 commented 5 days ago

Hi @euripedesrocha

From my reading of the code, we are checking if there is valid transport to set. What you want is a way to set it to NULL and use the transport initialized by the mqtt client. Am I correct?

Yes, we are unable to "clear" the previously defined TCP transport handle and let the MQTT client to create its own default TCP handle.

Could you also please add a small note to the field documentation in the header?

Done, see latest commit.

Regards, Jackson