Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Linux and CoreBluetooth L2Cap Channels #18

Closed
wants to merge 3 commits into from

Conversation

abezukor
Copy link

Partly Addresses #3.

This PR does 3 things.

  • Adds a feature flag for enabling and disabling L2Cap channel support.
  • Changes the L2Cap implementation to implement AsyncRead + AsyncWrite. These traits seem better than using custom read and write methods as they integrate better with the rest of the rust async ecosystem. They also have read and write methods. I am using the tokio versions instead of the futures versions because the bluer (Linux) implementation relies on the tokio version and making an adapter seems like unnecessary complexity. I also think it is important to keep the interface the same for all platforms. Note that bluest can still be used without tokio if the l2cap feature is disabled.
  • Adds L2Cap channel support to Linux and CoreBluetooth.

I have only tested on MacOS, IOS, and Linux. I have written an android implementation but it may have bugs, or be entirely the wrong way of doing things. @Dirbaio can you take a look at the android code?

@Dirbaio
Copy link
Contributor

Dirbaio commented Jun 11, 2024

Changes the L2Cap implementation to implement AsyncRead + AsyncWrite

I intentionally did not do this, I don't think we should do it.

AsyncRead/AsyncWrite (and std::io::Read/Write) model a byte stream. This is a "pipe" with some bytes in the buffer, where write puts some bytes in the buffer and read removes some. Once bytes are in the buffer, they are all smooshed together, they "forget" which write call put them in. You can write "abcd" and then read "a" then "b" etc. you can write "ab" then "cd" and then read all bytes with a single read "abcd".

L2CAP channels are NOT byte streams. They're packet streams. The "buffer" holds packets of a known size (Vec<u8>), not just bytes (u8). write pushes one packet, read pops one packet. Bytes in the buffer "remember" which packet they're from. If you write "abcd" then read, you will get "abcd". if you read with a too short buffer (e.g 2 bytes) you get an error if the packet doesn't fit, you don't get "ab" leaving "cd" in the buffer like you would with a packet stream.

This distinction is important because higher layers building on top of L2CAP channels can use the packet boundaries as framing. For example the IPSP protocol sends IPv6 packets as L2CAP packets. It needs a packet stream, it wouldn't work over a byte stream because reads could return half of an IPv6 packet, or two IPv6 packets smooshed together.

See how std's UdpSocket doesn't implement Read/Write, while TcpSocket does. This is because UdpSocket is also packet-based, like L2CAP CoC channels.

@abezukor
Copy link
Author

abezukor commented Jun 12, 2024

I still think that having the stream option is useful, but I also see your point about preserving any information from packet boundaries. How about we emulate bluer and have both a stream based, and packet based option? Device could have two methods, open_l2cap_channel, and open_l2cap_stream.

@Dirbaio
Copy link
Contributor

Dirbaio commented Jun 12, 2024

stream l2cap channels are BR/EDR only (Bluetooth Classic only). Bluetooth Low Energy only has packet-based l2cap channels, and this library focuses only on BLE.

@jmagnuson
Copy link

stream l2cap channels are BR/EDR only (Bluetooth Classic only).

The connect/bind APIs for the socket types that @abezukor linked allow passing in a SocketAddr containing an AddressType enum, which can be either BR/EDR or LE. I currently use these in my application to bind different address types to different PSMs for a given (streaming) protocol, and it works well.

@Dirbaio
Copy link
Contributor

Dirbaio commented Jun 12, 2024

If you read the bluetooth Core Specification, it says the only allowed L2CAP modes of operation for BLE are

  • "Basic L2CAP Mode" - for signaling only, not available for user data.
  • "LE Credit Based Flow Control Mode" - this is what you get when you ask the OS to establish a L2CAP channel on BLE.

And "LE Credit Based Flow Control Mode" is packet-based.

All the other modes (Flow Control Mode, Retransmission Mode, Enhanced Retransmission Mode, Streaming Mode, Enhanced Credit Based Flow Control Mode) are BR/EDR-only. They're not usable on BLE.

I don't know what Linux/BlueZ does if you request a channel of the "wrong" type in BLE. Perhaps it behaves wrong, perhaps it helpfully emulates a byte-stream by buffering packets and letting you read/write individual bytes off packets. The Android/iOS APIs don't do that, and I'm not convinced bluest should be the one emulating it.

@alexmoon
Copy link
Owner

@Dirbaio are you sure the Android and CoreBluetooth interfaces operate in packet mode? They both seem to use the generic byte stream types from their respective platforms. For example, if the device sends two packets over an L2CAP channel before the host calls calls read on the stream will they only return the first packet from the next call to read or will they return both packets concatenated? Similarly for the write side.

If Android, iOS and Linux all provide an effective packet-based interface to the L2CAP channel, then I agree that we should preserve that functionality. However, if they actually treat them as generic byte streams I don't think we can count on packet based behavior without introducing race conditions.

@abezukor
Copy link
Author

A few notes from some tests that I have ran

  • CoreBluetooth L2CAP stream file descriptors are opened as SOCK_STREAM, not SOCK_SEQPACKET. (I checked using getsockopt). This makes me suspect that doing packet-based operations may be unreliable. I also could not find a public interface for getting the MTU, so Apple seems to be discouraging doing packet based operations.
  • I am able to use bluer::l2cap::Stream when both the local and remote addresses are AddressType::LePublic. The kernel buffers packets for reads when the socket is opened as SOCK_STREAM. See bt_sock_stream_recvmsg. Writes are handled by bluer. It sends a max of the mtu worth of data at a time (see bluer::l2cap::Stream::poll_write_priv). AsyncWrite::poll_write does not require all of the data to be written on each poll_write call.
    • A packetized interface is available using bluer::l2cap::SeqPacket, I suspect that this presevers all packet boundaries.

@alexmoon
Copy link
Owner

Thanks @abezukor. I suspect Android is also using a stream-oriented channel instead of a packet oriented one, though the documentation is not completely clear. Based on that it seems that Linux is the only platform to support a packet oriented L2CAP connection. Since Bluest aims to be cross platform and target the lowest-common-denominator between platforms, I think the stream model is what we need to support.

However, I'm not keen in making Tokio a required dependency or async runtime for users of Bluest on non-Linux platforms, even if L2CAP is an optional feature. I would rather implement the AsyncRead and AsyncWrite traits from futures-io.

I'd also like to see minimal changes to the existing Android implementation and the root device and l2cap_channel modules since they've been well tested already.

@abezukor
Copy link
Author

However, I'm not keen in making Tokio a required dependency or async runtime for users of Bluest on non-Linux platforms, even if L2CAP is an optional feature. I would rather implement the AsyncRead and AsyncWrite traits from futures-io.

There are two reasons that I used the tokio version instead of the futures version

  • As bluest aims to be cross platform, it should provide a platform independent api of interacting with l2cap streams. If the AsyncRead/AsyncWrite traits vary per platform, then library consumers will have to write platform specific code. For example, if a library consumer wanted to use any of the functions from AsyncReadExt, e.g read_exact, they would have to include the following in their code:
#[cfg(target_os = "linux")]
use tokio::io::AsyncReadExt;
#[cfg(not(target_os = "linux"))]
use futures_util::io::AsyncReadExt

Additionally some functions are only available in either futures_util::io::AsyncReadExt or tokio::io::AsyncReadExt, for example futures_util::io::AsyncReadExt::read_vectored and tokio::io::AsyncReadExt::read_buf. A library consumer could use one of these apis, only to find that there code is not cross platform due to this implementation detail of the library.
I think it is better to keep the same api on all platforms.

  • My CoreBluetooth implementation relies on tokio::net::UnixStream which implements the tokio traits. I could re-implement it using blocking io on separate threads (similar to the Android implementation), or write our own implementation using non-blocking io, but that seems like unnecessary complexity.

I will also note that tokio is only a required dependency if the l2cap feature is enabled. That was the main motivation for adding the feature flag.

@alexmoon
Copy link
Owner

  • As bluest aims to be cross platform, it should provide a platform independent api of interacting with l2cap streams. If the AsyncRead/AsyncWrite traits vary per platform, then library consumers will have to write platform specific code. For example, if a library consumer wanted to use any of the functions from AsyncReadExt, e.g read_exact, they would have to include the following in their code:

Agreed. We need to use the futures-io versions on all platforms, including Linux.

  • My CoreBluetooth implementation relies on tokio::net::UnixStream which implements the tokio traits. I could re-implement it using blocking io on separate threads (similar to the Android implementation), or write our own implementation using non-blocking io, but that seems like unnecessary complexity.

Minimizing both the number and weight of dependencies is a major design consideration for bluest and maintaining async runtime agnosticism is a high priority (with the exception of Linux since bluer is already tied to tokio). For CoreBluetooth your solution is very clever, but I'm not comfortable with the costs. I see two possible solutions. First would be using your same idea, with the async-io crate which is runtime agnostic and lighter weight than tokio (at the cost of spinning up an extra IO thread for tokio users). Second would be to use the NSStream delegate functionality to implement the read/write futures manually. The second is obviously the lighter solution, but it does involve a fair amount of unsafe code and interacting with objective-c APIs is not the easiest.

@abezukor
Copy link
Author

abezukor commented Aug 6, 2024

For my usecase, I need the tokio versions. I think the best way forward would be to implement the functionality manually using a delegate, and then create a feature in bluest to enable the tokio AsyncRead + AsyncWrite functionality. Would that be acceptable?

Regardless, I will not have time to work on this in the near future, so I am closing this PR. If someone else wants to pick it up, feel free.

@abezukor abezukor closed this Aug 6, 2024
@alexmoon
Copy link
Owner

alexmoon commented Aug 6, 2024

Yes, that would be fine. Tokio does come with adapters for the futures versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants