Add subscription manager for MQTT #1094

aggarw13 · 2020-08-04T01:23:27Z

Description of changes:
Add a subscription manager library (under /demos folder) for handling multiple subscriptions to using the same MQTT connection context.

Purpose
The subscription manager allows application to register different callbacks for the subscribed topic filters so that incoming PUBLISH messages on the MQTT connection can be dispatched to the specific callback of the matching topic filter.

Note: This PR sanitizes the draft implementation of the library shown in the draft PR #1091

Testing: A follow-up PR #1098 exhibits the use of the subscription manager in an MQTT demo (and thus, the subscription manager logic is tested through the demo)

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

codecov-commenter · 2020-08-05T19:55:48Z

Codecov Report

Merging #1094 into development will increase coverage by 1.91%.
The diff coverage is 81.48%.

@@               Coverage Diff               @@
##           development    #1094      +/-   ##
===============================================
+ Coverage        96.54%   98.46%   +1.91%     
===============================================
  Files                9        4       -5     
  Lines             5643     1301    -4342     
  Branches           641      383     -258     
===============================================
- Hits              5448     1281    -4167     
+ Misses               9        0       -9     
+ Partials           186       20     -166

Impacted Files	Coverage Δ
libraries/standard/mqtt/src/mqtt.c	`99.46% <ø> (+5.05%)`	⬆️
libraries/standard/mqtt/src/mqtt_lightweight.c	`97.44% <ø> (+2.92%)`	⬆️
libraries/standard/mqtt/src/mqtt_state.c	`98.46% <ø> (+2.52%)`	⬆️
libraries/standard/http/src/http_client.c	`98.53% <81.48%> (+11.05%)`	⬆️

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 cf0b2e7...3b1b7ec. Read the comment docs.

muneebahmed10 · 2020-08-05T21:39:18Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+
+    /* Check for an exact match if the incoming topic name and the registered
+     * topic filter length match. */
+    if( topicNameLength == topicFilterLength )


Might need to adjust the logic on this, since the topic filter/a should match the filter filter/#, right?

Have updated the logic to accommodate the case

muneebahmed10 · 2020-08-05T21:40:56Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+/**
+ * @brief The registry to store records of topic filters and their subscription callbacks.
+ */
+static SubscriptionManager_Record_t callbackRecordList[ MAX_SUBSCRIPTION_CALLBACK_RECORDS ] = { 0 };


So applications won't have access to the subscription list, they can only interface with it through the subscription manager. Is this intended?

Yes, that is the intended use of the manager here as a singleton. The alternative would be to use a context for the subscription manager that the application would pass for multiple instances of subscription manager.
I think for the purposes of both a single and multi-threaded demo, a singleton subscription manager is suffice for now. If we have a need for multiple subscription managers in the future, we can expose the context to be application provided

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

muneebahmed10 · 2020-08-05T21:47:25Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+                                            topicNameLength,
+                                            nameIndex,
+                                            filterIndex,
+                                            &status );


So &status is passed as the pMatch parameter of this function, and this function's return value (called status in its implementation) is assigned to matchFound. I think these names are a little confusing

Yes, that's a good point. I had most of the code as-is from the v4_beta branch here.
Have updated the names to be related to their meaning.

muneebahmed10 · 2020-08-05T22:04:25Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+    else
+    {
+        /* Should the topic string be copied? */
+        callbackRecordList[ availableIndex ].pTopicFilter = pTopicFilter;


Is the subscription manager supposed to be specific to one demo? If so, I'm wondering why it is in its own directory. If not, I think this should be changed to a static buffer and memcpy, since if anyone tries to tweak the demo then they'll see issues due to this code that's in a completely different directory

The subscription manager is meant to be used by other service client libraries (like Shadow), and is thus, placed in its own folder along with creation of its library target so that demo targets can link against it.
Is your concern that the customer may tweak the demo to use a stack based string pointer instead of a global, and thus, run into memory-out-of scope access problems?
Would it be fine to mitigate the concern by adding comments in the demo code about keeping the topic filters as global string literals?

Yes, we should at least document this somewhere, such as where SubscriptionManager_Record_t is typedef'ed. We should mention the limitation of using a const char * instead of a static buffer

Added a @note in the documentation of the RegisterCallback function

demos/mqtt/subscription-manager/mqtt_subscription_manager.h

muneebahmed10 · 2020-08-06T18:15:21Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+    else
+    {
+        /* Should the topic string be copied? */
+        callbackRecordList[ availableIndex ].pTopicFilter = pTopicFilter;


Yes, we should at least document this somewhere, such as where SubscriptionManager_Record_t is typedef'ed. We should mention the limitation of using a const char * instead of a static buffer

muneebahmed10 · 2020-08-06T18:21:17Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+     * topic filter length match. */
+    if( topicNameLength == topicFilterLength )
+    {
+        status = ( strncmp( pTopicName, pTopicFilter, topicNameLength ) == 0 ) ? true : false;


If we're not subjecting this to MISRA, then I think we should leave out the extraneous ternary

Suggested change

status = ( strncmp( pTopicName, pTopicFilter, topicNameLength ) == 0 ) ? true : false;

status = ( strncmp( pTopicName, pTopicFilter, topicNameLength ) == 0 );

I prefer that we keep it to be MISRA compliant, as we could consider making this a library (as subscription manager is needed by all MQTT based service client library demos)

muneebahmed10 · 2020-08-06T18:30:12Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+
+    /* Match against wildcard characters in topic filter only if the incoming
+     * PUBLISH topic name does not start with a "$" character. */
+    if( ( status == false ) && ( pTopicName[ 0 ] != '$' ) )


See Topics beginning with $. What if the topic name is $SYS/monitor and the topic filter is $SYS/#? Do we not want to match this as well?

Good catch! Have updated the logic to allow wildcard matching for reserved topics when wildcards are at a non-beginning position

muneebahmed10 · 2020-08-06T18:34:07Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+            ( strncmp( pTopicFilter, callbackRecordList[ availableIndex ].pTopicFilter, topicFilterLength )
+              == 0 ) )
+        {
+            recordExists = true;


I think we mentioned this in the other PR, but we ought to document that this doesn't allow multiple callbacks for the same topic filter. I think it's plausible that more than one application wants to receive messages from the same topic filter, in which case we should point to the code that needs to be changed in order to allow it.

Added a @note in the API documentation of the function in the header file

I don't think that adding documentation for supporting multiple callback registration per topic filter is necessarily required. (The source code is available for viewing and tweaking).
If we really think that there can be a customer need for multiple callbacks per topic filter, then we should just support that in the subscription manager. From the perspective of a single threaded application, I don;t see a reason why multiple callbacks per topic filter would be needed.

demos/mqtt/subscription-manager/mqtt_subscription_manager.h

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

muneebahmed10 · 2020-08-06T18:43:01Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+    if( ( availableIndex == MAX_SUBSCRIPTION_CALLBACK_RECORDS ) )
+    {
+        /* The record list is full. */
+        LogError( ( "Unable to register callback: Registry list is full: TopicFilter=%.*s",


In this case might be helpful to log what the max capacity actually is

Co-authored-by: Muneeb Ahmed <[email protected]>

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

aggarg · 2020-08-07T06:25:53Z

demos/mqtt/subscription-manager/CMakeLists.txt

@@ -0,0 +1,17 @@
+set( LIBRARY_NAME "mqtt_subscription_manager" )


Should be called DEMO_NAME.

This is being added as a library instead of a demo binary so that multiple demos can call it (and thereby, link against it)

aggarg · 2020-08-07T06:27:02Z

demos/mqtt/subscription-manager/CMakeLists.txt

+target_link_libraries(
+    ${LIBRARY_NAME}
+    PRIVATE
+        mqtt


Does it mean that Subscription manager links to MQTT? Should be other way round?

The mqtt library does not have any dependency on the subscription manager, but subscription manager depends on the mqtt for its types like MQTTPublishInfo_t.
Practically, only a header dependency on mqtt exists for the subscription manager; so a linking dependency is not strictly required. (I can remove this linking dependency if you think that will be better, but the header include path dependency would definitely remain)

demos/mqtt/subscription-manager/mqtt_subscription_manager.h

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

aggarg · 2020-08-07T08:30:48Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+static SubscriptionManager_Record_t callbackRecordList[ MAX_SUBSCRIPTION_CALLBACK_RECORDS ] = { 0 };
+
+/**
+ * @brief Handle special corner cases regarding wildcards at the end of topic


The description is vague. Please elaborate the description.

Updated description to mention the special cases handled in the utility

aggarg · 2020-08-07T08:35:05Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

+                               uint16_t filterIndex );
+
+/**
+ * @brief Attempt to match characters in a topic filter by wildcards.


Please elaborate the description. If the current character in the topic filter is '+', it consumes one level form the topic name. If the If the current character in the topic filter is '#', it assumes that the topic name matches and stops matching.

Added description for the matching logic performed in the function

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>

…ssivity

… functions based on review feedback

Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>

…ded-C into add-subscription-manager

muneebahmed10 · 2020-08-10T19:12:12Z

demos/mqtt/subscription-manager/mqtt_subscription_manager.h

+    /**
+     * @brief Success return value from Subscription Manager API.
+     */
+    SUBSCRIPTION_MANAGER_SUCCESS = 1,


Nit: Usually 0 means success, is there any special reason for this?

@dan4thewin has generally suggested to avoid using 0 as a success value as it could be a statically allocated default value assigned for variables and result in false positive validation in tests. We had followed the same philosophy for the simplied taskpool's enumeration values: https://github.com/FreeRTOS/FreeRTOS/blob/lts-development/FreeRTOS-Plus/Source/FreeRTOS-IoT-Libraries-LTS-Beta1/abstractions/platform/freertos/private/taskpool/taskpool_freertos.h#L43-L64

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

Co-authored-by: Muneeb Ahmed <[email protected]>

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

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

Co-authored-by: Muneeb Ahmed <[email protected]>

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

muneebahmed10

Some rewording

demos/mqtt/subscription-manager/mqtt_subscription_manager.h

demos/mqtt/subscription-manager/mqtt_subscription_manager.c

Co-authored-by: Muneeb Ahmed <[email protected]>

aggarw13 added 5 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

Fix subscription manager to not match topics for empty record entries

68d0cde

muneebahmed10 reviewed Aug 5, 2020

View reviewed changes

aggarw13 added 2 commits August 5, 2020 23:53

Address review comments

430afeb

Improve topic matching logic and documentation

aac92f1

muneebahmed10 reviewed Aug 6, 2020

View reviewed changes

aggarw13 and others added 3 commits August 6, 2020 12:59

Fix typos in header file

8d08d2c

Co-authored-by: Muneeb Ahmed <[email protected]>

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

aggarw13 requested a review from muneebahmed10 August 6, 2020 20:08

aggarw13 added 2 commits August 6, 2020 22:09

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

157580a

Merge branch 'development' into add-subscription-manager

c4a2162

aggarg suggested changes Aug 7, 2020

View reviewed changes

aggarw13 and others added 7 commits August 7, 2020 10:13

Changes to API documentation based on review comments

85626a6

Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>

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

Fix bug in RegisterCallback function

ceb0d44

Add separate line comment add the end of the source file

e72c706

Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>

aggarw13 requested a review from aggarg August 7, 2020 21:47

aggarw13 added 3 commits August 7, 2020 21:50

Minor fix in API doc

6e42571

Merge branch 'development' of github.com:aws/aws-iot-device-sdk-embed…

90ccdba

…ded-C into add-subscription-manager

Remove merge conflict related text

b35b815

muneebahmed10 reviewed Aug 10, 2020

View reviewed changes

Remove unnecessary extra parentheses in else-if expression

bbc1ed2

Co-authored-by: Muneeb Ahmed <[email protected]>

aggarw13 added 2 commits August 10, 2020 19:48

Minor hygiene in matchTopic function

396b5ba

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

9b4bd7f

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

muneebahmed10 previously approved these changes Aug 10, 2020

View reviewed changes

demos/mqtt/subscription-manager/mqtt_subscription_manager.c Outdated Show resolved Hide resolved

Remove extra parentheses from matchTopic function

750d61e

Co-authored-by: Muneeb Ahmed <[email protected]>

aggarw13 dismissed muneebahmed10’s stale review via 750d61e August 10, 2020 21:18

muneebahmed10 previously approved these changes Aug 10, 2020

View reviewed changes

aggarw13 added 3 commits August 10, 2020 14:55

Merge branch 'development' into add-subscription-manager

902ef2c

More hygiene improvements for doc and code style

b60ef01

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

00b14af

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

aggarw13 dismissed muneebahmed10’s stale review via 00b14af August 11, 2020 18:13

aggarg previously approved these changes Aug 11, 2020

View reviewed changes

Fix typo

f4d07df

aggarw13 dismissed aggarg’s stale review via f4d07df August 11, 2020 18:30

aggarg previously approved these changes Aug 11, 2020

View reviewed changes

muneebahmed10 reviewed Aug 11, 2020

View reviewed changes

Minor further doc improvements

b56e984

Co-authored-by: Muneeb Ahmed <[email protected]>

aggarw13 dismissed aggarg’s stale review via b56e984 August 11, 2020 18:43

aggarg approved these changes Aug 11, 2020

View reviewed changes

Merge branch 'development' into add-subscription-manager

3b1b7ec

muneebahmed10 approved these changes Aug 11, 2020

View reviewed changes

aggarw13 merged commit 9658ac6 into aws:development Aug 11, 2020

aggarw13 deleted the add-subscription-manager branch August 11, 2020 18:51

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

Add subscription manager for MQTT (aws#1094)

e4b73b7

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

Add subscription manager for MQTT (aws#1094)

de638a1

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

Add subscription manager for MQTT (aws#1094)

29b1054

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

Add subscription manager for MQTT (aws#1094)

744ff9b

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

Add subscription manager for MQTT (aws#1094)

9caba0b

	status = ( strncmp( pTopicName, pTopicFilter, topicNameLength ) == 0 ) ? true : false;
	status = ( strncmp( pTopicName, pTopicFilter, topicNameLength ) == 0 );

		@@ -0,0 +1,17 @@
		set( LIBRARY_NAME "mqtt_subscription_manager" )

Add subscription manager for MQTT #1094

Add subscription manager for MQTT #1094

Uh oh!

Conversation

aggarw13 commented Aug 4, 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!

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!

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!

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!

muneebahmed10 Aug 6, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aggarw13 commented Aug 4, 2020 •

edited

Loading

codecov-commenter commented Aug 5, 2020 •

edited

Loading

muneebahmed10 Aug 6, 2020 •

edited

Loading