Hey Luiz,
Thank you for the quick review!
On Wed, 8 May 2024 at 12:25, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
[...]
> > +// 1 sequence number, 4 for L2CAP header, 2 for SDU, and then 20ms of G.722
> > +#define ASHA_MTU 167
>
> Afaik the L2CAP header is already accounted for, or is it another header?
Good point. I guess both the L2CAP header and the SDU length are
accounted for. I've fixed this to be 161 now (sequence number + data).
[...]
> > + // We need to bind before connect to work around getting the wrong addr
> > + // type on older(?) kernels
>
> Lets use C style comments /* */ instead of //, so please fix that in
> other comments as well.
Done.
[...]
> > + { "Codecs", "q", get_codecs, NULL, NULL,
> > + G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
>
> Codec?
I'm exposing the bitmask from DeviceCapabilities, which is
theoretically a set of codecs. In practice, since we only have one
codec, it will always be 0x2, but I thought I'd stick to the semantics
of the spec.
> > + { "Device", "o", get_device, NULL, NULL,
> > + G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
> > + { "Transport", "o", get_transport, NULL, NULL,
> > + G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
> > + { }
> > +};
> > +
> > +static void asha_source_endpoint_register(struct asha_device *asha)
> > +{
> > + char *path;
> > + const struct media_endpoint *asha_ep;
> > +
> > + path = make_endpoint_path(asha);
> > + if (!path)
> > + goto error;
> > +
> > + if (g_dbus_register_interface(btd_get_dbus_connection(),
> > + path, MEDIA_ENDPOINT_INTERFACE,
> > + asha_ep_methods, NULL,
> > + asha_ep_properties,
> > + asha, NULL) == FALSE) {
> > + error("Could not register remote ep %s", path);
> > + goto error;
> > + }
> > +
> > + asha_ep = media_endpoint_get_asha();
> > + asha->transport = media_transport_create(asha->device, path, NULL, 0,
> > + (void *) asha_ep, asha);
> > +
> > +error:
> > + if (path)
> > + free(path);
> > + return;
> > +}
> > +
> > +static void asha_source_endpoint_unregister(struct asha_device *asha)
> > +{
> > + char *path;
> > +
> > + path = make_endpoint_path(asha);
> > + if (!path)
> > + goto error;
> > +
> > + g_dbus_unregister_interface(btd_get_dbus_connection(),
> > + path, MEDIA_ENDPOINT_INTERFACE);
> > +
> > + if (asha->transport) {
> > + media_transport_destroy(asha->transport);
> > + asha->transport = NULL;
> > + }
> > +
> > +error:
> > + if (path)
> > + free(path);
> > +}
> > +
> > +static int asha_source_accept(struct btd_service *service)
> > +{
> > + struct btd_device *device = btd_service_get_device(service);
> > + struct gatt_db *db = btd_device_get_gatt_db(device);
> > + struct bt_gatt_client *client = btd_device_get_gatt_client(device);
> > + struct asha_device *asha = btd_service_get_user_data(service);
> > + bt_uuid_t asha_uuid;
> > + char addr[18];
> > +
> > + ba2str(device_get_address(device), addr);
> > + DBG("Accepting ASHA connection on %s", addr);
> > +
> > + if (!asha) {
> > + // Can this actually happen?
> > + DBG("Not handling ASHA profile");
> > + return -1;
> > + }
> > +
> > + asha->db = gatt_db_ref(db);
> > + asha->client = bt_gatt_client_clone(client);
> > +
> > + bt_uuid16_create(&asha_uuid, ASHA_SERVICE);
> > + gatt_db_foreach_service(db, &asha_uuid, foreach_asha_service, asha);
> > +
> > + if (!asha->attr) {
> > + error("ASHA attribute not found");
> > + asha_device_reset(asha);
> > + return -1;
> > + }
> > +
> > + asha_source_endpoint_register(asha);
> > +
> > + btd_service_connecting_complete(service, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int asha_source_disconnect(struct btd_service *service)
> > +{
> > + struct btd_device *device = btd_service_get_device(service);
> > + struct gatt_db *db = btd_device_get_gatt_db(device);
> > + struct bt_gatt_client *client = btd_device_get_gatt_client(device);
> > + struct asha_device *asha = btd_service_get_user_data(service);
> > + bt_uuid_t asha_uuid;
> > + char addr[18];
> > +
> > + ba2str(device_get_address(device), addr);
> > + DBG("Disconnecting ASHA on %s", addr);
> > +
> > + if (!asha) {
> > + // Can this actually happen?
> > + DBG("Not handlihng ASHA profile");
> > + return -1;
> > + }
> > +
> > + asha_source_endpoint_unregister(asha);
> > + asha_device_reset(asha);
> > +
> > + btd_service_disconnecting_complete(service, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static struct btd_profile asha_source_profile = {
> > + .name = "asha-source",
> > + .priority = BTD_PROFILE_PRIORITY_MEDIUM,
> > + .remote_uuid = ASHA_PROFILE_UUID,
> > + .experimental = true,
> > +
> > + .device_probe = asha_source_device_probe,
> > + .device_remove = asha_source_device_remove,
> > +
> > + .auto_connect = true,
> > + .accept = asha_source_accept,
> > + .disconnect = asha_source_disconnect,
> > +};
> > +
> > +static int asha_init(void)
> > +{
> > + int err;
> > +
> > + err = btd_profile_register(&asha_source_profile);
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +static void asha_exit(void)
> > +{
> > + btd_profile_unregister(&asha_source_profile);
> > +}
> > +
> > +BLUETOOTH_PLUGIN_DEFINE(asha, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
> > + asha_init, asha_exit)
> > diff --git a/profiles/audio/asha.h b/profiles/audio/asha.h
> > new file mode 100644
> > index 000000000..0fc28e8a3
> > --- /dev/null
> > +++ b/profiles/audio/asha.h
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *
> > + * BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + * Copyright (C) 2024 Asymptotic Inc.
> > + *
> > + * Author: Arun Raghavan <arun@xxxxxxxxxxxxx>
> > + *
> > + *
> > + */
> > +
> > +#include <stdint.h>
> > +
> > +struct asha_device;
> > +
> > +typedef enum {
> > + ASHA_STOPPED = 0,
> > + ASHA_STARTING,
> > + ASHA_STARTED,
> > + ASHA_STOPPING,
> > +} asha_state_t;
> > +
> > +typedef void (*asha_cb_t)(int status, void *data);
> > +
> > +uint16_t asha_device_get_render_delay(struct asha_device *asha);
> > +asha_state_t asha_device_get_state(struct asha_device *asha);
> > +int asha_device_get_fd(struct asha_device *asha);
> > +uint16_t asha_device_get_mtu(struct asha_device *asha);
> > +
> > +unsigned int asha_device_start(struct asha_device *asha, asha_cb_t cb,
> > + void *user_data);
> > +unsigned int asha_device_stop(struct asha_device *asha, asha_cb_t cb,
> > + void *user_data);
>
> I'd suggest we split the protocol portion from the plugin and put it
> under src/shared so we can do unit testing without the D-Bus blocks,
> Id got with bt_asha for instance name, we can do it later if you don't
> feel like it is necessary right now.
Ah, I did not follow the reason for that split previously. Done now.
[...]
> Otherwise it looks pretty good.
Thank you for the quick review! I'll send out a v2 with these fixes
and whatever the linter caught.
Cheers,
Arun
