Mqttv5 connect #294

Pooja-Toshniwal · 2024-06-25T07:22:37Z

MQTTV5 Connect and Connack

Description

Introduced a config variable MQTT_VERSION_5_ENABLED, which enables the users to use version 5 of MQTT. To support the new properties of connect and connack packet introduced in the version 5, I have modified the existing data structures and functions, and added some new functions. For unit testing I have modified the existing folder structure of test to create a separate folder for v5.

Test Steps

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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.

Pooja-Toshniwal · 2024-06-30T14:34:31Z

/bot run uncrustify

amazonKamath · 2024-07-01T09:31:50Z

test/unit-test/MQTTv5/core_mqtt_config.h

+ *
+ * These unit tests expect retrying only twice.
+ */
+#define MQTT_MAX_CONNACK_RECEIVE_RETRY_COUNT    ( 2U )


Need to reduce duplication with MQTT3 UT folder.

amazonKamath · 2024-07-01T10:00:50Z

test/unit-test/MQTTv5/core_mqttv5_serializer_utest.c

+
+void test_MQTTV5_DeserializeConnackOnlyMaxQos( void )
+{
+    MQTTPacketInfo_t packetInfo;


I think there is duplication of the first few lines of code across the deserialize tests. Can you verify and see if duplication exists and can be wrapped in an helper function?

The common code and initialisation for each test case can be moved to "void setUp( void )"
Which is use to setup for each test case.

Check this for example

amazonKamath · 2024-07-01T10:13:16Z

test/unit-test/MQTTv5/core_mqttv5_serializer_utest.c

+    /*
+     * Max Packet Size cannot be null
+     */
+    properties.receiveMax = 24;


what about tests for other properties like session expiry? need tests for max bounds as well I guess? I think MQTT the GetConnectPropertiesSize needs further checks for testing MAX values of sessionexpiry, receive max and maxpacket size

Max possible value of session expiry is uint32 max, receive max is uint16 max and max packet size is 268,435,456 bytes but even if we set it beyond that it doesn't matter because it is theoretically impossible for max packet size to be more than this because of the limitation of the encoded remaining length.

moninom1 · 2024-07-02T05:25:00Z

source/include/core_mqtt.h

@@ -41,6 +41,8 @@
 /* Include transport interface. */
 #include "transport_interface.h"

+/* Include transport interface. */


Can be removed

moninom1 · 2024-07-02T05:26:29Z

source/core_mqtt.c

+ * @return The number of vectors added.
+ */
+
+    static size_t addEncodedStringToVectorWithId( uint8_t serializedLength[ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ],


Move all the static function declaration to the top of the file, same as "static int32_t sendBuffer"

I guess what Monika is suggesting is to declare this function first here and then define it down where other static functions are defined.

moninom1 · 2024-07-02T11:13:57Z

source/core_mqtt.c

+         * Connect flags            + 1 = 13
+         * Keep alive               + 2 = 15 */
+        uint8_t connectPacketHeader[ 15U ];
+    #else


Please move out the common code.

moninom1 · 2024-07-02T11:54:47Z

source/include/core_mqtt_serializer.h

@@ -194,6 +246,200 @@ typedef struct MQTTSubscribeInfo
    uint16_t topicFilterLength;
 } MQTTSubscribeInfo_t;



The code below should also be encapsulated within MQTT_VERSION_5_ENABLED flag?

Pooja-Toshniwal · 2024-07-03T09:07:07Z

/bot run uncrustify

moninom1 · 2024-07-03T11:00:59Z

source/include/core_mqtt_serializer.h

                          incomplete data; it should be called again (probably after
                          a delay). */
+
+    MQTTMalformedPacket=0x81,


Add description here as added for above options.

moninom1 · 2024-07-03T11:15:01Z

test/unit-test/MQTT/core_mqtt_serializer_utest.c

@@ -2878,4 +2878,5 @@ void test_MQTT_SerializeDisconnect_Happy_Path()
    TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
 }



Remove this as we are not updating this file.

moninom1 · 2024-07-03T11:15:53Z

test/unit-test/MQTT/core_mqtt_utest.c

@@ -1116,6 +1116,7 @@ void test_MQTT_Init_Happy_Path( void )
    TEST_ASSERT_EQUAL_MEMORY( &networkBuffer, &context.networkBuffer, sizeof( networkBuffer ) );
 }

+


Remove all extra lines added or removed which are not needed.

shubnil

Hi Pooja,
Thanks for the changes.
The PR looks pretty clean now. I can see that you have taken care of most of the comments from the last team review.
I have some minor comments and some clarifications.

shubnil · 2024-07-05T11:16:27Z

source/core_mqtt.c

+ * @brief Struct used to deserialize the will properties.
+ *
+ **/
+        typedef struct  UserPropertiesVector


Please left align this structure definition.

shubnil · 2024-07-05T11:52:48Z

source/core_mqtt.c

+ * @return The number of vectors added.
+ */
+
+    static size_t addEncodedStringToVectorWithId( uint8_t serializedLength[ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ],


I guess what Monika is suggesting is to declare this function first here and then define it down where other static functions are defined.

shubnil · 2024-07-05T11:54:13Z

source/core_mqtt.c

+ * @return The number of vectors added.
+ */
+
+    static size_t sendUserProperties( const MQTTUserProperties_t * pUserProperty,


Please follow the above comment for all static functions.

shubnil · 2024-07-05T15:07:32Z

source/core_mqtt_serializer.c

+            *pProperty = UINT16_DECODE( pVariableHeader );
+            pVariableHeader = &pVariableHeader[ sizeof( uint16_t ) ];
+            *pUsed = true;
+            *pPropertyLength -= sizeof( uint16_t );


Will there be a scenario where pPropertyLength can be greater than sizeof( uint16_t ). Same is true for all similar cases in the other decode functions

shubnil · 2024-07-05T15:11:02Z

source/core_mqtt_serializer.c

+    }
+
+    static MQTTStatus_t decodeutf_8( const char ** pProperty,
+                                     uint16_t * pLength,


Why we need the extra argument of pLength in this decode function while not in others. If we really need "pLength", then will the check in Line 1699 be against "pLength" instead of "sizeof( uint16_t )"

It is used to store the length of the UTF 8 string. Before decoding, property length is compared with 2 byte for validation and then length is decoded.

moninom1 · 2024-07-08T04:53:35Z

source/core_mqtt.c

@@ -2678,7 +3175,7 @@ MQTTStatus_t MQTT_CancelCallback( const MQTTContext_t * pContext,

 MQTTStatus_t MQTT_Connect( MQTTContext_t * pContext,
                           const MQTTConnectInfo_t * pConnectInfo,
-                           const MQTTPublishInfo_t * pWillInfo,


I see const is removed because we are updating pWillInfo->propertyLength, why is this specific update is needed for v5?

moninom1 · 2024-07-08T05:00:09Z

source/core_mqtt_serializer.c

+        else
+        {
+            /*Add authentication method and data if provided*/
+            if( pAuthInfo->authMethodLength != 0U )


Why do we need authMethodLength & authDataLength checks again? As they are already done above. Adding extra if checks.

moninom1 · 2024-07-08T15:20:46Z

source/core_mqtt_serializer.c

+        pIndexLocal = encodeRemainingLength( pIndexLocal, pConnectProperties->propertyLength );
+
+        /*Serialize session expiry if provided.*/
+        if( pConnectProperties->sessionExpiry != 0U )


Multiple of these if's can be true? if not, we should use if else.

moninom1 · 2024-07-08T15:28:48Z

source/core_mqtt_serializer.c

+            pIndexLocal[ 1 ] = UINT32_BYTE2( pPublishInfo->msgExpiryInterval );
+            pIndexLocal[ 2 ] = UINT32_BYTE1( pPublishInfo->msgExpiryInterval );
+            pIndexLocal[ 3 ] = UINT32_BYTE0( pPublishInfo->msgExpiryInterval );
+            pIndexLocal = &pIndexLocal[ 4 ];


Why are we incrementing the pointer in this different way than usual? Any reference to the existing code?

moninom1 · 2024-07-08T15:32:04Z

source/core_mqtt_serializer.c

+    uint8_t * MQTT_SerializePublishProperties( const MQTTPublishInfo_t * pPublishInfo,
+                                               uint8_t * pIndex )
+    {
+        uint8_t * pIndexLocal = pIndex;


How are we ensuring pIndexLocal or pIndex has enough space to do all the writes below?

Pooja-Toshniwal and others added 30 commits June 13, 2024 00:01

Changes in connect and connack

402eedd

Minor changes

f9a3588

For loop

bdef8fe

Minor changes

ef6e950

add

92bc1e5

ahuh

df5d948

encodedString

6ca485d

working

247c2bd

V5 unit tests

7617537

v3 error

19e8da8

testing serialize functions

a3860d8

Serialize api

e8d6048

Comments and deserialize test

dd04115

test

8890f4d

testing

9404fa8

Fixed C90 errors

897379f

coverage

01e0887

test

b9b47e4

njkj

8a27fd4

100%

90547d4

misra

ee0df66

misra

650fbb4

misra

8b7a9d7

misra

30091df

nj

543ec12

correct propertylength -2

0d4196d

Delete CMakeFiles directory

9c4e3e4

Delete cov-out directory

0682349

MQTTV5 connect and connack

ee61178

MQTTV5 connect and connack

4a9145e

Ubuntu and others added 9 commits June 25, 2024 09:59

MQTTV5 connect and connack

9d317a7

MQTTV5 connect and connack

4b8d616

MQTTV5 connect and connack

0681b4d

MQTTV5 connect and connack

1cf3ff2

MQTTV5 connect and connack

5087b3a

Uncrustify: triggered by comment.

4a194f3

MQTTV5 connect and connack

86cbfab

MQTTV5 connect and connack

5b3a53e

MQTTV5 CONNECT AND CONNACK

5d71643

actions-user and others added 2 commits June 30, 2024 14:35

Uncrustify: triggered by comment.

fcda31f

MQTTV5 CONNECT AND CONNACK

fa33b48

amazonKamath requested changes Jul 1, 2024

View reviewed changes

moninom1 reviewed Jul 2, 2024

View reviewed changes

Ubuntu and others added 4 commits July 3, 2024 07:49

MQTTV5 COnenct and connack

29d9bac

Uncrustify: triggered by comment.

e43dc29

Merge branch 'main' into MQTTV5_CONNECT

8b60997

Mqttv5 connect and connack

a14676b

Uncrustify: triggered by comment.

e81e068

moninom1 reviewed Jul 3, 2024

View reviewed changes

shubnil suggested changes Jul 5, 2024

View reviewed changes

moninom1 reviewed Jul 8, 2024

View reviewed changes

Pooja-Toshniwal closed this Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mqttv5 connect #294

Mqttv5 connect #294

Pooja-Toshniwal commented Jun 25, 2024 •

edited

Loading

Pooja-Toshniwal commented Jun 30, 2024

amazonKamath Jul 1, 2024

amazonKamath Jul 1, 2024

moninom1 Jul 2, 2024 •

edited

Loading

moninom1 Jul 2, 2024

amazonKamath Jul 1, 2024

Pooja-Toshniwal Jul 1, 2024 •

edited

Loading

moninom1 Jul 2, 2024

moninom1 Jul 2, 2024

shubnil Jul 5, 2024

moninom1 Jul 2, 2024 •

edited

Loading

moninom1 Jul 2, 2024 •

edited

Loading

Pooja-Toshniwal commented Jul 3, 2024

moninom1 Jul 3, 2024

moninom1 Jul 3, 2024

moninom1 Jul 3, 2024

shubnil left a comment •

edited

Loading

shubnil Jul 5, 2024

shubnil Jul 5, 2024

shubnil Jul 5, 2024

shubnil Jul 5, 2024

shubnil Jul 5, 2024

Pooja-Toshniwal Jul 7, 2024

moninom1 Jul 8, 2024

moninom1 Jul 8, 2024

moninom1 Jul 8, 2024 •

edited

Loading

moninom1 Jul 8, 2024

moninom1 Jul 8, 2024

		@@ -194,6 +246,200 @@ typedef struct MQTTSubscribeInfo
		uint16_t topicFilterLength;
		} MQTTSubscribeInfo_t;

		@@ -2878,4 +2878,5 @@ void test_MQTT_SerializeDisconnect_Happy_Path()
		TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
		}

		@@ -1116,6 +1116,7 @@ void test_MQTT_Init_Happy_Path( void )
		TEST_ASSERT_EQUAL_MEMORY( &networkBuffer, &context.networkBuffer, sizeof( networkBuffer ) );
		}

Mqttv5 connect #294

Mqttv5 connect #294

Conversation

Pooja-Toshniwal commented Jun 25, 2024 • edited Loading

Description

Test Steps

Checklist:

Related Issue

Pooja-Toshniwal commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moninom1 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pooja-Toshniwal Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moninom1 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

moninom1 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Pooja-Toshniwal commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubnil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moninom1 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pooja-Toshniwal commented Jun 25, 2024 •

edited

Loading

moninom1 Jul 2, 2024 •

edited

Loading

Pooja-Toshniwal Jul 1, 2024 •

edited

Loading

moninom1 Jul 2, 2024 •

edited

Loading

moninom1 Jul 2, 2024 •

edited

Loading

shubnil left a comment •

edited

Loading

moninom1 Jul 8, 2024 •

edited

Loading