Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Szymon Janc <szymon.janc@codecoup.pl>
To:     Szymon Czapracki <szymon.czapracki@codecoup.pl>
Cc:     linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v2 1/8] monitor: Update periodic sync commands.
Date:   Thu, 09 Jan 2020 11:39:19 +0100
Message-ID: <526190695.OFPH01XTTy@ix>
Organization: CODECOUP
In-Reply-To: <20200108114752.133076-1-szymon.czapracki@codecoup.pl>
References: <20200108114752.133076-1-szymon.czapracki@codecoup.pl>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Szymon,

On Wednesday, 8 January 2020 12:47:45 CET Szymon Czapracki wrote:
> Change fields in periodic_adv_(create/term)_sync_cmd, according to their
> description with 5.1 specification.
> 
> < HCI Command: LE Periodic Advertising Create Sync (0x08|0x0044) plen 14
>         Options: 0x0001
>         Use Periodic Advertiser List
>         Reporting initially enabled
>         SID: 0x00
>         Adv address type: Public (0x00)
>         Adv address: 00:00:00:00:00:00 (OUI 00-00-00)
>         Skip: 0x0000
>         Sync timeout: 0 msec (0x0000)
>         Sync CTE type: 0x0000
> 
> > HCI Event: Command Status (0x0f) plen 4
> 
>       LE Periodic Advertising Create Sync (0x08|0x0044) ncmd 1
>         Status: Unknown HCI Command (0x01)
> ---
>  monitor/bt.h     |  6 +--
>  monitor/packet.c | 96 ++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 83 insertions(+), 19 deletions(-)
> 
> diff --git a/monitor/bt.h b/monitor/bt.h
> index 8edc895e8..ecf3782c9 100644
> --- a/monitor/bt.h
> +++ b/monitor/bt.h
> @@ -2371,13 +2371,13 @@ struct bt_hci_le_ext_create_conn {
> 
>  #define BT_HCI_CMD_LE_PERIODIC_ADV_CREATE_SYNC		0x2044
>  struct bt_hci_cmd_le_periodic_adv_create_sync {
> -	uint8_t  filter_policy;
> +	uint8_t  options;
>  	uint8_t  sid;
>  	uint8_t  addr_type;
>  	uint8_t  addr[6];
>  	uint16_t skip;
>  	uint16_t sync_timeout;
> -	uint8_t  unused;
> +	uint8_t  sync_cte_type;
>  } __attribute__ ((packed));
> 
>  #define BT_HCI_CMD_LE_PERIODIC_ADV_CREATE_SYNC_CANCEL		0x2045
> @@ -3108,7 +3108,7 @@ struct bt_hci_le_per_adv_report {
>  	uint16_t handle;
>  	uint8_t  tx_power;
>  	int8_t   rssi;
> -	uint8_t  unused;
> +	uint8_t  cte_type;
>  	uint8_t  data_status;
>  	uint8_t  data_len;
>  	uint8_t  data[0];
> diff --git a/monitor/packet.c b/monitor/packet.c
> index ab8bbdee5..64f75cf8e 100644
> --- a/monitor/packet.c
> +++ b/monitor/packet.c
> @@ -7321,24 +7321,70 @@ static void le_ext_create_conn_cmd(const void *data,
> uint8_t size) print_ext_conn_phys(cmd->data, cmd->phys);
>  }
> 
> -static void le_periodic_adv_create_sync_cmd(const void *data, uint8_t size)
> +static const struct bitfield_data create_sync_cte_type[] = {
> +	{  0, "Do not sync to packets with AoA CTE"	},
> +	{  1, "Do not sync to packets with AoD CTE 1us"	},
> +	{  2, "Do not sync to packets with AoD CTE 2us"	},
> +	{  3, "Do not sync to packets with type 3 AoD"	},
> +	{  4, "Do not sync to packets without CTE"	},
> +	{ },
> +};
> +
> +static const struct bitfield_data create_sync_options[] = {
> +	{  0, "Use Periodic Advertiser List"	},
> +	{  1, "Reporting initially disabled"	},
> +	{ },
> +};
> +
> +static const struct bitfield_data create_sync_options_alt[] = {
> +	{  0, "Use advertising SID, Advertiser Address Type and address"},
> +	{  1, "Reporting initially enabled"				
},
> +	{ },
> +};
> +
> +static void print_create_sync_cte_type(uint8_t flags)
>  {
> -	const struct bt_hci_cmd_le_periodic_adv_create_sync *cmd = data;
> -	const char *str;
> +	uint8_t mask = flags;
> 
> -	switch (cmd->filter_policy) {
> -	case 0x00:
> -		str = "Use specified advertising parameters";
> -		break;
> -	case 0x01:
> -		str = "Use Periodic Advertiser List";
> -		break;
> -	default:
> -		str = "Reserved";
> -		break;
> +	print_field("Sync CTE type: 0x%4.4x", flags);
> +
> +	mask = print_bitfield(2, flags, create_sync_cte_type);
> +
> +	if (mask) {
> +		print_text(COLOR_UNKNOWN_ADV_FLAG,
> +				"Unknown sync CTE type properties 
(0x%4.4x)",
> +								
	mask);
>  	}
> +}
> 
> -	print_field("Filter policy: %s (0x%2.2x)", str, cmd->filter_policy);
> +static void print_create_sync_options(uint8_t flags)
> +{
> +	uint8_t mask = flags;
> +	int i;
> +
> +	print_field("Options: 0x%4.4x", flags);
> +
> +	for (i = 0; create_sync_options[i].str; i++) {
> +		if (flags & (1 << create_sync_options[i].bit)) {
> +			print_field("%s", create_sync_options[i].str);
> +			mask  &= ~(1 << create_sync_options[i].bit);
> +		} else {
> +			print_field("%s", 
create_sync_options_alt[i].str);
> +			mask  &= ~(1 << create_sync_options_alt[i].bit);
> +		}
> +	}
> +
> +	if (mask) {
> +		print_text(COLOR_UNKNOWN_ADV_FLAG,
> +					"  Unknown options 
(0x%4.4x)", mask);
> +	}
> +}
> +
> +static void le_periodic_adv_create_sync_cmd(const void *data, uint8_t size)
> +{
> +	const struct bt_hci_cmd_le_periodic_adv_create_sync *cmd = data;
> +
> +	print_create_sync_options(cmd->options);
>  	print_field("SID: 0x%2.2x", cmd->sid);
>  	print_addr_type("Adv address type", cmd->addr_type);
>  	print_addr("Adv address", cmd->addr, cmd->addr_type);
> @@ -7346,7 +7392,7 @@ static void le_periodic_adv_create_sync_cmd(const void
> *data, uint8_t size) print_field("Sync timeout: %d msec (0x%4.4x)",
>  					le16_to_cpu(cmd-
>sync_timeout) * 10,
>  					le16_to_cpu(cmd-
>sync_timeout));
> -	print_field("Unused: 0x%2.2x", cmd->unused);
> +	print_create_sync_cte_type(cmd->sync_cte_type);
>  }
> 
>  static void le_periodic_adv_term_sync_cmd(const void *data, uint8_t size)
> @@ -9648,7 +9694,25 @@ static void le_per_adv_report_evt(const void *data,
> uint8_t size) else
>  		print_field("RSSI: reserved (0x%2.2x)",
>  						(uint8_t) evt-
>rssi);
> -	print_field("Unused: (0x%2.2x)", evt->unused);
> +
> +	switch (evt->cte_type) {
> +	case 0x00:
> +		str = "AoA Constant Tone Extension";
> +		break;
> +	case 0x01:
> +		str = "AoA Constant Tone Extension with 1us slots";
> +		break;
> +	case 0x02:
> +		str = "AoD Constant Tone Extension with 2us slots";
> +		break;
> +	case 0xff:
> +		str = "No Constant Tone Extension";
> +		break;
> +	default:
> +		str = "Reserved";
> +		color_on = COLOR_RED;
> +		break;
> +	}
> 
>  	switch (evt->data_status) {
>  	case 0x00:

All patches in this set are now applied, thanks.
Note that I fixed small typo before pushing (addres -> address)

-- 
pozdrawiam
Szymon Janc