[PR from CLI tool] Add MQTT demo for connection sharing with subscription manager #1098

aggarw13 · 2020-08-05T21:26:52Z

MQTT demo for using subscription manager when sharing MQTT connection among multiple subscription topics

Description of changes:

Add demo to showcase use of subscription manager for registering subscription callbacks in a single threaded demo
Relocate subscription manager within the demo based on new decision about providing subscription manager specific references to demos (instead of a generic component utility that applies for all demos)
Remove all topic matching functions from subscription manager, and instead update it to use the MQTT_MatchTopic function (that is added in [PR from CLI tool] Add utility for MQTT topic filter and topic matching to library #1119)

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

…; update config files and CMakeLists.txt for demo

…T_PATH macro with absolute path of downloaded certificate, and update demo_config files to have #ifndef guards around the macro

codecov-commenter · 2020-08-05T21:37:58Z

Codecov Report

Merging #1098 into development will increase coverage by 2.21%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development    #1098      +/-   ##
===============================================
+ Coverage        96.54%   98.76%   +2.21%     
===============================================
  Files                9        4       -5     
  Lines             5643     1371    -4272     
  Branches           641      418     -223     
===============================================
- Hits              5448     1354    -4094     
+ Misses               9        0       -9     
+ Partials           186       17     -169

Impacted Files	Coverage Δ
libraries/standard/mqtt/src/mqtt.c	`99.10% <ø> (+4.69%)`	⬆️
libraries/standard/mqtt/src/mqtt_lightweight.c	`97.42% <ø> (+2.90%)`	⬆️
libraries/standard/mqtt/src/mqtt_state.c	`98.46% <ø> (+2.52%)`	⬆️
libraries/standard/http/src/http_client.c	`100.00% <100.00%> (+12.51%)`	⬆️

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 1aed50a...6be56c9. Read the comment docs.

…o/mqtt-connection-sharing

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

…t-device-sdk-embedded-C into add-subscription-manager

Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>

…ssivity

… functions based on review feedback

…ring

Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

muneebahmed10 · 2020-08-18T18:35:38Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+
+/*-----------------------------------------------------------*/
+
+static int publishToTopic( MQTTContext_t * pMqttContext,


Since this function now not only publishes to a topic, but receives publishes from the broker, its name should be updated

This function is only used for sending publishes to broker, and not for receiving incoming PUBLISHes

On line 1186, you use this function for receiving incoming publishes with the call to MQTT_ProcessLoop(). Otherwise, the check on line 1283 for receiving the temperature data would fail.

Ahh, okay good point! Have renamed the function to publishToTopicAndProcessIncomingMessage

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

muneebahmed10 · 2020-08-18T19:04:33Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+
+        /* Process Incoming UNSUBACK packet from the broker. */
+        mqttStatus = MQTT_ProcessLoop( pMqttContext, MQTT_PROCESS_LOOP_TIMEOUT_MS );
+


Would it be useful to have a global receivedUnsuback flag? If we do, we don't have to wait the entire timeout and can just wait until we receive the unsuback. If not, I don't really see why this function needs to call the process loop. Since we have three consecutive unsubscribeFromTopic calls, It would save 2 * MQTT_PROCESS_LOOP_TIMEOUT_MS milliseconds If we only called the process loop once after all three calls.

I can instead unsubscribe from all topic filters in the same UNSUBSCRIBE request

Have updated the private function to take in a list of topic filters to unsubscribe with a single packet.

…ection-sharing

libraries/standard/mqtt/lexicon.txt

libraries/standard/mqtt/src/mqtt.c

muneebahmed10 · 2020-08-20T06:30:01Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+ * publish to.
+ *
+ * The topic name starts with the client identifier to ensure that each demo
+ * interacts with a unique topic name.


I'm a little confused by this, so maybe this comment could have more clarification. Why does each demo need a unique topic name, and how does the client identifier make it unique? If it's for the case of a customer choosing to use a public broker like test.mosquitto.org instead of a local broker, we could mention that's the reason.

It's a copy-pasta from other demos. Now, that we have moved away from using the public broker, I agree that we can avoid this comment

I prefer that we don't mention about the test.mosquitto.org due to its reliability concerns.

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

muneebahmed10 · 2020-08-20T06:43:51Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+    }
+
+    /* Copy the available index into the output param. */
+    *pIndex = index;


Nit: would it make more sense to only set this on success?

Made the improvement in the basic TLS demo (as have removed the feature of publish resend from this demo)

muneebahmed10 · 2020-08-20T07:16:01Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+
+    /* Sends an MQTT Connect packet using the established TLS session,
+     * then waits for connection acknowledgment (CONNACK) packet. */
+    returnStatus = establishMqttSession( &mqttContext, pNetworkContext, false, &sessionPresent );


I understand if this was a requirement for this demo, but if it was only a design choice, does this demo need non-clean sessions? I feel like connection sharing and resending of publishes are separate features, and if we already show it in the TLS and mutual auth demos, then we don't need to show it again here. It would reduce the amount of code that's duplicated in multiple places.

That's a good suggestion in terms of reducing the number of lines of code in this demo. I have removed the session restore feature (of resending publish messages) from this demo.

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

muneebahmed10 · 2020-08-20T07:21:35Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+        /* Wait for acknowledgement packet (SUBACK) from the broker in response to the
+         * subscribe request. */
+        mqttStatus = MQTT_ProcessLoop( pMqttContext, MQTT_PROCESS_LOOP_TIMEOUT_MS );
+


If the intention here is to make sure the SUBACK was received, we could add a flag that's set in the event callback on SUBACK reception. Just a thought, I don't expect it to be changed for this PR.

If the suggestion is to call ProcessLoop avoid the large timeout value, we can consider that for a separate PR effort.

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

muneebahmed10 · 2020-08-20T17:21:37Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+ * @brief The MQTT message for the #DEMO_TEMPERATURE_HIGH_TOPIC topic published
+ * in this example.
+ */
+#define DEMO_TEMPERATURE_HIGH_MESSAGE           "Today's High is 80 degree F"


Perhaps we should use this opportunity to push Kelvin as the superior unit of measurement

Interesting point (to make the demo universally appealing) 😆

gkwicker

Approved, see comments. It looks like some of this demo code could be refactored out into a helpful library which many applications could use; not a blocker but something to think about.

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

gkwicker · 2020-08-20T19:44:27Z

demos/mqtt/mqtt_demo_connection_sharing/mqtt_demo_connection_sharing.c

+ * when a MQTT connection is established with a session that was already
+ * present. All the outgoing publish messages waiting to receive PUBACK
+ * are resent in this demo. In order to support retransmission all the outgoing
+ * publishes are stored until a PUBACK is received.


Unless I've missed something retransmission won't work across reboots, so you may want to mention this in the comments.

I have removed the text about re-transmission as I removed all the re-transmission related code from this demo to avoid code duplication across our demo files

…ection-sharing

…tion manager (aws#1098)

Archit Aggarwal added 9 commits August 4, 2020 00:17

Add files for a subscription manager module

da4e531

Minor changes

4c38e3d

Update demos/lexicon.txt for parameter names in subscription manager

21b0ec2

Add another parameter list to lexicon file

0e4322f

Add building implementation of connection sharing demo

ce6473f

Fix subscription manager to not match topics for empty record entries

a334864

Fix subscription manager to not match topics for empty record entries

68d0cde

Fix demo runtime failure caused from sending NULL character to server…

b9c6cc9

…; update config files and CMakeLists.txt for demo

Update CMake configuration for all MQTT demos to pooulate ROOT_CA_CER…

de18121

…T_PATH macro with absolute path of downloaded certificate, and update demo_config files to have #ifndef guards around the macro

aggarw13 changed the title ~~Test PR from CLI tool~~ [PR from CLI tool] Add MQTT demo for connection sharing with subscription manager Aug 5, 2020

aggarw13 mentioned this pull request Aug 5, 2020

Add subscription manager for MQTT #1094

Merged

Archit Aggarwal and others added 18 commits August 5, 2020 22:02

Fix spellcheck issues

7063e17

Address review comments

430afeb

Improve topic matching logic and documentation

aac92f1

Merge remote-tracking branch 'fork/add-subscription-manager' into dem…

c4f58ba

…o/mqtt-connection-sharing

Fix typos in header file

8d08d2c

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

Address review comments

6b53e18

Merge branch 'add-subscription-manager' of github.com:aggarw13/aws-io…

0d637fb

…t-device-sdk-embedded-C into add-subscription-manager

wasn't -> was not to avoid the wrath of the spell checker

157580a

Merge branch 'development' into add-subscription-manager

c4a2162

Changes to API documentation based on review comments

85626a6

Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>

Improvements in topic matching for correctness, readability and expre…

b75cc4a

…ssivity

Add doc explanation for name index decrement in matchWildcards function

cda35d7

Add "sys" to demos/lexicon.txt to fix spell check

6541697

Improvements to implementation of RegisterCallback and RemoveCallback…

edaa2d2

… functions based on review feedback

Merge branch 'add-subscription-manager' into demo/mqtt-connection-sha…

32210bf

…ring

Fix bug in RegisterCallback function

ceb0d44

Add separate line comment add the end of the source file

e72c706

Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>

Minor fix in API doc

6e42571

Hygiene changes based on review comments

f9fea38

muneebahmed10 reviewed Aug 18, 2020

View reviewed changes

Archit Aggarwal added 4 commits August 18, 2020 21:17

Merge remote-tracking branch 'origin/development' into demo/mqtt-conn…

d7895fb

…ection-sharing

More hygiene changes

cc9c221

Minor hygiene change of removing irrelevant log message

e81b12e

More hygiene changes

fcc65a3

aggarw13 requested a review from muneebahmed10 August 18, 2020 22:16

Archit Aggarwal added 4 commits August 18, 2020 22:28

Add "numOfTopicFilters" and "pTopicFilters" to lexicon list

dd63b3a

Merge remote-tracking branch 'origin/development' into demo/mqtt-conn…

c30af8f

…ection-sharing

Merge branch 'development' into demo/mqtt-connection-sharing

921eb04

Rename private function to a better suiting name for its functionality

473df17

muneebahmed10 reviewed Aug 20, 2020

View reviewed changes

Archit Aggarwal and others added 3 commits August 20, 2020 09:44

Hygiene improvements from review comments

9fdba1f

Co-authored-by: Muneeb Ahmed <54290492+muneebahmed10@users.noreply.github.com>

Remove support for resending publish messages in demo to reduce LoC

45c3922

Minor improvements in demos

de27681

aggarw13 requested a review from muneebahmed10 August 20, 2020 17:17

muneebahmed10 previously approved these changes Aug 20, 2020

View reviewed changes

gkwicker previously approved these changes Aug 20, 2020

View reviewed changes

Archit Aggarwal added 2 commits August 20, 2020 21:50

Minor doc fixes in demo

16e03cd

Merge remote-tracking branch 'origin/development' into demo/mqtt-conn…

6be56c9

…ection-sharing

aggarw13 dismissed stale reviews from gkwicker and muneebahmed10 via 6be56c9 August 20, 2020 21:51

muneebahmed10 approved these changes Aug 20, 2020

View reviewed changes

gkwicker approved these changes Aug 20, 2020

View reviewed changes

aggarw13 merged commit a66e664 into aws:development Aug 20, 2020

aggarw13 deleted the demo/mqtt-connection-sharing branch August 20, 2020 22:53

leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 27, 2020

[PR from CLI tool] Add MQTT demo for connection sharing with subscrip…

da4b963

…tion manager (aws#1098)

leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 28, 2020

[PR from CLI tool] Add MQTT demo for connection sharing with subscrip…

97f2c66

…tion manager (aws#1098)

leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 31, 2020

[PR from CLI tool] Add MQTT demo for connection sharing with subscrip…

c1fb780

…tion manager (aws#1098)

leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Sep 1, 2020

[PR from CLI tool] Add MQTT demo for connection sharing with subscrip…

e57b3fb

…tion manager (aws#1098)


		/-----------------------------------------------------------/

		static int publishToTopic( MQTTContext_t * pMqttContext,


		/* Process Incoming UNSUBACK packet from the broker. */
		mqttStatus = MQTT_ProcessLoop( pMqttContext, MQTT_PROCESS_LOOP_TIMEOUT_MS );

[PR from CLI tool] Add MQTT demo for connection sharing with subscription manager #1098

[PR from CLI tool] Add MQTT demo for connection sharing with subscription manager #1098

Uh oh!

Conversation

aggarw13 commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aggarw13 Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aggarw13 Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkwicker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aggarw13 commented Aug 5, 2020 •

edited

Loading

codecov-commenter commented Aug 5, 2020 •

edited

Loading

aggarw13 Aug 20, 2020 •

edited

Loading

aggarw13 Aug 20, 2020 •

edited

Loading