[RFC v0 0/7] Manager/Adapter transition to ObjectManager

[RFC v0 0/7] Manager/Adapter transition to ObjectManager

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

(This is sent as RFC since I haven't done proper testing, and furthermore the test scripts haven't been updated)

While thinking about how profile and their states should be exposed in D-Bus (see previous RFC), I found several potential improvements in the current version of the Manager and Adapter APIs.

Basically, this patchset removes some properties and methods that seem duplicated.

There are other similar methods/properties that could be simplified as well, but for practical reasons I started with the most obvious ones.

Mikel Astiz (7):
  manager: Expose default adapter using property
  manager: Remove redundant D-Bus signals
  manager: Remove redundant Adapters property
  adapter: Remove redundant D-Bus signals
  adapter: Remove DevicesFound signal
  adapter: Remove redundant Devices property
  adapter: Remove FindDevice method from D-Bus API

 doc/adapter-api.txt |  32 +--------
 doc/manager-api.txt |  28 +-------
 src/adapter.c       | 187 +---------------------------------------------------
 src/manager.c       | 103 +++++++----------------------
 4 files changed, 29 insertions(+), 321 deletions(-)

-- 
1.7.11.7

[RFC v0 1/7] manager: Expose default adapter using property

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Replace previous method in the Manager interface with a property. If no
default adapter exists, the property will not be present.
---
 doc/manager-api.txt | 20 +++------------
 src/manager.c       | 71 +++++++++++++++++++++++------------------------------
 2 files changed, 35 insertions(+), 56 deletions(-)

diff --git a/doc/manager-api.txt b/doc/manager-api.txt
index fe50556..1cebb27 100644
--- a/doc/manager-api.txt
+++ b/doc/manager-api.txt
@@ -14,13 +14,6 @@ Service		org.bluez
 Interface	org.bluez.Manager
 Object path	/
 
-		object DefaultAdapter()
-
-			Returns object path for the default adapter.
-
-			Possible errors: org.bluez.Error.InvalidArguments
-					 org.bluez.Error.NoSuchAdapter
-
 		object FindAdapter(string pattern)
 
 			Returns object path for the specified adapter. Valid
@@ -114,15 +107,10 @@ Signals		AdapterAdded(object adapter)
 
 			Parameter is object path of removed adapter.
 
-		DefaultAdapterChanged(object adapter)
-
-			Parameter is object path of the new default adapter.
-
-			In case all adapters are removed this signal will not
-			be emitted. The AdapterRemoved signal has to be used
-			to detect that no default adapter is selected or
-			available anymore.
-
 Properties	array{object} Adapters [readonly]
 
 			List of adapter object paths.
+
+		object DefaultAdapter [readonly, optional]
+
+			Object path for the default adapter, if any.
diff --git a/src/manager.c b/src/manager.c
index a96115b..598c081 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -64,29 +64,6 @@ const char *manager_get_base_path(void)
 	return base_path;
 }
 
-static DBusMessage *default_adapter(DBusConnection *conn,
-					DBusMessage *msg, void *data)
-{
-	DBusMessage *reply;
-	struct btd_adapter *adapter;
-	const gchar *path;
-
-	adapter = manager_find_adapter_by_id(default_adapter_id);
-	if (!adapter)
-		return btd_error_no_such_adapter(msg);
-
-	reply = dbus_message_new_method_return(msg);
-	if (!reply)
-		return NULL;
-
-	path = adapter_get_path(adapter);
-
-	dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
-				DBUS_TYPE_INVALID);
-
-	return reply;
-}
-
 static DBusMessage *find_adapter(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
@@ -155,10 +132,32 @@ static gboolean manager_property_get_adapters(
 	return TRUE;
 }
 
+static gboolean manager_property_get_default_adapter(
+					const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct btd_adapter *adapter;
+	const char *path;
+
+	adapter = manager_find_adapter_by_id(default_adapter_id);
+	path = adapter_get_path(adapter);
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, &path);
+
+	return TRUE;
+}
+
+static gboolean manager_property_exists_default_adapter(
+					const GDBusPropertyTable *property,
+					void *data)
+{
+	if (manager_find_adapter_by_id(default_adapter_id) == NULL)
+		return FALSE;
+	else
+		return TRUE;
+}
+
 static const GDBusMethodTable manager_methods[] = {
-	{ GDBUS_METHOD("DefaultAdapter",
-			NULL, GDBUS_ARGS({ "adapter", "o" }),
-			default_adapter) },
 	{ GDBUS_METHOD("FindAdapter",
 			GDBUS_ARGS({ "pattern", "s" }),
 			GDBUS_ARGS({ "adapter", "o" }),
@@ -177,13 +176,13 @@ static const GDBusSignalTable manager_signals[] = {
 			GDBUS_ARGS({ "adapter", "o" })) },
 	{ GDBUS_SIGNAL("AdapterRemoved",
 			GDBUS_ARGS({ "adapter", "o" })) },
-	{ GDBUS_SIGNAL("DefaultAdapterChanged",
-			GDBUS_ARGS({ "adapter", "o" })) },
 	{ }
 };
 
 static const GDBusPropertyTable manager_properties[] = {
 	{ "Adapters", "ao", manager_property_get_adapters },
+	{ "DefaultAdapter", "o", manager_property_get_default_adapter, NULL,
+				manager_property_exists_default_adapter },
 	{ }
 };
 
@@ -199,21 +198,13 @@ dbus_bool_t manager_init(const char *path)
 
 static void manager_set_default_adapter(int id)
 {
-	struct btd_adapter *adapter;
-	const gchar *path;
-
-	default_adapter_id = id;
-
-	adapter = manager_find_adapter_by_id(id);
-	if (!adapter)
+	if (id == default_adapter_id)
 		return;
 
-	path = adapter_get_path(adapter);
+	default_adapter_id = id;
 
-	g_dbus_emit_signal(btd_get_dbus_connection(), "/",
-				MANAGER_INTERFACE, "DefaultAdapterChanged",
-				DBUS_TYPE_OBJECT_PATH, &path,
-				DBUS_TYPE_INVALID);
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), "/",
+					MANAGER_INTERFACE, "DefaultAdapter");
 }
 
 struct btd_adapter *manager_get_default_adapter(void)
-- 
1.7.11.7

[RFC v0 2/7] manager: Remove redundant D-Bus signals

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The Manager interface already reports changes in the adapter list in
form of property changes, so there is no need to keep these two signals
any more.
---
 doc/manager-api.txt |  8 --------
 src/manager.c       | 20 +-------------------
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/doc/manager-api.txt b/doc/manager-api.txt
index 1cebb27..b6e4a0f 100644
--- a/doc/manager-api.txt
+++ b/doc/manager-api.txt
@@ -99,14 +99,6 @@ Object path	/
 
 			Possible errors: org.bluez.Error.DoesNotExist
 
-Signals		AdapterAdded(object adapter)
-
-			Parameter is object path of added adapter.
-
-		AdapterRemoved(object adapter)
-
-			Parameter is object path of removed adapter.
-
 Properties	array{object} Adapters [readonly]
 
 			List of adapter object paths.
diff --git a/src/manager.c b/src/manager.c
index 598c081..f07cefe 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -171,14 +171,6 @@ static const GDBusMethodTable manager_methods[] = {
 	{ }
 };
 
-static const GDBusSignalTable manager_signals[] = {
-	{ GDBUS_SIGNAL("AdapterAdded",
-			GDBUS_ARGS({ "adapter", "o" })) },
-	{ GDBUS_SIGNAL("AdapterRemoved",
-			GDBUS_ARGS({ "adapter", "o" })) },
-	{ }
-};
-
 static const GDBusPropertyTable manager_properties[] = {
 	{ "Adapters", "ao", manager_property_get_adapters },
 	{ "DefaultAdapter", "o", manager_property_get_default_adapter, NULL,
@@ -192,7 +184,7 @@ dbus_bool_t manager_init(const char *path)
 
 	return g_dbus_register_interface(btd_get_dbus_connection(),
 					"/", MANAGER_INTERFACE,
-					manager_methods, manager_signals,
+					manager_methods, NULL,
 					manager_properties, NULL, NULL);
 }
 
@@ -215,7 +207,6 @@ struct btd_adapter *manager_get_default_adapter(void)
 static void manager_remove_adapter(struct btd_adapter *adapter)
 {
 	uint16_t dev_id = adapter_get_dev_id(adapter);
-	const gchar *path = adapter_get_path(adapter);
 
 	adapters = g_slist_remove(adapters, adapter);
 
@@ -228,11 +219,6 @@ static void manager_remove_adapter(struct btd_adapter *adapter)
 		manager_set_default_adapter(new_default);
 	}
 
-	g_dbus_emit_signal(btd_get_dbus_connection(), "/",
-				MANAGER_INTERFACE, "AdapterRemoved",
-				DBUS_TYPE_OBJECT_PATH, &path,
-				DBUS_TYPE_INVALID);
-
 	adapter_remove(adapter);
 	btd_adapter_unref(adapter);
 
@@ -332,10 +318,6 @@ struct btd_adapter *btd_manager_register_adapter(int id, gboolean up)
 	}
 
 	path = adapter_get_path(adapter);
-	g_dbus_emit_signal(btd_get_dbus_connection(), "/",
-				MANAGER_INTERFACE, "AdapterAdded",
-				DBUS_TYPE_OBJECT_PATH, &path,
-				DBUS_TYPE_INVALID);
 
 	g_dbus_emit_property_changed(btd_get_dbus_connection(),  "/",
 					MANAGER_INTERFACE, "Adapters");
-- 
1.7.11.7

[RFC v0 3/7] manager: Remove redundant Adapters property

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The ObjectManager interface already reports the available adapters, so
the property can be entirely removed.
---
 doc/manager-api.txt |  6 +-----
 src/manager.c       | 30 ------------------------------
 2 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/doc/manager-api.txt b/doc/manager-api.txt
index b6e4a0f..d225239 100644
--- a/doc/manager-api.txt
+++ b/doc/manager-api.txt
@@ -99,10 +99,6 @@ Object path	/
 
 			Possible errors: org.bluez.Error.DoesNotExist
 
-Properties	array{object} Adapters [readonly]
-
-			List of adapter object paths.
-
-		object DefaultAdapter [readonly, optional]
+Properties	object DefaultAdapter [readonly, optional]
 
 			Object path for the default adapter, if any.
diff --git a/src/manager.c b/src/manager.c
index f07cefe..d3e37b1 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -109,29 +109,6 @@ done:
 	return reply;
 }
 
-static gboolean manager_property_get_adapters(
-					const GDBusPropertyTable *property,
-					DBusMessageIter *iter, void *data)
-{
-	DBusMessageIter entry;
-	GSList *l;
-
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
-				DBUS_TYPE_OBJECT_PATH_AS_STRING, &entry);
-
-	for (l = adapters; l != NULL; l = l->next) {
-		struct btd_adapter *adapter = l->data;
-		const char *path = adapter_get_path(adapter);
-
-		dbus_message_iter_append_basic(&entry, DBUS_TYPE_OBJECT_PATH,
-								&path);
-	}
-
-	dbus_message_iter_close_container(iter, &entry);
-
-	return TRUE;
-}
-
 static gboolean manager_property_get_default_adapter(
 					const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
@@ -172,7 +149,6 @@ static const GDBusMethodTable manager_methods[] = {
 };
 
 static const GDBusPropertyTable manager_properties[] = {
-	{ "Adapters", "ao", manager_property_get_adapters },
 	{ "DefaultAdapter", "o", manager_property_get_default_adapter, NULL,
 				manager_property_exists_default_adapter },
 	{ }
@@ -210,9 +186,6 @@ static void manager_remove_adapter(struct btd_adapter *adapter)
 
 	adapters = g_slist_remove(adapters, adapter);
 
-	g_dbus_emit_property_changed(btd_get_dbus_connection(), "/",
-					MANAGER_INTERFACE, "Adapters");
-
 	if (default_adapter_id == dev_id || default_adapter_id < 0) {
 		int new_default = hci_get_route(NULL);
 
@@ -319,9 +292,6 @@ struct btd_adapter *btd_manager_register_adapter(int id, gboolean up)
 
 	path = adapter_get_path(adapter);
 
-	g_dbus_emit_property_changed(btd_get_dbus_connection(),  "/",
-					MANAGER_INTERFACE, "Adapters");
-
 	btd_stop_exit_timer();
 
 	if (default_adapter_id < 0)
-- 
1.7.11.7

[RFC v0 4/7] adapter: Remove redundant D-Bus signals

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The Adapter interface already reports changes in the device list in form
of property changes, so there is no need to keep these two signals.
---
 doc/adapter-api.txt |  8 --------
 src/adapter.c       | 17 -----------------
 2 files changed, 25 deletions(-)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index b638586..132e60f 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -116,14 +116,6 @@ Signals		DevicesFound(array{object path, dict values})
 			The dictionary contains the properties from the
 			org.bluez.Device interface.
 
-		DeviceCreated(object device)
-
-			Parameter is object path of created device.
-
-		DeviceRemoved(object device)
-
-			Parameter is object path of removed device.
-
 Properties	string Address [readonly]
 
 			The Bluetooth device address.
diff --git a/src/adapter.c b/src/adapter.c
index f4bb622..1f02f0b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1014,7 +1014,6 @@ static struct btd_device *adapter_create_device(struct btd_adapter *adapter,
 						uint8_t bdaddr_type)
 {
 	struct btd_device *device;
-	const char *path;
 
 	DBG("%s", address);
 
@@ -1026,12 +1025,6 @@ static struct btd_device *adapter_create_device(struct btd_adapter *adapter,
 
 	adapter->devices = g_slist_append(adapter->devices, device);
 
-	path = device_get_path(device);
-	g_dbus_emit_signal(btd_get_dbus_connection(), adapter->path,
-			ADAPTER_INTERFACE, "DeviceCreated",
-			DBUS_TYPE_OBJECT_PATH, &path,
-			DBUS_TYPE_INVALID);
-
 	g_dbus_emit_property_changed(btd_get_dbus_connection(),
 				adapter->path, ADAPTER_INTERFACE, "Devices");
 
@@ -1059,7 +1052,6 @@ void adapter_remove_device(struct btd_adapter *adapter,
 						struct btd_device *dev,
 						gboolean remove_storage)
 {
-	const gchar *dev_path = device_get_path(dev);
 	struct discovery *discovery = adapter->discovery;
 	GList *l;
 
@@ -1091,11 +1083,6 @@ void adapter_remove_device(struct btd_adapter *adapter,
 	g_dbus_emit_property_changed(btd_get_dbus_connection(),
 				adapter->path, ADAPTER_INTERFACE, "Devices");
 
-	g_dbus_emit_signal(btd_get_dbus_connection(), adapter->path,
-			ADAPTER_INTERFACE, "DeviceRemoved",
-			DBUS_TYPE_OBJECT_PATH, &dev_path,
-			DBUS_TYPE_INVALID);
-
 	device_remove(dev, remove_storage);
 }
 
@@ -1687,10 +1674,6 @@ static const GDBusMethodTable adapter_methods[] = {
 };
 
 static const GDBusSignalTable adapter_signals[] = {
-	{ GDBUS_SIGNAL("DeviceCreated",
-			GDBUS_ARGS({ "device", "o" })) },
-	{ GDBUS_SIGNAL("DeviceRemoved",
-			GDBUS_ARGS({ "device", "o" })) },
 	{ GDBUS_SIGNAL("DevicesFound",
 			GDBUS_ARGS({ "devices", "a{oa{sv}}" })) },
 	{ }
-- 
1.7.11.7

[RFC v0 5/7] adapter: Remove DevicesFound signal

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

ObjectManager reports the D-Bus interfaces of all known devices,
including the ones detected during discovery. Therefore this signal is
not required.
---
 doc/adapter-api.txt |  13 ++-----
 src/adapter.c       | 102 ++--------------------------------------------------
 2 files changed, 4 insertions(+), 111 deletions(-)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index 132e60f..13941c9 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -45,8 +45,8 @@ Methods		void RequestSession()
 			resolving. Use StopDiscovery to release the sessions
 			acquired.
 
-			This process will start emitting DevicesFound and
-			PropertyChanged "Discovering" signals.
+			This process will start creating Device objects as
+			new devices are discovered.
 
 			Possible errors: org.bluez.Error.NotReady
 					 org.bluez.Error.Failed
@@ -107,15 +107,6 @@ Methods		void RequestSession()
 
 			Possible errors: org.bluez.Error.DoesNotExist
 
-Signals		DevicesFound(array{object path, dict values})
-
-			This signal will be sent every time devices have
-			been found. In general they only appear during a
-			device discovery.
-
-			The dictionary contains the properties from the
-			org.bluez.Device interface.
-
 Properties	string Address [readonly]
 
 			The Bluetooth device address.
diff --git a/src/adapter.c b/src/adapter.c
index 1f02f0b..9c4765e 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -82,7 +82,6 @@
 #define IO_CAPABILITY_INVALID		0xFF
 
 #define REMOVE_TEMP_TIMEOUT (3 * 60)
-#define PENDING_FOUND_MAX 5
 
 #define SETTINGS_PATH STORAGEDIR "/%s/settings"
 #define CACHE_PATH STORAGEDIR "/%s/cache/%s"
@@ -119,9 +118,7 @@ struct service_auth {
 };
 
 struct discovery {
-	guint id;
 	GSList *found;
-	GSList *pending;
 };
 
 struct btd_adapter {
@@ -560,49 +557,6 @@ static uint8_t get_needed_mode(struct btd_adapter *adapter, uint8_t mode)
 	return mode;
 }
 
-static void send_devices_found(struct btd_adapter *adapter)
-{
-	struct discovery *discovery = adapter->discovery;
-	DBusConnection *conn = btd_get_dbus_connection();
-	DBusMessageIter iter, props;
-	DBusMessage *signal;
-
-	if (!discovery || !discovery->pending)
-		return;
-
-	signal = dbus_message_new_signal(adapter->path, ADAPTER_INTERFACE,
-							"DevicesFound");
-	if (!signal) {
-		error("Unable to allocate DevicesFound signal");
-		g_slist_free(discovery->pending);
-		discovery->pending = NULL;
-		return;
-	}
-
-	dbus_message_iter_init_append(signal, &iter);
-	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{oa{sv}}",
-								&props);
-
-	while (discovery->pending) {
-		struct btd_device *dev = discovery->pending->data;
-		const char *path = device_get_path(dev);
-		DBusMessageIter entry;
-
-		dbus_message_iter_open_container(&props, DBUS_TYPE_DICT_ENTRY,
-								NULL, &entry);
-		dbus_message_iter_append_basic(&entry, DBUS_TYPE_OBJECT_PATH,
-									&path);
-		g_dbus_get_properties(conn, path, DEVICE_INTERFACE, &entry);
-		dbus_message_iter_close_container(&props, &entry);
-
-		discovery->pending = g_slist_remove(discovery->pending, dev);
-	}
-
-	dbus_message_iter_close_container(&iter, &props);
-
-	g_dbus_send_message(conn, signal);
-}
-
 static void invalidate_rssi(gpointer a)
 {
 	struct btd_device *dev = a;
@@ -617,11 +571,6 @@ static void discovery_cleanup(struct btd_adapter *adapter)
 	if (!discovery)
 		return;
 
-	if (discovery->id > 0)
-		g_source_remove(discovery->id);
-
-	send_devices_found(adapter);
-
 	adapter->discovery = NULL;
 
 	g_slist_free_full(discovery->found, invalidate_rssi);
@@ -1057,10 +1006,8 @@ void adapter_remove_device(struct btd_adapter *adapter,
 
 	adapter->devices = g_slist_remove(adapter->devices, dev);
 
-	if (discovery) {
+	if (discovery)
 		discovery->found = g_slist_remove(discovery->found, dev);
-		discovery->pending = g_slist_remove(discovery->pending, dev);
-	}
 
 	adapter->connections = g_slist_remove(adapter->connections, dev);
 
@@ -1673,12 +1620,6 @@ static const GDBusMethodTable adapter_methods[] = {
 	{ }
 };
 
-static const GDBusSignalTable adapter_signals[] = {
-	{ GDBUS_SIGNAL("DevicesFound",
-			GDBUS_ARGS({ "devices", "a{oa{sv}}" })) },
-	{ }
-};
-
 static const GDBusPropertyTable adapter_properties[] = {
 	{ "Address", "s", adapter_property_get_address },
 	{ "Name", "s", adapter_property_get_name, adapter_property_set_name },
@@ -2740,7 +2681,7 @@ struct btd_adapter *adapter_create(int id)
 
 	if (!g_dbus_register_interface(btd_get_dbus_connection(),
 					path, ADAPTER_INTERFACE,
-					adapter_methods, adapter_signals,
+					adapter_methods, NULL,
 					adapter_properties, adapter,
 					adapter_free)) {
 		error("Adapter interface init failed on path %s", path);
@@ -2803,26 +2744,6 @@ void adapter_set_allow_name_changes(struct btd_adapter *adapter,
 	adapter->allow_name_changes = allow_name_changes;
 }
 
-static gboolean send_found(gpointer user_data)
-{
-	struct btd_adapter *adapter = user_data;
-	struct discovery *discovery = adapter->discovery;
-
-	if (!discovery)
-		return FALSE;
-
-	discovery->id = 0;
-
-	if (!discovery->pending)
-		return FALSE;
-
-	send_devices_found(adapter);
-
-	discovery->id = g_timeout_add_seconds(1, send_found, adapter);
-
-	return FALSE;
-}
-
 static gboolean adapter_remove_temp(gpointer data)
 {
 	struct btd_adapter *adapter = data;
@@ -2847,14 +2768,11 @@ static gboolean adapter_remove_temp(gpointer data)
 void adapter_set_discovering(struct btd_adapter *adapter,
 						gboolean discovering)
 {
-	struct discovery *discovery;
 	guint connect_list_len;
 
 	if (discovering && !adapter->discovery)
 		adapter->discovery = g_new0(struct discovery, 1);
 
-	discovery = adapter->discovery;
-
 	g_dbus_emit_property_changed(btd_get_dbus_connection(), adapter->path,
 					ADAPTER_INTERFACE, "Discovering");
 
@@ -2863,7 +2781,6 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 			g_source_remove(adapter->remove_temp);
 			adapter->remove_temp = 0;
 		}
-		discovery->id = g_timeout_add_seconds(1, send_found, adapter);
 		return;
 	}
 
@@ -3016,21 +2933,6 @@ void adapter_update_found_devices(struct btd_adapter *adapter,
 
 	eir_data_free(&eir_data);
 
-	if (!g_slist_find(discovery->pending, dev)) {
-		guint pending_count;
-
-		discovery->pending = g_slist_prepend(discovery->pending, dev);
-
-		pending_count = g_slist_length(discovery->pending);
-
-		if (discovery->id == 0) {
-			discovery->id = g_idle_add(send_found, adapter);
-		} else if (pending_count > PENDING_FOUND_MAX) {
-			g_source_remove(discovery->id);
-			discovery->id = g_idle_add(send_found, adapter);
-		}
-	}
-
 	if (g_slist_find(discovery->found, dev))
 		return;
 
-- 
1.7.11.7

Re: [RFC v0 5/7] adapter: Remove DevicesFound signal

[Johan Hedberg]

Hi Mikel,

On Thu, Nov 08, 2012, Mikel Astiz wrote:
> ObjectManager reports the D-Bus interfaces of all known devices,
> including the ones detected during discovery. Therefore this signal is
> not required.
> ---
>  doc/adapter-api.txt |  13 ++-----
>  src/adapter.c       | 102 ++--------------------------------------------------
>  2 files changed, 4 insertions(+), 111 deletions(-)

If you look at the commit history you'll see that this signal was added
after the introduction of object manager and was part of the patches
that removed the old DeviceFound signal.

You're right in that our original plan was to replace the device
discovery completely with ObjectManager signals but that turned out to
be infeasible in practice. Even the python script for doing discovery
ended up being quite complex (meaning most other languages would be even
worse) and we didn't get any signals for discovered devices that already
existed in the bluetoothd object hierarchy (since nothing changes for
them from an ObjectManager perspective).

Due to these issues we decided to reintroduce a dedicated signal for
device discovery and at the same time fix the bottle neck we've seen
during the past years where the high frequency of signals in a busy
environment (e.g. UPF) consumes a lot of resources.

The DevicesFound signal is emitted once a second and contains all
devices found since the last period. If no device was found during the
last period the signal is sent as soon as the first device is found.
There's also an (adjustable through a define, right now 5) maximum size
for the list length which forces the signal to be sent as soon as the
limit is reached.

Johan

Re: [RFC v0 5/7] adapter: Remove DevicesFound signal

[Mikel Astiz]

Hi Johan,

On Sun, Nov 11, 2012 at 2:19 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Mikel,
>
> On Thu, Nov 08, 2012, Mikel Astiz wrote:
>> ObjectManager reports the D-Bus interfaces of all known devices,
>> including the ones detected during discovery. Therefore this signal is
>> not required.
>> ---
>>  doc/adapter-api.txt |  13 ++-----
>>  src/adapter.c       | 102 ++--------------------------------------------------
>>  2 files changed, 4 insertions(+), 111 deletions(-)
>
> If you look at the commit history you'll see that this signal was added
> after the introduction of object manager and was part of the patches
> that removed the old DeviceFound signal.

I didn't realize this was such a late addition. I guess I should have
opened the discussion before, sorry for that.

> You're right in that our original plan was to replace the device
> discovery completely with ObjectManager signals but that turned out to
> be infeasible in practice. Even the python script for doing discovery
> ended up being quite complex (meaning most other languages would be even

The ObjectManager approach might make our clients more complex but I
don't think this should be very strong argument to introduce
duplication in our APIs. I would suggest we consider compensating this
using utility libraries, since you have very similar problems
everywhere in the D-Bus API, for example with Adapter.FindDevice.

Unless there is some performance gain, we should IMO keep our APIs as
compact as possible, specially for the 5.0 release, where we could
extend the API in later versions.

> worse) and we didn't get any signals for discovered devices that already
> existed in the bluetoothd object hierarchy (since nothing changes for
> them from an ObjectManager perspective).

I agree this second point would be a strong enough argument. However,
can't we solve this problem in a nicer and more orthogonal
property-oriented way?

I can think of a solution that might fit and would also be useful for
other use-cases: we could add a property to each device which holds a
timestamp representing when this particular devices was "seen"
(discovered or connected-to) for the last time. A discovery client
would have to check the timestamp range he is interested in. I could
write some code if there is any interest.

Having said that, I'm not completely against the DevicesFound
approach, but I guess this would be the moment to consider other
approaches as well.

> Due to these issues we decided to reintroduce a dedicated signal for
> device discovery and at the same time fix the bottle neck we've seen
> during the past years where the high frequency of signals in a busy
> environment (e.g. UPF) consumes a lot of resources.

I can't imagine how DevicesFound could remove any D-Bus bottleneck
since it's a duplicated signal. As I understand, ObjectManager would
emit the InterfaceAdded signals in any case, so I don't see any real
benefit here.

> The DevicesFound signal is emitted once a second and contains all
> devices found since the last period. If no device was found during the
> last period the signal is sent as soon as the first device is found.
> There's also an (adjustable through a define, right now 5) maximum size
> for the list length which forces the signal to be sent as soon as the
> limit is reached.

The only gain I can think of is in case we had a client that listens
to DevicesAdded but is not interested in ObjectManager/Properties
signals. However this seems unlikely and even insignificant, since
most of the clients would anyway be waking up every time an interface
is added, right?

Cheers,
Mikel

[RFC v0 6/7] adapter: Remove redundant Devices property

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The ObjectManager interface already reports the list of devices, so the
the property can be entirely removed.
---
 doc/adapter-api.txt |  4 ----
 src/adapter.c       | 30 ------------------------------
 2 files changed, 34 deletions(-)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index 13941c9..98a3919 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -171,10 +171,6 @@ Properties	string Address [readonly]
 
 			Indicates that a device discovery procedure is active.
 
-		array{object} Devices [readonly]
-
-			List of device object paths.
-
 		array{string} UUIDs [readonly]
 
 			List of 128-bit UUIDs that represents the available
diff --git a/src/adapter.c b/src/adapter.c
index 9c4765e..4d9820f 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -974,9 +974,6 @@ static struct btd_device *adapter_create_device(struct btd_adapter *adapter,
 
 	adapter->devices = g_slist_append(adapter->devices, device);
 
-	g_dbus_emit_property_changed(btd_get_dbus_connection(),
-				adapter->path, ADAPTER_INTERFACE, "Devices");
-
 	return device;
 }
 
@@ -1027,9 +1024,6 @@ void adapter_remove_device(struct btd_adapter *adapter,
 		service_auth_cancel(auth);
 	}
 
-	g_dbus_emit_property_changed(btd_get_dbus_connection(),
-				adapter->path, ADAPTER_INTERFACE, "Devices");
-
 	device_remove(dev, remove_storage);
 }
 
@@ -1335,29 +1329,6 @@ static gboolean adapter_property_get_discovering(
 	return TRUE;
 }
 
-static gboolean adapter_property_get_devices(
-					const GDBusPropertyTable *property,
-					DBusMessageIter *iter, void *data)
-{
-	struct btd_adapter *adapter = data;
-	DBusMessageIter entry;
-	GSList *l;
-
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
-				DBUS_TYPE_OBJECT_PATH_AS_STRING, &entry);
-
-	for (l = adapter->devices; l != NULL; l = l->next) {
-		const char *path = device_get_path(l->data);
-
-		dbus_message_iter_append_basic(&entry, DBUS_TYPE_OBJECT_PATH,
-								&path);
-	}
-
-	dbus_message_iter_close_container(iter, &entry);
-
-	return TRUE;
-}
-
 static gboolean adapter_property_get_uuids(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
@@ -1636,7 +1607,6 @@ static const GDBusPropertyTable adapter_properties[] = {
 	{ "PairableTimeout", "u", adapter_property_get_pairable_timeout,
 				adapter_property_set_pairable_timeout },
 	{ "Discovering", "b", adapter_property_get_discovering },
-	{ "Devices", "ao", adapter_property_get_devices },
 	{ "UUIDs", "as", adapter_property_get_uuids },
 	{ }
 };
-- 
1.7.11.7

[RFC v0 7/7] adapter: Remove FindDevice method from D-Bus API

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

ObjectManager.GetManagedObjects() returns all devices and their
corresponding properties to any interested client. The device address is
included in the property dictionary and therefore having such a
FindDevice method is an unnecessary duplication.
---
 doc/adapter-api.txt |  7 -------
 src/adapter.c       | 38 --------------------------------------
 2 files changed, 45 deletions(-)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index 98a3919..f976992 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -64,13 +64,6 @@ Methods		void RequestSession()
 					 org.bluez.Error.Failed
 					 org.bluez.Error.NotAuthorized
 
-		object FindDevice(string address)
-
-			Returns the object path of device for given address.
-
-			Possible Errors: org.bluez.Error.DoesNotExist
-					 org.bluez.Error.InvalidArguments
-
 		void RemoveDevice(object device)
 
 			This removes the remote device object at the given
diff --git a/src/adapter.c b/src/adapter.c
index 4d9820f..2be01d6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1472,40 +1472,6 @@ static DBusMessage *remove_device(DBusConnection *conn, DBusMessage *msg,
 	return NULL;
 }
 
-static DBusMessage *find_device(DBusConnection *conn, DBusMessage *msg,
-								void *data)
-{
-	struct btd_adapter *adapter = data;
-	struct btd_device *device;
-	DBusMessage *reply;
-	const gchar *address;
-	GSList *l;
-	const gchar *dev_path;
-
-	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &address,
-						DBUS_TYPE_INVALID))
-		return btd_error_invalid_args(msg);
-
-	l = g_slist_find_custom(adapter->devices,
-			address, (GCompareFunc) device_address_cmp);
-	if (!l)
-		return btd_error_does_not_exist(msg);
-
-	device = l->data;
-
-	reply = dbus_message_new_method_return(msg);
-	if (!reply)
-		return NULL;
-
-	dev_path = device_get_path(device);
-
-	dbus_message_append_args(reply,
-				DBUS_TYPE_OBJECT_PATH, &dev_path,
-				DBUS_TYPE_INVALID);
-
-	return reply;
-}
-
 static void agent_removed(struct agent *agent, struct btd_adapter *adapter)
 {
 	mgmt_set_io_capability(adapter->dev_id, IO_CAPABILITY_NOINPUTNOOUTPUT);
@@ -1577,10 +1543,6 @@ static const GDBusMethodTable adapter_methods[] = {
 	{ GDBUS_ASYNC_METHOD("RemoveDevice",
 			GDBUS_ARGS({ "device", "o" }), NULL,
 			remove_device) },
-	{ GDBUS_METHOD("FindDevice",
-			GDBUS_ARGS({ "address", "s" }),
-			GDBUS_ARGS({ "device", "o" }),
-			find_device) },
 	{ GDBUS_METHOD("RegisterAgent",
 			GDBUS_ARGS({ "agent", "o" },
 					{ "capability", "s" }), NULL,
-- 
1.7.11.7

Re: [RFC v0 0/7] Manager/Adapter transition to ObjectManager

[Johan Hedberg]

Hi Mikel,

On Thu, Nov 08, 2012, Mikel Astiz wrote:
> (This is sent as RFC since I haven't done proper testing, and
> furthermore the test scripts haven't been updated)
> 
> While thinking about how profile and their states should be exposed in
> D-Bus (see previous RFC), I found several potential improvements in
> the current version of the Manager and Adapter APIs.
> 
> Basically, this patchset removes some properties and methods that seem
> duplicated.
> 
> There are other similar methods/properties that could be simplified as
> well, but for practical reasons I started with the most obvious ones.
> 
> Mikel Astiz (7):
>   manager: Expose default adapter using property
>   manager: Remove redundant D-Bus signals
>   manager: Remove redundant Adapters property
>   adapter: Remove redundant D-Bus signals
>   adapter: Remove DevicesFound signal
>   adapter: Remove redundant Devices property
>   adapter: Remove FindDevice method from D-Bus API
> 
>  doc/adapter-api.txt |  32 +--------
>  doc/manager-api.txt |  28 +-------
>  src/adapter.c       | 187 +---------------------------------------------------
>  src/manager.c       | 103 +++++++----------------------
>  4 files changed, 29 insertions(+), 321 deletions(-)

All patches except 5/7 (which needs a bit more discussion) have been
applied. Thanks.

Johan

Re: [RFC v0 0/7] Manager/Adapter transition to ObjectManager

[Johan Hedberg]

Hi Mikel,

> > Mikel Astiz (7):
> >   manager: Expose default adapter using property
> >   manager: Remove redundant D-Bus signals
> >   manager: Remove redundant Adapters property
> >   adapter: Remove redundant D-Bus signals
> >   adapter: Remove DevicesFound signal
> >   adapter: Remove redundant Devices property
> >   adapter: Remove FindDevice method from D-Bus API
> > 
> >  doc/adapter-api.txt |  32 +--------
> >  doc/manager-api.txt |  28 +-------
> >  src/adapter.c       | 187 +---------------------------------------------------
> >  src/manager.c       | 103 +++++++----------------------
> >  4 files changed, 29 insertions(+), 321 deletions(-)
> 
> All patches except 5/7 (which needs a bit more discussion) have been
> applied. Thanks.

I had to revert all of these since I didn't realize you hadn't updated
the test scripts (all of which broke with these patches). Also, it seems
the DefaultAdapter patch is also broken since I never saw that property
(e.g. with test/get-managed-objects). Please send a new patch set once
you've got the scripts converted and the DefaultAdapter patch fixed.
Thanks.

Johan

Re: [RFC v0 0/7] Manager/Adapter transition to ObjectManager

[Mikel Astiz]

Hi Johan,

On Mon, Nov 12, 2012 at 8:26 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Mikel,
>
>> > Mikel Astiz (7):
>> >   manager: Expose default adapter using property
>> >   manager: Remove redundant D-Bus signals
>> >   manager: Remove redundant Adapters property
>> >   adapter: Remove redundant D-Bus signals
>> >   adapter: Remove DevicesFound signal
>> >   adapter: Remove redundant Devices property
>> >   adapter: Remove FindDevice method from D-Bus API
>> >
>> >  doc/adapter-api.txt |  32 +--------
>> >  doc/manager-api.txt |  28 +-------
>> >  src/adapter.c       | 187 +---------------------------------------------------
>> >  src/manager.c       | 103 +++++++----------------------
>> >  4 files changed, 29 insertions(+), 321 deletions(-)
>>
>> All patches except 5/7 (which needs a bit more discussion) have been
>> applied. Thanks.
>
> I had to revert all of these since I didn't realize you hadn't updated
> the test scripts (all of which broke with these patches). Also, it seems
> the DefaultAdapter patch is also broken since I never saw that property
> (e.g. with test/get-managed-objects). Please send a new patch set once
> you've got the scripts converted and the DefaultAdapter patch fixed.
> Thanks.

You're right, we should keep the test scripts in sync all the time. I
tried to warn you about this in the cover-letter but no problem, I
will send v2 with the changes you suggest.

Regarding the DefaultAdapter property, it was working for me, but I
was using the Properties interface. The test script you mention is
indeed not reporting the property, but this is because the root object
is not a "managed object", and this property is associated to the
root. This is no real issue IMO but still, for convenience reasons: do
you think we should move the org.bluez.Manager interface to some other
object path such as /adapters? As a side effect, this could make it
possible that clients filter out signals (i.e. InterfacesAdded) based
on the object path.

Cheers,
Mikel