Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200717144007.3160721-1-alainm@chromium.org>
In-Reply-To: <20200717144007.3160721-1-alainm@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 17 Jul 2020 13:37:37 -0700
Message-ID: <CABBYNZ+VR5cNuVpTmJo0JTmxnyiwugQwpN5C8UOExu47OYJ3mA@mail.gmail.com>
Subject: Re: [Bluez PATCH v2] gatt: Support DeviceInfo Service when vid/pid is specified
To:     Alain Michaud <alainm@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain,

On Fri, Jul 17, 2020 at 7:43 AM Alain Michaud <alainm@chromium.org> wrote:
>
> This patch adds support for the PNPID characteristic when configured in
> main.conf.
>
> This was validated as read correclty both by manually reading the valud
> and confirming in the Ellisys Analyzer.
>
> ATT Read (PnP ID: Source=Bluetooth ID, Vendor=224, Product=50181,
> ATT Read Response Packet (Source=Bluetooth ID, Vendor=224,
> Product=50181, Version=86)     | OK     | 7 bytes (01 E0 00 05 C4 56 00)
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> I admit I don't know how to quantify the compatibility risk with adding
> the Device Info Service if the DeviceID is specified.  I can see that
> some system may be configured with an app to publish the DIS and this
> may break it.
>
> If the community feels it is necessary, I can include a DeviceIdOverLE
> configuration which defaults to false in main.conf to address this
> compatibility risk.
>
> Changes in v2:
>  - Removing file I didn't intend to commit (peripheral/gatt.c)
>
>  src/gatt-database.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 95ba39897..07d567078 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -57,6 +57,7 @@
>
>  #define UUID_GAP       0x1800
>  #define UUID_GATT      0x1801
> +#define UUID_DIS       0x180a
>
>  #ifndef MIN
>  #define MIN(a, b) ((a) < (b) ? (a) : (b))
> @@ -1233,11 +1234,51 @@ static void populate_gatt_service(struct btd_gatt_database *database)
>         database_add_record(database, service);
>  }
>
> +static void device_info_read_pnp_id_cb(struct gatt_db_attribute *attrib,
> +                                       unsigned int id, uint16_t offset,
> +                                       uint8_t opcode, struct bt_att *att,
> +                                       void *user_data)
> +{
> +       uint8_t pdu[7];
> +
> +       pdu[0] = main_opts.did_source;
> +       put_le16(main_opts.did_vendor, &pdu[1]);
> +       put_le16(main_opts.did_product, &pdu[3]);
> +       put_le16(main_opts.did_version, &pdu[5]);
> +
> +       gatt_db_attribute_read_result(attrib, id, 0, pdu, sizeof(pdu));
> +}
> +
> +static void populate_devinfo_service(struct btd_gatt_database *database)
> +{
> +       struct gatt_db_attribute *service;
> +       bt_uuid_t uuid;
> +
> +       bt_uuid16_create(&uuid, UUID_DIS);
> +       service = gatt_db_add_service(database->db, &uuid, true, 3);
> +
> +       if (main_opts.did_source > 0) {
> +               bt_uuid16_create(&uuid, GATT_CHARAC_PNP_ID);
> +               gatt_db_service_add_characteristic(service, &uuid,
> +                                               BT_ATT_PERM_READ,
> +                                               BT_GATT_CHRC_PROP_READ,
> +                                               device_info_read_pnp_id_cb,
> +                                               NULL, database);
> +       }
> +
> +       gatt_db_service_set_active(service, true);
> +
> +       database_add_record(database, service);
> +}
>
>  static void register_core_services(struct btd_gatt_database *database)
>  {
>         populate_gap_service(database);
>         populate_gatt_service(database);
> +
> +       if (main_opts.did_source > 0)
> +               populate_devinfo_service(database);
> +
>  }
>
>  static void conf_cb(void *user_data)
> --
> 2.28.0.rc0.105.gf9edc3c819-goog

Applied, thanks.

-- 
Luiz Augusto von Dentz