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.

Sign up for GitHub

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

Replace magic numbers with macros and add explaining comments #249

Merged
merged 5 commits into from
Apr 6, 2023
Merged

Replace magic numbers with macros and add explaining comments #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a0b64b
Replaced magic numbers with macros and added comments
AniruddhaKanhere Apr 4, 2023
7be7702
Fix spell check and build check
AniruddhaKanhere Apr 4, 2023
3a6892f
Fix formatting
AniruddhaKanhere Apr 4, 2023
110006d
Remove macros which are only used once
AniruddhaKanhere Apr 5, 2023
ce3f8b6
Fix formatting and build check
AniruddhaKanhere Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lexicon.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ serializeack
serializeconnect
serializeconnectpacket
serializedisconnect
serailizedlength
serializedlength
serializedpasswordlength
serializedpayloadlength
serializedtopiclength
Expand Down
143 changes: 104 additions & 39 deletions source/core_mqtt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,28 @@
#define MQTT_POST_STATE_UPDATE_HOOK( pContext )
#endif /* !MQTT_POST_STATE_UPDATE_HOOK */

/**
* @brief Bytes required to encode any string length in an MQTT packet header.
* Length is always encoded in two bytes according to the MQTT specification.
*/
#define CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ( 2U )

/**
* @brief Number of vectors required to encode one topic filter in a subscribe
* request. Three vectors are required as there are three fields in the
* subscribe request namely:
* 1. Topic filter length; 2. Topic filter; and 3. QoS in this order.
*/
#define CORE_MQTT_SUBSCRIBE_PER_TOPIC_VECTOR_LENGTH ( 3U )

/**
* @brief Number of vectors required to encode one topic filter in an
* unsubscribe request. Two vectors are required as there are two fields in the
* unsubscribe request namely:
* 1. Topic filter length; and 2. Topic filter in this order.
*/
#define CORE_MQTT_UNSUBSCRIBE_PER_TOPIC_VECTOR_LENGTH ( 2U )

/*-----------------------------------------------------------*/

/**
Expand Down Expand Up @@ -150,7 +172,7 @@ static int32_t sendMessageVector( MQTTContext_t * pContext,
* @brief Add a string and its length after serializing it in a manner outlined by
* the MQTT specification.
*
* @param[in] serailizedLength Array of two bytes to which the vector will point.
* @param[in] serializedLength Array of two bytes to which the vector will point.
* The array must remain in scope until the message has been sent.
* @param[in] string The string to be serialized.
* @param[in] length The length of the string to be serialized.
Expand All @@ -161,7 +183,7 @@ static int32_t sendMessageVector( MQTTContext_t * pContext,
*
* @return The number of vectors added.
*/
static size_t addEncodedStringToVector( uint8_t serailizedLength[ 2 ],
Copy link
Contributor

Choose a reason for hiding this comment

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

A typedef seems like a better choice here.

static size_t addEncodedStringToVector( uint8_t serializedLength[ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ],
const char * const string,
uint16_t length,
TransportOutVector_t * iterator,
Expand Down Expand Up @@ -1833,29 +1855,27 @@ static MQTTStatus_t validateSubscribeUnsubscribeParams( const MQTTContext_t * pC

/*-----------------------------------------------------------*/

static size_t addEncodedStringToVector( uint8_t serailizedLength[ 2 ],
static size_t addEncodedStringToVector( uint8_t serializedLength[ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ],
const char * const string,
uint16_t length,
TransportOutVector_t * iterator,
size_t * updatedLength )
{
size_t packetLength = 0U;
const size_t seralizedLengthFieldSize = 2U;
TransportOutVector_t * pLocalIterator = iterator;
/* This function always adds 2 vectors. */
size_t vectorsAdded = 0U;

/* When length is non-zero, the string must be non-NULL. */
assert( ( length != 0U ) == ( string != NULL ) );

serailizedLength[ 0 ] = ( ( uint8_t ) ( ( length ) >> 8 ) );
serailizedLength[ 1 ] = ( ( uint8_t ) ( ( length ) & 0x00ffU ) );
serializedLength[ 0 ] = ( ( uint8_t ) ( ( length ) >> 8 ) );
Copy link
Contributor

@paulbartell paulbartell Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A macro or static inline helper function could improve clarity here

serializedLength[ 1 ] = ( ( uint8_t ) ( ( length ) & 0x00ffU ) );

/* Add the serialized length of the string first. */
pLocalIterator[ 0 ].iov_base = serailizedLength;
pLocalIterator[ 0 ].iov_len = seralizedLengthFieldSize;
pLocalIterator[ 0 ].iov_base = serializedLength;
pLocalIterator[ 0 ].iov_len = CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES;
vectorsAdded++;
packetLength = seralizedLengthFieldSize;
packetLength = CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES;

/* Sometimes the string can be NULL that is, of 0 length. In that case,
* only the length field should be encoded in the vector. */
Expand All @@ -1882,22 +1902,26 @@ static MQTTStatus_t sendSubscribeWithoutCopy( MQTTContext_t * pContext,
size_t remainingLength )
{
MQTTStatus_t status = MQTTSuccess;
uint8_t subscribeheader[ 7 ];
uint8_t * pIndex;
TransportOutVector_t pIoVector[ MQTT_SUB_UNSUB_MAX_VECTORS ];
TransportOutVector_t * pIterator;
uint8_t serializedTopicFieldLength[ MQTT_SUB_UNSUB_MAX_VECTORS ][ 2 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a 16-bit length type would be a simpler way to handle this?

uint8_t serializedTopicFieldLength[ MQTT_SUB_UNSUB_MAX_VECTORS ][ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ];
size_t totalPacketLength = 0U;
size_t ioVectorLength = 0U;
size_t subscriptionsSent = 0U;
/* For subscribe, only three vector slots are required per topic string. */
const size_t subscriptionStringVectorSlots = 3U;
size_t vectorsAdded;
size_t topicFieldLengthIndex;

/* Maximum number of bytes required by the 'fixed' part of the SUBSCRIBE
* packet header according to the MQTT specification.
* MQTT Control Byte 0 + 1 = 1
* Remaining length (max) + 4 = 5
* Packet ID + 2 = 7 */
uint8_t subscribeheader[ 7U ];

/* The vector array should be at least three element long as the topic
* string needs these many vector elements to be stored. */
assert( MQTT_SUB_UNSUB_MAX_VECTORS >= subscriptionStringVectorSlots );
assert( MQTT_SUB_UNSUB_MAX_VECTORS >= CORE_MQTT_SUBSCRIBE_PER_TOPIC_VECTOR_LENGTH );

pIndex = subscribeheader;
pIterator = pIoVector;
Expand All @@ -1924,10 +1948,10 @@ static MQTTStatus_t sendSubscribeWithoutCopy( MQTTContext_t * pContext,

/* Check whether the subscription topic (with QoS) will fit in the
* given vector. */
while( ( ioVectorLength <= ( MQTT_SUB_UNSUB_MAX_VECTORS - subscriptionStringVectorSlots ) ) &&
while( ( ioVectorLength <= ( MQTT_SUB_UNSUB_MAX_VECTORS - CORE_MQTT_SUBSCRIBE_PER_TOPIC_VECTOR_LENGTH ) ) &&
( subscriptionsSent < subscriptionCount ) )
{
/* The topic filter gets sent next. */
/* The topic filter and the filter length gets sent next. */
vectorsAdded = addEncodedStringToVector( serializedTopicFieldLength[ topicFieldLengthIndex ],
pSubscriptionList[ subscriptionsSent ].pTopicFilter,
pSubscriptionList[ subscriptionsSent ].topicFilterLength,
Expand Down Expand Up @@ -1982,22 +2006,26 @@ static MQTTStatus_t sendUnsubscribeWithoutCopy( MQTTContext_t * pContext,
size_t remainingLength )
{
MQTTStatus_t status = MQTTSuccess;
uint8_t unsubscribeheader[ 7 ];
uint8_t * pIndex;
TransportOutVector_t pIoVector[ MQTT_SUB_UNSUB_MAX_VECTORS ];
TransportOutVector_t * pIterator;
uint8_t serializedTopicFieldLength[ MQTT_SUB_UNSUB_MAX_VECTORS ][ 2 ];
uint8_t serializedTopicFieldLength[ MQTT_SUB_UNSUB_MAX_VECTORS ][ CORE_MQTT_SERIALIZED_LENGTH_FIELD_BYTES ];
size_t totalPacketLength = 0U;
size_t unsubscriptionsSent = 0U;
size_t ioVectorLength = 0U;
/* For unsubscribe, only two vector slots are required per topic string. */
const size_t unsubscribeStringVectorSlots = 2U;
size_t vectorsAdded;
size_t topicFieldLengthIndex;

/* Maximum number of bytes required by the 'fixed' part of the UNSUBSCRIBE
* packet header according to the MQTT specification.
* MQTT Control Byte 0 + 1 = 1
* Remaining length (max) + 4 = 5
* Packet ID + 2 = 7 */
uint8_t unsubscribeheader[ 7U ];

/* The vector array should be at least three element long as the topic
* string needs these many vector elements to be stored. */
assert( MQTT_SUB_UNSUB_MAX_VECTORS >= unsubscribeStringVectorSlots );
assert( MQTT_SUB_UNSUB_MAX_VECTORS >= CORE_MQTT_UNSUBSCRIBE_PER_TOPIC_VECTOR_LENGTH );

pIndex = unsubscribeheader;
pIterator = pIoVector;
Expand All @@ -2023,7 +2051,7 @@ static MQTTStatus_t sendUnsubscribeWithoutCopy( MQTTContext_t * pContext,
topicFieldLengthIndex = 0;

/* Check whether the subscription topic will fit in the given vector. */
while( ( ioVectorLength <= ( MQTT_SUB_UNSUB_MAX_VECTORS - unsubscribeStringVectorSlots ) ) &&
while( ( ioVectorLength <= ( MQTT_SUB_UNSUB_MAX_VECTORS - CORE_MQTT_UNSUBSCRIBE_PER_TOPIC_VECTOR_LENGTH ) ) &&
( unsubscriptionsSent < subscriptionCount ) )
{
/* The topic filter gets sent next. */
Expand Down Expand Up @@ -2069,11 +2097,20 @@ static MQTTStatus_t sendPublishWithoutCopy( MQTTContext_t * pContext,
uint16_t packetId )
{
MQTTStatus_t status = MQTTSuccess;
uint8_t serializedPacketID[ 2 ];
TransportOutVector_t pIoVector[ 4 ];
size_t ioVectorLength;
size_t totalMessageLength;
const size_t packetIDLength = 2U;

/* Bytes required to encode the packet ID in an MQTT header according to
* the MQTT specification. */
uint8_t serializedPacketID[ 2U ];

/* Maximum number of vectors required to encode and send a publish
* packet. The breakdown is shown below.
* Fixed header (including topic string length) 0 + 1 = 1
* Topic string + 1 = 2
* Packet ID (only when QoS > QoS0) + 1 = 3
* Payload + 1 = 4 */
TransportOutVector_t pIoVector[ 4U ];

/* The header is sent first. */
pIoVector[ 0U ].iov_base = pMqttHeader;
Expand All @@ -2096,10 +2133,10 @@ static MQTTStatus_t sendPublishWithoutCopy( MQTTContext_t * pContext,
serializedPacketID[ 1 ] = ( ( uint8_t ) ( ( packetId ) & 0x00ffU ) );

pIoVector[ ioVectorLength ].iov_base = serializedPacketID;
pIoVector[ ioVectorLength ].iov_len = packetIDLength;
pIoVector[ ioVectorLength ].iov_len = sizeof( serializedPacketID );

ioVectorLength++;
totalMessageLength += packetIDLength;
totalMessageLength += sizeof( serializedPacketID );
}

/* Publish packets are allowed to contain no payload. */
Expand Down Expand Up @@ -2132,19 +2169,37 @@ static MQTTStatus_t sendConnectWithoutCopy( MQTTContext_t * pContext,
size_t ioVectorLength = 0U;
size_t totalMessageLength = 0U;
int32_t bytesSentOrError;

/* Connect packet header can be of maximum 15 bytes. */
uint8_t connectPacketHeader[ 15 ];
uint8_t * pIndex = connectPacketHeader;
TransportOutVector_t pIoVector[ 11 ];
uint8_t * pIndex;
uint8_t serializedClientIDLength[ 2 ];
uint8_t serializedTopicLength[ 2 ];
uint8_t serializedPayloadLength[ 2 ];
uint8_t serializedUsernameLength[ 2 ];
uint8_t serializedPasswordLength[ 2 ];
size_t vectorsAdded;

/* Maximum number of bytes required by the 'fixed' part of the CONNECT
* packet header according to the MQTT specification.
* MQTT Control Byte 0 + 1 = 1
* Remaining length (max) + 4 = 5
* Protocol Name Length + 2 = 7
* Protocol Name (MQTT) + 4 = 11
* Protocol level + 1 = 12
* Connect flags + 1 = 13
* Keep alive + 2 = 15 */
uint8_t connectPacketHeader[ 15U ];

/* The maximum vectors required to encode and send a connect packet. The
* breakdown is shown below.
* Fixed header 0 + 1 = 1
* Client ID + 2 = 3
* Will topic + 2 = 5
* Will payload + 2 = 7
* Username + 2 = 9
* Password + 2 = 11 */
TransportOutVector_t pIoVector[ 11U ];

iterator = pIoVector;
pIndex = connectPacketHeader;

/* Validate arguments. */
if( ( pWillInfo != NULL ) && ( pWillInfo->pTopicName == NULL ) )
Expand All @@ -2159,7 +2214,7 @@ static MQTTStatus_t sendConnectWithoutCopy( MQTTContext_t * pContext,
pWillInfo,
remainingLength );

assert( ( pIndex - connectPacketHeader ) <= 15 );
assert( ( pIndex - connectPacketHeader ) <= sizeof( connectPacketHeader ) );

/* The header gets sent first. */
iterator->iov_base = connectPacketHeader;
Expand Down Expand Up @@ -2754,9 +2809,18 @@ MQTTStatus_t MQTT_Publish( MQTTContext_t * pContext,
MQTTPublishState_t publishStatus = MQTTStateNull;
bool stateUpdateHookExecuted = false;

/* 1 header byte + 4 bytes (maximum) required for encoding the length +
* 2 bytes for topic string. */
uint8_t mqttHeader[ 7 ];
/* Maximum number of bytes required by the 'fixed' part of the PUBLISH
* packet header according to the MQTT specifications.
* Header byte 0 + 1 = 1
* Length (max) + 4 = 5
* Topic string length + 2 = 7
*
* Note that since publish is one of the most common operations in MQTT
* connection, we have moved the topic string length to the 'fixed' part of
* the header so efficiency. Otherwise, we would need an extra vector and
* an extra call to 'send' (in case writev is not defined) to send the
* topic length. */
uint8_t mqttHeader[ 7U ];

/* Validate arguments. */
MQTTStatus_t status = validatePublishParams( pContext, pPublishInfo, packetId );
Expand Down Expand Up @@ -2862,7 +2926,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext )
MQTTFixedBuffer_t localBuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MQTTFixedBuffer_t localBuffer;
uint8_t pingreqPacket[ MQTT_PACKET_PINGREQ_SIZE ];


localBuffer.pBuffer = pingreqPacket;
localBuffer.size = 2U;
localBuffer.size = sizeof( pingreqPacket );

if( pContext == NULL )
{
Expand All @@ -2877,6 +2941,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext )

Copy link
Contributor

Choose a reason for hiding this comment

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

Line above isnt necessary.

if( status == MQTTSuccess )
{
assert( packetSize == localBuffer.size );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert( packetSize == localBuffer.size );

LogDebug( ( "MQTT PINGREQ packet size is %lu.",
( unsigned long ) packetSize ) );
}
Expand Down Expand Up @@ -2904,7 +2969,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext )
* from the user provided buffers. Thus it can be sent directly. */
sendResult = sendBuffer( pContext,
localBuffer.pBuffer,
2U );
packetSize );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
packetSize );
MQTT_PACKET_PINGREQ_SIZE );


/* Give the mutex away. */
MQTT_POST_SEND_HOOK( pContext );
Expand Down