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,
