Skip to content

Conversation

sarenameas
Copy link
Contributor

@sarenameas sarenameas commented Aug 6, 2020

Description of changes:
For demonstration purposes MQTT_PublishToResend() is now used in the mqtt_basic_tls_demo and the mqtt_mutual_auth_demo. These demos use a Qos > 0.

Extra changes:
Updated the README.md in tools/spell for clarity on how to use the scripts.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2020

Codecov Report

Merging #1101 into development will increase coverage by 1.77%.
The diff coverage is 92.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1101      +/-   ##
===============================================
+ Coverage        96.54%   98.31%   +1.77%     
===============================================
  Files                9        4       -5     
  Lines             5643     1189    -4454     
  Branches           641      369     -272     
===============================================
- Hits              5448     1169    -4279     
+ Misses               9        0       -9     
+ Partials           186       20     -166     
Impacted Files Coverage Δ
libraries/standard/mqtt/src/mqtt_lightweight.c 96.92% <ø> (+2.40%) ⬆️
libraries/standard/mqtt/src/mqtt_state.c 98.42% <ø> (+2.48%) ⬆️
libraries/standard/http/src/http_client.c 98.53% <80.00%> (+11.05%) ⬆️
libraries/standard/mqtt/src/mqtt.c 99.39% <100.00%> (+4.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a292f5...93bc5ee. Read the comment docs.

@@ -51,6 +51,7 @@

/* MQTT API header. */
Copy link
Contributor

Choose a reason for hiding this comment

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

headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

leegeth
leegeth previously approved these changes Aug 6, 2020
Copy link
Contributor

@aggarw13 aggarw13 left a comment

Choose a reason for hiding this comment

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

I'm generally concerned that the use of MQTT_PublishToResend seems superfluous when the outgoingPublishPackets array already does the same work of maintaining PUBLISHes for resending?

We probably should show MQTT_PublishToResend in a way that makes its utility more meaningful as the only way of storing PUBLISH packet ID order; otherwise, the use of the utility carries no need for the customer...

Initialize packetIdTo resend to the invalid one.

Co-authored-by: Archit Aggarwal <[email protected]>
Update 0 to MQTT_PACKET_ID_INVALID

Co-authored-by: Archit Aggarwal <[email protected]>
@sarenameas
Copy link
Contributor Author

sarenameas commented Aug 6, 2020

I'm generally concerned that the use of MQTT_PublishToResend seems superfluous when the outgoingPublishPackets array already does the same work of maintaining PUBLISHes for resending?

We probably should show MQTT_PublishToResend in a way that makes its utility more meaningful as the only way of storing PUBLISH packet ID order; otherwise, the use of the utility carries no need for the customer...

What do you suggest? My understanding is that the application must still own their publish packets and associate them packet IDs.

@aggarw13
Copy link
Contributor

aggarw13 commented Aug 6, 2020

I'm generally concerned that the use of MQTT_PublishToResend seems superfluous when the outgoingPublishPackets array already does the same work of maintaining PUBLISHes for resending?

We probably should show MQTT_PublishToResend in a way that makes its utility more meaningful as the only way of storing PUBLISH packet ID order; otherwise, the use of the utility carries no need for the customer...

What do you suggest? My understanding is that the application must still own their publish packets and associate them packet IDs.

I spoke with @leegeth separately, and after some thinking through, we realized that the outgoingPublishPackets array in the current implementation actually does not preserve the message ordering. Thus, using the MQTT_PublishToResend is a must in these demo applications. Thus, we need to provide the correct documentation to suggest the value using this utility for preserving the message ordering

@sarenameas sarenameas merged commit edf5af8 into aws:development Aug 7, 2020
@sarenameas sarenameas deleted the mqtt/publishtoresend branch August 7, 2020 06:30
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 27, 2020
…s#1101)

MQTT_PublishToResend() is now used in the mqtt_basic_tls_demo and the mqtt_mutual_auth_demo because it preserves the ordering of MQTT publish messages. These demos use a Qos > 0.

Updated the README.md in tools/spell for clarity on how to use the scripts.
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 28, 2020
…s#1101)

MQTT_PublishToResend() is now used in the mqtt_basic_tls_demo and the mqtt_mutual_auth_demo because it preserves the ordering of MQTT publish messages. These demos use a Qos > 0.

Updated the README.md in tools/spell for clarity on how to use the scripts.
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 31, 2020
…s#1101)

MQTT_PublishToResend() is now used in the mqtt_basic_tls_demo and the mqtt_mutual_auth_demo because it preserves the ordering of MQTT publish messages. These demos use a Qos > 0.

Updated the README.md in tools/spell for clarity on how to use the scripts.
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Sep 1, 2020
…s#1101)

MQTT_PublishToResend() is now used in the mqtt_basic_tls_demo and the mqtt_mutual_auth_demo because it preserves the ordering of MQTT publish messages. These demos use a Qos > 0.

Updated the README.md in tools/spell for clarity on how to use the scripts.
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.

5 participants