If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

B2G emulator: support bluetooth pairing with virtual BT device

RESOLVED FIXED in 2.0 S3 (6june)

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jaliu, Assigned: jaliu)

Tracking

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(feature-b2g:2.0)

Details

(Whiteboard: [p=2])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

4 years ago
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.)
(Assignee)

Updated

4 years ago
Assignee: nobody → jaliu
Depends on: 860696
(Assignee)

Updated

4 years ago
Blocks: 860695
(Assignee)

Updated

4 years ago
Blocks: 968709
(Assignee)

Updated

4 years ago
Target Milestone: --- → 1.4 S6 (25apr)
(Assignee)

Comment 1

4 years ago
Created attachment 8408939 [details] [diff] [review]
(draft) Support bluetooth pairing with virtual BT device on emulator ICS.
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8410226 [details] [diff] [review]
(draft) Support bluetooth pairing with virtual BT device on emulator ICS.
Attachment #8408939 - Attachment is obsolete: true
Attachment #8410226 - Flags: feedback?(echou)

Updated

3 years ago
Whiteboard: [p=2]
Target Milestone: 1.4 S6 (25apr) → 2.0 S2 (23may)
(Assignee)

Comment 3

3 years ago
Created attachment 8419222 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS.  (v0)
Attachment #8410226 - Attachment is obsolete: true
Attachment #8410226 - Flags: feedback?(echou)
Attachment #8419222 - Flags: review?(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.
Attachment #8419222 - Flags: review?(echou) → review-

Updated

3 years ago
feature-b2g: --- → 2.0
(Assignee)

Comment 5

3 years ago
Created attachment 8423071 [details] [diff] [review]
Support bluetooth pairing with virtual BT device on emulator ICS. (v1)
Attachment #8419222 - Attachment is obsolete: true
Attachment #8423071 - Flags: review?(echou)
Attachment #8423071 - Flags: feedback?(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.
Attachment #8423071 - Flags: feedback?(vyang)
(Assignee)

Comment 7

3 years ago
Created attachment 8423538 [details] [diff] [review]
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.
Attachment #8423071 - Attachment is obsolete: true
Attachment #8423071 - Flags: review?(echou)
Attachment #8423538 - Flags: review?(echou)
Attachment #8423538 - Flags: feedback?(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 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?
Attachment #8423538 - Flags: review?(echou) → review-
(Assignee)

Comment 10

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

Thank Eric for his great reviews.
A few changes has been made.
Attachment #8423538 - Attachment is obsolete: true
Attachment #8423538 - Flags: feedback?(vyang)
Attachment #8426945 - Flags: review?(echou)
Attachment #8426945 - Flags: feedback?(vyang)
(Assignee)

Comment 11

3 years ago
(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 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. :)
Attachment #8426945 - Flags: review?(echou) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8427539 [details] [diff] [review]
(final) Support bluetooth pairing with virtual BT device on emulator ICS. (v2), r=echou

Verified by running marionette test in Bug 968709.
Attachment #8426945 - Attachment is obsolete: true
Attachment #8426945 - Flags: feedback?(vyang)
Attachment #8427539 - Flags: feedback?(vyang)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

3 years ago
Created attachment 8429144 [details]
[Pull request for qemu] Support bluetooth pairing with virtual BT device on emulator ICS. (v2), r=echou
Attachment #8427539 - Attachment is obsolete: true
Attachment #8427539 - Flags: feedback?(vyang)
master: https://github.com/mozilla-b2g/platform_external_qemu/commit/5688c04e38ceacb9069b6157db69b37beafab331
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

3 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.