* Re: [RFC v4 05/12] Bluetooth: Stop scanning on LE connection
From: Johan Hedberg @ 2013-12-18 11:11 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <1387289112.1231.21.camel@bali>
Hi Andre,
On Tue, Dec 17, 2013, Andre Guedes wrote:
> > > @@ -518,8 +518,17 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
> > > {
> > > struct hci_conn *conn;
> > >
> > > - if (status == 0)
> > > + if (status == 0) {
> > > + /* If the discovery procedure was running, we prematurely
> > > + * stopped it. So we have to change the discovery state.
> > > + */
> > > + if (hdev->discovery.state == DISCOVERY_FINDING) {
> > > + cancel_delayed_work(&hdev->le_scan_disable);
> > > + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > > + }
> > > +
> > > return;
> > > + }
> > >
> > > BT_ERR("HCI request failed to create LE connection: status 0x%2.2x",
> > > status);
> > > @@ -552,6 +561,18 @@ static int hci_create_le_conn(struct hci_conn *conn)
> > >
> > > hci_req_init(&req, hdev);
> > >
> > > + /* If controller is scanning, we stop it since some controllers are
> > > + * not able to scan and connect at the same time.
> > > + */
> > > + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags)) {
> > > + struct hci_cp_le_set_scan_enable enable_cp;
> > > +
> > > + memset(&enable_cp, 0, sizeof(enable_cp));
> > > + enable_cp.enable = LE_SCAN_DISABLE;
> > > + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> > > + &enable_cp);
> > > + }
> > > +
> > > memset(&cp, 0, sizeof(cp));
> > > cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
> > > cp.scan_window = cpu_to_le16(hdev->le_scan_window);
> >
> > The way this all hangs together with a discovery process started through
> > mgmt_start_discovery feels a bit flimsy to me. Particularly, have you
> > ensured that everything is fine if you've got inquiry ongoing when you
> > do hci_discovery_set_state(hdev, DISCOVERY_STOPPED). Also, are there any
> > risks of race conditions here, e.g. is it fine to let the
> > cancel_delayed_work() call be in create_le_conn_complete() instead of
> > doing it in hci_create_le_conn()?
>
> Yes, I did lots of testing, including the inquiry test you mentioned,
> and I didn't find any issues.
>
> I failed to see any race conditons since cancel_delayed_work() does not
> block. So it is fine to call cancel_delayed_work() in
> create_le_conn_complete() as well as in hci_create_le_conn().
I presume you'd want to prevent the delayed work from getting called as
soon as you've entered the if (test_bit(HCI_LE_SCAN)) section in
hci_create_le_conn? In theory it might get called before
create_le_conn_complete. In such a scenario
le_scan_disable_work_complete could e.g. trigger inquiry before
create_le_conn_complete is called, couldn't it?
> > What also makes this hard to track is that the condition you're testing
> > for first is the HCI_LE_SCAN bit, but then later you look at
> > discovery.state == DISCOVERY_FINDING. For the casual reader there's no
> > direct indication of how these two are releated for the various types of
> > discovery that are possible (LE-only, BR/EDR-only and interleaved).
>
> Yes, I see your point. However, we can't check HCI_LE_SCAN in
> create_le_conn_complete() because the flag was already cleared in
> hci_cc_le_set_scan_enable().
Understood.
> > I don't mean to say that this is a nack for the patch, but I'd like to
> > know that you've considered and tested this kind of cases. I had to
> > spend quite some time looking through the existing code and this patch
> > and still couldn't arrive at absolute confidence of its correctness,
> > meaning there should hopefully be some room for simplification.
>
> So I think we can do some simplification by moving this discovery
> handling from create_le_conn_complete() to hci_create_le_conn(), at
> least I think this code will become a bit easier to follow. What do you
> think?
I think it would at least close the potential race I described above,
i.e. the delayed work getting triggered before create_le_conn_complete.
Johan
^ permalink raw reply
* Re: [PATCH 1/3] android/tester: Check status returned from HAL calls
From: Johan Hedberg @ 2013-12-18 10:26 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1387356913-32666-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Wed, Dec 18, 2013, Andrei Emeltchenko wrote:
> In test setup phase check status returned from enable() call.
> ---
> android/android-tester.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
All three patches have been applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH v2 1/5] android/tester: Add UUIDS set prop fail test case
From: Andrei Emeltchenko @ 2013-12-18 10:25 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Wed, Dec 18, 2013 at 10:53:09AM +0100, Grzegorz Kolodziejczyk wrote:
> This adds UUIDS set property fail test case due to only get
> possibility.
> ---
> android/android-tester.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index 92e6080..0025f20 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -596,6 +596,18 @@ static const struct generic_data bluetooth_getprop_bdname_success_test = {
> .expected_property.len = 17
> };
>
> +static unsigned char setprop_uuids[] = { 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00,
> + 0x00, 0x80, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00 };
> +
> +static const struct generic_data bluetooth_setprop_uuid_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
Do we agree to use spaces before and after braces?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH v2 1/5] android/tester: Add UUIDS set prop fail test case
From: Johan Hedberg @ 2013-12-18 10:23 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Wed, Dec 18, 2013, Grzegorz Kolodziejczyk wrote:
> This adds UUIDS set property fail test case due to only get
> possibility.
> ---
> android/android-tester.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
All five patches have been applied. Thanks.
Johan
^ permalink raw reply
* [PATCH v2 5/5] android/tester: Add SERVICE_RECORD set prop fail test case
From: Grzegorz Kolodziejczyk @ 2013-12-18 9:53 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This adds SERVICE_RECORD set property fail test case due to only
get possibility.
---
android/android-tester.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index 9bae94d..3d439de 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -638,6 +638,21 @@ static const struct generic_data bluetooth_setprop_remote_rssi_invalid_test = {
.expected_property.len = sizeof(setprop_remote_rssi)
};
+static bt_service_record_t setprop_remote_service = {
+ .uuid = { {0x00} },
+ .channel = 12,
+ .name = "bt_name"
+};
+
+static const struct generic_data
+ bluetooth_setprop_service_record_invalid_test = {
+ .expected_hal_callbacks = {ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_FAIL,
+ .expected_property.type = BT_PROPERTY_SERVICE_RECORD,
+ .expected_property.val = &setprop_remote_service,
+ .expected_property.len = sizeof(setprop_remote_service)
+};
+
static bt_callbacks_t bt_callbacks = {
.size = sizeof(bt_callbacks),
.adapter_state_changed_cb = adapter_state_changed_cb,
@@ -1067,6 +1082,19 @@ static void test_setprop_rssi_invalid(const void *test_data)
check_expected_status(adapter_status);
}
+static void test_setprop_service_record_invalid(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct generic_data *test = data->test_data;
+ const bt_property_t *prop = &test->expected_property;
+ bt_status_t adapter_status;
+
+ init_test_conditions(data);
+
+ adapter_status = data->if_bluetooth->set_adapter_property(prop);
+ check_expected_status(adapter_status);
+}
+
#define test_bredrle(name, data, test_setup, test, test_teardown) \
do { \
struct test_data *user; \
@@ -1142,6 +1170,11 @@ int main(int argc, char *argv[])
setup_enabled_adapter,
test_setprop_rssi_invalid, teardown);
+ test_bredrle("Set SERVICE_RECORD - Invalid",
+ &bluetooth_setprop_service_record_invalid_test,
+ setup_enabled_adapter,
+ test_setprop_service_record_invalid, teardown);
+
test_bredrle("Socket Init", NULL, setup_socket_interface,
test_dummy, teardown);
--
1.8.4.2
^ permalink raw reply related
* [PATCH v2 4/5] android/tester: Add RSSI set prop fail test case
From: Grzegorz Kolodziejczyk @ 2013-12-18 9:53 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This adds RSSI set property fail test case due to be only remote device
property.
---
android/android-tester.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index 2f2bbff..9bae94d 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -628,6 +628,16 @@ static const struct generic_data bluetooth_setprop_tod_invalid_test = {
.expected_property.len = sizeof(setprop_type_of_device)
};
+static int32_t setprop_remote_rssi = 0;
+
+static const struct generic_data bluetooth_setprop_remote_rssi_invalid_test = {
+ .expected_hal_callbacks = {ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_FAIL,
+ .expected_property.type = BT_PROPERTY_REMOTE_RSSI,
+ .expected_property.val = &setprop_remote_rssi,
+ .expected_property.len = sizeof(setprop_remote_rssi)
+};
+
static bt_callbacks_t bt_callbacks = {
.size = sizeof(bt_callbacks),
.adapter_state_changed_cb = adapter_state_changed_cb,
@@ -1044,6 +1054,19 @@ static void test_setprop_tod_invalid(const void *test_data)
check_expected_status(adapter_status);
}
+static void test_setprop_rssi_invalid(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct generic_data *test = data->test_data;
+ const bt_property_t *prop = &test->expected_property;
+ bt_status_t adapter_status;
+
+ init_test_conditions(data);
+
+ adapter_status = data->if_bluetooth->set_adapter_property(prop);
+ check_expected_status(adapter_status);
+}
+
#define test_bredrle(name, data, test_setup, test, test_teardown) \
do { \
struct test_data *user; \
@@ -1114,6 +1137,11 @@ int main(int argc, char *argv[])
setup_enabled_adapter,
test_setprop_tod_invalid, teardown);
+ test_bredrle("Set REMOTE_RSSI - Invalid",
+ &bluetooth_setprop_remote_rssi_invalid_test,
+ setup_enabled_adapter,
+ test_setprop_rssi_invalid, teardown);
+
test_bredrle("Socket Init", NULL, setup_socket_interface,
test_dummy, teardown);
--
1.8.4.2
^ permalink raw reply related
* [PATCH v2 3/5] android/tester: Add TOD set prop fail test case
From: Grzegorz Kolodziejczyk @ 2013-12-18 9:53 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This adds TYPE_OF_DEVICE set property fail test case due to only
get possibility.
---
android/android-tester.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index 345d5c6..2f2bbff 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -618,6 +618,16 @@ static const struct generic_data bluetooth_setprop_cod_invalid_test = {
.expected_property.len = sizeof(setprop_class_of_device)
};
+static bt_device_type_t setprop_type_of_device = BT_DEVICE_DEVTYPE_BREDR;
+
+static const struct generic_data bluetooth_setprop_tod_invalid_test = {
+ .expected_hal_callbacks = {ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_FAIL,
+ .expected_property.type = BT_PROPERTY_TYPE_OF_DEVICE,
+ .expected_property.val = &setprop_type_of_device,
+ .expected_property.len = sizeof(setprop_type_of_device)
+};
+
static bt_callbacks_t bt_callbacks = {
.size = sizeof(bt_callbacks),
.adapter_state_changed_cb = adapter_state_changed_cb,
@@ -1021,6 +1031,19 @@ static void test_setprop_cod_invalid(const void *test_data)
check_expected_status(adapter_status);
}
+static void test_setprop_tod_invalid(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct generic_data *test = data->test_data;
+ const bt_property_t *prop = &test->expected_property;
+ bt_status_t adapter_status;
+
+ init_test_conditions(data);
+
+ adapter_status = data->if_bluetooth->set_adapter_property(prop);
+ check_expected_status(adapter_status);
+}
+
#define test_bredrle(name, data, test_setup, test, test_teardown) \
do { \
struct test_data *user; \
@@ -1086,6 +1109,11 @@ int main(int argc, char *argv[])
setup_enabled_adapter,
test_setprop_cod_invalid, teardown);
+ test_bredrle("Set TYPE_OF_DEVICE - Invalid",
+ &bluetooth_setprop_tod_invalid_test,
+ setup_enabled_adapter,
+ test_setprop_tod_invalid, teardown);
+
test_bredrle("Socket Init", NULL, setup_socket_interface,
test_dummy, teardown);
--
1.8.4.2
^ permalink raw reply related
* [PATCH v2 2/5] android/tester: Add COD set prop fail test case
From: Grzegorz Kolodziejczyk @ 2013-12-18 9:53 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387360393-853-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
This adds CLASS_OF_DEVICE set property fail test case due to only
get possibility.
---
android/android-tester.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index 0025f20..345d5c6 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -608,6 +608,16 @@ static const struct generic_data bluetooth_setprop_uuid_invalid_test = {
.expected_property.len = sizeof(setprop_uuids)
};
+static uint32_t setprop_class_of_device = 0;
+
+static const struct generic_data bluetooth_setprop_cod_invalid_test = {
+ .expected_hal_callbacks = {ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_FAIL,
+ .expected_property.type = BT_PROPERTY_CLASS_OF_DEVICE,
+ .expected_property.val = &setprop_class_of_device,
+ .expected_property.len = sizeof(setprop_class_of_device)
+};
+
static bt_callbacks_t bt_callbacks = {
.size = sizeof(bt_callbacks),
.adapter_state_changed_cb = adapter_state_changed_cb,
@@ -998,6 +1008,19 @@ static void test_setprop_uuid_invalid(const void *test_data)
check_expected_status(adapter_status);
}
+static void test_setprop_cod_invalid(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct generic_data *test = data->test_data;
+ const bt_property_t *prop = &test->expected_property;
+ bt_status_t adapter_status;
+
+ init_test_conditions(data);
+
+ adapter_status = data->if_bluetooth->set_adapter_property(prop);
+ check_expected_status(adapter_status);
+}
+
#define test_bredrle(name, data, test_setup, test, test_teardown) \
do { \
struct test_data *user; \
@@ -1058,6 +1081,11 @@ int main(int argc, char *argv[])
setup_enabled_adapter,
test_setprop_uuid_invalid, teardown);
+ test_bredrle("Set CLASS_OF_DEVICE - Invalid",
+ &bluetooth_setprop_cod_invalid_test,
+ setup_enabled_adapter,
+ test_setprop_cod_invalid, teardown);
+
test_bredrle("Socket Init", NULL, setup_socket_interface,
test_dummy, teardown);
--
1.8.4.2
^ permalink raw reply related
* [PATCH v2 1/5] android/tester: Add UUIDS set prop fail test case
From: Grzegorz Kolodziejczyk @ 2013-12-18 9:53 UTC (permalink / raw)
To: linux-bluetooth
This adds UUIDS set property fail test case due to only get
possibility.
---
android/android-tester.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index 92e6080..0025f20 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -596,6 +596,18 @@ static const struct generic_data bluetooth_getprop_bdname_success_test = {
.expected_property.len = 17
};
+static unsigned char setprop_uuids[] = { 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00,
+ 0x00, 0x80, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00 };
+
+static const struct generic_data bluetooth_setprop_uuid_invalid_test = {
+ .expected_hal_callbacks = {ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_FAIL,
+ .expected_property.type = BT_PROPERTY_UUIDS,
+ .expected_property.val = &setprop_uuids,
+ .expected_property.len = sizeof(setprop_uuids)
+};
+
static bt_callbacks_t bt_callbacks = {
.size = sizeof(bt_callbacks),
.adapter_state_changed_cb = adapter_state_changed_cb,
@@ -973,6 +985,19 @@ static void test_getprop_bdname_success(const void *test_data)
check_expected_status(adapter_status);
}
+static void test_setprop_uuid_invalid(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct generic_data *test = data->test_data;
+ const bt_property_t *prop = &test->expected_property;
+ bt_status_t adapter_status;
+
+ init_test_conditions(data);
+
+ adapter_status = data->if_bluetooth->set_adapter_property(prop);
+ check_expected_status(adapter_status);
+}
+
#define test_bredrle(name, data, test_setup, test, test_teardown) \
do { \
struct test_data *user; \
@@ -1028,6 +1053,11 @@ int main(int argc, char *argv[])
setup_enabled_adapter,
test_getprop_bdname_success, teardown);
+ test_bredrle("Set UUID - Invalid",
+ &bluetooth_setprop_uuid_invalid_test,
+ setup_enabled_adapter,
+ test_setprop_uuid_invalid, teardown);
+
test_bredrle("Socket Init", NULL, setup_socket_interface,
test_dummy, teardown);
--
1.8.4.2
^ permalink raw reply related
* Re: [PATCH 2/4] emulator: Add data handler for l2cap connections
From: Johan Hedberg @ 2013-12-18 9:40 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org development
In-Reply-To: <CABD6X-+XUgQDoJgetbEAjcCazQ396UGUG9qnKbfVHLQu0u+UfA@mail.gmail.com>
Hi Marcin,
On Wed, Dec 18, 2013, Marcin Kraglak wrote:
> On 18 December 2013 09:40, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi Marcin,
> >
> > On Tue, Dec 17, 2013, Marcin wrote:
> >> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
> >> + bthost_l2cap_data_cb handler,
> >> + uint16_t psm)
> >> +{
> >> + bthost->read_data_psm = psm;
> >> + bthost->l2cap_data_cb = handler;
> >> +}
> >
> > The function is called just "data handler" but it applies to a more
> > specific read_data_psm. Should this perhaps be simply called data_psm?
> >
> >
> >> +typedef void (*bthost_l2cap_data_cb) (const void *data, uint16_t len);
> >> +
> >> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
> >> + bthost_l2cap_data_cb handler,
> >> + uint16_t psm);
> >> +
> >
> > The problem with your approach is that it will not be usable for
> > protocols that have multiple channels to the same PSM such as AVDTP.
> > To cover this I'd suggest you to consider having something like the
> > following:
> >
> > typedef void (*bthost_l2cap_connect_cb) (uint16_t handle, uint16_t cid);
> >
> > void bthost_add_l2cap_server(struct bthost *bthost, uint16_t psm,
> > bthost_l2cap_connect_cb func);
> > /* the above could replace the existing bthost_set_server_psm */
> >
> > void bthost_connect_l2cap(struct bthost *bthost, uint16_t handle, uint16_t psm,
> > bthost_l2cap_connect_cb func);
> >
> > For sending and receiving data you wouldn't then need any new functions
> > but you could use the already existing bthost_add_cid_hook (for
> > receiving data) and bthost_send_cid (for sending data).
> >
> > Thoughts?
> >
> > Johan
>
> So first is to extend existing method bthost_add_l2cap_server and call
> back after connect, and create method bthost_connect_l2cap (instead of
> calling bthost_l2cap_req())?
Maybe you can skip adding this higher level bthost_connect_l2cap for now
and just use the existing bthost_l2cap_req. What I was thinking is that
this new feature is kind of above the existing lower-level access
functions but it's fine to try to keep using them as long as things
don't get too messy.
Johan
^ permalink raw reply
* Re: [PATCH 6/7] android/tester: Add RSSI set prop fail test case
From: Johan Hedberg @ 2013-12-18 8:57 UTC (permalink / raw)
To: Andrei Emeltchenko, Grzegorz Kolodziejczyk, linux-bluetooth
In-Reply-To: <20131218084600.GA26311@aemeltch-MOBL1>
Hi Andrei,
On Wed, Dec 18, 2013, Andrei Emeltchenko wrote:
> On Wed, Dec 18, 2013 at 10:00:34AM +0200, Johan Hedberg wrote:
> > Hi Grzegorz,
> >
> > On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> > > +static int32_t setprop_remote_rssi = 0;
> > > +
> > > +static const struct generic_data bluetooth_setprop_remote_rssi_invalid_test = {
> > > + .expected_hal_callbacks = {ADAPTER_TEST_END},
>
> btw: do we need to have space after "{" ?
Yes, and before } too.
Johan
^ permalink raw reply
* Re: [PATCH 2/4] emulator: Add data handler for l2cap connections
From: Marcin Kraglak @ 2013-12-18 8:56 UTC (permalink / raw)
To: Marcin, linux-bluetooth@vger.kernel.org development
In-Reply-To: <20131218084056.GH12649@x220.p-661hnu-f1>
Hi Johan,
On 18 December 2013 09:40, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Dec 17, 2013, Marcin wrote:
>> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
>> + bthost_l2cap_data_cb handler,
>> + uint16_t psm)
>> +{
>> + bthost->read_data_psm = psm;
>> + bthost->l2cap_data_cb = handler;
>> +}
>
> The function is called just "data handler" but it applies to a more
> specific read_data_psm. Should this perhaps be simply called data_psm?
>
>
>> +typedef void (*bthost_l2cap_data_cb) (const void *data, uint16_t len);
>> +
>> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
>> + bthost_l2cap_data_cb handler,
>> + uint16_t psm);
>> +
>
> The problem with your approach is that it will not be usable for
> protocols that have multiple channels to the same PSM such as AVDTP.
> To cover this I'd suggest you to consider having something like the
> following:
>
> typedef void (*bthost_l2cap_connect_cb) (uint16_t handle, uint16_t cid);
>
> void bthost_add_l2cap_server(struct bthost *bthost, uint16_t psm,
> bthost_l2cap_connect_cb func);
> /* the above could replace the existing bthost_set_server_psm */
>
> void bthost_connect_l2cap(struct bthost *bthost, uint16_t handle, uint16_t psm,
> bthost_l2cap_connect_cb func);
>
> For sending and receiving data you wouldn't then need any new functions
> but you could use the already existing bthost_add_cid_hook (for
> receiving data) and bthost_send_cid (for sending data).
>
> Thoughts?
>
> Johan
So first is to extend existing method bthost_add_l2cap_server and call
back after connect, and create method bthost_connect_l2cap (instead of
calling bthost_l2cap_req())?
BR
Marcin
^ permalink raw reply
* [PATCH 3/3] android/tester: Enable bthost after device is enabled
From: Andrei Emeltchenko @ 2013-12-18 8:55 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387356913-32666-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
This puts bthost to connectible mode allowing us to use powered setup
for connect to emulated device.
---
android/android-tester.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/android/android-tester.c b/android/android-tester.c
index 423dd6a..736cd70 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -33,10 +33,15 @@
#include "src/shared/mgmt.h"
#include "src/shared/hciemu.h"
+#include "emulator/bthost.h"
+#include "monitor/bt.h"
+
#include <hardware/hardware.h>
#include <hardware/bluetooth.h>
#include <hardware/bt_sock.h>
+#include "utils.h"
+
#define ADAPTER_PROPS ADAPTER_PROP_BDADDR, ADAPTER_PROP_BDNAME, \
ADAPTER_PROP_UUIDS, ADAPTER_PROP_COD, \
ADAPTER_PROP_TYPE, ADAPTER_PROP_SCAN_MODE, \
@@ -466,12 +471,48 @@ failed:
close(fd);
}
+static void emu_connectable_complete(uint16_t opcode, uint8_t status,
+ const void *param, uint8_t len,
+ void *user_data)
+{
+ switch (opcode) {
+ case BT_HCI_CMD_WRITE_SCAN_ENABLE:
+ case BT_HCI_CMD_LE_SET_ADV_ENABLE:
+ break;
+ default:
+ return;
+ }
+
+ tester_print("Emulated remote set connectable status 0x%02x", status);
+
+ if (status)
+ tester_setup_failed();
+ else
+ tester_setup_complete();
+}
+
+static void setup_powered_emulated_remote(void)
+{
+ struct test_data *data = tester_get_data();
+ struct bthost *bthost;
+
+ tester_print("Controller powered on");
+
+ bthost = hciemu_client_get_host(data->hciemu);
+ bthost_set_cmd_complete_cb(bthost, emu_connectable_complete, data);
+
+ if (data->hciemu_type == HCIEMU_TYPE_LE)
+ bthost_set_adv_enable(bthost, 0x01);
+ else
+ bthost_write_scan_enable(bthost, 0x03);
+}
+
static void adapter_state_changed_cb(bt_state_t state)
{
switch (state) {
case BT_STATE_ON:
if (is_empty_halcb_list())
- tester_setup_complete();
+ setup_powered_emulated_remote();
update_hal_cb_list(ADAPTER_STATE_CHANGED_ON);
break;
case BT_STATE_OFF:
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/3] android/tester: Check return status for enable()
From: Andrei Emeltchenko @ 2013-12-18 8:55 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387356913-32666-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Use check_expected_status helper for test for returned status in
enable() HAL call.
---
android/android-tester.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/android/android-tester.c b/android/android-tester.c
index 4a75704..423dd6a 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -542,7 +542,8 @@ fail:
static const struct generic_data bluetooth_enable_success_test = {
.expected_hal_callbacks = {ADAPTER_PROPS, ADAPTER_STATE_CHANGED_ON,
- ADAPTER_TEST_END}
+ ADAPTER_TEST_END},
+ .expected_adapter_status = BT_STATUS_SUCCESS
};
static const struct generic_data bluetooth_enable_done_test = {
@@ -726,10 +727,12 @@ static void teardown(const void *test_data)
static void test_enable(const void *test_data)
{
struct test_data *data = tester_get_data();
+ bt_status_t adapter_status;
init_test_conditions(data);
- data->if_bluetooth->enable();
+ adapter_status = data->if_bluetooth->enable();
+ check_expected_status(adapter_status);
}
static void test_enable_done(const void *test_data)
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/3] android/tester: Check status returned from HAL calls
From: Andrei Emeltchenko @ 2013-12-18 8:55 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
In test setup phase check status returned from enable() call.
---
android/android-tester.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/android/android-tester.c b/android/android-tester.c
index bd6fa02..4a75704 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -696,10 +696,13 @@ static void setup_base(const void *test_data)
static void setup_enabled_adapter(const void *test_data)
{
struct test_data *data = tester_get_data();
+ bt_status_t status;
setup(data);
- data->if_bluetooth->enable();
+ status = data->if_bluetooth->enable();
+ if (status != BT_STATUS_SUCCESS)
+ tester_setup_failed();
}
static void teardown(const void *test_data)
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 6/7] android/tester: Add RSSI set prop fail test case
From: Andrei Emeltchenko @ 2013-12-18 8:46 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk, linux-bluetooth
In-Reply-To: <20131218080034.GD12649@x220.p-661hnu-f1>
On Wed, Dec 18, 2013 at 10:00:34AM +0200, Johan Hedberg wrote:
> Hi Grzegorz,
>
> On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> > +static int32_t setprop_remote_rssi = 0;
> > +
> > +static const struct generic_data bluetooth_setprop_remote_rssi_invalid_test = {
> > + .expected_hal_callbacks = {ADAPTER_TEST_END},
btw: do we need to have space after "{" ?
Best regards
Andrei Emeltchenko
> > + .expected_adapter_status = BT_STATUS_FAIL,
> > + .expected_property.type = BT_PROPERTY_REMOTE_RSSI,
> > + .expected_property.val = &setprop_remote_rssi,
> > + .expected_property.len = sizeof(int32_t)
>
> sizeof(setprop_remote_rssi);
>
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] android/tester: Fix not returning in failure
From: Johan Hedberg @ 2013-12-18 8:42 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1387289853-8942-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Tue, Dec 17, 2013, Andrei Emeltchenko wrote:
> ---
> android/android-tester.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index 2a8c8d0..c193921 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -851,8 +851,10 @@ static void setup_socket_interface(const void *test_data)
> setup(data);
>
> sock = data->if_bluetooth->get_profile_interface(BT_PROFILE_SOCKETS_ID);
> - if (!sock)
> + if (!sock) {
> tester_setup_failed();
> + return;
> + }
>
> data->if_sock = sock;
Applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 2/4] emulator: Add data handler for l2cap connections
From: Johan Hedberg @ 2013-12-18 8:40 UTC (permalink / raw)
To: Marcin; +Cc: linux-bluetooth
In-Reply-To: <1387291779-14960-2-git-send-email-marcin.kraglak@tieto.com>
Hi Marcin,
On Tue, Dec 17, 2013, Marcin wrote:
> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
> + bthost_l2cap_data_cb handler,
> + uint16_t psm)
> +{
> + bthost->read_data_psm = psm;
> + bthost->l2cap_data_cb = handler;
> +}
The function is called just "data handler" but it applies to a more
specific read_data_psm. Should this perhaps be simply called data_psm?
> +typedef void (*bthost_l2cap_data_cb) (const void *data, uint16_t len);
> +
> +void bthost_l2cap_set_data_handler(struct bthost *bthost,
> + bthost_l2cap_data_cb handler,
> + uint16_t psm);
> +
The problem with your approach is that it will not be usable for
protocols that have multiple channels to the same PSM such as AVDTP.
To cover this I'd suggest you to consider having something like the
following:
typedef void (*bthost_l2cap_connect_cb) (uint16_t handle, uint16_t cid);
void bthost_add_l2cap_server(struct bthost *bthost, uint16_t psm,
bthost_l2cap_connect_cb func);
/* the above could replace the existing bthost_set_server_psm */
void bthost_connect_l2cap(struct bthost *bthost, uint16_t handle, uint16_t psm,
bthost_l2cap_connect_cb func);
For sending and receiving data you wouldn't then need any new functions
but you could use the already existing bthost_add_cid_hook (for
receiving data) and bthost_send_cid (for sending data).
Thoughts?
Johan
^ permalink raw reply
* Re: [PATCH 1/7] android/tester: Fix enum and define coding style
From: Johan Hedberg @ 2013-12-18 8:05 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Grzegorz Kolodziejczyk, linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZLCgpqR+hsR7vcadV52Y+x2jdRSWGQWfsSr3PhyPKCNTQ@mail.gmail.com>
Hi Luiz,
On Tue, Dec 17, 2013, Luiz Augusto von Dentz wrote:
> On Tue, Dec 17, 2013 at 2:37 PM, Grzegorz Kolodziejczyk
> <grzegorz.kolodziejczyk@tieto.com> wrote:
> > This changes all enums values and defines to uppercase instead of
> > lowercase according to coding style.
> > ---
> > android/android-tester.c | 86 ++++++++++++++++++++++++------------------------
> > 1 file changed, 43 insertions(+), 43 deletions(-)
> >
> > diff --git a/android/android-tester.c b/android/android-tester.c
> > index eb938d0..4eb265b 100644
> > --- a/android/android-tester.c
> > +++ b/android/android-tester.c
> > @@ -37,10 +37,10 @@
> > #include <hardware/bluetooth.h>
> > #include <hardware/bt_sock.h>
> >
> > -#define adapter_props adapter_prop_bdaddr, adapter_prop_bdname, \
> > - adapter_prop_uuids, adapter_prop_cod, \
> > - adapter_prop_type, adapter_prop_scan_mode, \
> > - adapter_prop_bonded_devices, adapter_prop_disc_timeout
> > +#define ADAPTER_PROPS ADAPTER_PROP_BDADDR, ADAPTER_PROP_BDNAME, \
> > + ADAPTER_PROP_UUIDS, ADAPTER_PROP_COD, \
> > + ADAPTER_PROP_TYPE, ADAPTER_PROP_SCAN_MODE, \
> > + ADAPTER_PROP_BONDED_DEVICES, ADAPTER_PROP_DISC_TIMEOUT
> >
> > static bt_scan_mode_t test_setprop_scanmode_val =
> > BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE;
> > @@ -52,19 +52,19 @@ static uint32_t test_setprop_disctimeout_val = 120;
> > */
> >
> > enum hal_bluetooth_callbacks_id {
> > - adapter_test_end,
> > - adapter_test_setup_mode,
> > - adapter_state_changed_on,
> > - adapter_state_changed_off,
> > - adapter_prop_bdaddr,
> > - adapter_prop_bdname,
> > - adapter_prop_uuids,
> > - adapter_prop_cod,
> > - adapter_prop_type,
> > - adapter_prop_scan_mode,
> > - adapter_prop_disc_timeout,
> > - adapter_prop_service_record,
> > - adapter_prop_bonded_devices
> > + ADAPTER_TEST_END,
> > + ADAPTER_TEST_SETUP_MODE,
> > + ADAPTER_STATE_CHANGED_ON,
> > + ADAPTER_STATE_CHANGED_OFF,
> > + ADAPTER_PROP_BDADDR,
> > + ADAPTER_PROP_BDNAME,
> > + ADAPTER_PROP_UUIDS,
> > + ADAPTER_PROP_COD,
> > + ADAPTER_PROP_TYPE,
> > + ADAPTER_PROP_SCAN_MODE,
> > + ADAPTER_PROP_DISC_TIMEOUT,
> > + ADAPTER_PROP_SERVICE_RECORD,
> > + ADAPTER_PROP_BONDED_DEVICES
> > };
>
> This is something I dislike right now, why are defining these and not
> using directly the values defined in hal-msg.h? We should probably
> have a struct with expected opcode and value e.g:
> .{HAL_EV_ADAPTER_PROPS_CHANGED, BT_STATE_ON} and don't try to invent
> another ID for each of those.
I missed this email from you and already applied the patch. You're right
that if these are already defined elsewhere they should not be
redefined. What bugged me most about this was the lower-case so whatever
fix is done will have to be on top of this patch.
Johan
^ permalink raw reply
* Re: [PATCH 2/2] android/hidhost: Simplify handle_uhid_output
From: Johan Hedberg @ 2013-12-18 8:03 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Szymon Janc, linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZLu11494VB7WKMj-nxaXr1GfzNt_unYezPcU==e_7HweQ@mail.gmail.com>
Hi Luiz & Szymon,
On Mon, Dec 16, 2013, Luiz Augusto von Dentz wrote:
> On Mon, Dec 16, 2013 at 4:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> > Make it use VLA for req buffer. This makes function simpler and also
> > fix cutting req to 255 bytes (req_len was uint8_t)
> > ---
> > android/hidhost.c | 17 +++++------------
> > 1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/android/hidhost.c b/android/hidhost.c
> > index 0e0391a..76322af 100644
> > --- a/android/hidhost.c
> > +++ b/android/hidhost.c
> > @@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
> > static void handle_uhid_output(struct hid_device *dev,
> > struct uhid_output_req *output)
> > {
> > - int fd, i;
> > - uint8_t *req = NULL;
> > - uint8_t req_size = 0;
> > + int fd;
> > + unsigned int i;
> > + uint8_t req[1 + (output->size / 2)];
>
> Im not a fan of VLA and Ive seem even some static analyzer that would
> warn about its use without first checking > 0 and have a check of
> upper bond limit, so perhaps just having it set to UHID_DATA_MAX or
> dynamically allocate a buffer to match the output MTU size of the
> control channel would better imo.
I've applied the first patch in this set, but was there some conclusion
on how to proceed here?
If output->size was an uint8_t this would be a non-issue, but as it's
uint16_t I'm a bit uncomfortable with the idea of potentially having a
huge variable on the stack.
Johan
^ permalink raw reply
* Re: [PATCH 7/7] android/tester: Add SERVICE_RECORD set prop fail test case
From: Johan Hedberg @ 2013-12-18 8:00 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387283874-29721-7-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> +static bt_service_record_t setprop_remote_service = {
> + .uuid = { {0x00} },
> + .channel = 12,
> + .name = "bt_name"
> +};
> +
> +static const struct generic_data
> + bluetooth_setprop_service_record_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
> + .expected_adapter_status = BT_STATUS_FAIL,
> + .expected_property.type = BT_PROPERTY_SERVICE_RECORD,
> + .expected_property.val = &setprop_remote_service,
> + .expected_property.len = sizeof(bt_service_record_t)
sizeof(setprop_remote_service);
Johan
^ permalink raw reply
* Re: [PATCH 6/7] android/tester: Add RSSI set prop fail test case
From: Johan Hedberg @ 2013-12-18 8:00 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387283874-29721-6-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> +static int32_t setprop_remote_rssi = 0;
> +
> +static const struct generic_data bluetooth_setprop_remote_rssi_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
> + .expected_adapter_status = BT_STATUS_FAIL,
> + .expected_property.type = BT_PROPERTY_REMOTE_RSSI,
> + .expected_property.val = &setprop_remote_rssi,
> + .expected_property.len = sizeof(int32_t)
sizeof(setprop_remote_rssi);
Johan
^ permalink raw reply
* Re: [PATCH 5/7] android/tester: Add TOD set prop fail test case
From: Johan Hedberg @ 2013-12-18 8:00 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387283874-29721-5-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> This adds TYPE_OF_DEVICE set property fail test case due to only
> get possibility.
> ---
> android/android-tester.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index a6f66c2..4923a6c 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -618,6 +618,16 @@ static const struct generic_data bluetooth_setprop_cod_invalid_test = {
> .expected_property.len = sizeof(uint32_t)
> };
>
> +static bt_device_type_t setprop_type_of_device = BT_DEVICE_DEVTYPE_BREDR;
> +
> +static const struct generic_data bluetooth_setprop_tod_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
> + .expected_adapter_status = BT_STATUS_FAIL,
> + .expected_property.type = BT_PROPERTY_TYPE_OF_DEVICE,
> + .expected_property.val = &setprop_type_of_device,
> + .expected_property.len = sizeof(bt_device_type_t)
sizeof(setprop_type_of_device)
Johan
^ permalink raw reply
* Re: [PATCH 4/7] android/tester: Add COD set prop fail test case
From: Johan Hedberg @ 2013-12-18 7:59 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387283874-29721-4-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> This adds CLASS_OF_DEVICE set property fail test case due to only
> get possibility.
> ---
> android/android-tester.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index 4fe0033..a6f66c2 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -608,6 +608,16 @@ static const struct generic_data bluetooth_setprop_uuid_invalid_test = {
> .expected_property.len = 17
> };
>
> +static uint32_t setprop_class_of_device = 0;
> +
> +static const struct generic_data bluetooth_setprop_cod_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
> + .expected_adapter_status = BT_STATUS_FAIL,
> + .expected_property.type = BT_PROPERTY_CLASS_OF_DEVICE,
> + .expected_property.val = &setprop_class_of_device,
> + .expected_property.len = sizeof(uint32_t)
sizeof(setprop_class_of_device) makes more sense than uint32_t.
Johan
^ permalink raw reply
* Re: [PATCH 3/7] android/tester: Add UUIDS set prop fail test case
From: Johan Hedberg @ 2013-12-18 7:58 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
In-Reply-To: <1387283874-29721-3-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Dec 17, 2013, Grzegorz Kolodziejczyk wrote:
> This adds UUIDS set property fail test case due to only get
> possibility.
> ---
> android/android-tester.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
The first two patches have been applied, but a minor thing with this
one:
> +static unsigned char setprop_uuids[] = { 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00,
> + 0x00, 0x80, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00 };
> +
> +static const struct generic_data bluetooth_setprop_uuid_invalid_test = {
> + .expected_hal_callbacks = {ADAPTER_TEST_END},
> + .expected_adapter_status = BT_STATUS_FAIL,
> + .expected_property.type = BT_PROPERTY_UUIDS,
> + .expected_property.val = &setprop_uuids,
> + .expected_property.len = 17
How about sizeof(setprop_uuids) here instead of 17?
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox