This bug covers Hid support in bluetooth daemon.

Hi Thomas, The patch implemented HID connection/disconnection part in Bluetooth Daemon. Would you like to review it? Thanks Louis

Comment on attachment 8702237 [details] Bug 1223732 - Support HID in Bluetooth Daemon, r=tzimmermann Hi The patch set is incomplete. I left comments for the code that is already there.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3) Hi Thomas, I was supposed to separate the HID support into two patches, but now i merge them into one. I had fixed the problems you mentioned on the comments. Please review again thanks. By the way, There are some questions about hid: 1)There is no bthh_handshake_callback() function declare in our hardware/bt_hh.h, so i am not sure how to define the function inside bt_hh_io.c. 2)The set_idle_time(), get_idle_time() and set_priority() opcode_cmd functions are not listed on the http://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt bluez spec, so i am not sure if i should define them in bt_hh_io.c. 3)The size of some command parameters such as $vendor_id, $product_id, etc are 2 bytes on the bluez spec, but in our daemon we use 'int' which is 4 bytes. Thanks Louis

Hi > 1)There is no bthh_handshake_callback() function declare in our > hardware/bt_hh.h, so i am not sure how to define the function inside > bt_hh_io.c. It is only present in L or later. You need one of these devices to implement and test. > 2)The set_idle_time(), get_idle_time() and set_priority() opcode_cmd > functions are not listed on the > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt > bluez spec, so i am not sure > if i should define them in bt_hh_io.c. Oh, they don't seem to support this in BlueZ. You should ask on the BlueZ mailing list [1] why these opcodes are missing. Sometimes they forget about functions; but it could also be a design decision. If the opcodes were forgotten, we provided a patch to BlueZ. Will Wang can help you with this, he wrote a BlueZ patch for one of the missing opcodes. > 3)The size of some command parameters such as $vendor_id, $product_id, etc > are 2 bytes on the bluez spec, but in our daemon we use 'int' which is 4 > bytes. You mean that |struct bthh_hid_info_t| uses int? In such cases, we transfer values as stated in the protocol spec and copy them into the right locations in bluetoothd. [1] http://www.bluez.org/development/lists/

Hi Thomas, > It is only present in L or later. You need one of these devices to implement > and test. Ok! I got it. > Oh, they don't seem to support this in BlueZ. You should ask on the BlueZ > mailing list [1] why these opcodes are missing. Sometimes they forget about > functions; but it could also be a design decision. If the opcodes were > forgotten, we provided a patch to BlueZ. Will Wang can help you with this, > he wrote a BlueZ patch for one of the missing opcodes. Sure, i will ask for help and send a mailing list to Bluez. > You mean that |struct bthh_hid_info_t| uses int? In such cases, we transfer > values as stated in the protocol spec and copy them into the right locations > in bluetoothd. You mean like $vendor_id (int type in |struct bthh_hid_info_t|), i have to use function read_pdu_at() with switch parameter 'S' (uint16_t type as definition in bluez spec) to transfer its type and read the pdu inside function opcode_set_info(). Also, in hid_info_cb(), use function append_to_pdu() with switch parameter 'S' to transfer its type and append to pdu? And sorry, i forgot to reset the flag.

Hi Yes, read the PDU and fill the structure with the values. I suggest to have a read function for |struct bthh_hid_info_t|. There are plenty of examples in the source code of bluetoothd. I'll wait for more information from BlueZ before reviewing the patches.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7) > Yes, read the PDU and fill the structure with the values. I suggest to have > a read function for |struct bthh_hid_info_t|. There are plenty of examples > in the source code of bluetoothd. > > I'll wait for more information from BlueZ before reviewing the patches. Ok, i will notify you asap once the BlueZ reply. Thanks

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5) > Hi > > > 1)There is no bthh_handshake_callback() function declare in our > > hardware/bt_hh.h, so i am not sure how to define the function inside > > bt_hh_io.c. > > It is only present in L or later. You need one of these devices to implement > and test. > > > > 2)The set_idle_time(), get_idle_time() and set_priority() opcode_cmd > > functions are not listed on the > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt > > bluez spec, so i am not sure > > if i should define them in bt_hh_io.c. > > Oh, they don't seem to support this in BlueZ. You should ask on the BlueZ > mailing list [1] why these opcodes are missing. Sometimes they forget about > functions; but it could also be a design decision. If the opcodes were > forgotten, we provided a patch to BlueZ. Will Wang can help you with this, > he wrote a BlueZ patch for one of the missing opcodes. > CAF added these functions, so again it's not standard AOSP BT HAL.

Hi Thomas, After having discussion w/ Shawn, we think that we don't need to implement commands |set_idle_time()|, |get_idle_time()| and |set_priority()| due to the reasons that 1) CAF added these functions and they are not standard AOSP BT HAL. 2) |set_idle_time()| and |get_idle_time()| are deprecated in the HID Profile spec v1.1 and |set_prioriity()| is not listed in HID Profile spec v1.1. I have submitted a new patch and it covers 1) remove |set_idle_time()|, |get_idle_time()| and |set_priority()| 2) add the read and write function for |struct bthh_hid_info_t| 3) support the |handsahke_cb()| with Android version >= 21 Thanks

Thanks for the follow-up. I'll review the patch set ASAP.

Comment on attachment 8702237 [details] Bug 1223732 - Support HID in Bluetooth Daemon, r=tzimmermann Thanks for the update. Reviewed with comments on Github.

Hi Thomas I have fixed the problems you mentioned in the comments. Please review again, thanks. i will handle hid_info->priority in gecko and just simply set it to the default value 100 which is PRIORITY_ON [1]. Here in daemon i just read & write to suite the format of struct |bthh_hid_info_t|. [1]https://github.com/mozilla-b2g/platform_frameworks_base/blob/master/core/java/android/bluetooth/BluetoothProfile.java Thanks

> i will handle hid_info->priority in gecko and just simply set it to the > default value 100 which is PRIORITY_ON [1]. Here in daemon i just read & > write to suite the format of struct |bthh_hid_info_t|. You won't need this field in Gecko, since you can't do anything with it anyway.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14) > You won't need this field in Gecko, since you can't do anything with it > anyway. But it is declared in the /hardware/bt_hh.h by Qualcomm. So should we just ignored it when reading or writing pdu?

Comment on attachment 8702237 [details] Bug 1223732 - Support HID in Bluetooth Daemon, r=tzimmermann From the pull request: + long off = read_pdu_at(pdu, offset, "SCCSSSCSS", + &attr_mask, &sub_class, &app_id, &vendor_id, &product_id, + &version, &ctry_code, &priority, &dl_len); 'Priority' is not part of the protocol, so it shouldn't show up here. Please respect the protocol spec. [1] And please align the arguments as illustrated in [2]. Here and in similar places. [1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt#n521 [2] https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/master/src/bt-gatt-io.c#L209

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16) > Comment on attachment 8702237 [details] > Bug 1223732 - Support HID in Bluetooth Daemon > > From the pull request: > > + long off = read_pdu_at(pdu, offset, "SCCSSSCSS", > + &attr_mask, &sub_class, &app_id, &vendor_id, &product_id, > + &version, &ctry_code, &priority, &dl_len); > > 'Priority' is not part of the protocol, so it shouldn't show up here. Please > respect the protocol spec. [1] And please align the arguments as illustrated > in [2]. Here and in similar places. > > [1] > https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api. > txt#n521 > [2] > https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/master/src/bt- > gatt-io.c#L209 I have fixed the hid_info->priority and the alignment issues. Thanks

> > I have fixed the hid_info->priority and the alignment issues. Was that a review request BTW?

> > > > I have fixed the hid_info->priority and the alignment issues. > > Was that a review request BTW? Sorry about that. Yes, please review again thanks!

Comment on attachment 8702237 [details] Bug 1223732 - Support HID in Bluetooth Daemon, r=tzimmermann It's getting there. With fixes to the remaining points, it should be fine.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20) > Comment on attachment 8702237 [details] > Bug 1223732 - Support HID in Bluetooth Daemon > > It's getting there. With fixes to the remaining points, it should be fine. Ok, i have fixed the problems mentioned in the comments. Please review again, thanks!

Comment on attachment 8702237 [details] Bug 1223732 - Support HID in Bluetooth Daemon, r=tzimmermann I still left a few comments, but it should be good with these fixed. Pleae, build this code on different versions of Android. Don't land unless you have the Gecko side ready.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #22) > Comment on attachment 8702237 [details] > Bug 1223732 - Support HID in Bluetooth Daemon > > I still left a few comments, but it should be good with these fixed. Pleae, > build this code on different versions of Android. Don't land unless you have > the Gecko side ready. Ok, thanks. I have fixed the remaining problems. And i will test it on other Android.