Page MenuHomeFreeBSD

Bluetooth 4.0 Support
ClosedPublic

Authored by takawata on Mar 7 2015, 1:49 PM.

Details

Summary

Adding Bluetooth 4.0 LE + ATT protocol Support.
This supports only master side; device side is not supported yet.

Test Plan

You may want to prepare some Bluetooth LE supported sensor(like pedomator)
And use test program that sent the other day.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

takawata updated this revision to Diff 4133.Mar 7 2015, 1:49 PM
takawata retitled this revision from to Bluetooth 4.0 Support.
takawata updated this object.
takawata edited the test plan for this revision. (Show Details)
takawata added a reviewer: rpaulo.
emax added a subscriber: emax.Mar 9 2015, 5:17 PM
emax removed a subscriber: emax.
emax added inline comments.Mar 9 2015, 7:41 PM
sys/netgraph/bluetooth/hci/ng_hci_evnt.c
272 ↗(On Diff #4133)

i'm a little bit confused here. can you please explain why reallink_type is needed here?

426 ↗(On Diff #4133)

since bdaddr, addrtype, page scan and clock offset do not seem to change, perhaps move those inside the block for new entry creation?

sys/netgraph/bluetooth/hci/ng_hci_ulpi.c
105 ↗(On Diff #4133)

since link_type is known here (as it comes from the message) would it make more sense to convert if-else into switch(link_type) { } here?

sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

perhaps better name, i.e. sockaddr_l2cap_le?

sys/netgraph/bluetooth/include/ng_l2cap.h
356 ↗(On Diff #4133)

is adding idtype filed here backward compatible with older Bluetooth versions?

sys/netgraph/bluetooth/l2cap/ng_l2cap_cmds.c
225 ↗(On Diff #4133)

no break; here? is this intended? as it is right now, the code will panic

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
2164 ↗(On Diff #4133)

remove printf() ?

hrs added a subscriber: hrs.Mar 10 2015, 7:51 AM
takawata added inline comments.Mar 10 2015, 8:58 AM
sys/netgraph/bluetooth/hci/ng_hci_evnt.c
272 ↗(On Diff #4133)

'send_data_packets' function is invoked when either ACL link or SCO link is available and the argument is determined by the available link.
Here, I have to compare link value with real direction.

426 ↗(On Diff #4133)

No objection.

sys/netgraph/bluetooth/hci/ng_hci_ulpi.c
105 ↗(On Diff #4133)

No objection.

sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

Should I keep this compatibility shim permanently?

sys/netgraph/bluetooth/include/ng_l2cap.h
356 ↗(On Diff #4133)

I don't consider in-kernel compatibility.

sys/netgraph/bluetooth/l2cap/ng_l2cap_cmds.c
225 ↗(On Diff #4133)

For now, I don't support device side. And this is device side request.
"
4.20 CONNECTION PARAMETER UPDATE REQUEST (CODE
0X12)
This command shall only be sent from the LE slave device to the LE master
device."

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
2164 ↗(On Diff #4133)

Yes.... I forget to remove it.

Implementation note.

Data type concepts

HCI LINK TYPE

To handle Bluetooth LE traffic, I add two link type:
NG_HCI_LINK_LE_PUBLIC
NG_HCI_LINK_LE_RANDOM

This is determined by remote address type. If address type is
BR/EDR, link type is NG_HCI_LINK_ACL. Other than NG_HCI_LINK_SCO packet,
all traffic is go through bulk USB endpoint from ubt driver

IDType
IDType defines channel identifier space.
Channel identifier is unique in same ID type.
There are 3 ID types:
NG_L2CAP_L2CA_IDTYPE_BREDR : Existing type.
NG_L2CAP_L2CA_IDTYPE_ATT : ATT protocol id type. Actually connection handle.
NG_L2CAP_L2CA_IDTYPE_LE : Not supported yet , it is supported in Bluetooth 4.2 LE channel.
BREDR type and LE has separated identifier space because the ID range is differe
nt.
ATT CID is fixed to 4, reserved range in recent Bluetooth Spec.

sockaddr_l2cap_new
This interface is inspired by Linux bluetooth stack.
Either l2cap_cid member or l2cap_psm member should be zero and
either should not be zero.
Only valid cid value is 4 in this implementation.(ATT) And,
Both LE and ATT channel other than ATT use psm to create a channel.
Internally sockaddr_l2cap is translated into this structure.

Addresstype
It is used in User API.
There are three value BDADDR_BREDR, BDADDR_LE_PUBLIC, BDADDR_LE_RANDOM.
It is used only in socket layer and convert into link type.

MBUF flags
ATT protocol does not require L2CAP layer connection request.
So, when connecting, inject L2CAP reply packet to send queue with
M_PROTO2 flag, and if the flag is set, the send queue dispatcher
will reflect the packet to receive queue.

Per file change

ng_hci_cmds.c:

Add LE-related command paramater processing routine.

ng_hci_evnt.c:

*Add LE-related event processing routine.
*Inject LE scan result into Advertising report list.
*Add LE dedicated connection handle routine.

ng_hci_misc.c

*Add LINK Family member in ng_hci_neighbor_p structure.

ng_hci_ulpi.c

*Add LE connection request handling.

ng_btsocket_l2cap.h

*Add address type in raw and dgram PCB.

ng_hci.h

*Add LE related HCI command and event definitions .

ng_l2cap.h

*Add CID definitions.
*Add LE L2CAP commands.

ng_l2cap_var.h

*Add LE cid management data in ng_l2cap_t

ng_btsocket_l2cap.c

  • compare pcb by idtype and cid.
  • Add linktype parameter in connection command.
emax edited edge metadata.Mar 11 2015, 6:49 PM

more inline comments

sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

i don't think so.

i wonder if you can use l2cap_len here. basically, cast everything to sockaddr_l2cap (old version) to begin with. then check l2cap_len and compare it to sizeof(sockaddr_l2cap_new). if size matches, re-cast to sockaddr_l2cap_new.

i think it should be possible to rename sockaddr_l2cap to sockaddr_l2cap_old and have new fields in sockaddr_l2cap by default.

this way, applications complied with old sockaddr_l2cap will use sockaddr_l2cap_old and still work.

so, i think, it would be better to add new fields to sockaddr_l2cap and have the rest of the code use it. compatibility shim probably should only exists in l2cap socket layer.

sys/netgraph/bluetooth/include/ng_l2cap.h
645 ↗(On Diff #4133)

struct name and type name are the same?

sys/netgraph/bluetooth/l2cap/ng_l2cap_cmds.c
225 ↗(On Diff #4133)

not supporting features is ok. what i was trying to confirm is that panic() in this case is intentional. if its not, then perhaps adding a break; here (with an optional error message) might be a good idea. again, this is to protect against the case where, somehow, this command ends up on the l2cap connection queue.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4133)

can you please explain why this is needed? is there no indication for received responses here?

sys/netgraph/bluetooth/l2cap/ng_l2cap_misc.c
87 ↗(On Diff #4133)

can you please explain why ng_l2cap_node_hook_info_ep is needed? as far as i can see, ng_l2cap_node_hook_info_ep has only one member, i.e. bdaddr_t

sys/netgraph/bluetooth/l2cap/ng_l2cap_ulpi.c
132 ↗(On Diff #4133)

i'm confused here. this is request path, but _ng_l2cap_con_rsp (response) is used. can you please explain if this something from the spec ?

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
1621 ↗(On Diff #4133)

since ng_l2cap_node_hook_info_ep was added, would it not be more correct to check for sizeof(ng_l2cap_node_hook_info_ep) instead of sizeof(bdaddr_t)?

takawata added inline comments.Mar 13 2015, 7:53 AM
sys/netgraph/bluetooth/l2cap/ng_l2cap_cmds.c
225 ↗(On Diff #4133)

It is intended, for now. At least it may have to be warned, I think.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4133)

It simulates L2CAP connection complete when HCI connection established.
When it established, this routine is called and start processing packet.
Then, the flagged Connection *Response* packet is reflected to receive queue and L2CAP layer tranfer state machine to another state.

sys/netgraph/bluetooth/l2cap/ng_l2cap_misc.c
87 ↗(On Diff #4133)

Need to add Address type later, Especially required to support random address.

sys/netgraph/bluetooth/l2cap/ng_l2cap_ulpi.c
132 ↗(On Diff #4133)

See above.

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
1621 ↗(On Diff #4133)

Yes.

takawata added inline comments.Mar 13 2015, 7:54 AM
sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

Renaming our new structure to sockaddr_l2cap would be good.
But we should note it breaks source compatibility, because uninitialized member may be passed to kernel.

emaste added a subscriber: emaste.Mar 15 2015, 7:55 AM
emax added inline comments.Mar 17 2015, 5:28 PM
sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

i'm sorry, but i do not follow. there is l2cap_len field. any binary application would (ot at least should) have it set to size of the old sockaddr_l2cap. what i'm suggesting is to check l2cap_len first and cast pointer appropriately. we can rename structure safely then.

sys/netgraph/bluetooth/l2cap/ng_l2cap_cmds.c
225 ↗(On Diff #4133)

agreed. warning is absolutely fine. panic() is a bit too drastic for me.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4133)

i'm sorry, but i'm completely lost here. ng_l2ap_lp_deliver() sends data to the HCI layer. ng_l2cap_lp_receive() receives data from the HCI layer. so, if i'm reading this right, the code tries to loopback some special packet marked with M_PROTO2 flag. instead of doing that, would it be possible to adjust state in upper layer and not send effectively dummy command down to l2cap layer?

sys/netgraph/bluetooth/l2cap/ng_l2cap_ulpi.c
132 ↗(On Diff #4133)

yes. i'm still confused, sorry. is there no other way? is it no possible to adjust state here?

takawata updated this revision to Diff 4270.Mar 18 2015, 3:20 PM
takawata edited edge metadata.

Fix stall bug.
Reflect review comments.

takawata added inline comments.Mar 18 2015, 5:20 PM
sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4133)

If a programmer use new structure as old structure,
struct size is changed as new structure(because he use sizeof(struct sockaddr_l2cap) to set the value), but s/he may still not initialize some member of new sockaddr_l2cap structure. So source compatibility may be broken.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4133)

I want to know when the packet processing start here, so state transition at caller is not desirable. And I used mbuf flag because I don't want to peek packet content here.

emax added a comment.Mar 25 2015, 3:54 PM

it looks mostly good to me. a few minor nit picks, and, two somewhat bigger issues

  1. the use of M_PROTO2 flag. can it be avoided? i'm mostly fine with it, just wanted to get a bit more explanation.
  1. adding new fields to sockaddr_l2cap (linux way) vs having l2cap socket option to set them. if we can guarantee that bzero()ing sockaddr_l2cap (with new fields) is enough to keep old code working (after recompile), then, i'm fine with it.
sys/netgraph/bluetooth/hci/ng_hci_evnt.c
421 ↗(On Diff #4270)

as we have discussed, please move those two inside block where new entry is created

sys/netgraph/bluetooth/hci/ng_hci_ulpi.c
111 ↗(On Diff #4270)

please add some dummy return here with a comment that says it should never get here

sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4270)

please either make new structure default, or, please move new fields into l2cap socket options. i think using l2cap_len works for binary backwards compatibility. source compatibility, where application fails to bzero(sockadd_l2cap) is a problem, but it will have to be addressed in the application.

i personally think that having socket option for Bluetooth LE is acceptable, and, in some ways cleaner. however, for sake of better linux compatibility i'm fine with adding new fields to sockaddr_l2cap.

sys/netgraph/bluetooth/include/ng_hci.h
1307 ↗(On Diff #4270)

please just remove it

sys/netgraph/bluetooth/include/ng_l2cap.h
645 ↗(On Diff #4270)

please do not use ng_l2cap_node_hook_info_ep as both struct name and type name.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4270)

can you please explain why loopback it here? is it not possible to loopback command at the socket layer?

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
246 ↗(On Diff #4270)

please remove #if 0 ... #endif block

1621 ↗(On Diff #4270)

i think this should read sizeof(*ep) instead of sizeof(bdaddr_t)

2146 ↗(On Diff #4270)

this looks good. if you choose to keep new fields in sockaddr_l2cap please rename sockaddr_l2cap to sockaddr_l2cap_old, and have new fields added to sockaddr_l2cap by default. if you choose to go with socket option, then, none of this is needed.

takawata added inline comments.Mar 31 2015, 4:20 PM
sys/netgraph/bluetooth/hci/ng_hci_evnt.c
421 ↗(On Diff #4270)

Address and address type should be in the block. OK.
other than this, which is now no effect, should be here, because rssi
will be updated properly

sys/netgraph/bluetooth/hci/ng_hci_ulpi.c
111 ↗(On Diff #4270)

ok.

sys/netgraph/bluetooth/include/ng_btsocket.h
243 ↗(On Diff #4270)

Ok, I will name old structure sockaddr_l2cap_compat and new one sockaddr_l2cap_compat.

Also warn structure change by preprossor directives.

Socket option was used in Linux. but it has been changed in some reason.

sys/netgraph/bluetooth/include/ng_hci.h
1307 ↗(On Diff #4270)

Ok.

sys/netgraph/bluetooth/include/ng_l2cap.h
645 ↗(On Diff #4270)

Ok. Just define typedef name.

sys/netgraph/bluetooth/l2cap/ng_l2cap_llpi.c
778 ↗(On Diff #4270)

To wait HCI connection event. Otherwise, it may possible to add outstanding ATT channel pointer to connection structure and change channel states if the channel pointer is valid.

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c
246 ↗(On Diff #4270)

Ok

1621 ↗(On Diff #4270)

Ok

2146 ↗(On Diff #4270)

Ok. I'll rename sockaddr_l2cap and change sockname, peername bind call structure also.

takawata updated this revision to Diff 4609.Apr 2 2015, 5:10 PM

reflect comments.
Make new version of sockaddr_l2cap default.

emax added a comment.Apr 5 2015, 6:45 PM

couple of minor nits. please see inline. my main concern is to make sure #warning on sockaddr_l2cap is not going to break the build. please assume that L2CAP_SOCKET_CHECKED is likely to not be set. otherwise looks fine.

sys/netgraph/bluetooth/hci/ng_hci_evnt.c
442 ↗(On Diff #4609)

can you please remove #if 0 block if this code is not needed?

sys/netgraph/bluetooth/include/ng_btsocket.h
247 ↗(On Diff #4609)

can you please make sure that this is not going to break the buildworld?

emax accepted this revision.Apr 5 2015, 6:46 PM
emax edited edge metadata.

i'm accepting this assuming that possible build breakage related to L2CAP_SOCKET_CHECKED is going to verified. thank you for your work!

This revision is now accepted and ready to land.Apr 5 2015, 6:46 PM
takawata removed a reviewer: rpaulo.Apr 6 2015, 4:11 PM
takawata added a subscriber: rpaulo.
takawata closed this revision.Apr 7 2015, 10:23 AM
takawata updated this revision to Diff 4719.

Closed by commit rS281198 (authored by @takawata).