* [RFC v1 0/7] One remote UUID per btd_profile
@ 2013-02-06 9:16 Mikel Astiz
2013-02-06 9:16 ` [RFC v1 1/7] avrcp: Refactor server registration Mikel Astiz
` (6 more replies)
0 siblings, 7 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
There was no feedback for v0, and therefore I resubmit this patchset rebased and with the following main changes:
- AVRCP gets split but both roles share a common config in audio.conf
- Health profile also split into two profiles (patch v1 6/7)
- Core simplified by limiting to one UUID per btd_profile (patch v1 7/7)
There's a lot more that code be simplified after 7/7 but this has been left aside for the moment.
>From original cover-letter:
Disclaimer: this RFC is WIP and hasn't been tested, specially because I don't have the necessary hardware for it.
The goal is to have one remote_uuid per btd_profile. There are only a few exceptions to this rule, and this patchset tries to fix three of them. This basically means splitting one btd_profile per role.
Mikel Astiz (7):
avrcp: Refactor server registration
audio: Split AVRCP into two btd_profile
proximity: Split internal monitor registration API
proximity: Split monitor into three btd_profile
gatt: List only GATT_UUID as remote UUID
health: Split health into two btd_profile
profile: Limit to one remote UUID per profile
profiles/audio/avrcp.c | 114 +++++++++++++-----
profiles/audio/avrcp.h | 6 +-
profiles/audio/manager.c | 75 ++++++++----
profiles/cyclingspeed/cyclingspeed.c | 2 +-
profiles/deviceinfo/deviceinfo.c | 2 +-
profiles/gatt/gas.c | 2 +-
profiles/health/hdp_manager.c | 20 +++-
profiles/heartrate/heartrate.c | 2 +-
profiles/input/hog.c | 2 +-
profiles/input/manager.c | 2 +-
profiles/network/manager.c | 6 +-
profiles/proximity/manager.c | 100 ++++++++++++----
profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++-------
profiles/proximity/monitor.h | 17 ++-
profiles/scanparam/scan.c | 2 +-
profiles/thermometer/thermometer.c | 2 +-
src/device.c | 49 +++-----
src/profile.c | 25 ++--
src/profile.h | 4 +-
19 files changed, 466 insertions(+), 186 deletions(-)
--
1.8.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC v1 1/7] avrcp: Refactor server registration
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-06 9:16 ` [RFC v1 2/7] audio: Split AVRCP into two btd_profile Mikel Astiz
` (5 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Use a helper function to install the AVRCP server, just like other audio
profiles such as in a2dp.c do.
---
profiles/audio/avrcp.c | 54 ++++++++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 00eeea1..277f9f3 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2681,9 +2681,9 @@ void avrcp_disconnect(struct audio_device *dev)
avctp_disconnect(session);
}
-int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
+static struct avrcp_server *avrcp_server_register(struct btd_adapter *adapter,
+ GKeyFile *config)
{
- sdp_record_t *record;
gboolean tmp, master = TRUE;
GError *err = NULL;
struct avrcp_server *server;
@@ -2698,18 +2698,39 @@ int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
master = tmp;
}
+ if (avctp_register(adapter, master) < 0)
+ return NULL;
+
server = g_new0(struct avrcp_server, 1);
+ server->adapter = btd_adapter_ref(adapter);
+
+ servers = g_slist_append(servers, server);
+
+ if (!avctp_id)
+ avctp_id = avctp_add_state_cb(state_changed, NULL);
+
+ return server;
+}
+
+int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
+{
+ sdp_record_t *record;
+ struct avrcp_server *server;
+
+ server = avrcp_server_register(adapter, config);
+ if (server == NULL)
+ return -EPROTONOSUPPORT;
record = avrcp_tg_record();
if (!record) {
error("Unable to allocate new service record");
- g_free(server);
+ avrcp_unregister(adapter);
return -1;
}
if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
error("Unable to register AVRCP target service record");
- g_free(server);
+ avrcp_unregister(adapter);
sdp_record_free(record);
return -1;
}
@@ -2718,32 +2739,18 @@ int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
record = avrcp_ct_record();
if (!record) {
error("Unable to allocate new service record");
- g_free(server);
+ avrcp_unregister(adapter);
return -1;
}
if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
error("Unable to register AVRCP service record");
sdp_record_free(record);
- g_free(server);
+ avrcp_unregister(adapter);
return -1;
}
server->ct_record_id = record->handle;
- if (avctp_register(adapter, master) < 0) {
- remove_record_from_server(server->ct_record_id);
- remove_record_from_server(server->tg_record_id);
- g_free(server);
- return -1;
- }
-
- server->adapter = btd_adapter_ref(adapter);
-
- servers = g_slist_append(servers, server);
-
- if (!avctp_id)
- avctp_id = avctp_add_state_cb(state_changed, NULL);
-
return 0;
}
@@ -2760,8 +2767,11 @@ void avrcp_unregister(struct btd_adapter *adapter)
servers = g_slist_remove(servers, server);
- remove_record_from_server(server->ct_record_id);
- remove_record_from_server(server->tg_record_id);
+ if (server->ct_record_id != 0)
+ remove_record_from_server(server->ct_record_id);
+
+ if (server->tg_record_id != 0)
+ remove_record_from_server(server->tg_record_id);
avctp_unregister(server->adapter);
btd_adapter_unref(server->adapter);
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 2/7] audio: Split AVRCP into two btd_profile
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 1/7] avrcp: Refactor server registration Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-07 9:43 ` Luiz Augusto von Dentz
2013-02-06 9:16 ` [RFC v1 3/7] proximity: Split internal monitor registration API Mikel Astiz
` (4 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Register a separate btd_profile for each role of AVRCP.
---
profiles/audio/avrcp.c | 80 +++++++++++++++++++++++++++++++++++++-----------
profiles/audio/avrcp.h | 6 ++--
profiles/audio/manager.c | 71 ++++++++++++++++++++++++++++++------------
3 files changed, 118 insertions(+), 39 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 277f9f3..fe6333b 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2712,41 +2712,63 @@ static struct avrcp_server *avrcp_server_register(struct btd_adapter *adapter,
return server;
}
-int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
+int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config)
{
sdp_record_t *record;
struct avrcp_server *server;
+ server = find_server(servers, adapter);
+ if (server != NULL)
+ goto done;
+
server = avrcp_server_register(adapter, config);
if (server == NULL)
return -EPROTONOSUPPORT;
+done:
record = avrcp_tg_record();
if (!record) {
error("Unable to allocate new service record");
- avrcp_unregister(adapter);
+ avrcp_target_unregister(adapter);
return -1;
}
if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
error("Unable to register AVRCP target service record");
- avrcp_unregister(adapter);
+ avrcp_target_unregister(adapter);
sdp_record_free(record);
return -1;
}
server->tg_record_id = record->handle;
+ return 0;
+}
+
+int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config)
+{
+ sdp_record_t *record;
+ struct avrcp_server *server;
+
+ server = find_server(servers, adapter);
+ if (server != NULL)
+ goto done;
+
+ server = avrcp_server_register(adapter, config);
+ if (server == NULL)
+ return -EPROTONOSUPPORT;
+
+done:
record = avrcp_ct_record();
if (!record) {
error("Unable to allocate new service record");
- avrcp_unregister(adapter);
+ avrcp_remote_unregister(adapter);
return -1;
}
if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
error("Unable to register AVRCP service record");
sdp_record_free(record);
- avrcp_unregister(adapter);
+ avrcp_remote_unregister(adapter);
return -1;
}
server->ct_record_id = record->handle;
@@ -2754,25 +2776,13 @@ int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
return 0;
}
-void avrcp_unregister(struct btd_adapter *adapter)
+static void avrcp_server_unregister(struct avrcp_server *server)
{
- struct avrcp_server *server;
-
- server = find_server(servers, adapter);
- if (!server)
- return;
-
g_slist_free_full(server->sessions, g_free);
g_slist_free_full(server->players, player_destroy);
servers = g_slist_remove(servers, server);
- if (server->ct_record_id != 0)
- remove_record_from_server(server->ct_record_id);
-
- if (server->tg_record_id != 0)
- remove_record_from_server(server->tg_record_id);
-
avctp_unregister(server->adapter);
btd_adapter_unref(server->adapter);
g_free(server);
@@ -2786,6 +2796,40 @@ void avrcp_unregister(struct btd_adapter *adapter)
}
}
+void avrcp_target_unregister(struct btd_adapter *adapter)
+{
+ struct avrcp_server *server;
+
+ server = find_server(servers, adapter);
+ if (!server)
+ return;
+
+ if (server->tg_record_id != 0) {
+ remove_record_from_server(server->tg_record_id);
+ server->tg_record_id = 0;
+ }
+
+ if (server->ct_record_id == 0)
+ avrcp_server_unregister(server);
+}
+
+void avrcp_remote_unregister(struct btd_adapter *adapter)
+{
+ struct avrcp_server *server;
+
+ server = find_server(servers, adapter);
+ if (!server)
+ return;
+
+ if (server->ct_record_id != 0) {
+ remove_record_from_server(server->ct_record_id);
+ server->ct_record_id = 0;
+ }
+
+ if (server->tg_record_id == 0)
+ avrcp_server_unregister(server);
+}
+
struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
struct avrcp_player_cb *cb,
void *user_data,
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index 3799da1..1f98090 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -93,8 +93,10 @@ struct avrcp_player_cb {
void *user_data);
};
-int avrcp_register(struct btd_adapter *adapter, GKeyFile *config);
-void avrcp_unregister(struct btd_adapter *adapter);
+int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config);
+void avrcp_target_unregister(struct btd_adapter *adapter);
+int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config);
+void avrcp_remote_unregister(struct btd_adapter *adapter);
gboolean avrcp_connect(struct audio_device *dev);
void avrcp_disconnect(struct audio_device *dev);
diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
index 934227e..3023249 100644
--- a/profiles/audio/manager.c
+++ b/profiles/audio/manager.c
@@ -229,7 +229,7 @@ static int a2dp_sink_disconnect(struct btd_device *dev,
return sink_disconnect(audio_dev, FALSE);
}
-static int avrcp_control_connect(struct btd_device *dev,
+static int avrcp_target_connect(struct btd_device *dev,
struct btd_profile *profile)
{
const char *path = device_get_path(dev);
@@ -246,7 +246,7 @@ static int avrcp_control_connect(struct btd_device *dev,
return control_connect(audio_dev);
}
-static int avrcp_control_disconnect(struct btd_device *dev,
+static int avrcp_target_disconnect(struct btd_device *dev,
struct btd_profile *profile)
{
const char *path = device_get_path(dev);
@@ -295,20 +295,36 @@ static void a2dp_sink_server_remove(struct btd_profile *p,
a2dp_sink_unregister(adapter);
}
-static int avrcp_server_probe(struct btd_profile *p,
+static int avrcp_target_server_probe(struct btd_profile *p,
struct btd_adapter *adapter)
{
DBG("path %s", adapter_get_path(adapter));
- return avrcp_register(adapter, config);
+ return avrcp_target_register(adapter, config);
}
-static void avrcp_server_remove(struct btd_profile *p,
+static int avrcp_remote_server_probe(struct btd_profile *p,
struct btd_adapter *adapter)
{
DBG("path %s", adapter_get_path(adapter));
- return avrcp_unregister(adapter);
+ return avrcp_remote_register(adapter, config);
+}
+
+static void avrcp_target_server_remove(struct btd_profile *p,
+ struct btd_adapter *adapter)
+{
+ DBG("path %s", adapter_get_path(adapter));
+
+ avrcp_target_unregister(adapter);
+}
+
+static void avrcp_remote_server_remove(struct btd_profile *p,
+ struct btd_adapter *adapter)
+{
+ DBG("path %s", adapter_get_path(adapter));
+
+ avrcp_remote_unregister(adapter);
}
static int media_server_probe(struct btd_adapter *adapter)
@@ -357,19 +373,30 @@ static struct btd_profile a2dp_sink_profile = {
.adapter_remove = a2dp_sink_server_remove,
};
-static struct btd_profile avrcp_profile = {
- .name = "audio-avrcp",
+static struct btd_profile avrcp_target_profile = {
+ .name = "audio-avrcp-target",
- .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID, AVRCP_REMOTE_UUID),
+ .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID),
.device_probe = avrcp_probe,
.device_remove = audio_remove,
.auto_connect = true,
- .connect = avrcp_control_connect,
- .disconnect = avrcp_control_disconnect,
+ .connect = avrcp_target_connect,
+ .disconnect = avrcp_target_disconnect,
+
+ .adapter_probe = avrcp_target_server_probe,
+ .adapter_remove = avrcp_target_server_remove,
+};
+
+static struct btd_profile avrcp_remote_profile = {
+ .name = "audio-avrcp-control",
- .adapter_probe = avrcp_server_probe,
- .adapter_remove = avrcp_server_remove,
+ .remote_uuids = BTD_UUIDS(AVRCP_REMOTE_UUID),
+ .device_probe = avrcp_probe,
+ .device_remove = audio_remove,
+
+ .adapter_probe = avrcp_remote_server_probe,
+ .adapter_remove = avrcp_remote_server_remove,
};
static struct btd_adapter_driver media_driver = {
@@ -400,12 +427,14 @@ void audio_source_disconnected(struct btd_device *dev, int err)
void audio_control_connected(struct btd_device *dev, int err)
{
- device_profile_connected(dev, &avrcp_profile, err);
+ device_profile_connected(dev, &avrcp_target_profile, err);
+ device_profile_connected(dev, &avrcp_remote_profile, err);
}
void audio_control_disconnected(struct btd_device *dev, int err)
{
- device_profile_disconnected(dev, &avrcp_profile, err);
+ device_profile_disconnected(dev, &avrcp_target_profile, err);
+ device_profile_disconnected(dev, &avrcp_remote_profile, err);
}
int audio_manager_init(GKeyFile *conf)
@@ -449,8 +478,10 @@ proceed:
if (enabled.sink)
btd_profile_register(&a2dp_sink_profile);
- if (enabled.control)
- btd_profile_register(&avrcp_profile);
+ if (enabled.control) {
+ btd_profile_register(&avrcp_remote_profile);
+ btd_profile_register(&avrcp_target_profile);
+ }
btd_register_adapter_driver(&media_driver);
@@ -470,8 +501,10 @@ void audio_manager_exit(void)
if (enabled.sink)
btd_profile_unregister(&a2dp_sink_profile);
- if (enabled.control)
- btd_profile_unregister(&avrcp_profile);
+ if (enabled.control) {
+ btd_profile_unregister(&avrcp_remote_profile);
+ btd_profile_unregister(&avrcp_target_profile);
+ }
btd_unregister_adapter_driver(&media_driver);
}
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 3/7] proximity: Split internal monitor registration API
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 1/7] avrcp: Refactor server registration Mikel Astiz
2013-02-06 9:16 ` [RFC v1 2/7] audio: Split AVRCP into two btd_profile Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-14 16:37 ` Claudio Takahasi
2013-02-06 9:16 ` [RFC v1 4/7] proximity: Split monitor into three btd_profile Mikel Astiz
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Split the monitor registration API into three independent registrations
each of them taking one specific GATT primary.
---
profiles/proximity/manager.c | 16 +++-
profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
profiles/proximity/monitor.h | 17 +++-
3 files changed, 204 insertions(+), 49 deletions(-)
diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
index b405f15..1c07267 100644
--- a/profiles/proximity/manager.c
+++ b/profiles/proximity/manager.c
@@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
struct btd_device *device, GSList *uuids)
{
struct gatt_primary *linkloss, *txpower, *immediate;
+ int err = 0;
immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
txpower = btd_device_get_primary(device, TX_POWER_UUID);
linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
- return monitor_register(device, linkloss, txpower, immediate, &enabled);
+ if (linkloss)
+ err = monitor_register_linkloss(device, &enabled, linkloss);
+
+ if (err >= 0 && txpower)
+ err = monitor_register_txpower(device, &enabled, txpower);
+
+ if (err >= 0 && immediate)
+ err = monitor_register_immediate(device, &enabled, immediate);
+
+ return err;
}
static void monitor_device_remove(struct btd_profile *p,
struct btd_device *device)
{
- monitor_unregister(device);
+ monitor_unregister_immediate(device);
+ monitor_unregister_txpower(device);
+ monitor_unregister_linkloss(device);
}
static struct btd_profile pxp_monitor_profile = {
diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
index 597f161..489ccdd 100644
--- a/profiles/proximity/monitor.c
+++ b/profiles/proximity/monitor.c
@@ -34,6 +34,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
+#include <glib.h>
#include <bluetooth/bluetooth.h>
@@ -83,6 +84,22 @@ struct monitor {
guint attioid;
};
+static GSList *monitors = NULL;
+
+static struct monitor *find_monitor(struct btd_device *device)
+{
+ GSList *l;
+
+ for (l = monitors; l; l = l->next) {
+ struct monitor *monitor = l->data;
+
+ if (monitor->device == device)
+ return monitor;
+ }
+
+ return NULL;
+}
+
static void write_proximity_config(struct btd_device *device, const char *alert,
const char *level)
{
@@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
{
struct monitor *monitor = user_data;
- if (monitor->immediateto)
- g_source_remove(monitor->immediateto);
-
- if (monitor->attioid)
- btd_device_remove_attio_callback(monitor->device,
- monitor->attioid);
- if (monitor->attrib)
- g_attrib_unref(monitor->attrib);
-
btd_device_unref(monitor->device);
- g_free(monitor->linkloss);
- g_free(monitor->immediate);
- g_free(monitor->txpower);
g_free(monitor->linklosslevel);
g_free(monitor->immediatelevel);
g_free(monitor->signallevel);
g_free(monitor);
+
+ monitors = g_slist_remove(monitors, monitor);
}
-int monitor_register(struct btd_device *device,
- struct gatt_primary *linkloss, struct gatt_primary *txpower,
- struct gatt_primary *immediate, struct enabled *enabled)
+static struct monitor *register_monitor(struct btd_device *device)
{
const char *path = device_get_path(device);
struct monitor *monitor;
char *level;
+ monitor = find_monitor(device);
+ if (monitor != NULL)
+ return monitor;
+
level = read_proximity_config(device, "LinkLossAlertLevel");
monitor = g_new0(struct monitor, 1);
@@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
monitor->signallevel = g_strdup("unknown");
monitor->immediatelevel = g_strdup("none");
+ monitors = g_slist_append(monitors, monitor);
+
if (g_dbus_register_interface(btd_get_dbus_connection(), path,
PROXIMITY_INTERFACE,
NULL, NULL, monitor_device_properties,
@@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
error("D-Bus failed to register %s interface",
PROXIMITY_INTERFACE);
monitor_destroy(monitor);
- return -1;
+ return NULL;
}
DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
- if (linkloss && enabled->linkloss) {
- monitor->linkloss = g_new0(struct att_range, 1);
- monitor->linkloss->start = linkloss->range.start;
- monitor->linkloss->end = linkloss->range.end;
-
- monitor->enabled.linkloss = TRUE;
- }
-
- if (immediate) {
- if (txpower && enabled->pathloss) {
- monitor->txpower = g_new0(struct att_range, 1);
- monitor->txpower->start = txpower->range.start;
- monitor->txpower->end = txpower->range.end;
-
- monitor->enabled.pathloss = TRUE;
- }
-
- if (enabled->pathloss || enabled->findme) {
- monitor->immediate = g_new0(struct att_range, 1);
- monitor->immediate->start = immediate->range.start;
- monitor->immediate->end = immediate->range.end;
- }
+ return monitor;
+}
- monitor->enabled.findme = enabled->findme;
- }
+static void update_monitor(struct monitor *monitor)
+{
+ if (monitor->txpower != NULL && monitor->immediate != NULL)
+ monitor->enabled.pathloss = TRUE;
+ else
+ monitor->enabled.pathloss = FALSE;
DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
monitor->enabled.linkloss ? "TRUE" : "FALSE",
monitor->enabled.pathloss ? "TRUE" : "FALSE",
monitor->enabled.findme ? "TRUE" : "FALSE");
- if (monitor->enabled.linkloss || monitor->enabled.pathloss)
- monitor->attioid = btd_device_add_attio_callback(device,
+ if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
+ return;
+
+ if (monitor->attioid != 0)
+ return;
+
+ monitor->attioid = btd_device_add_attio_callback(monitor->device,
attio_connected_cb,
attio_disconnected_cb,
monitor);
+}
+
+int monitor_register_linkloss(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *linkloss)
+{
+ struct monitor *monitor;
+
+ if (!enabled->linkloss)
+ return 0;
+
+ monitor = register_monitor(device);
+ if (monitor == NULL)
+ return -1;
+
+ monitor->linkloss = g_new0(struct att_range, 1);
+ monitor->linkloss->start = linkloss->range.start;
+ monitor->linkloss->end = linkloss->range.end;
+ monitor->enabled.linkloss = TRUE;
+
+ update_monitor(monitor);
return 0;
}
-void monitor_unregister(struct btd_device *device)
+int monitor_register_txpower(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *txpower)
{
+ struct monitor *monitor;
+
+ if (!enabled->pathloss)
+ return 0;
+
+ monitor = register_monitor(device);
+ if (monitor == NULL)
+ return -1;
+
+ monitor->txpower = g_new0(struct att_range, 1);
+ monitor->txpower->start = txpower->range.start;
+ monitor->txpower->end = txpower->range.end;
+
+ update_monitor(monitor);
+
+ return 0;
+}
+
+int monitor_register_immediate(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *immediate)
+{
+ struct monitor *monitor;
+
+ if (!enabled->pathloss && !enabled->findme)
+ return 0;
+
+ monitor = register_monitor(device);
+ if (monitor == NULL)
+ return -1;
+
+ monitor->immediate = g_new0(struct att_range, 1);
+ monitor->immediate->start = immediate->range.start;
+ monitor->immediate->end = immediate->range.end;
+ monitor->enabled.findme = enabled->findme;
+
+ update_monitor(monitor);
+
+ return 0;
+}
+
+static void cleanup_monitor(struct monitor *monitor)
+{
+ struct btd_device *device = monitor->device;
const char *path = device_get_path(device);
+ if (monitor->immediate != NULL || monitor->txpower != NULL)
+ return;
+
+ if (monitor->immediateto != 0) {
+ g_source_remove(monitor->immediateto);
+ monitor->immediateto = 0;
+ }
+
+ if (monitor->attioid != 0) {
+ btd_device_remove_attio_callback(device, monitor->attioid);
+ monitor->attioid = 0;
+ }
+
+ if (monitor->attrib != NULL) {
+ g_attrib_unref(monitor->attrib);
+ monitor->attrib = NULL;
+ }
+
+ if (monitor->linkloss != NULL)
+ return;
+
g_dbus_unregister_interface(btd_get_dbus_connection(), path,
PROXIMITY_INTERFACE);
}
+
+void monitor_unregister_linkloss(struct btd_device *device)
+{
+ struct monitor *monitor;
+
+ monitor = find_monitor(device);
+ if (monitor == NULL)
+ return;
+
+ g_free(monitor->linkloss);
+ monitor->linkloss = NULL;
+ monitor->enabled.linkloss = TRUE;
+
+ cleanup_monitor(monitor);
+}
+
+void monitor_unregister_txpower(struct btd_device *device)
+{
+ struct monitor *monitor;
+
+ monitor = find_monitor(device);
+ if (monitor == NULL)
+ return;
+
+ g_free(monitor->txpower);
+ monitor->txpower = NULL;
+ monitor->enabled.pathloss = FALSE;
+
+ cleanup_monitor(monitor);
+}
+
+void monitor_unregister_immediate(struct btd_device *device)
+{
+ struct monitor *monitor;
+
+ monitor = find_monitor(device);
+ if (monitor == NULL)
+ return;
+
+ g_free(monitor->immediate);
+ monitor->immediate = NULL;
+ monitor->enabled.findme = FALSE;
+ monitor->enabled.pathloss = FALSE;
+
+ cleanup_monitor(monitor);
+}
diff --git a/profiles/proximity/monitor.h b/profiles/proximity/monitor.h
index 191b562..d9a40c6 100644
--- a/profiles/proximity/monitor.h
+++ b/profiles/proximity/monitor.h
@@ -28,7 +28,16 @@ struct enabled {
gboolean findme;
};
-int monitor_register(struct btd_device *device, struct gatt_primary *linkloss,
- struct gatt_primary *txpower, struct gatt_primary *immediate,
- struct enabled *enabled);
-void monitor_unregister(struct btd_device *device);
+int monitor_register_linkloss(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *linkloss);
+int monitor_register_txpower(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *txpower);
+int monitor_register_immediate(struct btd_device *device,
+ struct enabled *enabled,
+ struct gatt_primary *immediate);
+
+void monitor_unregister_linkloss(struct btd_device *device);
+void monitor_unregister_txpower(struct btd_device *device);
+void monitor_unregister_immediate(struct btd_device *device);
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 4/7] proximity: Split monitor into three btd_profile
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
` (2 preceding siblings ...)
2013-02-06 9:16 ` [RFC v1 3/7] proximity: Split internal monitor registration API Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-06 9:16 ` [RFC v1 5/7] gatt: List only GATT_UUID as remote UUID Mikel Astiz
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Split into three btd_profile such that each of them handles one single
UUID.
---
profiles/proximity/manager.c | 102 +++++++++++++++++++++++++++++++------------
1 file changed, 75 insertions(+), 27 deletions(-)
diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
index 1c07267..7579be5 100644
--- a/profiles/proximity/manager.c
+++ b/profiles/proximity/manager.c
@@ -48,42 +48,79 @@ static struct enabled enabled = {
.findme = TRUE,
};
-static int monitor_device_probe(struct btd_profile *p,
+static int monitor_linkloss_probe(struct btd_profile *p,
struct btd_device *device, GSList *uuids)
{
- struct gatt_primary *linkloss, *txpower, *immediate;
- int err = 0;
+ struct gatt_primary *linkloss;
- immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
- txpower = btd_device_get_primary(device, TX_POWER_UUID);
linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
+ if (linkloss == NULL)
+ return -1;
- if (linkloss)
- err = monitor_register_linkloss(device, &enabled, linkloss);
+ return monitor_register_linkloss(device, &enabled, linkloss);
+}
- if (err >= 0 && txpower)
- err = monitor_register_txpower(device, &enabled, txpower);
+static int monitor_immediate_probe(struct btd_profile *p,
+ struct btd_device *device, GSList *uuids)
+{
+ struct gatt_primary *immediate;
- if (err >= 0 && immediate)
- err = monitor_register_immediate(device, &enabled, immediate);
+ immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
+ if (immediate == NULL)
+ return -1;
- return err;
+ return monitor_register_immediate(device, &enabled, immediate);
}
-static void monitor_device_remove(struct btd_profile *p,
+static int monitor_txpower_probe(struct btd_profile *p,
+ struct btd_device *device, GSList *uuids)
+{
+ struct gatt_primary *txpower;
+
+ txpower = btd_device_get_primary(device, TX_POWER_UUID);
+ if (txpower == NULL)
+ return -1;
+
+ return monitor_register_txpower(device, &enabled, txpower);
+}
+
+static void monitor_linkloss_remove(struct btd_profile *p,
+ struct btd_device *device)
+{
+ monitor_unregister_linkloss(device);
+}
+
+static void monitor_immediate_remove(struct btd_profile *p,
struct btd_device *device)
{
monitor_unregister_immediate(device);
+}
+
+static void monitor_txpower_remove(struct btd_profile *p,
+ struct btd_device *device)
+{
monitor_unregister_txpower(device);
- monitor_unregister_linkloss(device);
}
-static struct btd_profile pxp_monitor_profile = {
- .name = "Proximity Monitor GATT Driver",
- .remote_uuids = BTD_UUIDS(IMMEDIATE_ALERT_UUID,
- LINK_LOSS_UUID, TX_POWER_UUID),
- .device_probe = monitor_device_probe,
- .device_remove = monitor_device_remove,
+static struct btd_profile pxp_monitor_linkloss_profile = {
+ .name = "proximity-linkloss",
+ .remote_uuids = BTD_UUIDS(LINK_LOSS_UUID),
+ .device_probe = monitor_linkloss_probe,
+ .device_remove = monitor_linkloss_remove,
+};
+
+static struct btd_profile pxp_monitor_immediate_profile = {
+ .name = "proximity-immediate",
+ .remote_uuids = BTD_UUIDS(IMMEDIATE_ALERT_UUID),
+ .device_probe = monitor_immediate_probe,
+ .device_remove = monitor_immediate_remove,
+};
+
+static struct btd_profile pxp_monitor_txpower_profile = {
+ .name = "proximity-txpower",
+ .remote_uuids = BTD_UUIDS(TX_POWER_UUID),
+ .device_probe = monitor_txpower_probe,
+ .device_remove = monitor_txpower_remove,
};
static struct btd_profile pxp_reporter_profile = {
@@ -122,19 +159,30 @@ int proximity_manager_init(GKeyFile *config)
{
load_config_file(config);
- if (btd_profile_register(&pxp_monitor_profile) < 0)
- return -1;
+ if (btd_profile_register(&pxp_monitor_linkloss_profile) < 0)
+ goto fail;
- if (btd_profile_register(&pxp_reporter_profile) < 0) {
- btd_profile_unregister(&pxp_monitor_profile);
- return -1;
- }
+ if (btd_profile_register(&pxp_monitor_immediate_profile) < 0)
+ goto fail;
+
+ if (btd_profile_register(&pxp_monitor_txpower_profile) < 0)
+ goto fail;
+
+ if (btd_profile_register(&pxp_reporter_profile) < 0)
+ goto fail;
return 0;
+
+fail:
+ proximity_manager_exit();
+
+ return -1;
}
void proximity_manager_exit(void)
{
- btd_profile_unregister(&pxp_monitor_profile);
btd_profile_unregister(&pxp_reporter_profile);
+ btd_profile_unregister(&pxp_monitor_txpower_profile);
+ btd_profile_unregister(&pxp_monitor_immediate_profile);
+ btd_profile_unregister(&pxp_monitor_linkloss_profile);
}
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 5/7] gatt: List only GATT_UUID as remote UUID
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
` (3 preceding siblings ...)
2013-02-06 9:16 ` [RFC v1 4/7] proximity: Split monitor into three btd_profile Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-06 9:16 ` [RFC v1 6/7] health: Split health into two btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 7/7] profile: Limit to one remote UUID per profile Mikel Astiz
6 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
The probe function checks if both UUIDs are present, so there is no
need to list both in btd_profile.
---
profiles/gatt/gas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/gatt/gas.c b/profiles/gatt/gas.c
index 9360201..6a8571c 100644
--- a/profiles/gatt/gas.c
+++ b/profiles/gatt/gas.c
@@ -431,7 +431,7 @@ static void gatt_driver_remove(struct btd_profile *p,
static struct btd_profile gatt_profile = {
.name = "gap-gatt-profile",
- .remote_uuids = BTD_UUIDS(GAP_UUID, GATT_UUID),
+ .remote_uuids = BTD_UUIDS(GATT_UUID),
.device_probe = gatt_driver_probe,
.device_remove = gatt_driver_remove
};
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 6/7] health: Split health into two btd_profile
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
` (4 preceding siblings ...)
2013-02-06 9:16 ` [RFC v1 5/7] gatt: List only GATT_UUID as remote UUID Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
2013-02-06 9:16 ` [RFC v1 7/7] profile: Limit to one remote UUID per profile Mikel Astiz
6 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
Register a separate btd_profile for each health device role.
---
profiles/health/hdp_manager.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/profiles/health/hdp_manager.c b/profiles/health/hdp_manager.c
index 87e70cd..9df5b2b 100644
--- a/profiles/health/hdp_manager.c
+++ b/profiles/health/hdp_manager.c
@@ -65,9 +65,9 @@ static void hdp_driver_remove(struct btd_profile *p, struct btd_device *device)
hdp_device_unregister(device);
}
-static struct btd_profile hdp_profile = {
- .name = "hdp-profile",
- .remote_uuids = BTD_UUIDS(HDP_UUID, HDP_SOURCE_UUID, HDP_SINK_UUID),
+static struct btd_profile hdp_source_profile = {
+ .name = "hdp-source",
+ .remote_uuids = BTD_UUIDS(HDP_SOURCE_UUID),
.device_probe = hdp_driver_probe,
.device_remove = hdp_driver_remove,
@@ -76,19 +76,29 @@ static struct btd_profile hdp_profile = {
.adapter_remove = hdp_adapter_remove,
};
+static struct btd_profile hdp_sink_profile = {
+ .name = "hdp-sink",
+ .remote_uuids = BTD_UUIDS(HDP_SINK_UUID),
+
+ .device_probe = hdp_driver_probe,
+ .device_remove = hdp_driver_remove,
+};
+
int hdp_manager_init(void)
{
if (hdp_manager_start() < 0)
return -1;
- btd_profile_register(&hdp_profile);
+ btd_profile_register(&hdp_source_profile);
+ btd_profile_register(&hdp_sink_profile);
return 0;
}
void hdp_manager_exit(void)
{
- btd_profile_unregister(&hdp_profile);
+ btd_profile_unregister(&hdp_sink_profile);
+ btd_profile_unregister(&hdp_source_profile);
hdp_manager_stop();
}
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC v1 7/7] profile: Limit to one remote UUID per profile
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
` (5 preceding siblings ...)
2013-02-06 9:16 ` [RFC v1 6/7] health: Split health into two btd_profile Mikel Astiz
@ 2013-02-06 9:16 ` Mikel Astiz
6 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-06 9:16 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
The code can be considerably simplified by constraining struct
btd_profile to one single remote UUID.
---
profiles/audio/manager.c | 8 +++---
profiles/cyclingspeed/cyclingspeed.c | 2 +-
profiles/deviceinfo/deviceinfo.c | 2 +-
profiles/gatt/gas.c | 2 +-
profiles/health/hdp_manager.c | 4 +--
profiles/heartrate/heartrate.c | 2 +-
profiles/input/hog.c | 2 +-
profiles/input/manager.c | 2 +-
profiles/network/manager.c | 6 ++---
profiles/proximity/manager.c | 8 +++---
profiles/scanparam/scan.c | 2 +-
profiles/thermometer/thermometer.c | 2 +-
src/device.c | 49 +++++++++++++-----------------------
src/profile.c | 25 ++++++++----------
src/profile.h | 4 +--
15 files changed, 49 insertions(+), 71 deletions(-)
diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
index 3023249..5799e77 100644
--- a/profiles/audio/manager.c
+++ b/profiles/audio/manager.c
@@ -345,7 +345,7 @@ static struct btd_profile a2dp_source_profile = {
.name = "audio-source",
.priority = BTD_PROFILE_PRIORITY_MEDIUM,
- .remote_uuids = BTD_UUIDS(A2DP_SOURCE_UUID),
+ .remote_uuid = A2DP_SOURCE_UUID,
.device_probe = a2dp_source_probe,
.device_remove = audio_remove,
@@ -361,7 +361,7 @@ static struct btd_profile a2dp_sink_profile = {
.name = "audio-sink",
.priority = BTD_PROFILE_PRIORITY_MEDIUM,
- .remote_uuids = BTD_UUIDS(A2DP_SINK_UUID),
+ .remote_uuid = A2DP_SINK_UUID,
.device_probe = a2dp_sink_probe,
.device_remove = audio_remove,
@@ -376,7 +376,7 @@ static struct btd_profile a2dp_sink_profile = {
static struct btd_profile avrcp_target_profile = {
.name = "audio-avrcp-target",
- .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID),
+ .remote_uuid = AVRCP_TARGET_UUID,
.device_probe = avrcp_probe,
.device_remove = audio_remove,
@@ -391,7 +391,7 @@ static struct btd_profile avrcp_target_profile = {
static struct btd_profile avrcp_remote_profile = {
.name = "audio-avrcp-control",
- .remote_uuids = BTD_UUIDS(AVRCP_REMOTE_UUID),
+ .remote_uuid = AVRCP_REMOTE_UUID,
.device_probe = avrcp_probe,
.device_remove = audio_remove,
diff --git a/profiles/cyclingspeed/cyclingspeed.c b/profiles/cyclingspeed/cyclingspeed.c
index fc72791..125007e 100644
--- a/profiles/cyclingspeed/cyclingspeed.c
+++ b/profiles/cyclingspeed/cyclingspeed.c
@@ -1256,7 +1256,7 @@ static void csc_device_remove(struct btd_profile *p,
static struct btd_profile cscp_profile = {
.name = "Cycling Speed and Cadence GATT Driver",
- .remote_uuids = BTD_UUIDS(CYCLING_SC_UUID),
+ .remote_uuid = CYCLING_SC_UUID,
.adapter_probe = csc_adapter_probe,
.adapter_remove = csc_adapter_remove,
diff --git a/profiles/deviceinfo/deviceinfo.c b/profiles/deviceinfo/deviceinfo.c
index fb423fa..471241b 100644
--- a/profiles/deviceinfo/deviceinfo.c
+++ b/profiles/deviceinfo/deviceinfo.c
@@ -219,7 +219,7 @@ static void deviceinfo_driver_remove(struct btd_profile *p,
static struct btd_profile deviceinfo_profile = {
.name = "deviceinfo",
- .remote_uuids = BTD_UUIDS(DEVICE_INFORMATION_UUID),
+ .remote_uuid = DEVICE_INFORMATION_UUID,
.device_probe = deviceinfo_driver_probe,
.device_remove = deviceinfo_driver_remove
};
diff --git a/profiles/gatt/gas.c b/profiles/gatt/gas.c
index 6a8571c..bc8dbb5 100644
--- a/profiles/gatt/gas.c
+++ b/profiles/gatt/gas.c
@@ -431,7 +431,7 @@ static void gatt_driver_remove(struct btd_profile *p,
static struct btd_profile gatt_profile = {
.name = "gap-gatt-profile",
- .remote_uuids = BTD_UUIDS(GATT_UUID),
+ .remote_uuid = GATT_UUID,
.device_probe = gatt_driver_probe,
.device_remove = gatt_driver_remove
};
diff --git a/profiles/health/hdp_manager.c b/profiles/health/hdp_manager.c
index 9df5b2b..5428724 100644
--- a/profiles/health/hdp_manager.c
+++ b/profiles/health/hdp_manager.c
@@ -67,7 +67,7 @@ static void hdp_driver_remove(struct btd_profile *p, struct btd_device *device)
static struct btd_profile hdp_source_profile = {
.name = "hdp-source",
- .remote_uuids = BTD_UUIDS(HDP_SOURCE_UUID),
+ .remote_uuid = HDP_SOURCE_UUID,
.device_probe = hdp_driver_probe,
.device_remove = hdp_driver_remove,
@@ -78,7 +78,7 @@ static struct btd_profile hdp_source_profile = {
static struct btd_profile hdp_sink_profile = {
.name = "hdp-sink",
- .remote_uuids = BTD_UUIDS(HDP_SINK_UUID),
+ .remote_uuid = HDP_SINK_UUID,
.device_probe = hdp_driver_probe,
.device_remove = hdp_driver_remove,
diff --git a/profiles/heartrate/heartrate.c b/profiles/heartrate/heartrate.c
index 5c56d3f..0520f5c 100644
--- a/profiles/heartrate/heartrate.c
+++ b/profiles/heartrate/heartrate.c
@@ -861,7 +861,7 @@ static void heartrate_device_remove(struct btd_profile *p,
static struct btd_profile hrp_profile = {
.name = "Heart Rate GATT Driver",
- .remote_uuids = BTD_UUIDS(HEART_RATE_UUID),
+ .remote_uuid = HEART_RATE_UUID,
.device_probe = heartrate_device_probe,
.device_remove = heartrate_device_remove,
diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index a5269d9..eadc860 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -872,7 +872,7 @@ static void hog_remove(struct btd_profile *p, struct btd_device *device)
static struct btd_profile hog_profile = {
.name = "input-hog",
- .remote_uuids = BTD_UUIDS(HOG_UUID),
+ .remote_uuid = HOG_UUID,
.device_probe = hog_probe,
.device_remove = hog_remove,
};
diff --git a/profiles/input/manager.c b/profiles/input/manager.c
index 6ed12ee..d30ba67 100644
--- a/profiles/input/manager.c
+++ b/profiles/input/manager.c
@@ -89,7 +89,7 @@ static void hid_server_remove(struct btd_profile *p,
static struct btd_profile input_profile = {
.name = "input-hid",
.local_uuid = HID_UUID,
- .remote_uuids = BTD_UUIDS(HID_UUID),
+ .remote_uuid = HID_UUID,
.auto_connect = true,
.connect = input_device_connect,
diff --git a/profiles/network/manager.c b/profiles/network/manager.c
index bc553c4..53bb652 100644
--- a/profiles/network/manager.c
+++ b/profiles/network/manager.c
@@ -195,7 +195,7 @@ static void nap_server_remove(struct btd_profile *p,
static struct btd_profile panu_profile = {
.name = "network-panu",
.local_uuid = NAP_UUID,
- .remote_uuids = BTD_UUIDS(PANU_UUID),
+ .remote_uuid = PANU_UUID,
.device_probe = panu_probe,
.device_remove = network_remove,
.connect = panu_connect,
@@ -207,7 +207,7 @@ static struct btd_profile panu_profile = {
static struct btd_profile gn_profile = {
.name = "network-gn",
.local_uuid = PANU_UUID,
- .remote_uuids = BTD_UUIDS(GN_UUID),
+ .remote_uuid = GN_UUID,
.device_probe = gn_probe,
.device_remove = network_remove,
.connect = gn_connect,
@@ -219,7 +219,7 @@ static struct btd_profile gn_profile = {
static struct btd_profile nap_profile = {
.name = "network-nap",
.local_uuid = PANU_UUID,
- .remote_uuids = BTD_UUIDS(NAP_UUID),
+ .remote_uuid = NAP_UUID,
.device_probe = nap_probe,
.device_remove = network_remove,
.connect = nap_connect,
diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
index 7579be5..81bfc3b 100644
--- a/profiles/proximity/manager.c
+++ b/profiles/proximity/manager.c
@@ -104,28 +104,28 @@ static void monitor_txpower_remove(struct btd_profile *p,
static struct btd_profile pxp_monitor_linkloss_profile = {
.name = "proximity-linkloss",
- .remote_uuids = BTD_UUIDS(LINK_LOSS_UUID),
+ .remote_uuid = LINK_LOSS_UUID,
.device_probe = monitor_linkloss_probe,
.device_remove = monitor_linkloss_remove,
};
static struct btd_profile pxp_monitor_immediate_profile = {
.name = "proximity-immediate",
- .remote_uuids = BTD_UUIDS(IMMEDIATE_ALERT_UUID),
+ .remote_uuid = IMMEDIATE_ALERT_UUID,
.device_probe = monitor_immediate_probe,
.device_remove = monitor_immediate_remove,
};
static struct btd_profile pxp_monitor_txpower_profile = {
.name = "proximity-txpower",
- .remote_uuids = BTD_UUIDS(TX_POWER_UUID),
+ .remote_uuid = TX_POWER_UUID,
.device_probe = monitor_txpower_probe,
.device_remove = monitor_txpower_remove,
};
static struct btd_profile pxp_reporter_profile = {
.name = "Proximity Reporter GATT Driver",
- .remote_uuids = BTD_UUIDS(GATT_UUID),
+ .remote_uuid = GATT_UUID,
.device_probe = reporter_device_probe,
.device_remove = reporter_device_remove,
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index 268bdc8..abbd129 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -287,7 +287,7 @@ static void scan_param_remove(struct btd_profile *p, struct btd_device *device)
static struct btd_profile scan_profile = {
.name = "Scan Parameters Client Driver",
- .remote_uuids = BTD_UUIDS(SCAN_PARAMETERS_UUID),
+ .remote_uuid = SCAN_PARAMETERS_UUID,
.device_probe = scan_param_probe,
.device_remove = scan_param_remove,
};
diff --git a/profiles/thermometer/thermometer.c b/profiles/thermometer/thermometer.c
index 1b299e7..8550500 100644
--- a/profiles/thermometer/thermometer.c
+++ b/profiles/thermometer/thermometer.c
@@ -1313,7 +1313,7 @@ static void thermometer_adapter_remove(struct btd_profile *p,
static struct btd_profile thermometer_profile = {
.name = "Health Thermometer GATT driver",
- .remote_uuids = BTD_UUIDS(HEALTH_THERMOMETER_UUID),
+ .remote_uuid = HEALTH_THERMOMETER_UUID,
.device_probe = thermometer_device_probe,
.device_remove = thermometer_device_remove,
.adapter_probe = thermometer_adapter_probe,
diff --git a/src/device.c b/src/device.c
index 49f8957..adcad6c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1155,15 +1155,12 @@ static struct btd_profile *find_connectable_profile(struct btd_device *dev,
for (l = dev->profiles; l != NULL; l = g_slist_next(l)) {
struct btd_profile *p = l->data;
- int i;
- if (!p->connect || !p->remote_uuids)
+ if (!p->connect || !p->remote_uuid)
continue;
- for (i = 0; p->remote_uuids[i] != NULL; i++) {
- if (strcasecmp(uuid, p->remote_uuids[i]) == 0)
- return p;
- }
+ if (strcasecmp(uuid, p->remote_uuid) == 0)
+ return p;
}
return NULL;
@@ -2299,27 +2296,18 @@ GSList *device_get_uuids(struct btd_device *device)
return device->uuids;
}
-static GSList *device_match_profile(struct btd_device *device,
+static bool device_match_profile(struct btd_device *device,
struct btd_profile *profile,
GSList *uuids)
{
- const char **uuid;
- GSList *match_uuids = NULL;
-
- for (uuid = profile->remote_uuids; *uuid; uuid++) {
- GSList *match;
-
- /* skip duplicated uuids */
- if (g_slist_find_custom(match_uuids, *uuid, bt_uuid_strcmp))
- continue;
+ if (profile->remote_uuid == NULL)
+ return false;
- /* match profile uuid */
- match = g_slist_find_custom(uuids, *uuid, bt_uuid_strcmp);
- if (match)
- match_uuids = g_slist_append(match_uuids, match->data);
- }
+ if (g_slist_find_custom(uuids, profile->remote_uuid,
+ bt_uuid_strcmp) == NULL)
+ return false;
- return match_uuids;
+ return true;
}
struct probe_data {
@@ -2337,10 +2325,11 @@ static void dev_probe(struct btd_profile *p, void *user_data)
if (p->device_probe == NULL)
return;
- probe_uuids = device_match_profile(d->dev, p, d->uuids);
- if (!probe_uuids)
+ if (!device_match_profile(d->dev, p, d->uuids))
return;
+ probe_uuids = g_slist_append(NULL, (char *) p->remote_uuid);
+
err = p->device_probe(p, d->dev, probe_uuids);
if (err < 0) {
error("%s profile probe failed for %s", p->name, d->addr);
@@ -2363,10 +2352,11 @@ void device_probe_profile(gpointer a, gpointer b)
if (profile->device_probe == NULL)
return;
- probe_uuids = device_match_profile(device, profile, device->uuids);
- if (!probe_uuids)
+ if (!device_match_profile(device, profile, device->uuids))
return;
+ probe_uuids = g_slist_append(NULL, (char *) profile->remote_uuid);
+
ba2str(&device->bdaddr, addr);
err = profile->device_probe(profile, device, probe_uuids);
@@ -2451,15 +2441,10 @@ static void device_remove_profiles(struct btd_device *device, GSList *uuids)
for (l = device->profiles; l != NULL; l = next) {
struct btd_profile *profile = l->data;
- GSList *probe_uuids;
next = l->next;
- probe_uuids = device_match_profile(device, profile,
- device->uuids);
- if (probe_uuids != NULL) {
- g_slist_free(probe_uuids);
+ if (device_match_profile(device, profile, device->uuids))
continue;
- }
profile->device_remove(profile, device);
device->profiles = g_slist_remove(device->profiles, profile);
diff --git a/src/profile.c b/src/profile.c
index 631a03f..b31bcc5 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -519,7 +519,7 @@ struct ext_profile {
char *(*get_record)(struct ext_profile *ext, struct ext_io *l2cap,
struct ext_io *rfcomm);
- char **remote_uuids;
+ char *remote_uuid;
guint id;
@@ -851,7 +851,7 @@ static bool send_new_connection(struct ext_profile *ext, struct ext_io *conn)
DBusMessage *msg;
DBusMessageIter iter, dict;
struct prop_append_data data = { &dict, conn };
- const char *remote_uuid = ext->remote_uuids[0];
+ const char *remote_uuid = ext->remote_uuid;
const sdp_record_t *rec;
const char *path;
int fd;
@@ -1540,7 +1540,7 @@ static int resolve_service(struct ext_io *conn, const bdaddr_t *src,
uuid_t uuid;
int err;
- bt_string2uuid(&uuid, ext->remote_uuids[0]);
+ bt_string2uuid(&uuid, ext->remote_uuid);
sdp_uuid128_to_uuid(&uuid);
err = bt_search_service(src, dst, &uuid, record_cb, conn, NULL);
@@ -1910,8 +1910,7 @@ static void ext_set_defaults(struct ext_profile *ext)
ext->authorize = true;
ext->enable_client = true;
ext->enable_server = true;
-
- ext->remote_uuids = g_new0(char *, 2);
+ ext->remote_uuid = NULL;
for (i = 0; i < G_N_ELEMENTS(defaults); i++) {
struct default_settings *settings = &defaults[i];
@@ -1925,7 +1924,7 @@ static void ext_set_defaults(struct ext_profile *ext)
else
remote_uuid = ext->uuid;
- ext->remote_uuids[0] = g_strdup(remote_uuid);
+ ext->remote_uuid = g_strdup(remote_uuid);
if (settings->channel)
ext->local_chan = settings->channel;
@@ -2117,21 +2116,18 @@ static struct ext_profile *create_ext(const char *owner, const char *path,
if (!ext->name)
ext->name = g_strdup_printf("%s%s/%s", owner, path, uuid);
- if (!ext->remote_uuids[0]) {
+ if (!ext->remote_uuid) {
if (ext->service)
- ext->remote_uuids[0] = g_strdup(ext->service);
+ ext->remote_uuid = g_strdup(ext->service);
else
- ext->remote_uuids[0] = g_strdup(ext->uuid);
+ ext->remote_uuid = g_strdup(ext->uuid);
}
p = &ext->p;
p->name = ext->name;
p->local_uuid = ext->service ? ext->service : ext->uuid;
-
- /* Typecast can't really be avoided here:
- * http://c-faq.com/ansi/constmismatch.html */
- p->remote_uuids = (const char **) ext->remote_uuids;
+ p->remote_uuid = ext->remote_uuid;
if (ext->enable_server || ext->record || ext->get_record) {
p->adapter_probe = ext_adapter_probe;
@@ -2167,8 +2163,7 @@ static void remove_ext(struct ext_profile *ext)
g_slist_free_full(ext->servers, ext_io_destroy);
g_slist_free_full(ext->conns, ext_io_destroy);
- g_strfreev(ext->remote_uuids);
-
+ g_free(ext->remote_uuid);
g_free(ext->name);
g_free(ext->owner);
g_free(ext->uuid);
diff --git a/src/profile.h b/src/profile.h
index d858925..5d78b37 100644
--- a/src/profile.h
+++ b/src/profile.h
@@ -21,8 +21,6 @@
*
*/
-#define BTD_UUIDS(args...) ((const char *[]) { args, NULL } )
-
#define BTD_PROFILE_PRIORITY_LOW 0
#define BTD_PROFILE_PRIORITY_MEDIUM 1
#define BTD_PROFILE_PRIORITY_HIGH 2
@@ -32,7 +30,7 @@ struct btd_profile {
int priority;
const char *local_uuid;
- const char **remote_uuids;
+ const char *remote_uuid;
bool auto_connect;
--
1.8.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC v1 2/7] audio: Split AVRCP into two btd_profile
2013-02-06 9:16 ` [RFC v1 2/7] audio: Split AVRCP into two btd_profile Mikel Astiz
@ 2013-02-07 9:43 ` Luiz Augusto von Dentz
2013-02-07 10:43 ` Mikel Astiz
0 siblings, 1 reply; 14+ messages in thread
From: Luiz Augusto von Dentz @ 2013-02-07 9:43 UTC (permalink / raw)
To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz
Hi Mikel,
On Wed, Feb 6, 2013 at 11:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> Register a separate btd_profile for each role of AVRCP.
> ---
> profiles/audio/avrcp.c | 80 +++++++++++++++++++++++++++++++++++++-----------
> profiles/audio/avrcp.h | 6 ++--
> profiles/audio/manager.c | 71 ++++++++++++++++++++++++++++++------------
> 3 files changed, 118 insertions(+), 39 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 277f9f3..fe6333b 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -2712,41 +2712,63 @@ static struct avrcp_server *avrcp_server_register(struct btd_adapter *adapter,
> return server;
> }
>
> -int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
> +int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config)
> {
> sdp_record_t *record;
> struct avrcp_server *server;
>
> + server = find_server(servers, adapter);
> + if (server != NULL)
> + goto done;
> +
> server = avrcp_server_register(adapter, config);
> if (server == NULL)
> return -EPROTONOSUPPORT;
>
> +done:
> record = avrcp_tg_record();
> if (!record) {
> error("Unable to allocate new service record");
> - avrcp_unregister(adapter);
> + avrcp_target_unregister(adapter);
> return -1;
> }
>
> if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
> error("Unable to register AVRCP target service record");
> - avrcp_unregister(adapter);
> + avrcp_target_unregister(adapter);
> sdp_record_free(record);
> return -1;
> }
> server->tg_record_id = record->handle;
>
> + return 0;
> +}
> +
> +int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config)
> +{
> + sdp_record_t *record;
> + struct avrcp_server *server;
> +
> + server = find_server(servers, adapter);
> + if (server != NULL)
> + goto done;
> +
> + server = avrcp_server_register(adapter, config);
> + if (server == NULL)
> + return -EPROTONOSUPPORT;
> +
> +done:
> record = avrcp_ct_record();
> if (!record) {
> error("Unable to allocate new service record");
> - avrcp_unregister(adapter);
> + avrcp_remote_unregister(adapter);
> return -1;
> }
>
> if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
> error("Unable to register AVRCP service record");
> sdp_record_free(record);
> - avrcp_unregister(adapter);
> + avrcp_remote_unregister(adapter);
> return -1;
> }
> server->ct_record_id = record->handle;
> @@ -2754,25 +2776,13 @@ int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
> return 0;
> }
>
> -void avrcp_unregister(struct btd_adapter *adapter)
> +static void avrcp_server_unregister(struct avrcp_server *server)
> {
> - struct avrcp_server *server;
> -
> - server = find_server(servers, adapter);
> - if (!server)
> - return;
> -
> g_slist_free_full(server->sessions, g_free);
> g_slist_free_full(server->players, player_destroy);
>
> servers = g_slist_remove(servers, server);
>
> - if (server->ct_record_id != 0)
> - remove_record_from_server(server->ct_record_id);
> -
> - if (server->tg_record_id != 0)
> - remove_record_from_server(server->tg_record_id);
> -
> avctp_unregister(server->adapter);
> btd_adapter_unref(server->adapter);
> g_free(server);
> @@ -2786,6 +2796,40 @@ void avrcp_unregister(struct btd_adapter *adapter)
> }
> }
>
> +void avrcp_target_unregister(struct btd_adapter *adapter)
> +{
> + struct avrcp_server *server;
> +
> + server = find_server(servers, adapter);
> + if (!server)
> + return;
> +
> + if (server->tg_record_id != 0) {
> + remove_record_from_server(server->tg_record_id);
> + server->tg_record_id = 0;
> + }
> +
> + if (server->ct_record_id == 0)
> + avrcp_server_unregister(server);
> +}
> +
> +void avrcp_remote_unregister(struct btd_adapter *adapter)
> +{
> + struct avrcp_server *server;
> +
> + server = find_server(servers, adapter);
> + if (!server)
> + return;
> +
> + if (server->ct_record_id != 0) {
> + remove_record_from_server(server->ct_record_id);
> + server->ct_record_id = 0;
> + }
> +
> + if (server->tg_record_id == 0)
> + avrcp_server_unregister(server);
> +}
> +
> struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
> struct avrcp_player_cb *cb,
> void *user_data,
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> index 3799da1..1f98090 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -93,8 +93,10 @@ struct avrcp_player_cb {
> void *user_data);
> };
>
> -int avrcp_register(struct btd_adapter *adapter, GKeyFile *config);
> -void avrcp_unregister(struct btd_adapter *adapter);
> +int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config);
> +void avrcp_target_unregister(struct btd_adapter *adapter);
> +int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config);
> +void avrcp_remote_unregister(struct btd_adapter *adapter);
>
> gboolean avrcp_connect(struct audio_device *dev);
> void avrcp_disconnect(struct audio_device *dev);
> diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
> index 934227e..3023249 100644
> --- a/profiles/audio/manager.c
> +++ b/profiles/audio/manager.c
> @@ -229,7 +229,7 @@ static int a2dp_sink_disconnect(struct btd_device *dev,
> return sink_disconnect(audio_dev, FALSE);
> }
>
> -static int avrcp_control_connect(struct btd_device *dev,
> +static int avrcp_target_connect(struct btd_device *dev,
> struct btd_profile *profile)
> {
> const char *path = device_get_path(dev);
> @@ -246,7 +246,7 @@ static int avrcp_control_connect(struct btd_device *dev,
> return control_connect(audio_dev);
> }
>
> -static int avrcp_control_disconnect(struct btd_device *dev,
> +static int avrcp_target_disconnect(struct btd_device *dev,
> struct btd_profile *profile)
> {
> const char *path = device_get_path(dev);
> @@ -295,20 +295,36 @@ static void a2dp_sink_server_remove(struct btd_profile *p,
> a2dp_sink_unregister(adapter);
> }
>
> -static int avrcp_server_probe(struct btd_profile *p,
> +static int avrcp_target_server_probe(struct btd_profile *p,
> struct btd_adapter *adapter)
> {
> DBG("path %s", adapter_get_path(adapter));
>
> - return avrcp_register(adapter, config);
> + return avrcp_target_register(adapter, config);
> }
>
> -static void avrcp_server_remove(struct btd_profile *p,
> +static int avrcp_remote_server_probe(struct btd_profile *p,
> struct btd_adapter *adapter)
> {
> DBG("path %s", adapter_get_path(adapter));
>
> - return avrcp_unregister(adapter);
> + return avrcp_remote_register(adapter, config);
> +}
> +
> +static void avrcp_target_server_remove(struct btd_profile *p,
> + struct btd_adapter *adapter)
> +{
> + DBG("path %s", adapter_get_path(adapter));
> +
> + avrcp_target_unregister(adapter);
> +}
> +
> +static void avrcp_remote_server_remove(struct btd_profile *p,
> + struct btd_adapter *adapter)
> +{
> + DBG("path %s", adapter_get_path(adapter));
> +
> + avrcp_remote_unregister(adapter);
> }
>
> static int media_server_probe(struct btd_adapter *adapter)
> @@ -357,19 +373,30 @@ static struct btd_profile a2dp_sink_profile = {
> .adapter_remove = a2dp_sink_server_remove,
> };
>
> -static struct btd_profile avrcp_profile = {
> - .name = "audio-avrcp",
> +static struct btd_profile avrcp_target_profile = {
> + .name = "audio-avrcp-target",
>
> - .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID, AVRCP_REMOTE_UUID),
> + .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID),
> .device_probe = avrcp_probe,
> .device_remove = audio_remove,
>
> .auto_connect = true,
> - .connect = avrcp_control_connect,
> - .disconnect = avrcp_control_disconnect,
> + .connect = avrcp_target_connect,
> + .disconnect = avrcp_target_disconnect,
> +
> + .adapter_probe = avrcp_target_server_probe,
> + .adapter_remove = avrcp_target_server_remove,
> +};
> +
> +static struct btd_profile avrcp_remote_profile = {
> + .name = "audio-avrcp-control",
>
> - .adapter_probe = avrcp_server_probe,
> - .adapter_remove = avrcp_server_remove,
> + .remote_uuids = BTD_UUIDS(AVRCP_REMOTE_UUID),
> + .device_probe = avrcp_probe,
> + .device_remove = audio_remove,
> +
> + .adapter_probe = avrcp_remote_server_probe,
> + .adapter_remove = avrcp_remote_server_remove,
> };
You probably need a .connect callback above, even though normally
there should be both target and control records we should be able to
detect if AVCTP is already connecting just return -EALREADY or 0.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v1 2/7] audio: Split AVRCP into two btd_profile
2013-02-07 9:43 ` Luiz Augusto von Dentz
@ 2013-02-07 10:43 ` Mikel Astiz
0 siblings, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-07 10:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Mikel Astiz
Hi Luiz,
On Thu, Feb 7, 2013 at 10:43 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Mikel,
>
> On Wed, Feb 6, 2013 at 11:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>
>> Register a separate btd_profile for each role of AVRCP.
>> ---
>> profiles/audio/avrcp.c | 80 +++++++++++++++++++++++++++++++++++++-----------
>> profiles/audio/avrcp.h | 6 ++--
>> profiles/audio/manager.c | 71 ++++++++++++++++++++++++++++++------------
>> 3 files changed, 118 insertions(+), 39 deletions(-)
>>
>> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
>> index 277f9f3..fe6333b 100644
>> --- a/profiles/audio/avrcp.c
>> +++ b/profiles/audio/avrcp.c
>> @@ -2712,41 +2712,63 @@ static struct avrcp_server *avrcp_server_register(struct btd_adapter *adapter,
>> return server;
>> }
>>
>> -int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
>> +int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config)
>> {
>> sdp_record_t *record;
>> struct avrcp_server *server;
>>
>> + server = find_server(servers, adapter);
>> + if (server != NULL)
>> + goto done;
>> +
>> server = avrcp_server_register(adapter, config);
>> if (server == NULL)
>> return -EPROTONOSUPPORT;
>>
>> +done:
>> record = avrcp_tg_record();
>> if (!record) {
>> error("Unable to allocate new service record");
>> - avrcp_unregister(adapter);
>> + avrcp_target_unregister(adapter);
>> return -1;
>> }
>>
>> if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
>> error("Unable to register AVRCP target service record");
>> - avrcp_unregister(adapter);
>> + avrcp_target_unregister(adapter);
>> sdp_record_free(record);
>> return -1;
>> }
>> server->tg_record_id = record->handle;
>>
>> + return 0;
>> +}
>> +
>> +int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config)
>> +{
>> + sdp_record_t *record;
>> + struct avrcp_server *server;
>> +
>> + server = find_server(servers, adapter);
>> + if (server != NULL)
>> + goto done;
>> +
>> + server = avrcp_server_register(adapter, config);
>> + if (server == NULL)
>> + return -EPROTONOSUPPORT;
>> +
>> +done:
>> record = avrcp_ct_record();
>> if (!record) {
>> error("Unable to allocate new service record");
>> - avrcp_unregister(adapter);
>> + avrcp_remote_unregister(adapter);
>> return -1;
>> }
>>
>> if (add_record_to_server(adapter_get_address(adapter), record) < 0) {
>> error("Unable to register AVRCP service record");
>> sdp_record_free(record);
>> - avrcp_unregister(adapter);
>> + avrcp_remote_unregister(adapter);
>> return -1;
>> }
>> server->ct_record_id = record->handle;
>> @@ -2754,25 +2776,13 @@ int avrcp_register(struct btd_adapter *adapter, GKeyFile *config)
>> return 0;
>> }
>>
>> -void avrcp_unregister(struct btd_adapter *adapter)
>> +static void avrcp_server_unregister(struct avrcp_server *server)
>> {
>> - struct avrcp_server *server;
>> -
>> - server = find_server(servers, adapter);
>> - if (!server)
>> - return;
>> -
>> g_slist_free_full(server->sessions, g_free);
>> g_slist_free_full(server->players, player_destroy);
>>
>> servers = g_slist_remove(servers, server);
>>
>> - if (server->ct_record_id != 0)
>> - remove_record_from_server(server->ct_record_id);
>> -
>> - if (server->tg_record_id != 0)
>> - remove_record_from_server(server->tg_record_id);
>> -
>> avctp_unregister(server->adapter);
>> btd_adapter_unref(server->adapter);
>> g_free(server);
>> @@ -2786,6 +2796,40 @@ void avrcp_unregister(struct btd_adapter *adapter)
>> }
>> }
>>
>> +void avrcp_target_unregister(struct btd_adapter *adapter)
>> +{
>> + struct avrcp_server *server;
>> +
>> + server = find_server(servers, adapter);
>> + if (!server)
>> + return;
>> +
>> + if (server->tg_record_id != 0) {
>> + remove_record_from_server(server->tg_record_id);
>> + server->tg_record_id = 0;
>> + }
>> +
>> + if (server->ct_record_id == 0)
>> + avrcp_server_unregister(server);
>> +}
>> +
>> +void avrcp_remote_unregister(struct btd_adapter *adapter)
>> +{
>> + struct avrcp_server *server;
>> +
>> + server = find_server(servers, adapter);
>> + if (!server)
>> + return;
>> +
>> + if (server->ct_record_id != 0) {
>> + remove_record_from_server(server->ct_record_id);
>> + server->ct_record_id = 0;
>> + }
>> +
>> + if (server->tg_record_id == 0)
>> + avrcp_server_unregister(server);
>> +}
>> +
>> struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
>> struct avrcp_player_cb *cb,
>> void *user_data,
>> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
>> index 3799da1..1f98090 100644
>> --- a/profiles/audio/avrcp.h
>> +++ b/profiles/audio/avrcp.h
>> @@ -93,8 +93,10 @@ struct avrcp_player_cb {
>> void *user_data);
>> };
>>
>> -int avrcp_register(struct btd_adapter *adapter, GKeyFile *config);
>> -void avrcp_unregister(struct btd_adapter *adapter);
>> +int avrcp_target_register(struct btd_adapter *adapter, GKeyFile *config);
>> +void avrcp_target_unregister(struct btd_adapter *adapter);
>> +int avrcp_remote_register(struct btd_adapter *adapter, GKeyFile *config);
>> +void avrcp_remote_unregister(struct btd_adapter *adapter);
>>
>> gboolean avrcp_connect(struct audio_device *dev);
>> void avrcp_disconnect(struct audio_device *dev);
>> diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
>> index 934227e..3023249 100644
>> --- a/profiles/audio/manager.c
>> +++ b/profiles/audio/manager.c
>> @@ -229,7 +229,7 @@ static int a2dp_sink_disconnect(struct btd_device *dev,
>> return sink_disconnect(audio_dev, FALSE);
>> }
>>
>> -static int avrcp_control_connect(struct btd_device *dev,
>> +static int avrcp_target_connect(struct btd_device *dev,
>> struct btd_profile *profile)
>> {
>> const char *path = device_get_path(dev);
>> @@ -246,7 +246,7 @@ static int avrcp_control_connect(struct btd_device *dev,
>> return control_connect(audio_dev);
>> }
>>
>> -static int avrcp_control_disconnect(struct btd_device *dev,
>> +static int avrcp_target_disconnect(struct btd_device *dev,
>> struct btd_profile *profile)
>> {
>> const char *path = device_get_path(dev);
>> @@ -295,20 +295,36 @@ static void a2dp_sink_server_remove(struct btd_profile *p,
>> a2dp_sink_unregister(adapter);
>> }
>>
>> -static int avrcp_server_probe(struct btd_profile *p,
>> +static int avrcp_target_server_probe(struct btd_profile *p,
>> struct btd_adapter *adapter)
>> {
>> DBG("path %s", adapter_get_path(adapter));
>>
>> - return avrcp_register(adapter, config);
>> + return avrcp_target_register(adapter, config);
>> }
>>
>> -static void avrcp_server_remove(struct btd_profile *p,
>> +static int avrcp_remote_server_probe(struct btd_profile *p,
>> struct btd_adapter *adapter)
>> {
>> DBG("path %s", adapter_get_path(adapter));
>>
>> - return avrcp_unregister(adapter);
>> + return avrcp_remote_register(adapter, config);
>> +}
>> +
>> +static void avrcp_target_server_remove(struct btd_profile *p,
>> + struct btd_adapter *adapter)
>> +{
>> + DBG("path %s", adapter_get_path(adapter));
>> +
>> + avrcp_target_unregister(adapter);
>> +}
>> +
>> +static void avrcp_remote_server_remove(struct btd_profile *p,
>> + struct btd_adapter *adapter)
>> +{
>> + DBG("path %s", adapter_get_path(adapter));
>> +
>> + avrcp_remote_unregister(adapter);
>> }
>>
>> static int media_server_probe(struct btd_adapter *adapter)
>> @@ -357,19 +373,30 @@ static struct btd_profile a2dp_sink_profile = {
>> .adapter_remove = a2dp_sink_server_remove,
>> };
>>
>> -static struct btd_profile avrcp_profile = {
>> - .name = "audio-avrcp",
>> +static struct btd_profile avrcp_target_profile = {
>> + .name = "audio-avrcp-target",
>>
>> - .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID, AVRCP_REMOTE_UUID),
>> + .remote_uuids = BTD_UUIDS(AVRCP_TARGET_UUID),
>> .device_probe = avrcp_probe,
>> .device_remove = audio_remove,
>>
>> .auto_connect = true,
>> - .connect = avrcp_control_connect,
>> - .disconnect = avrcp_control_disconnect,
>> + .connect = avrcp_target_connect,
>> + .disconnect = avrcp_target_disconnect,
>> +
>> + .adapter_probe = avrcp_target_server_probe,
>> + .adapter_remove = avrcp_target_server_remove,
>> +};
>> +
>> +static struct btd_profile avrcp_remote_profile = {
>> + .name = "audio-avrcp-control",
>>
>> - .adapter_probe = avrcp_server_probe,
>> - .adapter_remove = avrcp_server_remove,
>> + .remote_uuids = BTD_UUIDS(AVRCP_REMOTE_UUID),
>> + .device_probe = avrcp_probe,
>> + .device_remove = audio_remove,
>> +
>> + .adapter_probe = avrcp_remote_server_probe,
>> + .adapter_remove = avrcp_remote_server_remove,
>> };
>
> You probably need a .connect callback above, even though normally
> there should be both target and control records we should be able to
> detect if AVCTP is already connecting just return -EALREADY or 0.
I agree with your point but the current codebase seems to return
-ENOTSUP in control_connect() for the remote role, so I see no point
in adding .connect callback before such a feature gets implemented.
I'm not deep into the AVRCP code but I guess it's not as simple as
getting rid of this role check in control_connect(), right?
Cheers,
Mikel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v1 3/7] proximity: Split internal monitor registration API
2013-02-06 9:16 ` [RFC v1 3/7] proximity: Split internal monitor registration API Mikel Astiz
@ 2013-02-14 16:37 ` Claudio Takahasi
2013-02-15 7:54 ` Mikel Astiz
0 siblings, 1 reply; 14+ messages in thread
From: Claudio Takahasi @ 2013-02-14 16:37 UTC (permalink / raw)
To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz
Hi Mikel:
On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> Split the monitor registration API into three independent registrations
> each of them taking one specific GATT primary.
> ---
> profiles/proximity/manager.c | 16 +++-
> profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
> profiles/proximity/monitor.h | 17 +++-
> 3 files changed, 204 insertions(+), 49 deletions(-)
>
> diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
> index b405f15..1c07267 100644
> --- a/profiles/proximity/manager.c
> +++ b/profiles/proximity/manager.c
> @@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
> struct btd_device *device, GSList *uuids)
> {
> struct gatt_primary *linkloss, *txpower, *immediate;
> + int err = 0;
>
> immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
> txpower = btd_device_get_primary(device, TX_POWER_UUID);
> linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
>
> - return monitor_register(device, linkloss, txpower, immediate, &enabled);
> + if (linkloss)
> + err = monitor_register_linkloss(device, &enabled, linkloss);
> +
> + if (err >= 0 && txpower)
> + err = monitor_register_txpower(device, &enabled, txpower);
> +
> + if (err >= 0 && immediate)
> + err = monitor_register_immediate(device, &enabled, immediate);
> +
I recommend to invert the logic here, checking if immediate alert is
available first. Immediate alert service is mandatory for Link Loss,
Path Loss(tx power) and find me.
> + return err;
> }
>
> static void monitor_device_remove(struct btd_profile *p,
> struct btd_device *device)
> {
> - monitor_unregister(device);
> + monitor_unregister_immediate(device);
> + monitor_unregister_txpower(device);
> + monitor_unregister_linkloss(device);
> }
>
> static struct btd_profile pxp_monitor_profile = {
> diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
> index 597f161..489ccdd 100644
> --- a/profiles/proximity/monitor.c
> +++ b/profiles/proximity/monitor.c
> @@ -34,6 +34,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/stat.h>
> +#include <glib.h>
>
> #include <bluetooth/bluetooth.h>
>
> @@ -83,6 +84,22 @@ struct monitor {
> guint attioid;
> };
>
> +static GSList *monitors = NULL;
> +
> +static struct monitor *find_monitor(struct btd_device *device)
> +{
> + GSList *l;
> +
> + for (l = monitors; l; l = l->next) {
> + struct monitor *monitor = l->data;
> +
> + if (monitor->device == device)
> + return monitor;
> + }
> +
> + return NULL;
> +}
> +
> static void write_proximity_config(struct btd_device *device, const char *alert,
> const char *level)
> {
> @@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
> {
> struct monitor *monitor = user_data;
>
> - if (monitor->immediateto)
> - g_source_remove(monitor->immediateto);
> -
> - if (monitor->attioid)
> - btd_device_remove_attio_callback(monitor->device,
> - monitor->attioid);
> - if (monitor->attrib)
> - g_attrib_unref(monitor->attrib);
> -
> btd_device_unref(monitor->device);
> - g_free(monitor->linkloss);
> - g_free(monitor->immediate);
> - g_free(monitor->txpower);
> g_free(monitor->linklosslevel);
> g_free(monitor->immediatelevel);
> g_free(monitor->signallevel);
> g_free(monitor);
> +
> + monitors = g_slist_remove(monitors, monitor);
> }
>
> -int monitor_register(struct btd_device *device,
> - struct gatt_primary *linkloss, struct gatt_primary *txpower,
> - struct gatt_primary *immediate, struct enabled *enabled)
> +static struct monitor *register_monitor(struct btd_device *device)
> {
> const char *path = device_get_path(device);
> struct monitor *monitor;
> char *level;
>
> + monitor = find_monitor(device);
> + if (monitor != NULL)
> + return monitor;
> +
> level = read_proximity_config(device, "LinkLossAlertLevel");
>
> monitor = g_new0(struct monitor, 1);
> @@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
> monitor->signallevel = g_strdup("unknown");
> monitor->immediatelevel = g_strdup("none");
>
> + monitors = g_slist_append(monitors, monitor);
> +
> if (g_dbus_register_interface(btd_get_dbus_connection(), path,
> PROXIMITY_INTERFACE,
> NULL, NULL, monitor_device_properties,
> @@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
> error("D-Bus failed to register %s interface",
> PROXIMITY_INTERFACE);
> monitor_destroy(monitor);
> - return -1;
> + return NULL;
> }
>
> DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
>
> - if (linkloss && enabled->linkloss) {
> - monitor->linkloss = g_new0(struct att_range, 1);
> - monitor->linkloss->start = linkloss->range.start;
> - monitor->linkloss->end = linkloss->range.end;
> -
> - monitor->enabled.linkloss = TRUE;
> - }
> -
> - if (immediate) {
> - if (txpower && enabled->pathloss) {
> - monitor->txpower = g_new0(struct att_range, 1);
> - monitor->txpower->start = txpower->range.start;
> - monitor->txpower->end = txpower->range.end;
> -
> - monitor->enabled.pathloss = TRUE;
> - }
> -
> - if (enabled->pathloss || enabled->findme) {
> - monitor->immediate = g_new0(struct att_range, 1);
> - monitor->immediate->start = immediate->range.start;
> - monitor->immediate->end = immediate->range.end;
> - }
> + return monitor;
> +}
>
> - monitor->enabled.findme = enabled->findme;
> - }
> +static void update_monitor(struct monitor *monitor)
> +{
> + if (monitor->txpower != NULL && monitor->immediate != NULL)
> + monitor->enabled.pathloss = TRUE;
> + else
> + monitor->enabled.pathloss = FALSE;
>
> DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
> monitor->enabled.linkloss ? "TRUE" : "FALSE",
> monitor->enabled.pathloss ? "TRUE" : "FALSE",
> monitor->enabled.findme ? "TRUE" : "FALSE");
>
> - if (monitor->enabled.linkloss || monitor->enabled.pathloss)
> - monitor->attioid = btd_device_add_attio_callback(device,
> + if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
> + return;
> +
> + if (monitor->attioid != 0)
> + return;
> +
> + monitor->attioid = btd_device_add_attio_callback(monitor->device,
> attio_connected_cb,
> attio_disconnected_cb,
> monitor);
> +}
> +
> +int monitor_register_linkloss(struct btd_device *device,
> + struct enabled *enabled,
> + struct gatt_primary *linkloss)
> +{
> + struct monitor *monitor;
> +
> + if (!enabled->linkloss)
> + return 0;
Link loss requires immediate alert service, it is necessary to
investigate if make sense to add this verification here or check the
value before calling this function.
The same comment is valid for path loss(tx power).
> +
> + monitor = register_monitor(device);
> + if (monitor == NULL)
> + return -1;
> +
> + monitor->linkloss = g_new0(struct att_range, 1);
> + monitor->linkloss->start = linkloss->range.start;
> + monitor->linkloss->end = linkloss->range.end;
> + monitor->enabled.linkloss = TRUE;
> +
> + update_monitor(monitor);
>
> return 0;
> }
>
> -void monitor_unregister(struct btd_device *device)
> +int monitor_register_txpower(struct btd_device *device,
> + struct enabled *enabled,
> + struct gatt_primary *txpower)
> {
> + struct monitor *monitor;
> +
> + if (!enabled->pathloss)
> + return 0;
> +
> + monitor = register_monitor(device);
> + if (monitor == NULL)
> + return -1;
> +
> + monitor->txpower = g_new0(struct att_range, 1);
> + monitor->txpower->start = txpower->range.start;
> + monitor->txpower->end = txpower->range.end;
> +
> + update_monitor(monitor);
> +
> + return 0;
> +}
> +
> +int monitor_register_immediate(struct btd_device *device,
> + struct enabled *enabled,
> + struct gatt_primary *immediate)
> +{
> + struct monitor *monitor;
> +
> + if (!enabled->pathloss && !enabled->findme)
> + return 0;
> +
> + monitor = register_monitor(device);
> + if (monitor == NULL)
> + return -1;
> +
> + monitor->immediate = g_new0(struct att_range, 1);
> + monitor->immediate->start = immediate->range.start;
> + monitor->immediate->end = immediate->range.end;
> + monitor->enabled.findme = enabled->findme;
> +
> + update_monitor(monitor);
> +
> + return 0;
> +}
> +
> +static void cleanup_monitor(struct monitor *monitor)
> +{
> + struct btd_device *device = monitor->device;
> const char *path = device_get_path(device);
>
> + if (monitor->immediate != NULL || monitor->txpower != NULL)
> + return;
> +
> + if (monitor->immediateto != 0) {
> + g_source_remove(monitor->immediateto);
> + monitor->immediateto = 0;
> + }
> +
> + if (monitor->attioid != 0) {
> + btd_device_remove_attio_callback(device, monitor->attioid);
> + monitor->attioid = 0;
> + }
> +
> + if (monitor->attrib != NULL) {
> + g_attrib_unref(monitor->attrib);
> + monitor->attrib = NULL;
> + }
> +
> + if (monitor->linkloss != NULL)
> + return;
Why this checking is here instead of check in the beginning of this function?
> +
> g_dbus_unregister_interface(btd_get_dbus_connection(), path,
> PROXIMITY_INTERFACE);
> }
> +
> +void monitor_unregister_linkloss(struct btd_device *device)
> +{
> + struct monitor *monitor;
> +
> + monitor = find_monitor(device);
> + if (monitor == NULL)
> + return;
> +
> + g_free(monitor->linkloss);
> + monitor->linkloss = NULL;
> + monitor->enabled.linkloss = TRUE;
If you are unregistering a service, is it necessary to set
enabled.linkloss = TRUE?
> +
> + cleanup_monitor(monitor);
> +}
> +
> +void monitor_unregister_txpower(struct btd_device *device)
> +{
> + struct monitor *monitor;
> +
> + monitor = find_monitor(device);
> + if (monitor == NULL)
> + return;
> +
> + g_free(monitor->txpower);
> + monitor->txpower = NULL;
> + monitor->enabled.pathloss = FALSE;
Same question here.
> +
> + cleanup_monitor(monitor);
> +}
> +
> +void monitor_unregister_immediate(struct btd_device *device)
> +{
> + struct monitor *monitor;
> +
> + monitor = find_monitor(device);
> + if (monitor == NULL)
> + return;
> +
> + g_free(monitor->immediate);
> + monitor->immediate = NULL;
> + monitor->enabled.findme = FALSE;
> + monitor->enabled.pathloss = FALSE;
Same question here.
Regards,
Claudio
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v1 3/7] proximity: Split internal monitor registration API
2013-02-14 16:37 ` Claudio Takahasi
@ 2013-02-15 7:54 ` Mikel Astiz
2013-02-27 7:46 ` Mikel Astiz
2013-02-28 19:34 ` Claudio Takahasi
0 siblings, 2 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-15 7:54 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
Hi Claudio,
On Thu, Feb 14, 2013 at 5:37 PM, Claudio Takahasi
<claudio.takahasi@openbossa.org> wrote:
> Hi Mikel:
>
> On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>
>> Split the monitor registration API into three independent registrations
>> each of them taking one specific GATT primary.
>> ---
>> profiles/proximity/manager.c | 16 +++-
>> profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
>> profiles/proximity/monitor.h | 17 +++-
>> 3 files changed, 204 insertions(+), 49 deletions(-)
>>
>> diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
>> index b405f15..1c07267 100644
>> --- a/profiles/proximity/manager.c
>> +++ b/profiles/proximity/manager.c
>> @@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
>> struct btd_device *device, GSList *uuids)
>> {
>> struct gatt_primary *linkloss, *txpower, *immediate;
>> + int err = 0;
>>
>> immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
>> txpower = btd_device_get_primary(device, TX_POWER_UUID);
>> linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
>>
>> - return monitor_register(device, linkloss, txpower, immediate, &enabled);
>> + if (linkloss)
>> + err = monitor_register_linkloss(device, &enabled, linkloss);
>> +
>> + if (err >= 0 && txpower)
>> + err = monitor_register_txpower(device, &enabled, txpower);
>> +
>> + if (err >= 0 && immediate)
>> + err = monitor_register_immediate(device, &enabled, immediate);
>> +
>
> I recommend to invert the logic here, checking if immediate alert is
> available first. Immediate alert service is mandatory for Link Loss,
> Path Loss(tx power) and find me.
According to the code being removed in monitor_register(), the
linkloss service seems independent from immediate alert. In fact, I
tried to clarify this in the spec and what I found is that the
linkloss service is mandatory while the rest are optional. Am I
missing something?
In any case, to start with, it makes sense that txpower gets
registered last to improve readability.
Also note that I tried to express the dependencies you mention in
update_monitor() and the registration functions assuming that, once
split, the profiles can be probed in any arbitrary order (i.e. txpower
probed before immediate alert).
>
>> + return err;
>> }
>>
>> static void monitor_device_remove(struct btd_profile *p,
>> struct btd_device *device)
>> {
>> - monitor_unregister(device);
>> + monitor_unregister_immediate(device);
>> + monitor_unregister_txpower(device);
>> + monitor_unregister_linkloss(device);
>> }
>>
>> static struct btd_profile pxp_monitor_profile = {
>> diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
>> index 597f161..489ccdd 100644
>> --- a/profiles/proximity/monitor.c
>> +++ b/profiles/proximity/monitor.c
>> @@ -34,6 +34,7 @@
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <sys/stat.h>
>> +#include <glib.h>
>>
>> #include <bluetooth/bluetooth.h>
>>
>> @@ -83,6 +84,22 @@ struct monitor {
>> guint attioid;
>> };
>>
>> +static GSList *monitors = NULL;
>> +
>> +static struct monitor *find_monitor(struct btd_device *device)
>> +{
>> + GSList *l;
>> +
>> + for (l = monitors; l; l = l->next) {
>> + struct monitor *monitor = l->data;
>> +
>> + if (monitor->device == device)
>> + return monitor;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static void write_proximity_config(struct btd_device *device, const char *alert,
>> const char *level)
>> {
>> @@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
>> {
>> struct monitor *monitor = user_data;
>>
>> - if (monitor->immediateto)
>> - g_source_remove(monitor->immediateto);
>> -
>> - if (monitor->attioid)
>> - btd_device_remove_attio_callback(monitor->device,
>> - monitor->attioid);
>> - if (monitor->attrib)
>> - g_attrib_unref(monitor->attrib);
>> -
>> btd_device_unref(monitor->device);
>> - g_free(monitor->linkloss);
>> - g_free(monitor->immediate);
>> - g_free(monitor->txpower);
>> g_free(monitor->linklosslevel);
>> g_free(monitor->immediatelevel);
>> g_free(monitor->signallevel);
>> g_free(monitor);
>> +
>> + monitors = g_slist_remove(monitors, monitor);
>> }
>>
>> -int monitor_register(struct btd_device *device,
>> - struct gatt_primary *linkloss, struct gatt_primary *txpower,
>> - struct gatt_primary *immediate, struct enabled *enabled)
>> +static struct monitor *register_monitor(struct btd_device *device)
>> {
>> const char *path = device_get_path(device);
>> struct monitor *monitor;
>> char *level;
>>
>> + monitor = find_monitor(device);
>> + if (monitor != NULL)
>> + return monitor;
>> +
>> level = read_proximity_config(device, "LinkLossAlertLevel");
>>
>> monitor = g_new0(struct monitor, 1);
>> @@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
>> monitor->signallevel = g_strdup("unknown");
>> monitor->immediatelevel = g_strdup("none");
>>
>> + monitors = g_slist_append(monitors, monitor);
>> +
>> if (g_dbus_register_interface(btd_get_dbus_connection(), path,
>> PROXIMITY_INTERFACE,
>> NULL, NULL, monitor_device_properties,
>> @@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
>> error("D-Bus failed to register %s interface",
>> PROXIMITY_INTERFACE);
>> monitor_destroy(monitor);
>> - return -1;
>> + return NULL;
>> }
>>
>> DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
>>
>> - if (linkloss && enabled->linkloss) {
>> - monitor->linkloss = g_new0(struct att_range, 1);
>> - monitor->linkloss->start = linkloss->range.start;
>> - monitor->linkloss->end = linkloss->range.end;
>> -
>> - monitor->enabled.linkloss = TRUE;
>> - }
>> -
>> - if (immediate) {
>> - if (txpower && enabled->pathloss) {
>> - monitor->txpower = g_new0(struct att_range, 1);
>> - monitor->txpower->start = txpower->range.start;
>> - monitor->txpower->end = txpower->range.end;
>> -
>> - monitor->enabled.pathloss = TRUE;
>> - }
>> -
>> - if (enabled->pathloss || enabled->findme) {
>> - monitor->immediate = g_new0(struct att_range, 1);
>> - monitor->immediate->start = immediate->range.start;
>> - monitor->immediate->end = immediate->range.end;
>> - }
>> + return monitor;
>> +}
>>
>> - monitor->enabled.findme = enabled->findme;
>> - }
>> +static void update_monitor(struct monitor *monitor)
>> +{
>> + if (monitor->txpower != NULL && monitor->immediate != NULL)
>> + monitor->enabled.pathloss = TRUE;
>> + else
>> + monitor->enabled.pathloss = FALSE;
>>
>> DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
>> monitor->enabled.linkloss ? "TRUE" : "FALSE",
>> monitor->enabled.pathloss ? "TRUE" : "FALSE",
>> monitor->enabled.findme ? "TRUE" : "FALSE");
>>
>> - if (monitor->enabled.linkloss || monitor->enabled.pathloss)
>> - monitor->attioid = btd_device_add_attio_callback(device,
>> + if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
>> + return;
>> +
>> + if (monitor->attioid != 0)
>> + return;
>> +
>> + monitor->attioid = btd_device_add_attio_callback(monitor->device,
>> attio_connected_cb,
>> attio_disconnected_cb,
>> monitor);
>> +}
>> +
>> +int monitor_register_linkloss(struct btd_device *device,
>> + struct enabled *enabled,
>> + struct gatt_primary *linkloss)
>> +{
>> + struct monitor *monitor;
>> +
>> + if (!enabled->linkloss)
>> + return 0;
>
> Link loss requires immediate alert service, it is necessary to
> investigate if make sense to add this verification here or check the
> value before calling this function.
> The same comment is valid for path loss(tx power).
Regarding link-loss, please confirm that this dependency to immediate
alert actually exists.
Regarding tx-power, as mentioned before, the motivation was the fact
that, if the profiles get split, the exact order in which they are
probed is undefined. Therefore, monitor_register_txpower() doesn't
actually set enabled.pathloss to true, but instead delegates this work
to update_monitor(), which should gracefully handle both possible
probe combinations (immediate alert probed before or after txpower).
>
>> +
>> + monitor = register_monitor(device);
>> + if (monitor == NULL)
>> + return -1;
>> +
>> + monitor->linkloss = g_new0(struct att_range, 1);
>> + monitor->linkloss->start = linkloss->range.start;
>> + monitor->linkloss->end = linkloss->range.end;
>> + monitor->enabled.linkloss = TRUE;
>> +
>> + update_monitor(monitor);
>>
>> return 0;
>> }
>>
>> -void monitor_unregister(struct btd_device *device)
>> +int monitor_register_txpower(struct btd_device *device,
>> + struct enabled *enabled,
>> + struct gatt_primary *txpower)
>> {
>> + struct monitor *monitor;
>> +
>> + if (!enabled->pathloss)
>> + return 0;
>> +
>> + monitor = register_monitor(device);
>> + if (monitor == NULL)
>> + return -1;
>> +
>> + monitor->txpower = g_new0(struct att_range, 1);
>> + monitor->txpower->start = txpower->range.start;
>> + monitor->txpower->end = txpower->range.end;
>> +
>> + update_monitor(monitor);
>> +
>> + return 0;
>> +}
>> +
>> +int monitor_register_immediate(struct btd_device *device,
>> + struct enabled *enabled,
>> + struct gatt_primary *immediate)
>> +{
>> + struct monitor *monitor;
>> +
>> + if (!enabled->pathloss && !enabled->findme)
>> + return 0;
>> +
>> + monitor = register_monitor(device);
>> + if (monitor == NULL)
>> + return -1;
>> +
>> + monitor->immediate = g_new0(struct att_range, 1);
>> + monitor->immediate->start = immediate->range.start;
>> + monitor->immediate->end = immediate->range.end;
>> + monitor->enabled.findme = enabled->findme;
>> +
>> + update_monitor(monitor);
>> +
>> + return 0;
>> +}
>> +
>> +static void cleanup_monitor(struct monitor *monitor)
>> +{
>> + struct btd_device *device = monitor->device;
>> const char *path = device_get_path(device);
>>
>> + if (monitor->immediate != NULL || monitor->txpower != NULL)
>> + return;
>> +
>> + if (monitor->immediateto != 0) {
>> + g_source_remove(monitor->immediateto);
>> + monitor->immediateto = 0;
>> + }
>> +
>> + if (monitor->attioid != 0) {
>> + btd_device_remove_attio_callback(device, monitor->attioid);
>> + monitor->attioid = 0;
>> + }
>> +
>> + if (monitor->attrib != NULL) {
>> + g_attrib_unref(monitor->attrib);
>> + monitor->attrib = NULL;
>> + }
>> +
>> + if (monitor->linkloss != NULL)
>> + return;
>
> Why this checking is here instead of check in the beginning of this function?
My understanding of the previous code was that the cleanup above
(before the linkloss check) belonged to immediate alert and txpower.
Having a careful look at the code I realized that monitor->attioid and
monitor->attrib should be moved to the end of the function, since
linkloss makes use of them.
Whether monitor->immediateto should also come after this check or not
depends again on what the exact dependencies are.
>
>> +
>> g_dbus_unregister_interface(btd_get_dbus_connection(), path,
>> PROXIMITY_INTERFACE);
>> }
>> +
>> +void monitor_unregister_linkloss(struct btd_device *device)
>> +{
>> + struct monitor *monitor;
>> +
>> + monitor = find_monitor(device);
>> + if (monitor == NULL)
>> + return;
>> +
>> + g_free(monitor->linkloss);
>> + monitor->linkloss = NULL;
>> + monitor->enabled.linkloss = TRUE;
>
> If you are unregistering a service, is it necessary to set
> enabled.linkloss = TRUE?
My mistake, this should have been FALSE.
>
>> +
>> + cleanup_monitor(monitor);
>> +}
>> +
>> +void monitor_unregister_txpower(struct btd_device *device)
>> +{
>> + struct monitor *monitor;
>> +
>> + monitor = find_monitor(device);
>> + if (monitor == NULL)
>> + return;
>> +
>> + g_free(monitor->txpower);
>> + monitor->txpower = NULL;
>> + monitor->enabled.pathloss = FALSE;
>
> Same question here.
The unregister function try to support partial unregistration to be
consistent with the registration procedure, but perhaps this is
overkill since typically all profiles will be removed at once.
In fact, many other profiles simplify the unregistration by just
removing more than one profile at the same time (see for example
audio_device_unregister()). Any thoughts on this? Is such a
simplification encouraged?
>
>> +
>> + cleanup_monitor(monitor);
>> +}
>> +
>> +void monitor_unregister_immediate(struct btd_device *device)
>> +{
>> + struct monitor *monitor;
>> +
>> + monitor = find_monitor(device);
>> + if (monitor == NULL)
>> + return;
>> +
>> + g_free(monitor->immediate);
>> + monitor->immediate = NULL;
>> + monitor->enabled.findme = FALSE;
>> + monitor->enabled.pathloss = FALSE;
>
> Same question here.
>
>
> Regards,
> Claudio
Cheers,
Mikel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v1 3/7] proximity: Split internal monitor registration API
2013-02-15 7:54 ` Mikel Astiz
@ 2013-02-27 7:46 ` Mikel Astiz
2013-02-28 19:34 ` Claudio Takahasi
1 sibling, 0 replies; 14+ messages in thread
From: Mikel Astiz @ 2013-02-27 7:46 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
On Fri, Feb 15, 2013 at 8:54 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> Hi Claudio,
>
> On Thu, Feb 14, 2013 at 5:37 PM, Claudio Takahasi
> <claudio.takahasi@openbossa.org> wrote:
>> Hi Mikel:
>>
>> On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>
>>> Split the monitor registration API into three independent registrations
>>> each of them taking one specific GATT primary.
>>> ---
>>> profiles/proximity/manager.c | 16 +++-
>>> profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
>>> profiles/proximity/monitor.h | 17 +++-
>>> 3 files changed, 204 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
>>> index b405f15..1c07267 100644
>>> --- a/profiles/proximity/manager.c
>>> +++ b/profiles/proximity/manager.c
>>> @@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
>>> struct btd_device *device, GSList *uuids)
>>> {
>>> struct gatt_primary *linkloss, *txpower, *immediate;
>>> + int err = 0;
>>>
>>> immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
>>> txpower = btd_device_get_primary(device, TX_POWER_UUID);
>>> linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
>>>
>>> - return monitor_register(device, linkloss, txpower, immediate, &enabled);
>>> + if (linkloss)
>>> + err = monitor_register_linkloss(device, &enabled, linkloss);
>>> +
>>> + if (err >= 0 && txpower)
>>> + err = monitor_register_txpower(device, &enabled, txpower);
>>> +
>>> + if (err >= 0 && immediate)
>>> + err = monitor_register_immediate(device, &enabled, immediate);
>>> +
>>
>> I recommend to invert the logic here, checking if immediate alert is
>> available first. Immediate alert service is mandatory for Link Loss,
>> Path Loss(tx power) and find me.
>
> According to the code being removed in monitor_register(), the
> linkloss service seems independent from immediate alert. In fact, I
> tried to clarify this in the spec and what I found is that the
> linkloss service is mandatory while the rest are optional. Am I
> missing something?
>
> In any case, to start with, it makes sense that txpower gets
> registered last to improve readability.
>
> Also note that I tried to express the dependencies you mention in
> update_monitor() and the registration functions assuming that, once
> split, the profiles can be probed in any arbitrary order (i.e. txpower
> probed before immediate alert).
>
>>
>>> + return err;
>>> }
>>>
>>> static void monitor_device_remove(struct btd_profile *p,
>>> struct btd_device *device)
>>> {
>>> - monitor_unregister(device);
>>> + monitor_unregister_immediate(device);
>>> + monitor_unregister_txpower(device);
>>> + monitor_unregister_linkloss(device);
>>> }
>>>
>>> static struct btd_profile pxp_monitor_profile = {
>>> diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
>>> index 597f161..489ccdd 100644
>>> --- a/profiles/proximity/monitor.c
>>> +++ b/profiles/proximity/monitor.c
>>> @@ -34,6 +34,7 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <sys/stat.h>
>>> +#include <glib.h>
>>>
>>> #include <bluetooth/bluetooth.h>
>>>
>>> @@ -83,6 +84,22 @@ struct monitor {
>>> guint attioid;
>>> };
>>>
>>> +static GSList *monitors = NULL;
>>> +
>>> +static struct monitor *find_monitor(struct btd_device *device)
>>> +{
>>> + GSList *l;
>>> +
>>> + for (l = monitors; l; l = l->next) {
>>> + struct monitor *monitor = l->data;
>>> +
>>> + if (monitor->device == device)
>>> + return monitor;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> static void write_proximity_config(struct btd_device *device, const char *alert,
>>> const char *level)
>>> {
>>> @@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
>>> {
>>> struct monitor *monitor = user_data;
>>>
>>> - if (monitor->immediateto)
>>> - g_source_remove(monitor->immediateto);
>>> -
>>> - if (monitor->attioid)
>>> - btd_device_remove_attio_callback(monitor->device,
>>> - monitor->attioid);
>>> - if (monitor->attrib)
>>> - g_attrib_unref(monitor->attrib);
>>> -
>>> btd_device_unref(monitor->device);
>>> - g_free(monitor->linkloss);
>>> - g_free(monitor->immediate);
>>> - g_free(monitor->txpower);
>>> g_free(monitor->linklosslevel);
>>> g_free(monitor->immediatelevel);
>>> g_free(monitor->signallevel);
>>> g_free(monitor);
>>> +
>>> + monitors = g_slist_remove(monitors, monitor);
>>> }
>>>
>>> -int monitor_register(struct btd_device *device,
>>> - struct gatt_primary *linkloss, struct gatt_primary *txpower,
>>> - struct gatt_primary *immediate, struct enabled *enabled)
>>> +static struct monitor *register_monitor(struct btd_device *device)
>>> {
>>> const char *path = device_get_path(device);
>>> struct monitor *monitor;
>>> char *level;
>>>
>>> + monitor = find_monitor(device);
>>> + if (monitor != NULL)
>>> + return monitor;
>>> +
>>> level = read_proximity_config(device, "LinkLossAlertLevel");
>>>
>>> monitor = g_new0(struct monitor, 1);
>>> @@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
>>> monitor->signallevel = g_strdup("unknown");
>>> monitor->immediatelevel = g_strdup("none");
>>>
>>> + monitors = g_slist_append(monitors, monitor);
>>> +
>>> if (g_dbus_register_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE,
>>> NULL, NULL, monitor_device_properties,
>>> @@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
>>> error("D-Bus failed to register %s interface",
>>> PROXIMITY_INTERFACE);
>>> monitor_destroy(monitor);
>>> - return -1;
>>> + return NULL;
>>> }
>>>
>>> DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
>>>
>>> - if (linkloss && enabled->linkloss) {
>>> - monitor->linkloss = g_new0(struct att_range, 1);
>>> - monitor->linkloss->start = linkloss->range.start;
>>> - monitor->linkloss->end = linkloss->range.end;
>>> -
>>> - monitor->enabled.linkloss = TRUE;
>>> - }
>>> -
>>> - if (immediate) {
>>> - if (txpower && enabled->pathloss) {
>>> - monitor->txpower = g_new0(struct att_range, 1);
>>> - monitor->txpower->start = txpower->range.start;
>>> - monitor->txpower->end = txpower->range.end;
>>> -
>>> - monitor->enabled.pathloss = TRUE;
>>> - }
>>> -
>>> - if (enabled->pathloss || enabled->findme) {
>>> - monitor->immediate = g_new0(struct att_range, 1);
>>> - monitor->immediate->start = immediate->range.start;
>>> - monitor->immediate->end = immediate->range.end;
>>> - }
>>> + return monitor;
>>> +}
>>>
>>> - monitor->enabled.findme = enabled->findme;
>>> - }
>>> +static void update_monitor(struct monitor *monitor)
>>> +{
>>> + if (monitor->txpower != NULL && monitor->immediate != NULL)
>>> + monitor->enabled.pathloss = TRUE;
>>> + else
>>> + monitor->enabled.pathloss = FALSE;
>>>
>>> DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
>>> monitor->enabled.linkloss ? "TRUE" : "FALSE",
>>> monitor->enabled.pathloss ? "TRUE" : "FALSE",
>>> monitor->enabled.findme ? "TRUE" : "FALSE");
>>>
>>> - if (monitor->enabled.linkloss || monitor->enabled.pathloss)
>>> - monitor->attioid = btd_device_add_attio_callback(device,
>>> + if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
>>> + return;
>>> +
>>> + if (monitor->attioid != 0)
>>> + return;
>>> +
>>> + monitor->attioid = btd_device_add_attio_callback(monitor->device,
>>> attio_connected_cb,
>>> attio_disconnected_cb,
>>> monitor);
>>> +}
>>> +
>>> +int monitor_register_linkloss(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *linkloss)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->linkloss)
>>> + return 0;
>>
>> Link loss requires immediate alert service, it is necessary to
>> investigate if make sense to add this verification here or check the
>> value before calling this function.
>> The same comment is valid for path loss(tx power).
>
> Regarding link-loss, please confirm that this dependency to immediate
> alert actually exists.
>
> Regarding tx-power, as mentioned before, the motivation was the fact
> that, if the profiles get split, the exact order in which they are
> probed is undefined. Therefore, monitor_register_txpower() doesn't
> actually set enabled.pathloss to true, but instead delegates this work
> to update_monitor(), which should gracefully handle both possible
> probe combinations (immediate alert probed before or after txpower).
>
>>
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->linkloss = g_new0(struct att_range, 1);
>>> + monitor->linkloss->start = linkloss->range.start;
>>> + monitor->linkloss->end = linkloss->range.end;
>>> + monitor->enabled.linkloss = TRUE;
>>> +
>>> + update_monitor(monitor);
>>>
>>> return 0;
>>> }
>>>
>>> -void monitor_unregister(struct btd_device *device)
>>> +int monitor_register_txpower(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *txpower)
>>> {
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->txpower = g_new0(struct att_range, 1);
>>> + monitor->txpower->start = txpower->range.start;
>>> + monitor->txpower->end = txpower->range.end;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int monitor_register_immediate(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *immediate)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss && !enabled->findme)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->immediate = g_new0(struct att_range, 1);
>>> + monitor->immediate->start = immediate->range.start;
>>> + monitor->immediate->end = immediate->range.end;
>>> + monitor->enabled.findme = enabled->findme;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void cleanup_monitor(struct monitor *monitor)
>>> +{
>>> + struct btd_device *device = monitor->device;
>>> const char *path = device_get_path(device);
>>>
>>> + if (monitor->immediate != NULL || monitor->txpower != NULL)
>>> + return;
>>> +
>>> + if (monitor->immediateto != 0) {
>>> + g_source_remove(monitor->immediateto);
>>> + monitor->immediateto = 0;
>>> + }
>>> +
>>> + if (monitor->attioid != 0) {
>>> + btd_device_remove_attio_callback(device, monitor->attioid);
>>> + monitor->attioid = 0;
>>> + }
>>> +
>>> + if (monitor->attrib != NULL) {
>>> + g_attrib_unref(monitor->attrib);
>>> + monitor->attrib = NULL;
>>> + }
>>> +
>>> + if (monitor->linkloss != NULL)
>>> + return;
>>
>> Why this checking is here instead of check in the beginning of this function?
>
> My understanding of the previous code was that the cleanup above
> (before the linkloss check) belonged to immediate alert and txpower.
> Having a careful look at the code I realized that monitor->attioid and
> monitor->attrib should be moved to the end of the function, since
> linkloss makes use of them.
>
> Whether monitor->immediateto should also come after this check or not
> depends again on what the exact dependencies are.
>
>>
>>> +
>>> g_dbus_unregister_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE);
>>> }
>>> +
>>> +void monitor_unregister_linkloss(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->linkloss);
>>> + monitor->linkloss = NULL;
>>> + monitor->enabled.linkloss = TRUE;
>>
>> If you are unregistering a service, is it necessary to set
>> enabled.linkloss = TRUE?
>
> My mistake, this should have been FALSE.
>
>>
>>> +
>>> + cleanup_monitor(monitor);
>>> +}
>>> +
>>> +void monitor_unregister_txpower(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->txpower);
>>> + monitor->txpower = NULL;
>>> + monitor->enabled.pathloss = FALSE;
>>
>> Same question here.
>
> The unregister function try to support partial unregistration to be
> consistent with the registration procedure, but perhaps this is
> overkill since typically all profiles will be removed at once.
>
> In fact, many other profiles simplify the unregistration by just
> removing more than one profile at the same time (see for example
> audio_device_unregister()). Any thoughts on this? Is such a
> simplification encouraged?
>
>>
>>> +
>>> + cleanup_monitor(monitor);
>>> +}
>>> +
>>> +void monitor_unregister_immediate(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->immediate);
>>> + monitor->immediate = NULL;
>>> + monitor->enabled.findme = FALSE;
>>> + monitor->enabled.pathloss = FALSE;
>>
>> Same question here.
>>
>>
>> Regards,
>> Claudio
>
> Cheers,
> Mikel
Ping.
Cheers,
Mikel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v1 3/7] proximity: Split internal monitor registration API
2013-02-15 7:54 ` Mikel Astiz
2013-02-27 7:46 ` Mikel Astiz
@ 2013-02-28 19:34 ` Claudio Takahasi
1 sibling, 0 replies; 14+ messages in thread
From: Claudio Takahasi @ 2013-02-28 19:34 UTC (permalink / raw)
To: Mikel Astiz; +Cc: linux-bluetooth
Hi Mikel:
On Fri, Feb 15, 2013 at 5:54 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> Hi Claudio,
>
> On Thu, Feb 14, 2013 at 5:37 PM, Claudio Takahasi
> <claudio.takahasi@openbossa.org> wrote:
>> Hi Mikel:
>>
>> On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
>>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>>
>>> Split the monitor registration API into three independent registrations
>>> each of them taking one specific GATT primary.
>>> ---
>>> profiles/proximity/manager.c | 16 +++-
>>> profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
>>> profiles/proximity/monitor.h | 17 +++-
>>> 3 files changed, 204 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
>>> index b405f15..1c07267 100644
>>> --- a/profiles/proximity/manager.c
>>> +++ b/profiles/proximity/manager.c
>>> @@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
>>> struct btd_device *device, GSList *uuids)
>>> {
>>> struct gatt_primary *linkloss, *txpower, *immediate;
>>> + int err = 0;
>>>
>>> immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
>>> txpower = btd_device_get_primary(device, TX_POWER_UUID);
>>> linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
>>>
>>> - return monitor_register(device, linkloss, txpower, immediate, &enabled);
>>> + if (linkloss)
>>> + err = monitor_register_linkloss(device, &enabled, linkloss);
>>> +
>>> + if (err >= 0 && txpower)
>>> + err = monitor_register_txpower(device, &enabled, txpower);
>>> +
>>> + if (err >= 0 && immediate)
>>> + err = monitor_register_immediate(device, &enabled, immediate);
>>> +
>>
>> I recommend to invert the logic here, checking if immediate alert is
>> available first. Immediate alert service is mandatory for Link Loss,
>> Path Loss(tx power) and find me.
>
> According to the code being removed in monitor_register(), the
> linkloss service seems independent from immediate alert. In fact, I
> tried to clarify this in the spec and what I found is that the
> linkloss service is mandatory while the rest are optional. Am I
> missing something?
Indeed, they are independent services. I messed everything up, the
Proximity/Find Profile and their roles.
Immediate Alert service is required for Proximity Path Loss and Find Me.
>
> In any case, to start with, it makes sense that txpower gets
> registered last to improve readability.
>
> Also note that I tried to express the dependencies you mention in
> update_monitor() and the registration functions assuming that, once
> split, the profiles can be probed in any arbitrary order (i.e. txpower
> probed before immediate alert).
>
>>
>>> + return err;
>>> }
>>>
>>> static void monitor_device_remove(struct btd_profile *p,
>>> struct btd_device *device)
>>> {
>>> - monitor_unregister(device);
>>> + monitor_unregister_immediate(device);
>>> + monitor_unregister_txpower(device);
>>> + monitor_unregister_linkloss(device);
>>> }
>>>
>>> static struct btd_profile pxp_monitor_profile = {
>>> diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
>>> index 597f161..489ccdd 100644
>>> --- a/profiles/proximity/monitor.c
>>> +++ b/profiles/proximity/monitor.c
>>> @@ -34,6 +34,7 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <sys/stat.h>
>>> +#include <glib.h>
>>>
>>> #include <bluetooth/bluetooth.h>
>>>
>>> @@ -83,6 +84,22 @@ struct monitor {
>>> guint attioid;
>>> };
>>>
>>> +static GSList *monitors = NULL;
>>> +
>>> +static struct monitor *find_monitor(struct btd_device *device)
>>> +{
>>> + GSList *l;
>>> +
>>> + for (l = monitors; l; l = l->next) {
>>> + struct monitor *monitor = l->data;
>>> +
>>> + if (monitor->device == device)
>>> + return monitor;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> static void write_proximity_config(struct btd_device *device, const char *alert,
>>> const char *level)
>>> {
>>> @@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
>>> {
>>> struct monitor *monitor = user_data;
>>>
>>> - if (monitor->immediateto)
>>> - g_source_remove(monitor->immediateto);
>>> -
>>> - if (monitor->attioid)
>>> - btd_device_remove_attio_callback(monitor->device,
>>> - monitor->attioid);
>>> - if (monitor->attrib)
>>> - g_attrib_unref(monitor->attrib);
>>> -
>>> btd_device_unref(monitor->device);
>>> - g_free(monitor->linkloss);
>>> - g_free(monitor->immediate);
>>> - g_free(monitor->txpower);
>>> g_free(monitor->linklosslevel);
>>> g_free(monitor->immediatelevel);
>>> g_free(monitor->signallevel);
>>> g_free(monitor);
>>> +
>>> + monitors = g_slist_remove(monitors, monitor);
>>> }
>>>
>>> -int monitor_register(struct btd_device *device,
>>> - struct gatt_primary *linkloss, struct gatt_primary *txpower,
>>> - struct gatt_primary *immediate, struct enabled *enabled)
>>> +static struct monitor *register_monitor(struct btd_device *device)
>>> {
>>> const char *path = device_get_path(device);
>>> struct monitor *monitor;
>>> char *level;
>>>
>>> + monitor = find_monitor(device);
>>> + if (monitor != NULL)
>>> + return monitor;
>>> +
>>> level = read_proximity_config(device, "LinkLossAlertLevel");
>>>
>>> monitor = g_new0(struct monitor, 1);
>>> @@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
>>> monitor->signallevel = g_strdup("unknown");
>>> monitor->immediatelevel = g_strdup("none");
>>>
>>> + monitors = g_slist_append(monitors, monitor);
>>> +
>>> if (g_dbus_register_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE,
>>> NULL, NULL, monitor_device_properties,
>>> @@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
>>> error("D-Bus failed to register %s interface",
>>> PROXIMITY_INTERFACE);
>>> monitor_destroy(monitor);
>>> - return -1;
>>> + return NULL;
>>> }
>>>
>>> DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
>>>
>>> - if (linkloss && enabled->linkloss) {
>>> - monitor->linkloss = g_new0(struct att_range, 1);
>>> - monitor->linkloss->start = linkloss->range.start;
>>> - monitor->linkloss->end = linkloss->range.end;
>>> -
>>> - monitor->enabled.linkloss = TRUE;
>>> - }
>>> -
>>> - if (immediate) {
>>> - if (txpower && enabled->pathloss) {
>>> - monitor->txpower = g_new0(struct att_range, 1);
>>> - monitor->txpower->start = txpower->range.start;
>>> - monitor->txpower->end = txpower->range.end;
>>> -
>>> - monitor->enabled.pathloss = TRUE;
>>> - }
>>> -
>>> - if (enabled->pathloss || enabled->findme) {
>>> - monitor->immediate = g_new0(struct att_range, 1);
>>> - monitor->immediate->start = immediate->range.start;
>>> - monitor->immediate->end = immediate->range.end;
>>> - }
>>> + return monitor;
>>> +}
>>>
>>> - monitor->enabled.findme = enabled->findme;
>>> - }
>>> +static void update_monitor(struct monitor *monitor)
>>> +{
>>> + if (monitor->txpower != NULL && monitor->immediate != NULL)
>>> + monitor->enabled.pathloss = TRUE;
>>> + else
>>> + monitor->enabled.pathloss = FALSE;
>>>
>>> DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
>>> monitor->enabled.linkloss ? "TRUE" : "FALSE",
>>> monitor->enabled.pathloss ? "TRUE" : "FALSE",
>>> monitor->enabled.findme ? "TRUE" : "FALSE");
>>>
>>> - if (monitor->enabled.linkloss || monitor->enabled.pathloss)
>>> - monitor->attioid = btd_device_add_attio_callback(device,
>>> + if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
>>> + return;
>>> +
>>> + if (monitor->attioid != 0)
>>> + return;
>>> +
>>> + monitor->attioid = btd_device_add_attio_callback(monitor->device,
>>> attio_connected_cb,
>>> attio_disconnected_cb,
>>> monitor);
>>> +}
>>> +
>>> +int monitor_register_linkloss(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *linkloss)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->linkloss)
>>> + return 0;
>>
>> Link loss requires immediate alert service, it is necessary to
>> investigate if make sense to add this verification here or check the
>> value before calling this function.
>> The same comment is valid for path loss(tx power).
>
> Regarding link-loss, please confirm that this dependency to immediate
> alert actually exists.
As mentioned previously, only path loss requires immediate alert.
>
> Regarding tx-power, as mentioned before, the motivation was the fact
> that, if the profiles get split, the exact order in which they are
> probed is undefined. Therefore, monitor_register_txpower() doesn't
> actually set enabled.pathloss to true, but instead delegates this work
> to update_monitor(), which should gracefully handle both possible
> probe combinations (immediate alert probed before or after txpower).
>
>>
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->linkloss = g_new0(struct att_range, 1);
>>> + monitor->linkloss->start = linkloss->range.start;
>>> + monitor->linkloss->end = linkloss->range.end;
>>> + monitor->enabled.linkloss = TRUE;
>>> +
>>> + update_monitor(monitor);
>>>
>>> return 0;
>>> }
>>>
>>> -void monitor_unregister(struct btd_device *device)
>>> +int monitor_register_txpower(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *txpower)
>>> {
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->txpower = g_new0(struct att_range, 1);
>>> + monitor->txpower->start = txpower->range.start;
>>> + monitor->txpower->end = txpower->range.end;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int monitor_register_immediate(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *immediate)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss && !enabled->findme)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->immediate = g_new0(struct att_range, 1);
>>> + monitor->immediate->start = immediate->range.start;
>>> + monitor->immediate->end = immediate->range.end;
>>> + monitor->enabled.findme = enabled->findme;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void cleanup_monitor(struct monitor *monitor)
>>> +{
>>> + struct btd_device *device = monitor->device;
>>> const char *path = device_get_path(device);
>>>
>>> + if (monitor->immediate != NULL || monitor->txpower != NULL)
>>> + return;
>>> +
>>> + if (monitor->immediateto != 0) {
>>> + g_source_remove(monitor->immediateto);
>>> + monitor->immediateto = 0;
>>> + }
>>> +
>>> + if (monitor->attioid != 0) {
>>> + btd_device_remove_attio_callback(device, monitor->attioid);
>>> + monitor->attioid = 0;
>>> + }
>>> +
>>> + if (monitor->attrib != NULL) {
>>> + g_attrib_unref(monitor->attrib);
>>> + monitor->attrib = NULL;
>>> + }
>>> +
>>> + if (monitor->linkloss != NULL)
>>> + return;
>>
>> Why this checking is here instead of check in the beginning of this function?
>
> My understanding of the previous code was that the cleanup above
> (before the linkloss check) belonged to immediate alert and txpower.
> Having a careful look at the code I realized that monitor->attioid and
> monitor->attrib should be moved to the end of the function, since
> linkloss makes use of them.
>
> Whether monitor->immediateto should also come after this check or not
> depends again on what the exact dependencies are.
Ok..
F.Y.I. the timeout is not even defined in the spec. We added this
harmless alert "reset" just because we noticed that it was required
for some implementations.
>
>>
>>> +
>>> g_dbus_unregister_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE);
>>> }
>>> +
>>> +void monitor_unregister_linkloss(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->linkloss);
>>> + monitor->linkloss = NULL;
>>> + monitor->enabled.linkloss = TRUE;
>>
>> If you are unregistering a service, is it necessary to set
>> enabled.linkloss = TRUE?
>
> My mistake, this should have been FALSE.
>
>>
>>> +
>>> + cleanup_monitor(monitor);
>>> +}
>>> +
>>> +void monitor_unregister_txpower(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->txpower);
>>> + monitor->txpower = NULL;
>>> + monitor->enabled.pathloss = FALSE;
>>
>> Same question here.
>
> The unregister function try to support partial unregistration to be
> consistent with the registration procedure, but perhaps this is
> overkill since typically all profiles will be removed at once.
>
> In fact, many other profiles simplify the unregistration by just
> removing more than one profile at the same time (see for example
> audio_device_unregister()). Any thoughts on this? Is such a
> simplification encouraged?
Maybe in the next round. It is better to keep your patch series small ;-)
Regards,
Claudio
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-02-28 19:34 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06 9:16 [RFC v1 0/7] One remote UUID per btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 1/7] avrcp: Refactor server registration Mikel Astiz
2013-02-06 9:16 ` [RFC v1 2/7] audio: Split AVRCP into two btd_profile Mikel Astiz
2013-02-07 9:43 ` Luiz Augusto von Dentz
2013-02-07 10:43 ` Mikel Astiz
2013-02-06 9:16 ` [RFC v1 3/7] proximity: Split internal monitor registration API Mikel Astiz
2013-02-14 16:37 ` Claudio Takahasi
2013-02-15 7:54 ` Mikel Astiz
2013-02-27 7:46 ` Mikel Astiz
2013-02-28 19:34 ` Claudio Takahasi
2013-02-06 9:16 ` [RFC v1 4/7] proximity: Split monitor into three btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 5/7] gatt: List only GATT_UUID as remote UUID Mikel Astiz
2013-02-06 9:16 ` [RFC v1 6/7] health: Split health into two btd_profile Mikel Astiz
2013-02-06 9:16 ` [RFC v1 7/7] profile: Limit to one remote UUID per profile Mikel Astiz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.