1227907 - [bluedroid] Support PAN feature

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Tom Tung [:tt, :ttung]]

Support PAN profile for bluedroid

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Probably blocks bug 972780.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: 0001-Bug-1227907-v1-bluetoothd_change.patch

Support PAN feature on bluetoothd

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change

It looks like we need to require the root permission (modify the "init.bluetooth.rc") to build special device nodes(e.g. dev/tun, socket, ..., etc.) for supporting PAN profile. I will file another bug to discussing whether we need to require the root permission on bluetoothd.

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung from comment #3)
> Created attachment 8692461 [details] [review]
> Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change
> 
> It looks like we need to require the root permission (modify the
> "init.bluetooth.rc") to build special device nodes(e.g. dev/tun, socket,
> ..., etc.) for supporting PAN profile. I will file another bug to discussing
> whether we need to require the root permission on bluetoothd.

Bug 1228283

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8692461 [details] [review]
Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change

Thanks! Having PAN support is pretty cool.

Just r- until the comments have been addresses, but most of it is trivial.

I'd prefer to not land this patch until there's a patch for the Gecko side and some example code that makes use of PAN.

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Comment on attachment 8692461 [details] [review]
> Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change
> 
> Thanks! Having PAN support is pretty cool.
> 
> Just r- until the comments have been addresses, but most of it is trivial.
> 
> I'd prefer to not land this patch until there's a patch for the Gecko side
> and some example code that makes use of PAN.

Hi Thomas,

Thank you for your reviews and comments! I revise my code and update it to [1]. 

Actually I have done part of basic functions(connect/disconnect) of PAN profile for the Gecko side and uploaded them as a patch to Bug 972780. Would you like to review it?

[1] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/40

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Tom

> Actually I have done part of basic functions(connect/disconnect) of PAN
> profile for the Gecko side and uploaded them as a patch to Bug 972780. Would
> you like to review it?

Ah, I see. WebIDL is not my domain, but I can review the IPC protocol code from that patch.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> [1] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/40

I left more comments in the pull request.

Comment 9

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7) 
> Ah, I see. WebIDL is not my domain, but I can review the IPC protocol code
> from that patch.

Hi Thomas,

Thanks! I will split my patch into two patches. One of them will be daemon part and I will ask you for review.

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> > [1] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/40
> 
> I left more comments in the pull request.

Thanks! I revise my code and update it again.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Tom Tung from comment #10)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> > > [1] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/40
> > 
> > I left more comments in the pull request.
> 
> Thanks! I revise my code and update it again.

I left some comments.

Comment 12

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> > > > [1] https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/40
> 
> I left some comments.

Thanks you! I revise them and update it to [1].

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8692461 [details] [review]
Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change

A few more nits, but it looks good now. Thanks!

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> Comment on attachment 8692461 [details] [review]
> Bug 1227907 - Support PAN feature on bluetoothd - bluetoothd change
> 
> A few more nits, but it looks good now. Thanks!

Hi Thomas,

Thank you!! But I changed a little part of code, such as (1)PAN's order in |register_func| and |unregister_service| at "src/service.c", (2)Error handling for exceeding interface name bytes in |append_bt_ifname| at "src/bt-pan-io.c", and (3) add "static" for |append_bt_ifname|. I don't change the declaration method for IFNAME_LEN to |static const|, and the reason is that C's compiler don't treats |static const|'s variable as constant. Thus, if I do so, the |padding[IFNAME_LEN]| will cause error. 

I just have a little afraid whether there is anything wrong. Can you review it again?

Tom

Comment 15

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/c7316de760bb69412c7792f1ebb9a0ee863cb4c2

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted in https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/570531db9e81a871c421b27025ae558022b1015a due to bug 1230589.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Tom,

please test the patch on JB, KK and L. The platform APIs aren't overly stable and this kind of breakage is common. I asked you to not land this patch until you have the Gecko side ready. Please don't re-land before all patches in bug 972780 have an r+.

Comment 18

[Tom Tung [:tt, :ttung]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
Thomas,

OK, right now I only test the patch on KK. I will test the patch on other environment. 
Sorry for that. I just thought you changed the "r-" to "r+" means I can land this patch, and I was wrong. I will wait until all the patches bug 972780 have an r+.

Tom

Comment 19

[Tom Tung [:tt, :ttung]]

(In reply to Wes Kocher (:KWierso) from comment #16)

Thanks, really sorry about that.

Comment 20

[Tom Tung [:tt, :ttung]]

Reset the assignee to default since I'm not working on this bug anymore.

Comment 21

[BMO Automation]

Firefox OS is not being worked on