[BlueZ PATCH v2 1/5] monitor: Fix median packet size

[BlueZ PATCH v2 1/5] monitor: Fix median packet size

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Calculating the median packet based on the current median + size / 2
does not account for last packet could smaller e.g. opp transfer could
end with just 1 byte which would cut the median in a half, so this
switch to more traditional means of calculating by doing total bytes
sent / num of packets so each an every packet has the same weight.
---
 monitor/analyze.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index 839c2f7e9..5e0957ad1 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -61,6 +61,7 @@ struct hci_conn {
 	unsigned long rx_num;
 	unsigned long tx_num;
 	unsigned long tx_num_comp;
+	size_t tx_bytes;
 	struct timeval last_tx;
 	struct timeval last_tx_comp;
 	struct timeval tx_lat_min;
@@ -99,6 +100,8 @@ static void conn_destroy(void *data)
 		break;
 	}
 
+	conn->tx_pkt_med = conn->tx_bytes / conn->tx_num;
+
 	printf("  Found %s connection with handle %u\n", str, conn->handle);
 	printf("    BD_ADDR %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
 			conn->bdaddr[5], conn->bdaddr[4], conn->bdaddr[3],
@@ -485,16 +488,12 @@ static void acl_pkt(struct timeval *tv, uint16_t index, bool out,
 	if (out) {
 		conn->tx_num++;
 		conn->last_tx = *tv;
+		conn->tx_bytes += size;
 
 		if (!conn->tx_pkt_min || size < conn->tx_pkt_min)
 			conn->tx_pkt_min = size;
 		if (!conn->tx_pkt_max || size > conn->tx_pkt_max)
 			conn->tx_pkt_max = size;
-		if (conn->tx_pkt_med) {
-			conn->tx_pkt_med += (size + 1);
-			conn->tx_pkt_med /= 2;
-		} else
-			conn->tx_pkt_med = size;
 	} else {
 		conn->rx_num++;
 	}
-- 
2.31.1

[BlueZ PATCH v2 2/5] monitor: Fix minimun packet latency

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

It seems timer_sub can produce negative values leading to median packet
latency to be negative e.g conn->last_tx_compl is ahead of
conn->last_tx, in which case it should be discarded.
---
 monitor/analyze.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index 5e0957ad1..d504c8d84 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -386,8 +386,9 @@ static void evt_num_completed_packets(struct hci_dev *dev, struct timeval *tv,
 		if (timerisset(&conn->last_tx)) {
 			timersub(&conn->last_tx_comp, &conn->last_tx, &res);
 
-			if (!timerisset(&conn->tx_lat_min) ||
-					timercmp(&res, &conn->tx_lat_min, <))
+			if ((!timerisset(&conn->tx_lat_min) ||
+					timercmp(&res, &conn->tx_lat_min, <)) &&
+					res.tv_sec >= 0 && res.tv_usec >= 0)
 				conn->tx_lat_min = res;
 
 			if (!timerisset(&conn->tx_lat_max) ||
@@ -408,6 +409,8 @@ static void evt_num_completed_packets(struct hci_dev *dev, struct timeval *tv,
 						tmp.tv_usec -= 1000000;
 					}
 				}
+
+				conn->tx_lat_med = tmp;
 			} else
 				conn->tx_lat_med = res;
 
-- 
2.31.1

[BlueZ PATCH v2 3/5] monitor: Fix not accouting for multiple outstanding packets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Analyze code was not accounting for the fact that multiple outstanding
packets could be pending which will cause the last_tx to be overwritten
but its latency would be calculated against the very first packet
complete.
---
 monitor/analyze.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index d504c8d84..aae153f94 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -62,8 +62,7 @@ struct hci_conn {
 	unsigned long tx_num;
 	unsigned long tx_num_comp;
 	size_t tx_bytes;
-	struct timeval last_tx;
-	struct timeval last_tx_comp;
+	struct queue *tx_queue;
 	struct timeval tx_lat_min;
 	struct timeval tx_lat_max;
 	struct timeval tx_lat_med;
@@ -121,6 +120,7 @@ static void conn_destroy(void *data)
 	printf("    %u octets TX max packet size\n", conn->tx_pkt_max);
 	printf("    %u octets TX median packet size\n", conn->tx_pkt_med);
 
+	queue_destroy(conn->tx_queue, free);
 	free(conn);
 }
 
@@ -133,6 +133,7 @@ static struct hci_conn *conn_alloc(struct hci_dev *dev, uint16_t handle,
 
 	conn->handle = handle;
 	conn->type = type;
+	conn->tx_queue = queue_new();
 
 	return conn;
 }
@@ -372,6 +373,7 @@ static void evt_num_completed_packets(struct hci_dev *dev, struct timeval *tv,
 		uint16_t count = get_le16(data + 2);
 		struct hci_conn *conn;
 		struct timeval res;
+		struct timeval *last_tx;
 
 		data += 4;
 		size -= 4;
@@ -381,10 +383,10 @@ static void evt_num_completed_packets(struct hci_dev *dev, struct timeval *tv,
 			continue;
 
 		conn->tx_num_comp += count;
-		conn->last_tx_comp = *tv;
 
-		if (timerisset(&conn->last_tx)) {
-			timersub(&conn->last_tx_comp, &conn->last_tx, &res);
+		last_tx = queue_pop_head(conn->tx_queue);
+		if (last_tx) {
+			timersub(tv, last_tx, &res);
 
 			if ((!timerisset(&conn->tx_lat_min) ||
 					timercmp(&res, &conn->tx_lat_min, <)) &&
@@ -414,7 +416,7 @@ static void evt_num_completed_packets(struct hci_dev *dev, struct timeval *tv,
 			} else
 				conn->tx_lat_med = res;
 
-			timerclear(&conn->last_tx);
+			free(last_tx);
 		}
 	}
 }
@@ -489,8 +491,12 @@ static void acl_pkt(struct timeval *tv, uint16_t index, bool out,
 		return;
 
 	if (out) {
+		struct timeval *last_tx;
+
 		conn->tx_num++;
-		conn->last_tx = *tv;
+		last_tx = new0(struct timeval, 1);
+		memcpy(last_tx, tv, sizeof(*tv));
+		queue_push_tail(conn->tx_queue, last_tx);
 		conn->tx_bytes += size;
 
 		if (!conn->tx_pkt_min || size < conn->tx_pkt_min)
-- 
2.31.1

[BlueZ PATCH v2 4/5] monitor: Make --analyze output latencies in msec

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Milisecconds is probably the best unit to have since it is unlikely that
the controller can respond in under 1 msec as well as most time
sensitive connection e.g. A2DP, HFP, etc, also don't expect the
latencies to be over 1 sec.
---
 monitor/analyze.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index aae153f94..bee05f467 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -110,12 +110,15 @@ static void conn_destroy(void *data)
 	printf("    %lu RX packets\n", conn->rx_num);
 	printf("    %lu TX packets\n", conn->tx_num);
 	printf("    %lu TX completed packets\n", conn->tx_num_comp);
-	printf("    %ld.%06ld seconds min latency\n",
-			conn->tx_lat_min.tv_sec, conn->tx_lat_min.tv_usec);
-	printf("    %ld.%06ld seconds max latency\n",
-			conn->tx_lat_max.tv_sec, conn->tx_lat_max.tv_usec);
-	printf("    %ld.%06ld seconds median latency\n",
-			conn->tx_lat_med.tv_sec, conn->tx_lat_med.tv_usec);
+	printf("    %ld msec min latency\n",
+			conn->tx_lat_min.tv_sec * 1000 +
+			conn->tx_lat_min.tv_usec / 1000);
+	printf("    %ld msec max latency\n",
+			conn->tx_lat_max.tv_sec * 1000 +
+			conn->tx_lat_max.tv_usec / 1000);
+	printf("    %ld msec median latency\n",
+			conn->tx_lat_med.tv_sec * 1000 +
+			conn->tx_lat_med.tv_usec / 1000);
 	printf("    %u octets TX min packet size\n", conn->tx_pkt_min);
 	printf("    %u octets TX max packet size\n", conn->tx_pkt_max);
 	printf("    %u octets TX median packet size\n", conn->tx_pkt_med);
-- 
2.31.1

[BlueZ PATCH v2 5/5] monitor: Make --analyze print address OUI

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This makes analyze.c use packet_print_addr which does take care of
decoding OUI portion of the address.
---
 monitor/analyze.c  | 30 ++++++++++++-----------
 monitor/broadcom.c |  2 +-
 monitor/intel.c    | 10 ++++----
 monitor/l2cap.c    | 60 +---------------------------------------------
 monitor/lmp.c      |  2 +-
 monitor/packet.c   |  9 ++++---
 monitor/packet.h   |  2 +-
 7 files changed, 29 insertions(+), 86 deletions(-)

diff --git a/monitor/analyze.c b/monitor/analyze.c
index bee05f467..4755c6ca6 100644
--- a/monitor/analyze.c
+++ b/monitor/analyze.c
@@ -13,6 +13,7 @@
 #include <config.h>
 #endif
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <string.h>
 #include <sys/time.h>
@@ -23,7 +24,9 @@
 #include "src/shared/queue.h"
 #include "src/shared/btsnoop.h"
 #include "monitor/bt.h"
-#include "analyze.h"
+#include "monitor/display.h"
+#include "monitor/packet.h"
+#include "monitor/analyze.h"
 
 struct hci_dev {
 	uint16_t index;
@@ -102,26 +105,25 @@ static void conn_destroy(void *data)
 	conn->tx_pkt_med = conn->tx_bytes / conn->tx_num;
 
 	printf("  Found %s connection with handle %u\n", str, conn->handle);
-	printf("    BD_ADDR %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
-			conn->bdaddr[5], conn->bdaddr[4], conn->bdaddr[3],
-			conn->bdaddr[2], conn->bdaddr[1], conn->bdaddr[0]);
+	/* TODO: Store address type */
+	packet_print_addr("Address", conn->bdaddr, 0x00);
 	if (!conn->setup_seen)
-		printf("    Connection setup missing\n");
-	printf("    %lu RX packets\n", conn->rx_num);
-	printf("    %lu TX packets\n", conn->tx_num);
-	printf("    %lu TX completed packets\n", conn->tx_num_comp);
-	printf("    %ld msec min latency\n",
+		print_field("Connection setup missing");
+	print_field("%lu RX packets", conn->rx_num);
+	print_field("%lu TX packets", conn->tx_num);
+	print_field("%lu TX completed packets", conn->tx_num_comp);
+	print_field("%ld msec min latency",
 			conn->tx_lat_min.tv_sec * 1000 +
 			conn->tx_lat_min.tv_usec / 1000);
-	printf("    %ld msec max latency\n",
+	print_field("%ld msec max latency",
 			conn->tx_lat_max.tv_sec * 1000 +
 			conn->tx_lat_max.tv_usec / 1000);
-	printf("    %ld msec median latency\n",
+	print_field("%ld msec median latency",
 			conn->tx_lat_med.tv_sec * 1000 +
 			conn->tx_lat_med.tv_usec / 1000);
-	printf("    %u octets TX min packet size\n", conn->tx_pkt_min);
-	printf("    %u octets TX max packet size\n", conn->tx_pkt_max);
-	printf("    %u octets TX median packet size\n", conn->tx_pkt_med);
+	print_field("%u octets TX min packet size", conn->tx_pkt_min);
+	print_field("%u octets TX max packet size", conn->tx_pkt_max);
+	print_field("%u octets TX median packet size", conn->tx_pkt_med);
 
 	queue_destroy(conn->tx_queue, free);
 	free(conn);
diff --git a/monitor/broadcom.c b/monitor/broadcom.c
index b144032c3..5ee4ba04a 100644
--- a/monitor/broadcom.c
+++ b/monitor/broadcom.c
@@ -233,7 +233,7 @@ static void status_rsp(const void *data, uint8_t size)
 
 static void write_bd_addr_cmd(const void *data, uint8_t size)
 {
-	packet_print_addr("Address", data, false);
+	packet_print_addr("Address", data, 0x00);
 }
 
 static void update_uart_baud_rate_cmd(const void *data, uint8_t size)
diff --git a/monitor/intel.c b/monitor/intel.c
index e9984bfe3..728bff587 100644
--- a/monitor/intel.c
+++ b/monitor/intel.c
@@ -233,7 +233,7 @@ static void print_version_tlv_min_fw(const struct intel_version_tlv *tlv,
 static void print_version_tlv_otp_bdaddr(const struct intel_version_tlv *tlv,
 					 char *type_str)
 {
-	packet_print_addr(type_str, tlv->val, false);
+	packet_print_addr(type_str, tlv->val, 0x00);
 }
 
 static void print_version_tlv_unknown(const struct intel_version_tlv *tlv,
@@ -535,7 +535,7 @@ static void write_bd_data_cmd(const void *data, uint8_t size)
 {
 	uint8_t features[8];
 
-	packet_print_addr("Address", data, false);
+	packet_print_addr("Address", data, 0x00);
 	packet_hexdump(data + 6, 6);
 
 	memcpy(features, data + 12, 8);
@@ -553,13 +553,13 @@ static void read_bd_data_rsp(const void *data, uint8_t size)
 	uint8_t status = get_u8(data);
 
 	print_status(status);
-	packet_print_addr("Address", data + 1, false);
+	packet_print_addr("Address", data + 1, 0x00);
 	packet_hexdump(data + 7, size - 7);
 }
 
 static void write_bd_address_cmd(const void *data, uint8_t size)
 {
-	packet_print_addr("Address", data, false);
+	packet_print_addr("Address", data, 0x00);
 }
 
 static void act_deact_traces_cmd(const void *data, uint8_t size)
@@ -1119,7 +1119,7 @@ static void sco_rejected_via_lmp_evt(const void *data, uint8_t size)
 {
 	uint8_t reason = get_u8(data + 6);
 
-	packet_print_addr("Address", data, false);
+	packet_print_addr("Address", data, 0x00);
 	packet_print_error("Reason", reason);
 }
 
diff --git a/monitor/l2cap.c b/monitor/l2cap.c
index f16f82532..083e53561 100644
--- a/monitor/l2cap.c
+++ b/monitor/l2cap.c
@@ -2749,63 +2749,6 @@ static void att_packet(uint16_t index, bool in, uint16_t handle,
 	opcode_data->func(&frame);
 }
 
-static void print_addr(const uint8_t *addr, uint8_t addr_type)
-{
-	const char *str;
-
-	switch (addr_type) {
-	case 0x00:
-		print_field("Address: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
-						addr[5], addr[4], addr[3],
-						addr[2], addr[1], addr[0]);
-		break;
-	case 0x01:
-		switch ((addr[5] & 0xc0) >> 6) {
-		case 0x00:
-			str = "Non-Resolvable";
-			break;
-		case 0x01:
-			str = "Resolvable";
-			break;
-		case 0x03:
-			str = "Static";
-			break;
-		default:
-			str = "Reserved";
-			break;
-		}
-
-		print_field("Address: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X"
-					" (%s)", addr[5], addr[4], addr[3],
-					addr[2], addr[1], addr[0], str);
-		break;
-	default:
-		print_field("Address: %2.2X-%2.2X-%2.2X-%2.2X-%2.2X-%2.2X",
-						addr[5], addr[4], addr[3],
-						addr[2], addr[1], addr[0]);
-		break;
-	}
-}
-
-static void print_addr_type(uint8_t addr_type)
-{
-	const char *str;
-
-	switch (addr_type) {
-	case 0x00:
-		str = "Public";
-		break;
-	case 0x01:
-		str = "Random";
-		break;
-	default:
-		str = "Reserved";
-		break;
-	}
-
-	print_field("Address type: %s (0x%2.2x)", str, addr_type);
-}
-
 static void print_smp_io_capa(uint8_t io_capa)
 {
 	const char *str;
@@ -3038,8 +2981,7 @@ static void smp_ident_addr_info(const struct l2cap_frame *frame)
 {
 	const struct bt_l2cap_smp_ident_addr_info *pdu = frame->data;
 
-	print_addr_type(pdu->addr_type);
-	print_addr(pdu->addr, pdu->addr_type);
+	packet_print_addr("Address", pdu->addr, pdu->addr_type);
 
 	keys_update_identity_addr(pdu->addr, pdu->addr_type);
 }
diff --git a/monitor/lmp.c b/monitor/lmp.c
index 0d17f5c98..037adf2e2 100644
--- a/monitor/lmp.c
+++ b/monitor/lmp.c
@@ -310,7 +310,7 @@ static void slot_offset(const void *data, uint8_t size)
 	const struct bt_lmp_slot_offset *pdu = data;
 
 	print_field("Offset: %u usec", le16_to_cpu(pdu->offset));
-	packet_print_addr("Address", pdu->bdaddr, false);
+	packet_print_addr("Address", pdu->bdaddr, 0x00);
 }
 
 static void page_scan_mode_req(const void *data, uint8_t size)
diff --git a/monitor/packet.c b/monitor/packet.c
index 025b7e9b0..9e849acc0 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -713,10 +713,9 @@ static void print_addr_resolve(const char *label, const uint8_t *addr,
 	}
 }
 
-static void print_addr(const char *label, const uint8_t *addr,
-						uint8_t addr_type)
+static void print_addr(const char *label, const uint8_t *addr, uint8_t type)
 {
-	print_addr_resolve(label, addr, addr_type, true);
+	print_addr_resolve(label, addr, type, true);
 }
 
 static void print_bdaddr(const uint8_t *bdaddr)
@@ -3780,9 +3779,9 @@ static void print_eir(const uint8_t *eir, uint8_t eir_len, bool le)
 		packet_hexdump(eir, eir_len - len);
 }
 
-void packet_print_addr(const char *label, const void *data, bool random)
+void packet_print_addr(const char *label, const void *data, uint8_t type)
 {
-	print_addr(label ? : "Address", data, random ? 0x01 : 0x00);
+	print_addr(label ? : "Address", data, type);
 }
 
 void packet_print_handle(uint16_t handle)
diff --git a/monitor/packet.h b/monitor/packet.h
index f859fa945..34f988e86 100644
--- a/monitor/packet.h
+++ b/monitor/packet.h
@@ -37,7 +37,7 @@ void packet_print_error(const char *label, uint8_t error);
 void packet_print_version(const char *label, uint8_t version,
 				const char *sublabel, uint16_t subversion);
 void packet_print_company(const char *label, uint16_t company);
-void packet_print_addr(const char *label, const void *data, bool random);
+void packet_print_addr(const char *label, const void *data, uint8_t type);
 void packet_print_handle(uint16_t handle);
 void packet_print_rssi(const char *label, int8_t rssi);
 void packet_print_ad(const void *data, uint8_t size);
-- 
2.31.1

RE: [BlueZ,v2,1/5] monitor: Fix median packet size

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=528765

---Test result---

Test Summary:
CheckPatch                    PASS      1.41 seconds
GitLint                       PASS      0.65 seconds
Prep - Setup ELL              PASS      50.18 seconds
Build - Prep                  PASS      0.14 seconds
Build - Configure             PASS      8.71 seconds
Build - Make                  PASS      221.35 seconds
Make Check                    PASS      9.39 seconds
Make Distcheck                PASS      260.28 seconds
Build w/ext ELL - Configure   PASS      8.99 seconds
Build w/ext ELL - Make        PASS      210.04 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth