Reworking MQTT 5 Properties and integrating paho to RPC samples and code #2651

RLeclair · 2023-10-05T23:26:35Z

In this PR:

Integration of Paho Async C with the RPC code.
Two new samples for Paho RPC.
Re-work of MQTT property read and free.

NOTE: Added topic filter to received topic matching function using az_span. This is temporary.

vaavva · 2023-10-09T20:10:03Z

sdk/samples/core/mosquitto_rpc_client_sample.c

@@ -45,6 +45,11 @@ static uint8_t correlation_id_buffers[RPC_CLIENT_MAX_PENDING_COMMANDS]
                                     [AZ_MQTT5_RPC_CORRELATION_ID_LENGTH];
 static pending_commands_array pending_commands;

+static az_mqtt5_options options = {
+  .certificate_authority_trusted_roots = AZ_SPAN_LITERAL_EMPTY,
+  .disable_tls_validation = true,


Aren't we using TLS in this sample?

Reverted back to using TLS

vaavva

will finish reviewing the test files tomorrow

vaavva · 2023-10-09T20:10:20Z

sdk/samples/core/paho_rpc_client_sample.c

+
+/**
+ * @file
+ * @brief RPC client sample for Mosquitto MQTT


Suggested change

* @brief RPC client sample for Mosquitto MQTT

* @brief RPC client sample for Paho MQTT

vaavva · 2023-10-09T21:17:57Z

sdk/samples/core/paho_rpc_client_sample.c

@@ -0,0 +1,314 @@
+/* Copyright (c) Microsoft Corporation. All rights reserved. */


As a future work item, I'd recommend either combining the paho and mosquitto samples into one file with ifdefs or creating common functions for these samples to reduce duplication.

vaavva · 2023-10-09T21:19:05Z

sdk/samples/core/paho_rpc_server_sample.c

+
+/**
+ * @file
+ * @brief Mosquitto rpc server sample


Suggested change

* @brief Mosquitto rpc server sample

* @brief RPC server sample for Paho MQTT

vaavva · 2023-10-09T21:32:12Z

sdk/src/azure/core/az_mqtt5_rpc_client_hfsm.c

-    az_result rc = _parse_response(
-        recv_data, &correlation_data, &status, &error_message, &content_type, &resp_data);
+    az_result rc = AZ_OK;
+    if (recv_data->properties == NULL)


Does this check anything (would it ever be null)?

Removed check

vaavva · 2023-10-09T21:36:15Z

sdk/src/azure/core/az_mqtt5_rpc_client_hfsm.c

+      resp_data.error_message = AZ_SPAN_FROM_STR("Response does not have properties.");
+      rc = AZ_ERROR_ITEM_NOT_FOUND;
+    }
+    else if (az_span_ptr(correlation_data_bindata) == NULL)


Suggested change

else if (az_span_ptr(correlation_data_bindata) == NULL)

else if (az_span_is_content_equal(correlation_data_bindata, AZ_SPAN_EMPTY))

for this and below, unless you'd expect size being non-zero an invalid case that we need to handle as well

Since these values would be AZ_SPAN_EMPTY on any errors reading them, why do we need to create new az_spans for them in lines 385-388 instead of setting them directly to resp_data.?

(realized this doesn't make sense for status/error message, but I think it makes sense for correlation_id and maybe content type?)

I changed the correlation_id and content type to be assigned directly when creating resp_data and changed the check to az_span_is_content_equal, thanks for pointing this out!

vaavva · 2023-10-09T21:41:44Z

sdk/src/azure/core/az_mqtt5_rpc_client_hfsm.c

+    }
+    else
+    {
+      resp_data.correlation_id = correlation_data_bindata;


Ideally, if the correlation id exists, it would be good to set it on resp_data even if there are other errors so the application can try and correlate the failure to the proper command request even if there are other issues with the message

vaavva · 2023-10-09T21:58:38Z

sdk/src/azure/core/az_mqtt5_rpc_client_hfsm.c

+      // az_mqtt5_property_string response_topic_property = az_mqtt5_property_create_string(
+      // test_span);


Suggested change

// az_mqtt5_property_string response_topic_property = az_mqtt5_property_create_string(

// test_span);

vaavva · 2023-10-09T22:04:39Z

sdk/src/azure/core/az_mqtt5_rpc_server_hfsm.c

+  az_span correlation_data_bindata = az_mqtt5_property_get_binarydata(&correlation_data);
+  az_span content_type_str = az_mqtt5_property_get_string(&content_type);
+
+  if (az_span_ptr(response_topic_str) != NULL && az_span_ptr(correlation_data_bindata) != NULL


If any of these are null, we probably need to set ret to a failure value to avoid changing the behavior.

vaavva · 2023-10-10T00:16:58Z

sdk/src/azure/platform/az_pahoasync5.c

@@ -429,6 +429,7 @@ AZ_NODISCARD az_result az_mqtt5_property_bag_clear(az_mqtt5_property_bag* proper
 {
  _az_PRECONDITION_NOT_NULL(property_bag);

+  MQTTProperties_free(property_bag->pahoasync_properties);
  property_bag->pahoasync_properties->count = 0;


I think this line is now redundant - is this done within MQTTProperties_free?

It does zero out all fields, removed line.

vaavva · 2023-10-10T20:32:28Z

sdk/samples/core/paho_multiple_rpc_sample.c

+
+/**
+ * @file
+ * @brief RPC Server & Client sample for Mosquitto MQTT that supports multiple commands under the


Suggested change

* @brief RPC Server & Client sample for Mosquitto MQTT that supports multiple commands under the

* @brief RPC Server & Client sample for Paho MQTT that supports multiple commands under the

vaavva · 2023-10-10T20:44:12Z

sdk/tests/core/test_az_mqtt5_rpc_client_hfsm.c

@@ -880,6 +887,10 @@ static void test_az_mqtt5_rpc_client_invoke_begin_faulted_failure(void** state)
      az_mqtt5_rpc_client_invoke_begin(&test_rpc_client, NULL), AZ_ERROR_HFSM_INVALID_STATE);

  assert_int_equal(ref_pub_req, 0);
+
+#if defined(TRANSPORT_PAHO)
+  MQTTAsync_destroy(&test_client);


Do we need MQTTProperties_free here too?

RLeclair requested review from CIPop, ericwolz, ewertons, jspaith, vaavva, ahsonkhan, antkmsft, RickWinter, vhvb1989, gearama and LarryOsterman as code owners October 5, 2023 23:26

vaavva reviewed Oct 9, 2023

View reviewed changes

vaavva reviewed Oct 10, 2023

View reviewed changes

RLeclair added 7 commits October 10, 2023 23:39

Paho sample rpc

807d152

Adding multiple RPC paho sample

2a4837a

Unit test memory fix, adding safeguards to RPC HFSMs

7b5fed4

Cleaning up paho at end of rpc sample

6fea843

Addressing comments

6456e1b

style

7973efa

Integrating callback change

41cbb0b

RLeclair force-pushed the pahorpc branch from 406eedc to 41cbb0b Compare October 10, 2023 23:59

RLeclair enabled auto-merge (squash) October 11, 2023 00:08

vaavva approved these changes Oct 11, 2023

View reviewed changes

RLeclair merged commit 6cc11b5 into Azure:feature/v2 Oct 11, 2023
23 checks passed

RLeclair deleted the pahorpc branch October 11, 2023 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworking MQTT 5 Properties and integrating paho to RPC samples and code #2651

Reworking MQTT 5 Properties and integrating paho to RPC samples and code #2651

RLeclair commented Oct 5, 2023

vaavva Oct 9, 2023

RLeclair Oct 10, 2023

vaavva left a comment

vaavva Oct 9, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

RLeclair Oct 10, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

RLeclair Oct 10, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

vaavva Oct 9, 2023

vaavva Oct 10, 2023

RLeclair Oct 10, 2023

vaavva Oct 10, 2023

vaavva Oct 10, 2023

	* @brief RPC client sample for Mosquitto MQTT
	* @brief RPC client sample for Paho MQTT

		@@ -0,0 +1,314 @@
		/* Copyright (c) Microsoft Corporation. All rights reserved. */

	* @brief Mosquitto rpc server sample
	* @brief RPC server sample for Paho MQTT

	else if (az_span_ptr(correlation_data_bindata) == NULL)
	else if (az_span_is_content_equal(correlation_data_bindata, AZ_SPAN_EMPTY))

		// az_mqtt5_property_string response_topic_property = az_mqtt5_property_create_string(
		// test_span);

	* @brief RPC Server & Client sample for Mosquitto MQTT that supports multiple commands under the
	* @brief RPC Server & Client sample for Paho MQTT that supports multiple commands under the

Reworking MQTT 5 Properties and integrating paho to RPC samples and code #2651

Reworking MQTT 5 Properties and integrating paho to RPC samples and code #2651

Conversation

RLeclair commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaavva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment