linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Manish Mandlik <mmandlik@google.com>
Cc: "linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>,
	Tedd Ho-Jeong An <hj.tedd.an@gmail.com>
Subject: Re: [PATCH BlueZ 1/7] monitor: Add packet definitions for MSFT extension
Date: Mon, 18 Oct 2021 09:24:16 -0700	[thread overview]
Message-ID: <CABBYNZJVLD1gw062NepifuHssKfekk9LHEx+xu5XSX6AwWts_w@mail.gmail.com> (raw)
In-Reply-To: <CAGPPCLDFYFeiwfiyRX=6PquYYQ-Fp_LpN4Gw2745jyWzQKEBRQ@mail.gmail.com>

Hi Manish,

On Mon, Oct 18, 2021 at 7:53 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Fri, Oct 15, 2021 at 1:09 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This adds proper packet definitions for command and response of MSFT
>> extension.
>> ---
>>  monitor/msft.h | 148 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 148 insertions(+)
>>
>> diff --git a/monitor/msft.h b/monitor/msft.h
>> index a268f4bc7..90a64117a 100644
>> --- a/monitor/msft.h
>> +++ b/monitor/msft.h
>> @@ -24,6 +24,154 @@
>>
>>  #include <stdint.h>
>>
>> +#define MSFT_SUBCMD_READ_SUPPORTED_FEATURES    0x00
>> +
>> +struct msft_cmd_read_supported_features {
>> +       uint8_t subcmd;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_read_supported_features {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +       uint8_t  features[8];
>> +       uint8_t  evt_prefix_len;
>> +       uint8_t  evt_prefix[];
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_MONITOR_RSSI               0x01
>> +
>> +struct msft_cmd_monitor_rssi {
>> +       uint8_t  subcmd;
>> +       uint16_t handle;
>> +       int8_t   rssi_high;
>> +       int8_t   rssi_low;
>> +       uint8_t  rssi_low_interval;
>> +       uint8_t  rssi_period;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_monitor_rssi {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_CANCEL_MONITOR_RSSI                0x02
>> +
>> +struct msft_cmd_cancel_monitor_rssi {
>> +       uint8_t  subcmd;
>> +       uint16_t handle;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_cancel_monitor_rssi {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_LE_MONITOR_ADV             0x03
>> +
>> +struct msft_le_monitor_pattern {
>> +       uint8_t  len;
>> +       uint8_t  type;
>> +       uint8_t  start;
>> +       uint8_t  data[];
>> +} __attribute__((packed));
>> +
>
>
> +    #define MSFT_COND_LE_MONITOR_ADV_PATTERN                0x01
>>
>> +struct msft_le_monitor_adv_pattern_type {
>> +       uint8_t num_patterns;
>> +       struct msft_le_monitor_pattern data[];
>> +} __attribute__((packed));
>> +
>
>
> +    #define MSFT_COND_LE_MONITOR_ADV_UUID                0x02
>>
>> +struct msft_le_monitor_adv_uuid_type {
>> +       uint8_t  type;
>> +       union {
>> +               uint16_t u16;
>> +               uint32_t u32;
>> +               uint8_t  u128[8];
>> +       } value;
>> +} __attribute__((packed));
>> +
>
>
> +   #define MSFT_COND_LE_MONITOR_ADV_IRK                0x03
>>
>> +struct msft_le_monitor_adv_irk_type {
>> +       uint8_t  irk[8];
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_LE_MONITOR_ADV_ADDR                0x04
>
> I think this is not a subcommand. Instead, it is a condition type. So we can rename this to something else, e.g. MSFT_COND_LE_MONITOR_ADV_ADDR.
> Similarly, we'll have to define other three condition types as well for msft_le_monitor_adv_pattern_type, msft_le_monitor_adv_uuid_type and msft_le_monitor_adv_irk_type as mentioned above.

Right I will fix it since the intent was to have it as conditions,
thanks for reviewing.

>> +struct msft_le_monitor_adv_addr {
>> +       uint8_t  type;
>> +       uint8_t  addr[6];
>> +} __attribute__((packed));
>> +
>> +struct msft_cmd_le_monitor_adv {
>> +       uint8_t  subcmd;
>> +       int8_t   rssi_low;
>> +       int8_t   rssi_high;
>
> Order should be:
> +       int8_t   rssi_high;
> +       int8_t   rssi_low;
>>
>> +       uint8_t  rssi_low_interval;
>> +       uint8_t  rssi_period;
>> +       uint8_t  type;
>> +       uint8_t  data[];
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_le_monitor_adv {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +       uint8_t  handle;
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_LE_CANCEL_MONITOR_ADV      0x04
>> +
>> +struct msft_cmd_le_cancel_monitor_adv {
>> +       uint8_t  subcmd;
>> +       uint8_t  handle;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_le_cancel_monitor_adv {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_LE_MONITOR_ADV_ENABLE      0x05
>> +
>> +struct msft_cmd_le_monitor_adv_enable {
>> +       uint8_t  subcmd;
>> +       uint8_t  enable;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_le_monitor_adv_enable {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBCMD_READ_ABS_RSSI              0x06
>> +
>> +struct msft_cmd_read_abs_rssi {
>> +       uint8_t  subcmd;
>> +       uint16_t handle;
>> +} __attribute__((packed));
>> +
>> +struct msft_rsp_read_abs_rssi {
>> +       uint8_t  status;
>> +       uint8_t  subcmd;
>> +       uint16_t handle;
>> +       uint8_t  rssi;

Ack.

> 'int8_t rssi' instead of 'uint8_t rssi'
>
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBEVT_RSSI                       0x01
>> +
>> +struct msft_evt_rssi {
>> +       uint8_t  subevt;
>> +       uint8_t  status;
>> +       uint16_t handle;
>> +       uint8_t  rssi;
>
> same as above - 'int8_t rssi' instead of 'uint8_t rssi'

Ack.

>
>> +} __attribute__((packed));
>> +
>> +#define MSFT_SUBEVT_MONITOR_DEVICE             0x02
>> +
>> +struct msft_evt_monitor_device {
>> +       uint8_t  subevt;
>> +       uint8_t  addr_type;
>> +       uint8_t  addr[6];
>> +       uint8_t  handle;
>> +       uint8_t  state;
>> +} __attribute__((packed));
>> +
>>  struct vendor_ocf;
>>  struct vendor_evt;
>>
>> --
>> 2.31.1
>>
>
> Rest of the changes look good to me.
>
> Thanks,
> Manish.



-- 
Luiz Augusto von Dentz

  parent reply	other threads:[~2021-10-18 16:24 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-15  5:09 Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 2/7] monitor: Make use of MSFT packet definitions Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 3/7] vhci: Read the controller index Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 4/7] vhci: Use io.h instead of mainloop.h Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 5/7] hciemu: Use vhci_open to instanciate a vhci btdev Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 6/7] vhci: Add functions to interface with debugfs Luiz Augusto von Dentz
2021-10-15  5:09 ` [PATCH BlueZ 7/7] mgmt-tester: Make use of vhci_set_force_suspend/vhci_set_force_wakeup Luiz Augusto von Dentz
2021-10-15  5:36 ` [BlueZ,1/7] monitor: Add packet definitions for MSFT extension bluez.test.bot
2021-10-15 20:10   ` Luiz Augusto von Dentz
     [not found] ` <CAGPPCLDFYFeiwfiyRX=6PquYYQ-Fp_LpN4Gw2745jyWzQKEBRQ@mail.gmail.com>
2021-10-18 16:24   ` Luiz Augusto von Dentz [this message]
2021-10-18 20:05     ` [PATCH BlueZ 1/7] " Luiz Augusto von Dentz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABBYNZJVLD1gw062NepifuHssKfekk9LHEx+xu5XSX6AwWts_w@mail.gmail.com \
    --to=luiz.dentz@gmail.com \
    --cc=hj.tedd.an@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=mmandlik@google.com \
    --subject='Re: [PATCH BlueZ 1/7] monitor: Add packet definitions for MSFT extension' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).