Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking] Moving CONNECT willProperties into MQTTPacket_connectData #245

Closed
wants to merge 14 commits into from

Conversation

CIPop
Copy link

@CIPop CIPop commented Aug 4, 2023

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.

assert("good rc from serialize connect", rc > 0, "rc was %d\n", rc);

rc = MQTTV5Deserialize_connect(&outWillProperties, &outConnectProperties, &data_after, buf, buflen);
data_after.will.properties = &outWillProperties;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is debatable: in previous cases we wouldn't ask for the application to provide a value for out-data. Now this is required in order to provide the memory allocation for properties.

@@ -47,6 +47,9 @@ ADD_EXECUTABLE(
# warning: ‘ftime’ is deprecated [-Wdeprecated-declarations]
target_compile_options(test15 PRIVATE -Wno-deprecated-declarations)

# To allow existing MQTTv3 applications using the MQTTv5 library, MQTTV5 structures must be used.
target_compile_definitions(test15 PRIVATE MQTTV5)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that I'm not very happy about but I'm not sure about the backward compatibility scenario given that MQTTv5 has a few different behaviors.
Proposed V5 client in #244 will likely complicate things even more as more structs will be modified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CIPop we could instead, and is what I think I thought you were originally suggesting, have an entirely separate MQTT 5.0 client library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed: I will update this PR to remove access to v3 APIs when MQTTV5Packet.h is used.

@icraggs
Copy link
Contributor

icraggs commented Aug 8, 2023

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

@CIPop
Copy link
Author

CIPop commented Aug 10, 2023

@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 CIPop marked this pull request as draft August 10, 2023 23:26
@CIPop
Copy link
Author

CIPop commented Aug 11, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants