eclipse-paho / paho.mqtt.embedded-c

Paho MQTT C client library for embedded systems. Paho is an Eclipse IoT project (https://iot.eclipse.org/)
https://eclipse.org/paho
Other
1.37k stars 757 forks source link

[Breaking] Moving CONNECT willProperties into MQTTPacket_connectData #245

Closed CIPop closed 1 year ago

CIPop commented 1 year ago

This is a breaking change as existing applications will need to define MQTTV5 prior to including the existing MQTTClient or MQTTPacket (non-V5) used in conjunction with the v5 libs. This is needed to provide the correct MQTTPacket_connectData and MQTTPacket_willOptions versions (and corresponding initializers).

Test15 was modified to define MQTTV5 within cmake.

icraggs commented 1 year ago

@CIPop if we do this, why not also have the connect properties in the connectData structure? Moving just one seems inconsistent to me.

CIPop commented 1 year ago

@CIPop if we do this, why not also have the connect properties in the connectData structure? Moving just one seems inconsistent to me.

While I've 👍 above, I'm now reconsidering. All other v5 APIs accept the properties as a parameter:

MQTTV5Deserialize_connack(**&recv_properties**, &sessionPresent, &connack_rc, buf, buflen)
MQTTV5Serialize_subscribe(buf, buflen, 0, msgid, **&sub_properties**, 1, &topicString, &req_qos, &sub_options);
MQTTV5Deserialize_suback(&submsgid, **&recv_properties**, 1, &subcount, &reason_code, buf, buflen);

I've preserved the pattern with connect:

MQTTV5Serialize_connect(buf, buflen, &data, **&conn_properties**);

In this case, the will member of MQTTPacket_connectData is specifying a will message which should be encoded similarly to a publish:

MQTTV5Serialize_publish(buf, buflen, 0, 0, 0, 0, topicString, **&send_properties**, (unsigned char *)payload, payloadlen);

This PR changes the code to encapsulate this "will publish" within MQTTPacket_willOptions:

typedef struct
{
    /** The eyecatcher for this structure.  must be MQTW. */
    char struct_id[4];
    /** The version number of this structure.  Must be 0 */
    int struct_version;
    /** The LWT topic to which the LWT message will be published. */
    MQTTString topicName;
    /** The LWT payload. */
    MQTTString message;
    /**
      * The retained flag for the LWT message (see MQTTAsync_message.retained).
      */
    unsigned char retained;
    /**
      * The quality of service setting for the LWT message (see
      * MQTTAsync_message.qos and @ref qos).
      */
    char qos;
#if defined(MQTTV5)
    /**
      * LWT properties.
      */
    MQTTProperties* properties;
#endif
} MQTTPacket_willOptions;
CIPop commented 1 year ago

The changes to completely split V5 and V3 are larger than I've originally expected. Closing for now and will follow-up with a separate PR containing this proposal as well as the public API split (together with the removal of test15).