MQTT - handling re-sends when a session is reestablished. #980
Conversation
There was a problem hiding this comment.
The complexity reduction in this PR seems premature, since a lot of it is already handled by other PRs. This will likely make it more time-consuming when dealing with merge conflicts later. I'd suggest reverting those changes for now and handling them in a follow up PR to minimize merge conflicts
oh ok. If it is handled in a different way in another PRs, I agree with your comment that we should come back to it a after merging those changes. Otherwise merges will be difficult. I will revert my refactoring after taking a look at the changes in other PRs |
leegeth
changed the title
| { | ||
| LogDebug( ( "DUP is 0." ) ); | ||
| } | ||
| pPublishInfo->dup = UINT8_CHECK_BIT( publishFlags, MQTT_PUBLISH_FLAG_DUP ); |
There was a problem hiding this comment.
MISRA: Add a ? true : false; to the end of this. Reasoning here: #993 (comment)
| } | ||
| else | ||
| { | ||
| records = ( incomingPublish ) ? pMqttContext->incomingPublishRecords |
There was a problem hiding this comment.
MISRA:
| records = ( incomingPublish ) ? pMqttContext->incomingPublishRecords | |
| records = ( incomingPublish == true ) ? pMqttContext->incomingPublishRecords |
| else | ||
| { | ||
| /*This is considered as a new incoming publish. */ | ||
| } |
There was a problem hiding this comment.
Don't really think we need this else here
| { | ||
| MQTTStatus_t mqttStatus = MQTTSuccess; | ||
| MQTTPacketInfo_t incomingPacket = { 0 }; | ||
| size_t pingreqSize = MQTT_PACKET_PINGREQ_SIZE; | ||
| bool expectMoreCalls = true; | ||
| bool hasIncomingPacketAlreadyReceived = false, isStateUpdateNeededAfterSendingAck = true; |
There was a problem hiding this comment.
Still not a fan of these long names. What's wrong with packetAlreadyReceived and updateAfterAck?
|
|
||
| if( newState == MQTTStateNull ) | ||
| /* Only update the state machine if the state update is required. */ | ||
| if( stateNeedUpdate ) |
There was a problem hiding this comment.
MISRA:
| if( stateNeedUpdate ) | |
| if( stateNeedUpdate == true ) |
| /* Resend pending PUBREL. */ | ||
| status = sendPublishAcks( pContext, | ||
| packetId, | ||
| &publishRecordState, |
There was a problem hiding this comment.
Do we expect sendPublishAcks to update publishRecordState? In that case, shouldn't it be reset to MQTTPubRelSend in every iteration of the loop?
| assert( pContext != NULL ); | ||
| assert( state == MQTTPubCompPending || state == MQTTPubRelSend ); | ||
|
|
||
| /* Resending PUBREL packet. */ | ||
| MQTTPublishState_t publishRecordState = MQTTPubRelSend; |
There was a problem hiding this comment.
I think variables need to be declared before asserts to conform to our style
|
|
||
| /* TODO. Need to remove this update once MQTT_UpdateStatePublish is | ||
| * refactored with return type of MQTTStatus_t. */ | ||
| status = MQTTBadParameter; |
There was a problem hiding this comment.
I think MQTTIllegalState is more appropriate here than MQTTBadParameter
| if( pPublishInfo->qos > MQTTQoS0 ) | ||
| /* Reserve state for publish message. Only to be done for QoS1 or QoS2 | ||
| * and a non duplicate publish. */ | ||
| if( ( pPublishInfo->qos > MQTTQoS0 ) && ( !pPublishInfo->dup ) ) |
There was a problem hiding this comment.
MISRA:
| if( ( pPublishInfo->qos > MQTTQoS0 ) && ( !pPublishInfo->dup ) ) | |
| if( ( pPublishInfo->qos > MQTTQoS0 ) && ( pPublishInfo->dup == false ) ) |
| @@ -1410,6 +1875,7 @@ MQTTStatus_t MQTT_ProcessLoop( MQTTContext_t * const pContext, | |||
| { | |||
| /* Receive packet. Remaining time is recalculated at the end of the loop. */ | |||
| status = receivePacket( pContext, incomingPacket, remainingTimeMs ); | |||
| LogDebug( ( "After receiving packet with status %u.", status ) ); | |||
There was a problem hiding this comment.
Reword to something like "Packet reception concluded with result ..."
|
/bot run checks |
|
Closing this PR as this change is refactored and handled in a different approach in PR #1007. |
Description of changes:
Handling the resend and receive of duplicate packets when an MQTT session is reestablished.
These are the cases that is handled in this PR,
When a session is reestablished,
Outgoing publishes:
Incoming publishes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.