Add L2CAP service

[zxzxwu]

Including fixed channels.
Note: This proto is not compatible with l2cap.proto on AOSP, since the API is not compatible with Bumble(and even most hosts).

[zxzxwu]

Sample implementation: zxzxwu/bumble#1

[uael]

I like your direction 👍 I had things to do today, let me review that tomorrow

[uael]

I was thinking about something like this:

service L2CAP {
  rpc Open(OpenRequest) returns (OpenResponse);
  rpc WaitOpen(OpenRequest) returns (stream OpenResponse);
  rpc Close(Channel) returns (CloseResponse);
  rpc WaitClose(Channel) returns (CloseResponse);
  rpc Receive(Channel) returns (stream bytes);
  rpc Send(SendRequest) returns (SendResponse);
}

message Channel {
  google.protobuf.Any cookie = 1;
}

message OpenRequest {
  Connection connection = 1;
  oneof type {
    // Fixed channel identifier (returns a fixed channel).
    uint16 cid = 2; // provide some constants ?
    // Protocol/Service Multiplexer (returns a dynamic channel).
    uint16 psm = 3; // provide some constants ?
  }
  // Maximum transmission unit.
  // Shall not be set for fixed channels.
  uint16 mtu = 4;
  // Maximum PDU payload size.
  // Shall not be set for fixed channels.
  // Required for dynamic channels over LE.
  // For BR/EDR connections, if set (and supported by the host), a credits based channel is opened.
  uint16 mps = 5;
}

message OpenResponse {
  oneof result {
    int8 error = 1; // enum ?
    Channel channel = 2;
  }
}

message CloseResponse {
  oneof result {
    int8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

message SendRequest {
  Channel channel = 1;
  bytes data = 2;
}

message SendResponse {
  oneof result {
    int8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

What do you think ?

[zxzxwu]

I was thinking about something like this:

service L2CAP {
  rpc Open(OpenRequest) returns (OpenResponse);
  rpc WaitOpen(OpenRequest) returns (stream OpenResponse);
  rpc Close(Channel) returns (CloseResponse);
  rpc WaitClose(Channel) returns (CloseResponse);
  rpc Receive(Channel) returns (stream bytes);
  rpc Send(SendRequest) returns (stream SendResponse);
}

message Channel {
  google.protobuf.Any cookie = 1;
}

message OpenRequest {
  Connection connection = 1;
  oneof type {
    // Fixed channel identifier (returns a fixed channel).
    uint16 cid = 2; // provide some constants ?
    // Protocol/Service Multiplexer (returns a dynamic channel).
    uint16 psm = 3; // provide some constants ?
  }
  // Maximum transmission unit.
  // Shall not be set for fixed channels.
  uint16 mtu = 4;
  // Maximum PDU payload size.
  // Shall not be set for fixed channels.
  // Required for dynamic channels over LE.
  // For BR/EDR connections, if set (and supported by the host), a credits based channel is opened.
  uint16 mps = 5;
}

message OpenResponse {
  oneof result {
    int8 error = 1; // enum ?
    Channel channel = 2;
  }
}

message CloseResponse {
  oneof result {
    int8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

message SendRequest {
  Channel channel = 1;
  bytes data = 2;
}

message SendResponse {
  oneof result {
    int8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

What do you think ?

I also considered whether we should use a socket-like API set. The reason I choose callback style is that:

• For server side, it needs to keep global queues because data may be sent right after opening, but client may not start receiving yet.
• For client side, disconnected will always be known as events instead of something need to be handled as exception or implicit wait in the background

[uael]

For server side, it needs to keep global queues because data may be sent right after opening, but client may not start receiving yet.

It's true for the queue (by channel), but not really a big deal I think. Also it will better align on other services like A2dp.

For client side, disconnected will always be known as events instead of something need to be handled as exception or implicit wait in the background

I was seeing the disconnect as the end of the receive stream or as an error on the send, do we really need a specific event ?

[zxzxwu]

Right, so it should still be better to have a synchronized model. I will propose another version tomorrow.

[uael]

I have a couple of questions:

• I there a requirements to wait for a server at the device level instead of the connection level ? To avoid the Transport enum.
• Don't we want to provide a way to open credits based channel for BR/EDR ?
• Is there a reason to not expose error handling ?
• I there a requirements to expose dynamic PSM allocation ? There might be one, but in that case we need the SDP service in order to expose it to the remote.

Also, I prefer to keep a naming close to the other services.

I propose you to start from the bellow version instead, and then to iterate over it:

service L2CAP {
  rpc Connect(ConnectRequest) returns (ConnectResponse);
  rpc WaitConnection(WaitConnectionRequest) returns (stream WaitConnectionResponse);
  rpc Disconnect(DisconnectRequest) returns (DisconnectResponse);
  rpc WaitDisconnection(WaitDisconnectionRequest) returns (WaitDisconnectionResponse);
  rpc Receive(ReceiveRequest) returns (stream ReceiveResponse);
  rpc Send(SendRequest) returns (SendResponse);
}

message Channel {
  google.protobuf.Any cookie = 1;
}

message ConnectRequest {
  Connection connection = 1;
  oneof type {
    // Fixed channel identifier (returns a fixed channel).
    uint16 cid = 2; // provide some constants ?
    // Protocol/Service Multiplexer (returns a dynamic channel).
    uint16 psm = 3; // provide some constants ?
  }
  // Maximum transmission unit.
  // Shall not be set for fixed channels.
  uint16 mtu = 4;
  // Maximum PDU payload size.
  // Shall not be set for fixed channels.
  // Required for dynamic channels over LE.
  // For BR/EDR connections, if set (and supported by the host), a credits based channel is opened.
  uint16 mps = 5;
}

message ConnectResponse {
  oneof result {
    uint8 error = 1; // enum ?
    Channel channel = 2;
  }
}

message WaitConnectionRequest {
  Connection connection = 1;
  oneof type {
    // Fixed channel identifier (returns a fixed channel).
    uint16 cid = 2; // provide some constants ?
    // Protocol/Service Multiplexer (returns a dynamic channel).
    uint16 psm = 3; // provide some constants ?
  }
  // Maximum transmission unit.
  // Shall not be set for fixed channels.
  uint16 mtu = 4;
  // Maximum PDU payload size.
  // Shall not be set for fixed channels.
  // Required for dynamic channels over LE.
  // For BR/EDR connections, if set (and supported by the host), a credits based channel is opened.
  uint16 mps = 5;
}

message WaitConnectionResponse {
  oneof result {
    uint8 error = 1; // enum ?
    Channel channel = 2;
  }
}

message DisconnectRequest {
  Channel channel = 1;
}

message DisconnectResponse {
  oneof result {
    uint8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

message WaitDisconnectionRequest {
  Channel channel = 1;
}

message WaitDisconnectionResponse {
  oneof result {
    int8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

message ReceiveRequest {
  Channel channel = 1;
}

message ReceiveResponse {
  bytes data = 1;
}

message SendRequest {
  Channel channel = 1;
  bytes data = 1;
}

message SendResponse {
  oneof result {
    uint8 error = 1; // enum ?
    google.protobuf.Empty success = 2;
  }
}

It's the closest from the spec I came with, but I might be wrong !
The main imposed design points are:

• Naming.
• Error handling.
• XxxRequest and XxxResponse for each Xxx rpc call, if not empty.

The rest is all subject to change, according to your feedback, android APIs, testing requirements, ...

[zxzxwu]

Sorry that was a draft sync...

[zxzxwu]

But for the server questions:

I there a requirements to wait for a server at the device level instead of the connection level ? To avoid the Transport enum.

This is relatively impossible, especially for fixed channels which are usually established right after ACL established. If servers must be registered on connection level, there might be some cases hard to handle.
An alternative way is to define a ChannelType enum instead of Transport enum, so that we can also descritbe other channel types better such as Credit/Connectionless channels.

I there a requirements to expose dynamic PSM allocation ? There might be one, but in that case we need the SDP service in order to expose it to the remote.

Not for my requirement(in fact I only need fixed channels for the near future), but on many platforms(like Android), dynamic channels cannot be registered without allocation(though there is a hidden API but not promised for usage).

Also, I prefer to keep a naming close to the other services.

This is not fully applicable because

pandora/l2cap.proto:47:9: "pandora.ConnectRequest" is already defined in file "pandora/host.proto".
pandora/l2cap.proto: "pandora.ConnectResponse.result" is already defined in file "pandora/host.proto".
pandora/l2cap.proto:65:9: "pandora.ConnectResponse" is already defined in file "pandora/host.proto".
pandora/l2cap.proto:72:9: "pandora.WaitConnectionRequest" is already defined in file "pandora/host.proto".
pandora/l2cap.proto: "pandora.WaitConnectionResponse.result" is already defined in file "pandora/host.proto".
pandora/l2cap.proto:89:9: "pandora.WaitConnectionResponse" is already defined in file "pandora/host.proto".
pandora/l2cap.proto:96:9: "pandora.DisconnectRequest" is already defined in file "pandora/host.proto".
pandora/l2cap.proto:107:9: "pandora.WaitDisconnectionRequest" is already defined in file "pandora/host.proto".

[uael]

This is relatively impossible, especially for fixed channels which are usually established right after ACL established. If servers must be registered on connection level, there might be some cases hard to handle.
An alternative way is to define a ChannelType enum instead of Transport enum, so that we can also descritbe other channel types better such as Credit/Connectionless channels.

Humm maybe we could do something like this ?

message DynamicChannelRequest {
  // BR/EDR or Low-Energy connection.
  Connection connection = 1;
  // Protocol/Service Multiplexer.
  uint16 psm = 2;
  // Maximum transmission unit.
  uint16 mtu = 4;
  // Maximum PDU payload size.
  // Required for dynamic channels over LE.
  // For BR/EDR connections, if set (and supported by the host), a credits based channel is opened.
  uint16 mps = 5;
}

message ConnectRequest {
  oneof type {
    // Create a fixed channel from a fixed channel CID.
    uint16 fixed = 1;
    // Create a dynamic channel over BR/EDR or Low-Energy.
    DynamicChannelRequest dynamic = 2;
  }
}

message WaitConnectionRequest {
  oneof type {
    // Create a fixed channel from a fixed channel CID.
    uint16 fixed = 1;
    // Create a dynamic channel over BR/EDR or Low-Energy.
    DynamicChannelRequest dynamic = 2;
  }
}

Not for my requirement(in fact I only need fixed channels for the near future), but on many platforms(like Android), dynamic channels cannot be registered without allocation(though there is a hidden API but not promised for usage).

Ok, I guess we can implement this in a follow-up PR, what do you think ?

This is not fully applicable because ...

Can you try changing the package name package pandora to package pandora.l2cap ?
It may require some modifications in the code generator, but it's the way to go I think.

Thank you for your patience, I known it's not easy to handle such a review with our different timelines :/ ..

[zxzxwu]

Generally it's good enough, but one thing left is that we still need a flag to handle whether enhanced credit based connection is used in LE.

Ok, I guess we can implement this in a follow-up PR, what do you think ?

Sure

Can you try changing the package name package pandora to package pandora.l2cap ?
It may require some modifications in the code generator, but it's the way to go I think.

NP

[uael]

Generally it's good enough, but one thing left is that we still need a flag to handle whether enhanced credit based connection is used in LE.

True!

message ConnectRequest {
  oneof type {
    // ...
    // Create a Enhanced credits based dynamic channel over BR/EDR or Low-Energy.
    DynamicChannelRequest enhanced_credits_based = 3;
  }
}

message WaitConnectionRequest {
  oneof type {
    // ...
    // Create a Enhanced credits based dynamic channel over BR/EDR or Low-Energy.
    DynamicChannelRequest enhanced_credits_based = 3;
  }
}

Or something like this ?

[DeltaEvo]

Left some small nitpicks, I really like this interface thanks @zxzxwu and @uael !

[zxzxwu]

Modified based on comments.
I will provide an implementation for verification soon so we are not hurry to merge this.

[uael]

Perfect! Some documentation and we are good to go 😃

[uael]

@DeltaEvo Could you review this as I will be OOO ?

[zxzxwu]

Sample implementations:

• zxzxwu/bumble@c0e53e3
• zxzxwu/avatar@05f3e77