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 13:05:27 -0700	[thread overview]
Message-ID: <CABBYNZJBYi_tTHY+j9f=6-tBi-VoCkY9zzVVJDZw126Ocj1E9Q@mail.gmail.com> (raw)
In-Reply-To: <CABBYNZJVLD1gw062NepifuHssKfekk9LHEx+xu5XSX6AwWts_w@mail.gmail.com>

Hi,

On Mon, Oct 18, 2021 at 9:24 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> 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

Pushed after making the suggested changes.

-- 
Luiz Augusto von Dentz

      reply	other threads:[~2021-10-18 20:05 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   ` [PATCH BlueZ 1/7] " Luiz Augusto von Dentz
2021-10-18 20:05     ` Luiz Augusto von Dentz [this message]

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='CABBYNZJBYi_tTHY+j9f=6-tBi-VoCkY9zzVVJDZw126Ocj1E9Q@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).