Adding Bluetooth 4.0 LE + ATT protocol Support.
This supports only master side; device side is not supported yet.
Details
Details
- Reviewers
emax - Commits
- rS281198: Initial Bluetooth LE support.
You may want to prepare some Bluetooth LE supported sensor(like pedomator)
And use test program that sent the other day.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| 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() ? |
| 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. |
| 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. |
| sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c | ||
| 2164 ↗ | (On Diff #4133) | Yes.... I forget to remove it. |
Comment Actions
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.
Comment Actions
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)? |
| 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. |
| 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. |
| sys/netgraph/bluetooth/include/ng_btsocket.h | ||
|---|---|---|
| 243 ↗ | (On Diff #4133) | Renaming our new structure to sockaddr_l2cap would be good. |
| 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? |
| sys/netgraph/bluetooth/include/ng_btsocket.h | ||
|---|---|---|
| 243 ↗ | (On Diff #4133) | If a programmer use new structure as old structure, |
| 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. |
Comment Actions
it looks mostly good to me. a few minor nit picks, and, two somewhat bigger issues
- the use of M_PROTO2 flag. can it be avoided? i'm mostly fine with it, just wanted to get a bit more explanation.
- 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. |
| sys/netgraph/bluetooth/hci/ng_hci_evnt.c | ||
|---|---|---|
| 421 ↗ | (On Diff #4270) | Address and address type should be in the block. OK. |
| 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. |
Comment Actions
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? |
Comment Actions
i'm accepting this assuming that possible build breakage related to L2CAP_SOCKET_CHECKED is going to verified. thank you for your work!