968624 - B2G emulator: support bluetooth pairing with virtual BT device

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

To provide automated testing of BT API, we need to support bluetooth pairing with virtual BT device.

Android use QEMU to create a virtual ARM SoC called Goldfish.
Goldfish executes ARM instructions and emulate ARM SoC, however, it's lack of some abilities of bluetooth chip.

Implement the paring procedure of controller side to support bluetooth pairing on emulator.
(Since QEMU support the transport Layer of BT, we plan to use existing HCI to communicate with BT stack.)

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (draft) Support bluetooth pairing with virtual BT device on emulator ICS.

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (draft) Support bluetooth pairing with virtual BT device on emulator ICS.

Comment 3

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Support bluetooth pairing with virtual BT device on emulator ICS. (v0)

Comment 4

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8419222 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS.  (v0)

Review of attachment 8419222 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jamin,

Thanks for your hard work. Overall looks good, but you may need to respond with Command Complete instead of Command Status at those places I mentioned.

::: hw/bt-hci.c
@@ +1658,5 @@
>  
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY):
> +        LENGTH_CHECK(link_key_neg_reply);
> +
> +        bt_hci_event_status(hci, HCI_SUCCESS);

bt_hci_event_status should be incorrect. According to Bluetooth spec, an "Command Complete" event has to be replied for a link key negative reply. So I think you should use bt_hci_event_complete instead.

@@ +1666,5 @@
> +
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_IO_CAPABILITY_REQ_REPLY):
> +        LENGTH_CHECK(io_capability_req_reply);
> +
> +        bt_hci_event_status(hci, HCI_SUCCESS);

Ditto. A "Command Complete" event should be replied here.

@@ +1667,5 @@
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_IO_CAPABILITY_REQ_REPLY):
> +        LENGTH_CHECK(io_capability_req_reply);
> +
> +        bt_hci_event_status(hci, HCI_SUCCESS);
> +        bt_hci_response_io_capability(hci,

Please add comments right before this line to document that "the remote io capability exchangement has been done".

@@ +1679,5 @@
> +
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_USER_CONFIRMATION_REQ_REPLY):
> +        LENGTH_CHECK(user_confirmation_req_reply);
> +
> +        bt_hci_event_status(hci, HCI_SUCCESS);

Ditto.

@@ +1691,5 @@
> +                !bacmp(&hci->lm.handle[i].link->slave->bd_addr,
> +                    &PARAM(user_confirmation_req_reply, bdaddr)))
> +            {
> +                bt_hci_event_auth_complete(hci,
> +                        hci->lm.handle[i].link->handle);

nit: weird alignment

@@ +1824,5 @@
>  
> +    case cmd_opcode_pack(OGF_HOST_CTL, OCF_DELETE_STORED_LINK_KEY):
> +        LENGTH_CHECK(delete_stored_link_key);
> +
> +        bt_hci_event_status(hci, HCI_SUCCESS);

Ditto.

@@ +1838,5 @@
>          break;
>  
>      case cmd_opcode_pack(OGF_HOST_CTL, OCF_RESET):
>          bt_hci_reset(hci);
> +        bt_hci_event_complete_status(hci, HCI_SUCCESS);

Good catch.

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Support bluetooth pairing with virtual BT device on emulator ICS. (v1)

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8423071 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS. (v1)

Review of attachment 8423071 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a wrong patch.

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Support bluetooth pairing with virtual BT device on emulator ICS. (v1)

I put a wrong patch by accident last night.
Replace the wrong patch by this one.
Sorry for any inconvenience caused.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

I have a question on the first sight of the patch.  Does this mean we're able to pair with physical device with bug 972274?  I know you're probably not testing on it.  Just a question in my mind because Bluetooth has been included in qemu for a long time, so I'd hope we don't break its original functions in a way that can't be corrected some day in the future.

Comment 9

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8423538 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS. (v1)

Review of attachment 8423538 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jamin, please see my comments below.

::: hw/bt-hci.c
@@ +1653,2 @@
>              bt_hci_event_status(hci, HCI_SUCCESS);
> +            bt_hci_event_auth_complete(hci, auth_req_handle);

Authentication Complete should be fired once link key has been notified to the host. Firing the event here must not be right. Please remove it.

@@ +1658,5 @@
>  
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY):
> +        LENGTH_CHECK(link_key_neg_reply);
> +
> +        bt_hci_event_complete_status(hci, HCI_SUCCESS);

You should use bt_hci_event_complete() instead of bt_hci_event_complete_status() because an extra parameter must be attached (BD_ADDR). See 7.1.11 in Bluetooth 3.0 HCI spec.

@@ +1666,5 @@
> +
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_IO_CAPABILITY_REQ_REPLY):
> +        LENGTH_CHECK(io_capability_req_reply);
> +
> +        bt_hci_event_complete_status(hci, HCI_SUCCESS);

You should use  bt_hci_event_complete() instead of bt_hci_event_complete_status() because an extra parameter must be attached (BD_ADDR). See 7.1.29 in Bluetooth 3.0 HCI spec.

@@ +1672,5 @@
> +        bt_hci_response_io_capability(hci,
> +                        &PARAM(io_capability_req_reply, bdaddr),
> +                        PARAM(io_capability_req_reply, capability),
> +                        PARAM(io_capability_req_reply, oob_data),
> +                        PARAM(io_capability_req_reply, authentication));

Here we assume that the remote device has the same io capability as local. Fine for now, but we may want to cover more cases in the future.

@@ +1680,5 @@
> +
> +    case cmd_opcode_pack(OGF_LINK_CTL, OCF_USER_CONFIRMATION_REQ_REPLY):
> +        LENGTH_CHECK(user_confirmation_req_reply);
> +
> +        bt_hci_event_complete_status(hci, HCI_SUCCESS);

You should use bt_hci_event_complete() instead of bt_hci_event_complete_status() because an extra parameter must be attached (BD_ADDR). See 7.1.30 in Bluetooth 3.0 HCI spec.

@@ +1824,5 @@
>  
> +    case cmd_opcode_pack(OGF_HOST_CTL, OCF_DELETE_STORED_LINK_KEY):
> +        LENGTH_CHECK(delete_stored_link_key);
> +
> +        bt_hci_event_complete_status(hci, HCI_SUCCESS);

No need to reply a Command Complete event here, not to mention the return parameters are wrong -- you miss a "Num_Keys_Deleted" paramter. The correct one would be fired in bt_hci_request_delete_link_key, so please remove this line.

@@ +2378,5 @@
> +
> +    bt_hci_event(hci, EVT_LINK_KEY_NOTIFY, &params, EVT_LINK_KEY_NOTIFY_SIZE);
> +}
> +
> +void bt_hci_bdaddr_tostr(const bdaddr_t *addr, char *addr_str)

There is already a function ba_to_str() defined in bt.c. Can we use that?

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Support bluetooth pairing with virtual BT device on emulator ICS. (v2)

Thank Eric for his great reviews.
A few changes has been made.

Comment 11

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #8)
> I have a question on the first sight of the patch.  Does this mean we're
> able to pair with physical device with bug 972274?  I know you're probably
> not testing on it.  Just a question in my mind because Bluetooth has been
> included in qemu for a long time, so I'd hope we don't break its original
> functions in a way that can't be corrected some day in the future.

That's a great question.
It would be nice if we can use a host BT dongle with emulator build someday.
This bug wouldn't help to achieve the goal of Bug 972274, however, I believe it wouldn't cause serious problems when Bug 972274 is ready neither.

I try to avoid to change the existing functions and the basic HCI flow in QEMU.
The main changes in #8426945 are extending the HCI commands which aren't supported yet.
There're only three existing functions are modified.

1. Don't reply 'authentication complete' directly when controller receives OCF_AUTH_REQUESTED.
   QEMU did this way since it hasn't support the link key exchange.

   Please refer to comment #9 which Eric posted.
   > Authentication Complete should be fired once link key has been notified to the host. 

2. Change the reply function to "Command Complete" event for the command OCF_RESET.
   Please refer to comment #4 which Eric posted.

3. Disable the emulation of attribute list of SDP service record by adding a preprocessor flag.
   Consider that the BT connecting are not supported yet, the sdp_service_record_build() might cause stability issues. 
   Since it's easy to revert the preprocessor flag when the SDP is ready, I think it wouldn't be a problem to Bug 972274.

Comment 12

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8426945 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS. (v2)

Review of attachment 8426945 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks. :)

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (final) Support bluetooth pairing with virtual BT device on emulator ICS. (v2), r=echou

Verified by running marionette test in Bug 968709.

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Pull request for qemu] Support bluetooth pairing with virtual BT device on emulator ICS. (v2), r=echou

Comment 15

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/platform_external_qemu/commit/5688c04e38ceacb9069b6157db69b37beafab331