On Thu, 2025-06-19 at 11:12 +0800, Ye He wrote: > Hi Luiz, Bastien > > [ EXTERNAL EMAIL ] > > > > Hi Bastien, > > > > On Wed, Jun 18, 2025 at 4:09 AM Bastien Nocera <hadess@xxxxxxxxxx> > > wrote: > > > On Wed, 2025-06-18 at 10:39 +0800, Ye He via B4 Relay wrote: > > > > From: Ye He <ye.he@xxxxxxxxxxx> > > > > > > > > This patch add a new Connected(string bearer) signal to > > > > indicate > > > > which transport > > > > (LE or BR/EDR) has connected. Also extend the Disconnected > > > > signal to > > > > include > > > > a bearer argument. > > > > > > > > This allows applications to distinguish transport-specific > > > > connection > > > > events in dual-mode scenarios. > > > > > > > > Fixes: https://github.com/bluez/bluez/issues/1350 > > > This is an API change, the commit needs to explain when the > > > Disconnected signal API was introduced, and probably mention that > > > the > > > because it was introduced in 5.82, it didn't have time to be > > > used. > > > > > > It might also be a better idea for both signals to send out a > > > dictionary of values, so that the "bearer" can be added without > > > an API > > > change, and further info can also be passed without an API > > > change. > > +1, I don't really like the idea of introducing bearer specific API > > into Device interface, using dedicated interface is probably better > > to > > avoid confusions and we can then introduce bearer specific methods > > and > > properties as well. > Thanks for the feedback. I understand and agree with your point — > introducing dedicated org.bluez.Device.LE and org.bluez.Device.BREDR > interfaces would be a cleaner long term solution. It would certainly > make the transport specific behavior more explicit and extensible. > > At the same time, this approach will be a significant change, > requiring > substantial refactoring and extensive testing across the stack. This > may > be too large an effort for a single contributor, and it would take > some > time to design, implement, and validate thoroughly. > > As an interim solution, I'd propose extending the existing Device1 > interface: > -Adding the new Connected(string bearer) signal, which has no > backward > compatibility risk because it is a completely new signal. > -Extending the Disconnected signal introduced in BlueZ 5.82 with an > additional bearer parameter. Since this signal is very new, it's > unlikely that any application depends on its original signature, so > updating it now should have minimal impact. No. Make those pass a dict of the properties you're exporting. There's really no point in continuing on with a non-extensible pattern like the one already in place, or the one you're proposing. > > Then, as a next step, we can gradually work towards introducing > dedicated Device.LE and Device.BREDR interfaces and migrating to that > design when the community has enough bandwidth and agreement. > > Would this approach be acceptable? > > > Cheers > > > > > > > Signed-off-by: Ye He <ye.he@xxxxxxxxxxx> > > > > --- > > > > doc/org.bluez.Device.rst | 33 > > > > ++++++++++++++++++++++++++++++++- > > > > src/device.c | 34 ++++++++++++++++++++++++++----- > > > > --- > > > > 2 files changed, 58 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/doc/org.bluez.Device.rst > > > > b/doc/org.bluez.Device.rst > > > > index > > > > 646e2c77ec2ddcbf7e6897336165d228c349fe00..1022402d95a1cd34dea88 > > > > 103ba6 > > > > 6fb06f3007de7 100644 > > > > --- a/doc/org.bluez.Device.rst > > > > +++ b/doc/org.bluez.Device.rst > > > > @@ -157,7 +157,26 @@ Possible errors: > > > > Signals > > > > ------- > > > > > > > > -void Disconnected(string reason, string message) > > > > +void Connected(string bearer) > > > > +```````````````````````````````````````````````` > > > > + > > > > +This signal is emitted when a device establishes a connection, > > > > indicating the > > > > +bearer (transport type) over which the connection occurred. > > > > + > > > > +Client applications may use this signal to take actions such > > > > as > > > > stopping discovery > > > > +or advertising, depending on their internal policy. > > > > + > > > > +Possible bearer: > > > > + > > > > +:"le": > > > > + > > > > + LE transport is cconnected. > > > > + > > > > +:"bredr": > > > > + > > > > + BR/EDR transport is connected. > > > > + > > > > +void Disconnected(string reason, string message, string > > > > bearer) > > > > ```````````````````````````````````````````````` > > > > > > > > This signal is launched when a device is disconnected, with > > > > the > > > > reason of the > > > > @@ -208,6 +227,18 @@ Possible reasons: > > > > > > > > Connection terminated by local host for suspend. > > > > > > > > +The additional 'bearer' field indicates which transport was > > > > disconnected. > > > > + > > > > +Possible bearer: > > > > + > > > > +:"le": > > > > + > > > > + LE transport is disconnected. > > > > + > > > > +:"bredr": > > > > + > > > > + BR/EDR transport is disconnected. > > > > + > > > > Properties > > > > ---------- > > > > > > > > diff --git a/src/device.c b/src/device.c > > > > index > > > > 902c4aa44d21eb89076eff3a47ffa727420967a8..ae6196eb2e5b6eca10a8e > > > > 1c3360 > > > > b85023dcddec2 100644 > > > > --- a/src/device.c > > > > +++ b/src/device.c > > > > @@ -3491,8 +3491,10 @@ static const GDBusMethodTable > > > > device_methods[] > > > > = { > > > > }; > > > > > > > > static const GDBusSignalTable device_signals[] = { > > > > + { GDBUS_SIGNAL("Connected", > > > > + GDBUS_ARGS({ "bearer", "s" })) }, > > > > { GDBUS_SIGNAL("Disconnected", > > > > - GDBUS_ARGS({ "name", "s" }, { "message", > > > > "s" > > > > })) }, > > > > + GDBUS_ARGS({ "name", "s" }, { "message", > > > > "s" > > > > }, { "bearer", "s" })) }, > > > > { } > > > > }; > > > > > > > > @@ -3676,6 +3678,7 @@ void device_add_connection(struct > > > > btd_device > > > > *dev, uint8_t bdaddr_type, > > > > uint32_t > > > > flags) > > > > { > > > > struct bearer_state *state = get_state(dev, > > > > bdaddr_type); > > > > + const char *bearer; > > > > > > > > device_update_last_seen(dev, bdaddr_type, true); > > > > device_update_last_used(dev, bdaddr_type); > > > > @@ -3691,14 +3694,22 @@ void device_add_connection(struct > > > > btd_device > > > > *dev, uint8_t bdaddr_type, > > > > dev->conn_bdaddr_type = dev->bdaddr_type; > > > > > > > > /* If this is the first connection over this bearer */ > > > > - if (bdaddr_type == BDADDR_BREDR) > > > > + if (bdaddr_type == BDADDR_BREDR) { > > > > device_set_bredr_support(dev); > > > > - else > > > > + bearer = "bredr"; > > > > + } else { > > > > device_set_le_support(dev, bdaddr_type); > > > > + bearer = "le"; > > > > + } > > > > > > > > state->connected = true; > > > > state->initiator = flags & BIT(3); > > > > > > > > + g_dbus_emit_signal(dbus_conn, dev->path, > > > > DEVICE_INTERFACE, > > > > + "Connected", > > > > + DBUS_TYPE_STRING, &bearer, > > > > + DBUS_TYPE_INVALID); > > > > + > > > > if (dev->le_state.connected && dev- > > > > >bredr_state.connected) > > > > return; > > > > > > > > @@ -3747,7 +3758,7 @@ static void set_temporary_timer(struct > > > > btd_device *dev, unsigned int timeout) > > > > > > > > dev, > > > > NULL); > > > > } > > > > > > > > -static void device_disconnected(struct btd_device *device, > > > > uint8_t > > > > reason) > > > > +static void device_disconnected(struct btd_device *device, > > > > uint8_t > > > > reason, const char *bearer) > > > > { > > > > const char *name; > > > > const char *message; > > > > @@ -3787,6 +3798,7 @@ static void device_disconnected(struct > > > > btd_device *device, uint8_t reason) > > > > "Disconnected", > > > > > > > > DBUS_TYPE_STRING, > > > > &name, > > > > > > > > DBUS_TYPE_STRING, > > > > &message, > > > > + DBUS_TYPE_STRING, > > > > &bearer, > > > > > > > > DBUS_TYPE_INVALID); > > > > } > > > > > > > > @@ -3798,10 +3810,16 @@ void device_remove_connection(struct > > > > btd_device *device, uint8_t bdaddr_type, > > > > DBusMessage *reply; > > > > bool remove_device = false; > > > > bool paired_status_updated = false; > > > > + const char *bearer; > > > > > > > > if (!state->connected) > > > > return; > > > > > > > > + if (bdaddr_type == BDADDR_BREDR) > > > > + bearer = "bredr"; > > > > + else > > > > + bearer = "le"; > > > > + > > > > state->connected = false; > > > > state->initiator = false; > > > > device->general_connect = FALSE; > > > > @@ -3854,15 +3872,15 @@ void device_remove_connection(struct > > > > btd_device *device, uint8_t bdaddr_type, > > > > > > > > DEVICE_INTERFACE, > > > > "Paired"); > > > > > > > > - if (device->bredr_state.connected || device- > > > > > le_state.connected) > > > > - return; > > > > - > > > > device_update_last_seen(device, bdaddr_type, true); > > > > > > > > g_slist_free_full(device->eir_uuids, g_free); > > > > device->eir_uuids = NULL; > > > > > > > > - device_disconnected(device, reason); > > > > + device_disconnected(device, reason, bearer); > > > > + > > > > + if (device->bredr_state.connected || device- > > > > > le_state.connected) > > > > + return; > > > > > > > > g_dbus_emit_property_changed(dbus_conn, device->path, > > > > > > > > DEVICE_INTERFACE, > > > > "Connected"); > > > > > > > > --- > > > > base-commit: dc8db3601001de9a085da063e0c5e456074b8963 > > > > change-id: 20250618-device-bearer-level-conn-state-3e29d56bda88 > > > > > > > > Best regards, > > > > -- > > Luiz Augusto von Dentz >
