[PATCH BlueZ 0/4] API changes for forward compatibility

[PATCH BlueZ 0/4] API changes for forward compatibility

[Inga Stotland]

The changes are contained to Management and Provisioner APIs. 

  The following methods are modified to allow for future development:
    
    Interface org.bluez.mesh.Management1:
    
    Old: void UnprovisionedScan(uint16 seconds)
    New: void UnprovisionedScan(dict options)
    
        The options parameter is a dictionary with the following keys defined:
        uint16 Seconds
                    Specifies number of seconds for scanning to be active.
                    If set to 0 or if this key is not present, then the
                    scanning will continue until UnprovisionedScanCancel()
                    or AddNode() methods are called.
        other keys TBD
    
    Old: void AddNode(array{byte}[16] uuid)
    New: void AddNode(array{byte}[16] uuid, dict options)
    
        The options parameter is currently an empty dictionary
    
    Interface org.bluez.mesh.Provisioner1
    
    Old: void ScanResult(int16 rssi, array{byte} data)
    New: void ScanResult(int16 rssi, array{byte} data, dict options)
    
        The options parameter is currently an empty dictionary

Inga Stotland (4):
  doc/mesh-api: Forward compatibility modifications
  mesh: Update UnprovisionedScan, AddNode & ScanResult
  test/test-mesh: Update to match modified APIs
  tools/mesh-cfgclient: Update to match modified APIs

 doc/mesh-api.txt       | 28 +++++++++++++++++++++-------
 mesh/manager.c         | 39 ++++++++++++++++++++++++++++++---------
 test/test-mesh         | 39 +++++++++++++++++++++++++--------------
 tools/mesh-cfgclient.c | 36 ++++++++++++++++++++++++++++++------
 4 files changed, 106 insertions(+), 36 deletions(-)

-- 
2.21.1

[PATCH BlueZ 1/4] doc/mesh-api: Forward compatibility modifications

[Inga Stotland]

The following methods are modified to allow for future development:

Interface org.bluez.mesh.Management1:

Old: void UnprovisionedScan(uint16 seconds)
New: void UnprovisionedScan(dict options)

    The options parameter is a dictionary with the following keys defined:
    uint16 Seconds
                Specifies number of seconds for scanning to be active.
                If set to 0 or if this key is not present, then the
                scanning will continue until UnprovisionedScanCancel()
                or AddNode() methods are called.
    other keys TBD

Old: void AddNode(array{byte}[16] uuid)
New: void AddNode(array{byte}[16] uuid, dict options)

    The options parameter is currently an empty dictionary

Interface org.bluez.mesh.Provisioner1

Old: void ScanResult(int16 rssi, array{byte} data)
New: void ScanResult(int16 rssi, array{byte} data, dict options)

    The options parameter is currently an empty dictionary
---
 doc/mesh-api.txt | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/doc/mesh-api.txt b/doc/mesh-api.txt
index 131de6bfd..cc351b635 100644
--- a/doc/mesh-api.txt
+++ b/doc/mesh-api.txt
@@ -455,14 +455,20 @@ Object path	/org/bluez/mesh/node<uuid>
 		CreateNetwork() or Import()
 
 Methods:
-	void UnprovisionedScan(uint16 seconds)
+	void UnprovisionedScan(dict options)
 
 		This method is used by the application that supports
 		org.bluez.mesh.Provisioner1 interface to start listening
-		(scanning) for unprovisioned devices in the area. Scanning
-		will continue for the specified number of seconds, or, if 0 is
-		specified, then continuously until UnprovisionedScanCancel() is
-		called or if AddNode() method is called.
+		(scanning) for unprovisioned devices in the area.
+
+		The options parameter is a dictionary with the following keys
+		defined:
+
+		uint16 Seconds
+			Specifies number of seconds for scanning to be active.
+			If set to 0 or if this key is not present, then the
+			scanning will continue until UnprovisionedScanCancel()
+			or AddNode() methods are called.
 
 		Each time a unique unprovisioned beacon is heard, the
 		ScanResult() method on the app will be called with the result.
@@ -482,7 +488,7 @@ Methods:
 			org.bluez.mesh.Error.InvalidArguments
 			org.bluez.mesh.Error.NotAuthorized
 
-	void AddNode(array{byte}[16] uuid)
+	void AddNode(array{byte}[16] uuid, dict options)
 
 		This method is used by the application that supports
 		org.bluez.mesh.Provisioner1 interface to add the
@@ -491,6 +497,10 @@ Methods:
 		The uuid parameter is a 16-byte array that contains Device UUID
 		of the unprovisioned device to be added to the network.
 
+		The options parameter is a dictionary that may contain
+		additional configuration info (currently an empty placeholder
+		for forward compatibility).
+
 		PossibleErrors:
 			org.bluez.mesh.Error.InvalidArguments
 			org.bluez.mesh.Error.NotAuthorized
@@ -927,7 +937,7 @@ Service		unique name
 Interface	org.bluez.mesh.Provisioner1
 Object path	freely definable
 
-	void ScanResult(int16 rssi, array{byte} data)
+	void ScanResult(int16 rssi, array{byte} data, dict options)
 
 		The method is called from the bluetooth-meshd daemon when a
 		unique UUID has been seen during UnprovisionedScan() for
@@ -943,6 +953,10 @@ Object path	freely definable
 		bit set in OOB mask). Whether these fields exist or not is a
 		decision of the remote device.
 
+		The options parameter is a dictionary that may contain
+		additional scan result info (currently an empty placeholder for
+		forward compatibility).
+
 		If a beacon with a UUID that has already been reported is
 		recieved by the daemon, it will be silently discarded unless it
 		was recieved at a higher rssi power level.
-- 
2.21.1

[PATCH BlueZ 2/4] mesh: Update UnprovisionedScan, AddNode & ScanResult

[Inga Stotland]

The following methods are modified to allow for future development:

Interface org.bluez.mesh.Management1:

Old: void UnprovisionedScan(uint16 seconds)
New: void UnprovisionedScan(dict options)

    The options parameter is a dictionary with the following keys defined:
    uint16 Seconds
                Specifies number of seconds for scanning to be active.
                If set to 0 or if this key is not present, then the
                scanning will continue until UnprovisionedScanCancel()
                or AddNode() methods are called.
    other keys TBD

Old: void AddNode(array{byte}[16] uuid)
New: void AddNode(array{byte}[16] uuid, dict options)

    The options parameter is currently an empty dictionary

Interface org.bluez.mesh.Provisioner1

Old: void ScanResult(int16 rssi, array{byte} data)
New: void ScanResult(int16 rssi, array{byte} data, dict options)

    The options parameter is currently an empty dictionary
---
 mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/mesh/manager.c b/mesh/manager.c
index 0909c7e16..8e948e47d 100644
--- a/mesh/manager.c
+++ b/mesh/manager.c
@@ -217,21 +217,22 @@ static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
 						void *user_data)
 {
 	struct mesh_node *node = user_data;
-	struct l_dbus_message_iter iter_uuid;
+	struct l_dbus_message_iter iter_uuid, options;
 	struct l_dbus_message *reply;
 	uint8_t *uuid;
-	uint32_t n;
+	uint32_t n = 22;
 
 	l_debug("AddNode request");
 
-	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
+	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
 
 	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
-								|| n != 16)
+	    || n != 16) {
+		l_debug("n = %u", n);
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
 							"Bad device UUID");
-
+	}
 	/* Allow AddNode to cancel Scanning if from the same node */
 	if (scan_node) {
 		if (scan_node != node)
@@ -361,6 +362,9 @@ static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
 	builder = l_dbus_message_builder_new(msg);
 	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
 	dbus_append_byte_array(builder, data + 2, len -2);
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+	/* TODO: populate with options when defined */
+	l_dbus_message_builder_leave_array(builder);
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
@@ -372,17 +376,34 @@ static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
 						void *user_data)
 {
 	struct mesh_node *node = user_data;
-	uint16_t duration;
+	uint16_t duration = 0;
 	struct mesh_io *io;
 	struct mesh_net *net;
+	const char *key;
+	struct l_dbus_message_iter options, var;
 	const char *sender = l_dbus_message_get_sender(msg);
 
 	if (strcmp(sender, node_get_owner(node)))
 		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
 
-	if (!l_dbus_message_get_arguments(msg, "q", &duration))
+	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
 		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
 
+	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
+		bool failed = true;
+
+		if (!strcmp(key, "Seconds")) {
+			if (l_dbus_message_iter_get_variant(&var, "q",
+							    &duration)) {
+				failed = false;
+			}
+		}
+
+		if (failed)
+			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
+							"Invalid options");
+	}
+
 	if (scan_node && scan_node != node)
 		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
 
@@ -752,13 +773,13 @@ static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
 static void setup_management_interface(struct l_dbus_interface *iface)
 {
 	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
-								"ay", "uuid");
+						"aya{sv}", "uuid", "options");
 	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
 				"", "qyay", "primary", "count", "dev_key");
 	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
 						"", "qy", "primary", "count");
 	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
-							"", "q", "seconds");
+							"", "a{sv}", "options");
 	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
 						cancel_scan_call, "", "");
 	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,
-- 
2.21.1

[PATCH BlueZ 3/4] test/test-mesh: Update to match modified APIs

[Inga Stotland]

This handles updated parameter list in UnprovisionedScan(),
AddNode() and ScanResult() D-Bus methods
---
 test/test-mesh | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/test/test-mesh b/test/test-mesh
index 6a5ddbd17..5db1d6d1a 100755
--- a/test/test-mesh
+++ b/test/test-mesh
@@ -474,13 +474,22 @@ class Application(dbus.service.Object):
 	def JoinFailed(self, value):
 		print(set_error('JoinFailed '), value)
 
-	@dbus.service.method(MESH_PROV_IFACE,
-					in_signature="nay", out_signature="")
-	def ScanResult(self, rssi, uuid):
-		uuid_str = array_to_string(uuid)
-		print(set_yellow('ScanResult RSSI ')
-					+ set_green(format(rssi, 'd'))
-					+ ' ' + uuid_str)
+	@dbus.service.method(MESH_PROV_IFACE, in_signature="naya{sv}",
+							out_signature="")
+	def ScanResult(self, rssi, data, options):
+		global remote_uuid
+		remote_uuid = data[:16]
+		uuid_str = array_to_string(remote_uuid)
+		data_str = array_to_string(data[16:])
+		if len(data_str) == 0:
+			data_str = 'Not Present'
+
+		print(set_yellow('ScanResult >> RSSI: ') +
+					set_green(format(rssi, 'd')) +
+					set_yellow(format(' UUID: ')) +
+					set_green(format(uuid_str, 's')) +
+					set_yellow(format(' OOB Data: ')) +
+					set_green(format(data_str, 's')))
 
 	@dbus.service.method(MESH_PROV_IFACE,
 					in_signature="y", out_signature="qq")
@@ -946,8 +955,6 @@ class MainMenu(Menu):
 		uuid = bytearray.fromhex("0a0102030405060708090A0B0C0D0E0F")
 		random.shuffle(uuid)
 		uuid_str = array_to_string(uuid)
-		caps = ["out-numeric"]
-		oob = ["other"]
 
 		print(set_yellow('Joining with UUID ') + set_green(uuid_str))
 		mesh_net.Join(app.get_path(), uuid,
@@ -955,23 +962,27 @@ class MainMenu(Menu):
 			error_handler=join_error_cb)
 
 	def __cmd_scan(self):
+		options = {}
+		options['Seconds'] = dbus.UInt16(0)
 
 		print(set_yellow('Scanning...'))
-		node_mgr.UnprovisionedScan(0, reply_handler=add_cb,
-						error_handler=add_error_cb)
+		node_mgr.UnprovisionedScan(options,
+						reply_handler=scan_cb,
+						error_handler=scan_error_cb)
 
 	def __cmd_add(self):
 		global user_input
+		global remote_uuid
+
 		if agent == None:
 			print(set_error('Provisioning agent not found'))
 			return
 
 		uuid_str = array_to_string(remote_uuid)
-		caps = ["in-numeric"]
-		oob = ["other"]
+		options = {}
 
 		print(set_yellow('Adding dev UUID ') + set_green(uuid_str))
-		node_mgr.AddNode(remote_uuid, reply_handler=add_cb,
+		node_mgr.AddNode(remote_uuid, options, reply_handler=add_cb,
 						error_handler=add_error_cb)
 
 	def __cmd_attach(self):
-- 
2.21.1

[PATCH BlueZ 4/4] tools/mesh-cfgclient: Update to match modified APIs

[Inga Stotland]

This handles updated parameter list in UnprovisionedScan(),
AddNode() and ScanResult() D-Bus methods
---
 tools/mesh-cfgclient.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/mesh-cfgclient.c b/tools/mesh-cfgclient.c
index ae13c4409..d1c673182 100644
--- a/tools/mesh-cfgclient.c
+++ b/tools/mesh-cfgclient.c
@@ -232,6 +232,21 @@ struct key_data {
 	bool update;
 };
 
+static void append_dict_entry_basic(struct l_dbus_message_builder *builder,
+					const char *key, const char *signature,
+					const void *data)
+{
+	if (!builder)
+		return;
+
+	l_dbus_message_builder_enter_dict(builder, "sv");
+	l_dbus_message_builder_append_basic(builder, 's', key);
+	l_dbus_message_builder_enter_variant(builder, signature);
+	l_dbus_message_builder_append_basic(builder, signature[0], data);
+	l_dbus_message_builder_leave_variant(builder);
+	l_dbus_message_builder_leave_dict(builder);
+}
+
 static void append_byte_array(struct l_dbus_message_builder *builder,
 					unsigned char *data, unsigned int len)
 {
@@ -769,9 +784,15 @@ static void scan_reply(struct l_dbus_proxy *proxy, struct l_dbus_message *msg,
 
 static void scan_setup(struct l_dbus_message *msg, void *user_data)
 {
-	int32_t secs = L_PTR_TO_UINT(user_data);
+	uint16_t secs = (uint16_t) L_PTR_TO_UINT(user_data);
+	struct l_dbus_message_builder *builder;
 
-	l_dbus_message_set_arguments(msg, "q", (uint16_t) secs);
+	builder = l_dbus_message_builder_new(msg);
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+	append_dict_entry_basic(builder, "Seconds", "q", &secs);
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
 }
 
 static void cmd_scan_unprov(int argc, char *argv[])
@@ -1284,6 +1305,9 @@ static void add_node_setup(struct l_dbus_message *msg, void *user_data)
 
 	builder = l_dbus_message_builder_new(msg);
 	append_byte_array(builder, uuid, 16);
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+	/* TODO: populate with options when defined */
+	l_dbus_message_builder_leave_array(builder);
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
 
@@ -1508,17 +1532,17 @@ static struct l_dbus_message *scan_result_call(struct l_dbus *dbus,
 						struct l_dbus_message *msg,
 						void *user_data)
 {
-	struct l_dbus_message_iter iter;
+	struct l_dbus_message_iter iter, opts;
 	int16_t rssi;
 	uint32_t n;
 	uint8_t *prov_data;
 	char *str;
 	struct unprov_device *dev;
+	const char *sig = "naya{sv}";
 
-	if (!l_dbus_message_get_arguments(msg, "nay", &rssi, &iter)) {
+	if (!l_dbus_message_get_arguments(msg, sig, &rssi, &iter, &opts)) {
 		l_error("Cannot parse scan results");
 		return l_dbus_message_new_error(msg, dbus_err_args, NULL);
-
 	}
 
 	if (!l_dbus_message_iter_get_fixed_array(&iter, &prov_data, &n) ||
@@ -1669,7 +1693,7 @@ static struct l_dbus_message *add_node_fail_call(struct l_dbus *dbus,
 static void setup_prov_iface(struct l_dbus_interface *iface)
 {
 	l_dbus_interface_method(iface, "ScanResult", 0, scan_result_call, "",
-							"nay", "rssi", "data");
+						"naya{sv}", "rssi", "data");
 
 	l_dbus_interface_method(iface, "RequestProvData", 0, req_prov_call,
 				"qq", "y", "net_index", "unicast", "count");
-- 
2.21.1

Re: [PATCH BlueZ 2/4] mesh: Update UnprovisionedScan, AddNode & ScanResult

[Gix, Brian]

Hi Inga,
On Fri, 2020-03-27 at 11:42 -0700, Inga Stotland wrote:
> The following methods are modified to allow for future development:
> 
> Interface org.bluez.mesh.Management1:
> 
> Old: void UnprovisionedScan(uint16 seconds)
> New: void UnprovisionedScan(dict options)
> 
>     The options parameter is a dictionary with the following keys defined:
>     uint16 Seconds
>                 Specifies number of seconds for scanning to be active.
>                 If set to 0 or if this key is not present, then the
>                 scanning will continue until UnprovisionedScanCancel()
>                 or AddNode() methods are called.
>     other keys TBD
> 
> Old: void AddNode(array{byte}[16] uuid)
> New: void AddNode(array{byte}[16] uuid, dict options)
> 
>     The options parameter is currently an empty dictionary
> 
> Interface org.bluez.mesh.Provisioner1
> 
> Old: void ScanResult(int16 rssi, array{byte} data)
> New: void ScanResult(int16 rssi, array{byte} data, dict options)
> 
>     The options parameter is currently an empty dictionary
> ---
>  mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/mesh/manager.c b/mesh/manager.c
> index 0909c7e16..8e948e47d 100644
> --- a/mesh/manager.c
> +++ b/mesh/manager.c
> @@ -217,21 +217,22 @@ static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
>  						void *user_data)
>  {
>  	struct mesh_node *node = user_data;
> -	struct l_dbus_message_iter iter_uuid;
> +	struct l_dbus_message_iter iter_uuid, options;
>  	struct l_dbus_message *reply;
>  	uint8_t *uuid;
> -	uint32_t n;
> +	uint32_t n = 22;
>  
>  	l_debug("AddNode request");
>  
> -	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
> +	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
>  	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
> -								|| n != 16)
> +	    || n != 16) {
> +		l_debug("n = %u", n);
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
>  							"Bad device UUID");
> -
> +	}
>  	/* Allow AddNode to cancel Scanning if from the same node */
>  	if (scan_node) {
>  		if (scan_node != node)
> @@ -361,6 +362,9 @@ static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
>  	builder = l_dbus_message_builder_new(msg);
>  	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
>  	dbus_append_byte_array(builder, data + 2, len -2);
> +	l_dbus_message_builder_enter_array(builder, "{sv}");
> +	/* TODO: populate with options when defined */
> +	l_dbus_message_builder_leave_array(builder);
>  	l_dbus_message_builder_finalize(builder);
>  	l_dbus_message_builder_destroy(builder);
>  
> @@ -372,17 +376,34 @@ static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
>  						void *user_data)
>  {
>  	struct mesh_node *node = user_data;
> -	uint16_t duration;
> +	uint16_t duration = 0;
>  	struct mesh_io *io;
>  	struct mesh_net *net;
> +	const char *key;
> +	struct l_dbus_message_iter options, var;
>  	const char *sender = l_dbus_message_get_sender(msg);
>  
>  	if (strcmp(sender, node_get_owner(node)))
>  		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
>  
> -	if (!l_dbus_message_get_arguments(msg, "q", &duration))
> +	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
>  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
>  
> +	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
> +		bool failed = true;
> +
> +		if (!strcmp(key, "Seconds")) {
> +			if (l_dbus_message_iter_get_variant(&var, "q",
> +							    &duration)) {
> +				failed = false;
> +			}
> +		}

I think failing in this in this way is not truely "Forward Compatible". If a key that is *not* Seconds is found
in the dictionary, this will always return an error.  I think it would be better if the key is ignored.

The only "fail" case should be if a key has a known valid, but has an incorrect type (not "q" in the case of
Seconds), or if the key is supported, but the value is outside the acceptable range (is there an acceptable
range for this)?

We agreed, I think, that the *non* existance of Seconds means "Unlimited".

> +
> +		if (failed)
> +			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> +							"Invalid options");
> +	}
> +
>  	if (scan_node && scan_node != node)
>  		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
>  
> @@ -752,13 +773,13 @@ static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
>  static void setup_management_interface(struct l_dbus_interface *iface)
>  {
>  	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
> -								"ay", "uuid");
> +						"aya{sv}", "uuid", "options");
>  	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
>  				"", "qyay", "primary", "count", "dev_key");
>  	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
>  						"", "qy", "primary", "count");
>  	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
> -							"", "q", "seconds");
> +							"", "a{sv}", "options");
>  	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
>  						cancel_scan_call, "", "");
>  	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,

Re: [PATCH BlueZ 2/4] mesh: Update UnprovisionedScan, AddNode & ScanResult

[Stotland, Inga]

Hi Brian,

On Fri, 2020-03-27 at 21:17 +0000, Gix, Brian wrote:
> Hi Inga,
> On Fri, 2020-03-27 at 11:42 -0700, Inga Stotland wrote:
> > The following methods are modified to allow for future development:
> > 
> > Interface org.bluez.mesh.Management1:
> > 
> > Old: void UnprovisionedScan(uint16 seconds)
> > New: void UnprovisionedScan(dict options)
> > 
> >     The options parameter is a dictionary with the following keys defined:
> >     uint16 Seconds
> >                 Specifies number of seconds for scanning to be active.
> >                 If set to 0 or if this key is not present, then the
> >                 scanning will continue until UnprovisionedScanCancel()
> >                 or AddNode() methods are called.
> >     other keys TBD
> > 
> > Old: void AddNode(array{byte}[16] uuid)
> > New: void AddNode(array{byte}[16] uuid, dict options)
> > 
> >     The options parameter is currently an empty dictionary
> > 
> > Interface org.bluez.mesh.Provisioner1
> > 
> > Old: void ScanResult(int16 rssi, array{byte} data)
> > New: void ScanResult(int16 rssi, array{byte} data, dict options)
> > 
> >     The options parameter is currently an empty dictionary
> > ---
> >  mesh/manager.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/mesh/manager.c b/mesh/manager.c
> > index 0909c7e16..8e948e47d 100644
> > --- a/mesh/manager.c
> > +++ b/mesh/manager.c
> > @@ -217,21 +217,22 @@ static struct l_dbus_message *add_node_call(struct l_dbus *dbus,
> >  						void *user_data)
> >  {
> >  	struct mesh_node *node = user_data;
> > -	struct l_dbus_message_iter iter_uuid;
> > +	struct l_dbus_message_iter iter_uuid, options;
> >  	struct l_dbus_message *reply;
> >  	uint8_t *uuid;
> > -	uint32_t n;
> > +	uint32_t n = 22;
> >  
> >  	l_debug("AddNode request");
> >  
> > -	if (!l_dbus_message_get_arguments(msg, "ay", &iter_uuid))
> > +	if (!l_dbus_message_get_arguments(msg, "aya{sv}", &iter_uuid, &options))
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >  
> >  	if (!l_dbus_message_iter_get_fixed_array(&iter_uuid, &uuid, &n)
> > -								|| n != 16)
> > +	    || n != 16) {
> > +		l_debug("n = %u", n);
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> >  							"Bad device UUID");
> > -
> > +	}
> >  	/* Allow AddNode to cancel Scanning if from the same node */
> >  	if (scan_node) {
> >  		if (scan_node != node)
> > @@ -361,6 +362,9 @@ static void prov_beacon_recv(void *user_data, struct mesh_io_recv_info *info,
> >  	builder = l_dbus_message_builder_new(msg);
> >  	l_dbus_message_builder_append_basic(builder, 'n', &rssi);
> >  	dbus_append_byte_array(builder, data + 2, len -2);
> > +	l_dbus_message_builder_enter_array(builder, "{sv}");
> > +	/* TODO: populate with options when defined */
> > +	l_dbus_message_builder_leave_array(builder);
> >  	l_dbus_message_builder_finalize(builder);
> >  	l_dbus_message_builder_destroy(builder);
> >  
> > @@ -372,17 +376,34 @@ static struct l_dbus_message *start_scan_call(struct l_dbus *dbus,
> >  						void *user_data)
> >  {
> >  	struct mesh_node *node = user_data;
> > -	uint16_t duration;
> > +	uint16_t duration = 0;
> >  	struct mesh_io *io;
> >  	struct mesh_net *net;
> > +	const char *key;
> > +	struct l_dbus_message_iter options, var;
> >  	const char *sender = l_dbus_message_get_sender(msg);
> >  
> >  	if (strcmp(sender, node_get_owner(node)))
> >  		return dbus_error(msg, MESH_ERROR_NOT_AUTHORIZED, NULL);
> >  
> > -	if (!l_dbus_message_get_arguments(msg, "q", &duration))
> > +	if (!l_dbus_message_get_arguments(msg, "a{sv}", &options))
> >  		return dbus_error(msg, MESH_ERROR_INVALID_ARGS, NULL);
> >  
> > +	while (l_dbus_message_iter_next_entry(&options, &key, &var)) {
> > +		bool failed = true;
> > +
> > +		if (!strcmp(key, "Seconds")) {
> > +			if (l_dbus_message_iter_get_variant(&var, "q",
> > +							    &duration)) {
> > +				failed = false;
> > +			}
> > +		}
> 
> I think failing in this in this way is not truely "Forward Compatible". If a key that is *not* Seconds is found
> in the dictionary, this will always return an error.  I think it would be better if the key is ignored.
> 
> The only "fail" case should be if a key has a known valid, but has an incorrect type (not "q" in the case of
> Seconds), or if the key is supported, but the value is outside the acceptable range (is there an acceptable
> range for this)?
> 

"Forward compatible" means that further keys shall be documented in
mesh-api.txt and then correctly processed by the daemon.
I would prefer to fail on unrecognized keywords.

> We agreed, I think, that the *non* existance of Seconds means "Unlimited".

Either 0 or absense of 'Seconds' keyword mean continuous scan.

> 
> > +
> > +		if (failed)
> > +			return dbus_error(msg, MESH_ERROR_INVALID_ARGS,
> > +							"Invalid options");
> > +	}
> > +
> >  	if (scan_node && scan_node != node)
> >  		return dbus_error(msg, MESH_ERROR_BUSY, NULL);
> >  
> > @@ -752,13 +773,13 @@ static struct l_dbus_message *set_key_phase_call(struct l_dbus *dbus,
> >  static void setup_management_interface(struct l_dbus_interface *iface)
> >  {
> >  	l_dbus_interface_method(iface, "AddNode", 0, add_node_call, "",
> > -								"ay", "uuid");
> > +						"aya{sv}", "uuid", "options");
> >  	l_dbus_interface_method(iface, "ImportRemoteNode", 0, import_node_call,
> >  				"", "qyay", "primary", "count", "dev_key");
> >  	l_dbus_interface_method(iface, "DeleteRemoteNode", 0, delete_node_call,
> >  						"", "qy", "primary", "count");
> >  	l_dbus_interface_method(iface, "UnprovisionedScan", 0, start_scan_call,
> > -							"", "q", "seconds");
> > +							"", "a{sv}", "options");
> >  	l_dbus_interface_method(iface, "UnprovisionedScanCancel", 0,
> >  						cancel_scan_call, "", "");
> >  	l_dbus_interface_method(iface, "CreateSubnet", 0, create_subnet_call,

Re: [PATCH BlueZ 0/4] API changes for forward compatibility

[Gix, Brian]

Patchset Applied
On Fri, 2020-03-27 at 11:42 -0700, Inga Stotland wrote:
> The changes are contained to Management and Provisioner APIs. 
> 
>   The following methods are modified to allow for future development:
>     
>     Interface org.bluez.mesh.Management1:
>     
>     Old: void UnprovisionedScan(uint16 seconds)
>     New: void UnprovisionedScan(dict options)
>     
>         The options parameter is a dictionary with the following keys defined:
>         uint16 Seconds
>                     Specifies number of seconds for scanning to be active.
>                     If set to 0 or if this key is not present, then the
>                     scanning will continue until UnprovisionedScanCancel()
>                     or AddNode() methods are called.
>         other keys TBD
>     
>     Old: void AddNode(array{byte}[16] uuid)
>     New: void AddNode(array{byte}[16] uuid, dict options)
>     
>         The options parameter is currently an empty dictionary
>     
>     Interface org.bluez.mesh.Provisioner1
>     
>     Old: void ScanResult(int16 rssi, array{byte} data)
>     New: void ScanResult(int16 rssi, array{byte} data, dict options)
>     
>         The options parameter is currently an empty dictionary
> 
> Inga Stotland (4):
>   doc/mesh-api: Forward compatibility modifications
>   mesh: Update UnprovisionedScan, AddNode & ScanResult
>   test/test-mesh: Update to match modified APIs
>   tools/mesh-cfgclient: Update to match modified APIs
> 
>  doc/mesh-api.txt       | 28 +++++++++++++++++++++-------
>  mesh/manager.c         | 39 ++++++++++++++++++++++++++++++---------
>  test/test-mesh         | 39 +++++++++++++++++++++++++--------------
>  tools/mesh-cfgclient.c | 36 ++++++++++++++++++++++++++++++------
>  4 files changed, 106 insertions(+), 36 deletions(-)
>