|
|
OLD | NEW |
---|---|
1 // Copyright 2016 The Chromium Authors. All rights reserved. | 1 // Copyright 2016 The Chromium Authors. All rights reserved. |
2 // Use of this source code is governed by a BSD-style license that can be | 2 // Use of this source code is governed by a BSD-style license that can be |
3 // found in the LICENSE file. | 3 // found in the LICENSE file. |
4 | 4 |
5 module bluetooth.mojom; | 5 module bluetooth.mojom; |
6 | 6 |
7 struct AdvertisingPacket { | |
8 uint64 timestamp; | |
9 int8 rssi; | |
10 DeviceInfo device; | |
11 }; | |
12 | |
7 struct DeviceInfo { | 13 struct DeviceInfo { |
8 string? name; | 14 string? name; |
9 string name_for_display; | 15 string name_for_display; |
10 string address; | 16 string address; |
11 }; | 17 }; |
12 | 18 |
13 interface Device { | 19 interface Device { |
14 // Gets basic information about the device. Returns null, if no device is | 20 // Gets basic information about the device. Returns null, if no device is |
15 // available. | 21 // available. |
16 GetInfo() => (DeviceInfo? info); | 22 GetInfo() => (DeviceInfo? info); |
23 | |
24 // Sets the client that listens to events from this device. | |
25 SetClient(GattClient client); | |
17 }; | 26 }; |
27 | |
28 interface GattClient { | |
29 // Called when one of the following properties of the device changes: | |
30 // Address, appearance, Bluetooth class, Inquiry RSSI, Inquiry TX Power, | |
31 // Service UUIDs, Connectionable state, Connection state, Pairing state, | |
32 // Trustable state. | |
33 // Generally called for each advertisement packet recevied, but this is not | |
34 // guaranteed on ChromeOS or Linux. Because the RSSI is always changing, | |
35 // it's very likely this will be called for each advertising packet. | |
36 DeviceChanged(AdvertisingPacket packet); | |
ortuno
2016/10/13 22:22:49
In the spirit of "Just a Thin Wrapper™️" I think t
In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the adapter
client and it should match DeviceChanged more closely i.e. just call with the
address :(
Alternatively we could call it with a DeviceInfo struct since that would save
clients a round trip but that might be too much.
scheib: Do you have any thoughts?
scheib
2016/10/14 07:14:51
We've deviated a bit already with DeviceInfo aggre
On 2016/10/13 22:22:49, ortuno wrote:
> In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
adapter
> client and it should match DeviceChanged more closely i.e. just call with the
> address :(
>
> Alternatively we could call it with a DeviceInfo struct since that would save
> clients a round trip but that might be too much.
>
> scheib: Do you have any thoughts?
We've deviated a bit already with DeviceInfo aggregating all the accessors for
e.g. GetName, GetAddress. If we continue that route and stick to the
DeviceChanged only replying with an address then the client will still need to
call GetInfo. At that point we'll need to include GetInquiryRSSI in the
DeviceInfo packet as well.
One could argue that populating those packet members will happen either in a
call to GetInfo or if we do it directly in a response to DeviceChanged. So, even
in our "just a thin wrapper" we'll have some untested accessors populating a
struct. If we know those only have meaning in a DeviceChanged event that I think
we may as well make the calls in that event and save the DeviceChanged ->
address -> GetInfo step and just return the AdvertisingPacket as written here.
In other words, I think it's OK to clean up naming, and move functionality into
better clusters of interfaces, when the work is doing the same thing regardless
(populating struct members with accessor calls).
What wouldn't be OK is when we start adding logic, caching state, or changing
the way algorithms work.
mbrunson
2016/10/14 18:25:25
I remember ortuno recommending the Mojo clients to
On 2016/10/14 07:14:51, scheib wrote:
> On 2016/10/13 22:22:49, ortuno wrote:
> > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> adapter
> > client and it should match DeviceChanged more closely i.e. just call with
the
> > address :(
> >
> > Alternatively we could call it with a DeviceInfo struct since that would
save
> > clients a round trip but that might be too much.
> >
> > scheib: Do you have any thoughts?
>
> We've deviated a bit already with DeviceInfo aggregating all the accessors for
> e.g. GetName, GetAddress. If we continue that route and stick to the
> DeviceChanged only replying with an address then the client will still need to
> call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> DeviceInfo packet as well.
>
> One could argue that populating those packet members will happen either in a
> call to GetInfo or if we do it directly in a response to DeviceChanged. So,
even
> in our "just a thin wrapper" we'll have some untested accessors populating a
> struct. If we know those only have meaning in a DeviceChanged event that I
think
> we may as well make the calls in that event and save the DeviceChanged ->
> address -> GetInfo step and just return the AdvertisingPacket as written here.
>
> In other words, I think it's OK to clean up naming, and move functionality
into
> better clusters of interfaces, when the work is doing the same thing
regardless
> (populating struct members with accessor calls).
>
> What wouldn't be OK is when we start adding logic, caching state, or changing
> the way algorithms work.
I remember ortuno recommending the Mojo clients to change, noting that the way
Mac or Android does it seems better. I was planning to follow the BluetoothGatt
pattern from Android where a single client attached to the device interface
handles the GATT-related callbacks for that device. Let me know if you think
this is a good idea because GattConnection and the Service interface will be my
next CLs.
scheib
2016/10/14 20:25:40
I dislike the Android and Core Bluetooth way of ge
On 2016/10/14 18:25:25, mbrunson wrote:
> On 2016/10/14 07:14:51, scheib wrote:
> > On 2016/10/13 22:22:49, ortuno wrote:
> > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> > adapter
> > > client and it should match DeviceChanged more closely i.e. just call with
> the
> > > address :(
> > >
> > > Alternatively we could call it with a DeviceInfo struct since that would
> save
> > > clients a round trip but that might be too much.
> > >
> > > scheib: Do you have any thoughts?
> >
> > We've deviated a bit already with DeviceInfo aggregating all the accessors
for
> > e.g. GetName, GetAddress. If we continue that route and stick to the
> > DeviceChanged only replying with an address then the client will still need
to
> > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > DeviceInfo packet as well.
> >
> > One could argue that populating those packet members will happen either in a
> > call to GetInfo or if we do it directly in a response to DeviceChanged. So,
> even
> > in our "just a thin wrapper" we'll have some untested accessors populating a
> > struct. If we know those only have meaning in a DeviceChanged event that I
> think
> > we may as well make the calls in that event and save the DeviceChanged ->
> > address -> GetInfo step and just return the AdvertisingPacket as written
here.
> >
> > In other words, I think it's OK to clean up naming, and move functionality
> into
> > better clusters of interfaces, when the work is doing the same thing
> regardless
> > (populating struct members with accessor calls).
> >
> > What wouldn't be OK is when we start adding logic, caching state, or
changing
> > the way algorithms work.
>
> I remember ortuno recommending the Mojo clients to change, noting that the way
> Mac or Android does it seems better. I was planning to follow the
BluetoothGatt
> pattern from Android where a single client attached to the device interface
> handles the GATT-related callbacks for that device. Let me know if you think
> this is a good idea because GattConnection and the Service interface will be
my
> next CLs.
I dislike the Android and Core Bluetooth way of getting results from e.g.
characteristic reads or notifications on a single client interface -- and then
needing to interpret / route the request.
Mojo will make this easier for us, as we have an asynchronous response that can
provide a value. So, we should be able to characteristic.Read().then(status,
value); That's what is sketched out on the go/fizzbluetoothinternals doc.
Perhaps update that doc with GattClient (as is here) and other similar changes
and we can look again quickly to verify it makes sense.
Notifications look like the one thing that will need a client, but I'd prefer to
keep the current design doc style of a CharacteristicClient that can be set
per-characteristic and receive notifications just for that object. (versus
having the GattClient receive notifications)
I'm curious ortuno's thoughts.
ortuno
2016/10/17 05:03:31
If we are going to go ahead with this pattern I ha
On 2016/10/14 at 20:25:40, scheib wrote:
> On 2016/10/14 18:25:25, mbrunson wrote:
> > On 2016/10/14 07:14:51, scheib wrote:
> > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> > > adapter
> > > > client and it should match DeviceChanged more closely i.e. just call
with
> > the
> > > > address :(
> > > >
> > > > Alternatively we could call it with a DeviceInfo struct since that would
> > save
> > > > clients a round trip but that might be too much.
> > > >
> > > > scheib: Do you have any thoughts?
> > >
> > > We've deviated a bit already with DeviceInfo aggregating all the accessors
for
> > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > DeviceChanged only replying with an address then the client will still
need to
> > > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > > DeviceInfo packet as well.
> > >
> > > One could argue that populating those packet members will happen either in
a
> > > call to GetInfo or if we do it directly in a response to DeviceChanged.
So,
> > even
> > > in our "just a thin wrapper" we'll have some untested accessors populating
a
> > > struct. If we know those only have meaning in a DeviceChanged event that I
> > think
> > > we may as well make the calls in that event and save the DeviceChanged ->
> > > address -> GetInfo step and just return the AdvertisingPacket as written
here.
> > >
> > > In other words, I think it's OK to clean up naming, and move functionality
> > into
> > > better clusters of interfaces, when the work is doing the same thing
> > regardless
> > > (populating struct members with accessor calls).
> > >
> > > What wouldn't be OK is when we start adding logic, caching state, or
changing
> > > the way algorithms work.
> >
If we are going to go ahead with this pattern I have a couple of concerns:
1. We are adding logic since now Device has to check if the event corresponds
to that specific Bluetooth Device.
2. We will be adding a ton of observers. Say we have 100 devices each of those
device will receive an event and will have to check if the event corresponds
that device.
3. Most importantly though, I don't think this event should go in Device. Say
you are interested in all Advertisement Packets. Where would you get them? Would
you have to get a Device interface for each device so you can get all
advertisements?
I'm not opposed to the event but I think it should live in AdapterClient.
> > I remember ortuno recommending the Mojo clients to change, noting that the
way
> > Mac or Android does it seems better. I was planning to follow the
BluetoothGatt
> > pattern from Android where a single client attached to the device interface
> > handles the GATT-related callbacks for that device. Let me know if you think
> > this is a good idea because GattConnection and the Service interface will be
my
> > next CLs.
>
> I dislike the Android and Core Bluetooth way of getting results from e.g.
characteristic reads or notifications on a single client interface -- and then
needing to interpret / route the request.
>
> Mojo will make this easier for us, as we have an asynchronous response that
can provide a value. So, we should be able to characteristic.Read().then(status,
value); That's what is sketched out on the go/fizzbluetoothinternals doc.
Perhaps update that doc with GattClient (as is here) and other similar changes
and we can look again quickly to verify it makes sense.
>
> Notifications look like the one thing that will need a client, but I'd prefer
to keep the current design doc style of a CharacteristicClient that can be set
per-characteristic and receive notifications just for that object. (versus
having the GattClient receive notifications)
>
> I'm curious ortuno's thoughts.
(I remember a while ago, I was really excited about Mojo and how I was going to
be able to have an interface for services and another one for characteristics,
etc. But then scheib was like: "But why?"[1] so I started going in the other
direction of just having one interface haha)
My suggestion was more regarding how we perform read/writes rather than where
the events arrive. The asynchronous nature of Mojo means that we will need two
callbacks to get a characteristic or a service which I think makes the code much
harder to follow[2]. Anyway I think we should go for the interface that leads to
the easiest implementation on top of device/bluetooth.
I would personally go for the following due to the ease of implementation and
lack of extra interfaces:
interface Device { // Note that everything is in the device.
GetServices() => (array<ServiceInfo> services);
GetCharacteristics(string service_id) => (array<CharacteristicInfo>
characteristics);
GetDescriptors(string service_id, string characteristic_id) =>
(array<DescriptorInfo> descriptors);
ReadValueForCharacteristic(string service_id, string characteristic_id) =>
(array<uint8_t> value);
WriteValueForCharacteristic(string service_id, string characteristic_id,
array<uint8_t>) => ();
StartNotificationsForCharacteristic(string service_id, string
characteristic_id);
SetNotificationsClient(string service_id, string characteristic_id, client);
}
// Device.cpp
ReadValueForCharacteristic(string service_id, string characteristic_id,
callback) {
auto* device = adapter_->GetDevice(device_address_);
if (device == nullptr)
callback.Run(/* some error */);
auto* service = device->GetService(service_id);
if (service == nullptr)
callback.Run(/* some error */);
auto* characteristic = service->GetCharacteristic(characteristic_id);
if (characteristic == nullptr)
callback.Run(/* some error */);
characteristic->ReadValue(base::Bind(..., callback));
}
I don't think we can get around the routing since we are not allowed to keep
pointers to any objects :(
[1] http://i.imgur.com/Br00TCn.gif?noredirect
[2]
Interface per object:
interface Service {
GetCharacteristic(CharacteristicId id);
GetCharacteristics() => (array<CharacteristicInfo> characteristics);
}
How Web Bluetooth would use it:
// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
...));
});
OnGetCharacteristics(uuid, characteristics, promise) {
for (characteristic : characteristics) {
if (characteristic->uuid == uuid) {
service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
characteristic->id, promise));
return;
}
}
}
OnGetCharacteristic(uuid, id, characteristic, promise) {
promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
}
As opposed to:
interface GattRemoteServer {
GetServices() => (array<ServiceInfo> services);
GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
characteristics);
}
How Web Bluetooth would use it:
// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
uuid, ...));
}
OnGetCharacteristics(uuid, promise, characteristics) {
for (characteristic : characteristics) {
if (characteristic->uuid == uuid) {
promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
}
}
promise.reject("Not Found Error");
}
Regarding notifications, I think we can still have a client just for
notifications:
interface RemoteGATTServer {
SetCharacteristicNotificationsClient(CharacteristicId id,
CharacteristicNotificationClient client)
}
interface CharacteristicNotificationClient {
NotificationReceived(type, value, ...);
}
or something like that.
mbrunson
2016/10/17 21:33:38
I've moved PacketReceived to Adapter.
Given your
On 2016/10/17 05:03:31, ortuno wrote:
> On 2016/10/14 at 20:25:40, scheib wrote:
> > On 2016/10/14 18:25:25, mbrunson wrote:
> > > On 2016/10/14 07:14:51, scheib wrote:
> > > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in
the
> > > > adapter
> > > > > client and it should match DeviceChanged more closely i.e. just call
> with
> > > the
> > > > > address :(
> > > > >
> > > > > Alternatively we could call it with a DeviceInfo struct since that
would
> > > save
> > > > > clients a round trip but that might be too much.
> > > > >
> > > > > scheib: Do you have any thoughts?
> > > >
> > > > We've deviated a bit already with DeviceInfo aggregating all the
accessors
> for
> > > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > > DeviceChanged only replying with an address then the client will still
> need to
> > > > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > > > DeviceInfo packet as well.
> > > >
> > > > One could argue that populating those packet members will happen either
in
> a
> > > > call to GetInfo or if we do it directly in a response to DeviceChanged.
> So,
> > > even
> > > > in our "just a thin wrapper" we'll have some untested accessors
populating
> a
> > > > struct. If we know those only have meaning in a DeviceChanged event that
I
> > > think
> > > > we may as well make the calls in that event and save the DeviceChanged
->
> > > > address -> GetInfo step and just return the AdvertisingPacket as written
> here.
> > > >
> > > > In other words, I think it's OK to clean up naming, and move
functionality
> > > into
> > > > better clusters of interfaces, when the work is doing the same thing
> > > regardless
> > > > (populating struct members with accessor calls).
> > > >
> > > > What wouldn't be OK is when we start adding logic, caching state, or
> changing
> > > > the way algorithms work.
> > >
>
> If we are going to go ahead with this pattern I have a couple of concerns:
>
> 1. We are adding logic since now Device has to check if the event corresponds
> to that specific Bluetooth Device.
> 2. We will be adding a ton of observers. Say we have 100 devices each of
those
> device will receive an event and will have to check if the event corresponds
> that device.
> 3. Most importantly though, I don't think this event should go in Device. Say
> you are interested in all Advertisement Packets. Where would you get them?
Would
> you have to get a Device interface for each device so you can get all
> advertisements?
>
> I'm not opposed to the event but I think it should live in AdapterClient.
>
> > > I remember ortuno recommending the Mojo clients to change, noting that the
> way
> > > Mac or Android does it seems better. I was planning to follow the
> BluetoothGatt
> > > pattern from Android where a single client attached to the device
interface
> > > handles the GATT-related callbacks for that device. Let me know if you
think
> > > this is a good idea because GattConnection and the Service interface will
be
> my
> > > next CLs.
> >
> > I dislike the Android and Core Bluetooth way of getting results from e.g.
> characteristic reads or notifications on a single client interface -- and then
> needing to interpret / route the request.
> >
> > Mojo will make this easier for us, as we have an asynchronous response that
> can provide a value. So, we should be able to
characteristic.Read().then(status,
> value); That's what is sketched out on the go/fizzbluetoothinternals doc.
> Perhaps update that doc with GattClient (as is here) and other similar changes
> and we can look again quickly to verify it makes sense.
> >
> > Notifications look like the one thing that will need a client, but I'd
prefer
> to keep the current design doc style of a CharacteristicClient that can be set
> per-characteristic and receive notifications just for that object. (versus
> having the GattClient receive notifications)
> >
> > I'm curious ortuno's thoughts.
>
> (I remember a while ago, I was really excited about Mojo and how I was going
to
> be able to have an interface for services and another one for characteristics,
> etc. But then scheib was like: "But why?"[1] so I started going in the other
> direction of just having one interface haha)
>
> My suggestion was more regarding how we perform read/writes rather than where
> the events arrive. The asynchronous nature of Mojo means that we will need two
> callbacks to get a characteristic or a service which I think makes the code
much
> harder to follow[2]. Anyway I think we should go for the interface that leads
to
> the easiest implementation on top of device/bluetooth.
>
> I would personally go for the following due to the ease of implementation and
> lack of extra interfaces:
>
> interface Device { // Note that everything is in the device.
> GetServices() => (array<ServiceInfo> services);
> GetCharacteristics(string service_id) => (array<CharacteristicInfo>
> characteristics);
> GetDescriptors(string service_id, string characteristic_id) =>
> (array<DescriptorInfo> descriptors);
>
> ReadValueForCharacteristic(string service_id, string characteristic_id) =>
> (array<uint8_t> value);
> WriteValueForCharacteristic(string service_id, string characteristic_id,
> array<uint8_t>) => ();
>
> StartNotificationsForCharacteristic(string service_id, string
> characteristic_id);
> SetNotificationsClient(string service_id, string characteristic_id, client);
>
> }
>
>
> // Device.cpp
>
> ReadValueForCharacteristic(string service_id, string characteristic_id,
> callback) {
> auto* device = adapter_->GetDevice(device_address_);
> if (device == nullptr)
> callback.Run(/* some error */);
>
> auto* service = device->GetService(service_id);
> if (service == nullptr)
> callback.Run(/* some error */);
>
> auto* characteristic = service->GetCharacteristic(characteristic_id);
> if (characteristic == nullptr)
> callback.Run(/* some error */);
>
> characteristic->ReadValue(base::Bind(..., callback));
> }
>
> I don't think we can get around the routing since we are not allowed to keep
> pointers to any objects :(
>
> [1] http://i.imgur.com/Br00TCn.gif?noredirect
>
> [2]
> Interface per object:
>
> interface Service {
> GetCharacteristic(CharacteristicId id);
> GetCharacteristics() => (array<CharacteristicInfo> characteristics);
> }
>
> How Web Bluetooth would use it:
>
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
> service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
> ...));
> });
>
> OnGetCharacteristics(uuid, characteristics, promise) {
> for (characteristic : characteristics) {
> if (characteristic->uuid == uuid) {
> service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
> characteristic->id, promise));
> return;
> }
> }
> }
>
> OnGetCharacteristic(uuid, id, characteristic, promise) {
> promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
> }
>
> As opposed to:
>
> interface GattRemoteServer {
> GetServices() => (array<ServiceInfo> services);
> GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
> characteristics);
> }
>
> How Web Bluetooth would use it:
>
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
> gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
> uuid, ...));
> }
>
> OnGetCharacteristics(uuid, promise, characteristics) {
> for (characteristic : characteristics) {
> if (characteristic->uuid == uuid) {
> promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
> }
> }
> promise.reject("Not Found Error");
> }
>
> Regarding notifications, I think we can still have a client just for
> notifications:
>
> interface RemoteGATTServer {
> SetCharacteristicNotificationsClient(CharacteristicId id,
> CharacteristicNotificationClient client)
> }
>
> interface CharacteristicNotificationClient {
> NotificationReceived(type, value, ...);
> }
>
> or something like that.
I've moved PacketReceived to Adapter.
Given your suggestion, where would the GATT connection logic go? Instead of
having the functions in Device, putting them in RemoteGattServer and having the
Device call "ConnectGatt" that returns a RemoteGattServer seems like a good idea
(I've noted these changes in the go/fizzbluetoothinternals design doc). This
leaves room for other Bluetooth device logic (pairing, other types of
connections, etc.) and keeps the GATT connection's lifetime tied to the lifetime
of the RemoteGattServer instance. Thoughts?
| |
37 }; | |
OLD | NEW |