MQTTv5 Implementation #316
Conversation
| @@ -74,6 +74,14 @@ | |||
| */ | |||
| #define CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ( 2U ) | |||
|
|
|||
| #define UINT8_SET_BIT( x, position ) ( ( x ) = ( uint8_t ) ( ( x ) | ( 0x01U << ( position ) ) ) ) | |||
| #define UINT16_HIGH_BYTE( x ) ( ( uint8_t ) ( ( x ) >> 8 ) ) | |||
There was a problem hiding this comment.
It would be good if we added comments at the top of these macros explaining in breif waht they do. Similar to how it is done for other similar macros.
| /** | ||
| * @brief Get the 4th byte of a 32-bit unsigned integer. | ||
| */ | ||
| #define UINT32_BYTE3( x ) ( ( uint8_t ) ( ( x ) >> 24 ) ) |
There was a problem hiding this comment.
Is there any specific reason why we did not choose to use a macro like: UINT32_GET_BYTE( x, byteNumber ) = ( ( uint8_t ) ( ( x ) >> 8 * byteNumber ) ) but made separate macros for every byte position?
source/core_mqtt.c
Outdated
| /** | ||
| * @ingroup mqtt_constants | ||
| * @brief The size of MQTT PUBACK, PUBREC, PUBREL, and PUBCOMP packets with reason code, packet id. | ||
| */ |
There was a problem hiding this comment.
NIT: Indentation of the comments needs to be rectifed.
| /** | ||
| * @brief Struct used to deserialize the will properties. | ||
| * | ||
| **/ |
There was a problem hiding this comment.
This might be a copy-pasted comment from the WillPropertiesVector struct. Please correct the description of this struct accordingly.
| @@ -347,7 +576,7 @@ static MQTTStatus_t handleKeepAlive( MQTTContext_t * pContext ); | |||
| * | |||
| * @return MQTTSuccess, MQTTIllegalState or deserialization error. | |||
| */ | |||
| static MQTTStatus_t handleIncomingPublish( MQTTContext_t * pContext, | |||
| MQTTStatus_t handleIncomingPublish( MQTTContext_t * pContext, | |||
There was a problem hiding this comment.
I see that this function is used just in this file. Is there any specific reason why we made it non-static?
| /** | ||
| * @brief Utf 8 encoded string has 2 byte length field and 1 byte property id. | ||
| */ | ||
| #define MQTT_UTF8_LENGTH_SIZE ( 3U ) |
There was a problem hiding this comment.
It would be good if we keep all the macro definitions in one place. Consider moving these macro definitions above the function declarations where all the macros are defined.
| * @param[out] pProperty To store the decoded property. | ||
| * @param[out] pPropertyLength Size of the length. | ||
| * @param[out] pUsed Whether the property is decoded befire. | ||
| * @param[out] pIndex Pointer to the current index of the buffer. |
There was a problem hiding this comment.
Please rectify the out/in types for the parameters in the description, fix typo for "before" in the third param and make the description of the first param clearer (I suppose it was supposed to be "Pointer to store the decoded property"). Similarly for other such decode functions.
| const MQTTSubscribeInfo_t * pSubscriptionList, | ||
| size_t subscriptionCount, | ||
| uint16_t packetId ); | ||
|
|
There was a problem hiding this comment.
If the function is not used anymore remove its descriptions as well.
| packetSize = 1U + remainingLengthEncodedSize( remainingLength ) | ||
| + remainingLength; | ||
| connectPacketSize += propLen; | ||
| connectPacketSize += remainingLengthEncodedSize(propLen); |
There was a problem hiding this comment.
Should we consider changing the name of this function to variableLengthEncodedSize?
| * 268,435,455 - max remaining length according to spec MQTT-v5; | ||
| * | ||
| * */ | ||
| if( remainingLength > MQTT_MAX_REMAINING_LENGTH ) |
There was a problem hiding this comment.
We can add a comment here - "It is possible that the remaining length becomes more than the maximum allowed by the MQTTV5-Spec, i.e. 268,435,455. This is because the user may enter a large number of user properties for the connect packet and/or the last will. Hence we need to have a check for this case."
Upgrading MQTT Library to support both v3.1.1 and v5 features
Description
Test Steps
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.