[PATCH 1/8] doc: document AccessPointDiagnostic interface

[PATCH 1/8] doc: document AccessPointDiagnostic interface

[James Prestwood]

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

---
 doc/access-point-diagnostics.txt | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 doc/access-point-diagnostics.txt

diff --git a/doc/access-point-diagnostics.txt b/doc/access-point-diagnostics.txt
new file mode 100644
index 00000000..d28dd6a9
--- /dev/null
+++ b/doc/access-point-diagnostics.txt
@@ -0,0 +1,36 @@
+Access Point diagnostic hierarchy [experimental]
+============================
+
+Service		net.connman.iwd
+Interface	net.connman.iwd.AccessPointDiagnostic
+Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
+
+Methods		dict[] GetDiagnostics()
+
+			Get all diagnostic information for this access points
+			connected clients. This will return an array of
+			dictionaries, each corresponding to an individual
+			client. Below is a list of possible diagnostic
+			dictionary values:
+
+			Address - The station's MAC address
+
+			RSSI [optional] - The RSSI of the currently connected BSS.
+
+			RxMode [optional] -	The phy technology being used
+						(802.11n, 802.11ac or 802.11ax).
+
+			RxRate [optional] - Receive rate in 100kbit/s
+
+			RxMCS [optional] - Receiving MCS index
+
+			TxMode [optional] -	Same meaning as RxMode, just for
+						transmission.
+
+			TxRate [optional] - Transmission rate in 100kbit/s
+
+			TxMCS [optional] - Transmitting MCS index
+
+			Possible errors: net.connman.iwd.Failed
+					 net.connman.iwd.NotConnected
+					 net.connman.iwd.NotFound
-- 
2.26.2

[PATCH 2/8] netdev: add netdev_get_all_stations

[James Prestwood]

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

This is a nl80211 dump version of netdev_get_station aimed at
AP mode. This will dump all stations, parse into
netdev_station_info structs, and call the callback for each
individual station found. Once the dump is completed the destroy
callback is called.
---
 src/netdev.c | 26 ++++++++++++++++++++++++++
 src/netdev.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/src/netdev.c b/src/netdev.c
index 0fc0600a..e3ce270a 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -4240,6 +4240,32 @@ int netdev_get_current_station(struct netdev *netdev,
 					user_data, destroy);
 }
 
+int netdev_get_all_stations(struct netdev *netdev, netdev_get_station_cb_t cb,
+				void *user_data, netdev_destroy_func_t destroy)
+{
+	struct l_genl_msg *msg;
+
+	if (netdev->get_station_cmd_id)
+		return -EBUSY;
+
+	msg = l_genl_msg_new_sized(NL80211_CMD_GET_STATION, 64);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &netdev->index);
+
+	netdev->get_station_cmd_id = l_genl_family_dump(nl80211, msg,
+						netdev_get_station_cb, netdev,
+						netdev_get_station_destroy);
+	if (!netdev->get_station_cmd_id) {
+		l_genl_msg_unref(msg);
+		return -EIO;
+	}
+
+	netdev->get_station_cb = cb;
+	netdev->get_station_data = user_data;
+	netdev->get_station_destroy = destroy;
+
+	return 0;
+}
+
 static int netdev_cqm_rssi_update(struct netdev *netdev)
 {
 	struct l_genl_msg *msg;
diff --git a/src/netdev.h b/src/netdev.h
index 074dc61f..e7a1c060 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -211,6 +211,8 @@ int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
 int netdev_get_current_station(struct netdev *netdev,
 			netdev_get_station_cb_t cb, void *user_data,
 			netdev_destroy_func_t destroy);
+int netdev_get_all_stations(struct netdev *netdev, netdev_get_station_cb_t cb,
+				void *user_data, netdev_destroy_func_t destroy);
 
 void netdev_handshake_failed(struct handshake_state *hs, uint16_t reason_code);
 
-- 
2.26.2

[PATCH 3/8] dbus: add AccessPointDiagnostic interface

[James Prestwood]

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

---
 src/dbus.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dbus.h b/src/dbus.h
index 93055bf6..f98ade7a 100644
--- a/src/dbus.h
+++ b/src/dbus.h
@@ -40,6 +40,7 @@
 #define IWD_P2P_SERVICE_MANAGER_INTERFACE "net.connman.iwd.p2p.ServiceManager"
 #define IWD_P2P_WFD_INTERFACE "net.connman.iwd.p2p.Display"
 #define IWD_STATION_DIAGNOSTIC_INTERFACE "net.connman.iwd.StationDiagnostic"
+#define IWD_AP_DIAGNOSTIC_INTERFACE "net.connman.iwd.AccessPointDiagnostic"
 
 #define IWD_BASE_PATH "/net/connman/iwd"
 #define IWD_AGENT_MANAGER_PATH IWD_BASE_PATH
-- 
2.26.2

[PATCH 4/8] ap: add AP diagnostic interface

[James Prestwood]

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

This adds a new AccessPointDiagnostic interface. This interface
provides similar low level functionality as StationDiagnostic, but
for when IWD is in AP mode. This uses netdev_get_all_stations
which will dump all stations, parse, and return each station in
an individual callback. Once the dump is complete the destroy is
called and all data is packaged as an array of dictionaries.
---
 src/ap.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index d9e7e404..6b3fb129 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2963,6 +2963,155 @@ static void ap_destroy_interface(void *user_data)
 	l_free(ap_if);
 }
 
+struct diagnostic_data {
+	struct l_dbus_message *pending;
+	struct l_dbus_message_builder *builder;
+};
+
+static void ap_get_station_cb(const struct netdev_station_info *info,
+				void *user_data)
+{
+	struct diagnostic_data *data = user_data;
+	int16_t rssi;
+
+	/* First station info */
+	if (!data->builder) {
+		struct l_dbus_message *reply =
+				l_dbus_message_new_method_return(data->pending);
+
+		data->builder = l_dbus_message_builder_new(reply);
+
+		l_dbus_message_builder_enter_array(data->builder, "a{sv}");
+	}
+
+	l_dbus_message_builder_enter_array(data->builder, "{sv}");
+	dbus_append_dict_basic(data->builder, "Address", 's',
+					util_address_to_string(info->addr));
+
+	rssi = (int16_t)info->cur_rssi;
+
+	if (info->have_cur_rssi)
+		dbus_append_dict_basic(data->builder, "RSSI", 'n', &rssi);
+
+	if (info->have_rx_mcs) {
+		switch (info->rx_mcs_type) {
+		case NETDEV_MCS_TYPE_HT:
+			dbus_append_dict_basic(data->builder, "RxMode", 's',
+						"802.11n");
+			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
+						&info->rx_mcs);
+			break;
+		case NETDEV_MCS_TYPE_VHT:
+			dbus_append_dict_basic(data->builder, "RxMode", 's',
+						"802.11ac");
+			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
+						&info->rx_mcs);
+			break;
+		case NETDEV_MCS_TYPE_HE:
+			dbus_append_dict_basic(data->builder, "RxMode", 's',
+						"802.11ax");
+			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
+						&info->rx_mcs);
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (info->have_tx_mcs) {
+		switch (info->tx_mcs_type) {
+		case NETDEV_MCS_TYPE_HT:
+			dbus_append_dict_basic(data->builder, "TxMode", 's',
+						"802.11n");
+			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
+						&info->tx_mcs);
+			break;
+		case NETDEV_MCS_TYPE_VHT:
+			dbus_append_dict_basic(data->builder, "TxMode", 's',
+						"802.11ac");
+			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
+						&info->tx_mcs);
+			break;
+		case NETDEV_MCS_TYPE_HE:
+			dbus_append_dict_basic(data->builder, "TxMode", 's',
+						"802.11ax");
+			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
+						&info->tx_mcs);
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (info->have_tx_bitrate)
+		dbus_append_dict_basic(data->builder, "TxBitrate", 'u',
+					&info->tx_bitrate);
+
+	if (info->have_rx_bitrate)
+		dbus_append_dict_basic(data->builder, "RxBitrate", 'u',
+					&info->rx_bitrate);
+
+	if (info->have_expected_throughput)
+		dbus_append_dict_basic(data->builder, "ExpectedThroughput", 'u',
+					&info->expected_throughput);
+
+	l_dbus_message_builder_leave_array(data->builder);
+}
+
+static void ap_get_station_destroy(void *user_data)
+{
+	struct diagnostic_data *data = user_data;
+	struct l_dbus_message *reply;
+
+	if (!data->builder) {
+		reply = l_dbus_message_new_method_return(data->pending);
+
+		data->builder = l_dbus_message_builder_new(reply);
+
+		l_dbus_message_builder_enter_array(data->builder, "a{sv}");
+	}
+
+	l_dbus_message_builder_leave_array(data->builder);
+	reply = l_dbus_message_builder_finalize(data->builder);
+	l_dbus_message_builder_destroy(data->builder);
+
+	dbus_pending_reply(&data->pending, reply);
+
+	l_free(data);
+}
+
+static struct l_dbus_message *ap_dbus_get_diagnostics(struct l_dbus *dbus,
+		struct l_dbus_message *message, void *user_data)
+{
+	struct ap_if_data *ap_if = user_data;
+	struct diagnostic_data *data;
+	int ret;
+
+	data = l_new(struct diagnostic_data, 1);
+	data->pending = l_dbus_message_ref(message);
+
+	ret = netdev_get_all_stations(ap_if->ap->netdev, ap_get_station_cb,
+					data, ap_get_station_destroy);
+
+	if (ret < 0) {
+		l_dbus_message_unref(data->pending);
+		l_free(data);
+		return dbus_error_from_errno(ret, message);
+	}
+
+	return NULL;
+}
+
+static void ap_setup_diagnostic_interface(struct l_dbus_interface *interface)
+{
+	l_dbus_interface_method(interface, "GetDiagnostics", 0,
+				ap_dbus_get_diagnostics, "aa{sv}", "", "diagnostic");
+}
+
+static void ap_diagnostic_interface_destroy(void *user_data)
+{
+}
+
 static void ap_add_interface(struct netdev *netdev)
 {
 	struct ap_if_data *ap_if;
@@ -2978,12 +3127,16 @@ static void ap_add_interface(struct netdev *netdev)
 	/* setup ap dbus interface */
 	l_dbus_object_add_interface(dbus_get_bus(),
 			netdev_get_path(netdev), IWD_AP_INTERFACE, ap_if);
+	l_dbus_object_add_interface(dbus_get_bus(), netdev_get_path(netdev),
+			IWD_AP_DIAGNOSTIC_INTERFACE, ap_if);
 }
 
 static void ap_remove_interface(struct netdev *netdev)
 {
 	l_dbus_object_remove_interface(dbus_get_bus(),
 			netdev_get_path(netdev), IWD_AP_INTERFACE);
+	l_dbus_object_remove_interface(dbus_get_bus(), netdev_get_path(netdev),
+			IWD_AP_DIAGNOSTIC_INTERFACE);
 }
 
 static void ap_netdev_watch(struct netdev *netdev,
@@ -3014,6 +3167,9 @@ static int ap_init(void)
 
 	l_dbus_register_interface(dbus_get_bus(), IWD_AP_INTERFACE,
 			ap_setup_interface, ap_destroy_interface, false);
+	l_dbus_register_interface(dbus_get_bus(), IWD_AP_DIAGNOSTIC_INTERFACE,
+			ap_setup_diagnostic_interface,
+			ap_diagnostic_interface_destroy, false);
 
 	/*
 	 * Reusing [General].EnableNetworkConfiguration as a switch to enable
-- 
2.26.2

[PATCH 5/8] client: add AccessPointDiagnostic interface definition

[James Prestwood]

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

---
 client/dbus-proxy.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/client/dbus-proxy.h b/client/dbus-proxy.h
index 9c5c57e5..a3ca270a 100644
--- a/client/dbus-proxy.h
+++ b/client/dbus-proxy.h
@@ -34,6 +34,7 @@ struct proxy_interface;
 #define IWD_WSC_INTERFACE                "net.connman.iwd.SimpleConfiguration"
 #define IWD_STATION_INTERFACE            "net.connman.iwd.Station"
 #define IWD_STATION_DIAGNOSTIC_INTERFACE "net.connman.iwd.StationDiagnostic"
+#define IWD_AP_DIAGNOSTIC_INTERFACE      "net.connman.iwd.AccessPointDiagnostic"
 
 typedef bool (*proxy_property_match_func_t) (const void *a, const void *b);
 
-- 
2.26.2

[PATCH 6/8] client: implement "ap <wlan> show"

[James Prestwood]

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

This command uses GetDiagnostics to show a list of connected
clients and some information about them. The information
contained for each connected station nearly maps 1:1 with the
station diagnostics information shown in "station <wlan> show"
apart from "ConnectedBss" which is now "Address".
---
 client/ap.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/client/ap.c b/client/ap.c
index a6a2681b..91d265a7 100644
--- a/client/ap.c
+++ b/client/ap.c
@@ -189,6 +189,100 @@ static enum cmd_status cmd_stop(const char *device_name, char **argv, int argc)
 	return CMD_STATUS_TRIGGERED;
 }
 
+static struct proxy_interface_type ap_diagnostic_interface_type = {
+	.interface = IWD_AP_DIAGNOSTIC_INTERFACE,
+};
+
+static void display_client(struct l_dbus_message_iter *iter)
+{
+	struct l_dbus_message_iter variant;
+	const char *key;
+
+	while (l_dbus_message_iter_next_entry(iter, &key, &variant)) {
+		const char *s_value;
+		uint32_t u_value;
+		int16_t i_value;
+		uint8_t y_value;
+
+		if (!strcmp(key, "Address") || !strcmp(key, "RxMode") ||
+				!strcmp(key, "TxMode")) {
+			/* String variants with no special handling */
+
+			l_dbus_message_iter_get_variant(&variant, "s",
+							&s_value);
+
+			display("%-*s%-*s\n", 20, key, 47, s_value);
+		} else if (!strcmp(key, "RxBitrate") ||
+				!strcmp(key, "TxBitrate")) {
+			/* Bitrates expressed in 100Kbit/s */
+
+			l_dbus_message_iter_get_variant(&variant, "u",
+							&u_value);
+			display("%-*s%u Kbit/s\n", 20, key, u_value * 100);
+		} else if (!strcmp(key, "ExpectedThroughput")) {
+			/* ExpectedThroughput expressed in Kbit/s */
+
+			l_dbus_message_iter_get_variant(&variant, "u",
+							&u_value);
+			display("%*s  %-*s%u Kbit/s\n", 0, "", 20,
+				key, u_value);
+		} else if (!strcmp(key, "RSSI")) {
+			/* RSSI expressed in dBm */
+
+			l_dbus_message_iter_get_variant(&variant, "n",
+							&i_value);
+			display("%-*s%i dBm\n", 20, key, i_value);
+		} else if (!strcmp(key, "RxMCS") || !strcmp(key, "TxMCS")) {
+			/* MCS index's are single byte integers */
+
+			l_dbus_message_iter_get_variant(&variant, "y",
+							&y_value);
+			display("%-*s%u\n", 20, key, y_value);
+		}
+	}
+}
+
+static void ap_get_diagnostics_callback(struct l_dbus_message *message,
+					void *user_data)
+{
+	struct l_dbus_message_iter array;
+	struct l_dbus_message_iter iter;
+	uint16_t idx = 0;
+	char client_num[15];
+
+	if (dbus_message_has_error(message))
+		return;
+
+	if (!l_dbus_message_get_arguments(message, "aa{sv}", &array)) {
+		display("Failed to parse GetDiagnostics message");
+		return;
+	}
+
+	while (l_dbus_message_iter_next_entry(&array, &iter)) {
+		sprintf(client_num, "Client %u", idx++);
+		display_table_header(client_num, "%-*s%-*s",
+					20, "Property", 20, "Value");
+		display_client(&iter);
+		display_table_footer();
+	}
+}
+
+static enum cmd_status cmd_show(const char *device_name, char **argv, int argc)
+{
+	const struct proxy_interface *ap_diagnostic =
+		device_proxy_find(device_name, IWD_AP_DIAGNOSTIC_INTERFACE);
+
+	if (!ap_diagnostic) {
+		display("No ap on device: '%s'\n", device_name);
+		return CMD_STATUS_INVALID_VALUE;
+	}
+
+	proxy_interface_method_call(ap_diagnostic, "GetDiagnostics", "",
+					ap_get_diagnostics_callback);
+
+	return CMD_STATUS_TRIGGERED;
+}
+
 static const struct command ap_commands[] = {
 	{ NULL, "list", NULL, cmd_list, "List devices in AP mode", true },
 	{ "<wlan>", "start", "<\"network name\"> <passphrase>", cmd_start,
@@ -196,6 +290,7 @@ static const struct command ap_commands[] = {
 		"name\" with\n\t\t\t\t\t\t    a passphrase" },
 	{ "<wlan>", "stop", NULL,   cmd_stop, "Stop a started access\n"
 		"\t\t\t\t\t\t    point" },
+	{ "<wlan", "show", NULL, cmd_show, "Show AP info", false },
 	{ }
 };
 
@@ -236,6 +331,7 @@ COMMAND_FAMILY(ap_command_family, ap_command_family_init,
 static int ap_interface_init(void)
 {
 	proxy_interface_type_register(&ap_interface_type);
+	proxy_interface_type_register(&ap_diagnostic_interface_type);
 
 	return 0;
 }
@@ -243,6 +339,7 @@ static int ap_interface_init(void)
 static void ap_interface_exit(void)
 {
 	proxy_interface_type_unregister(&ap_interface_type);
+	proxy_interface_type_unregister(&ap_diagnostic_interface_type);
 }
 
 INTERFACE_TYPE(ap_interface_type, ap_interface_init, ap_interface_exit)
-- 
2.26.2

[PATCH 7/8] test: add AccessPointDiagnostics to monitor-iwd

[James Prestwood]

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

---
 test/monitor-iwd | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/monitor-iwd b/test/monitor-iwd
index e801ef54..707b0c39 100755
--- a/test/monitor-iwd
+++ b/test/monitor-iwd
@@ -65,7 +65,8 @@ relevant_ifaces = [ "net.connman.iwd.Device",
                     "net.connman.iwd.Network",
                     "net.connman.iwd.AgentManager",
                     "net.connman.iwd.KnownNetwork",
-                    "net.connman.iwd.Station" ]
+                    "net.connman.iwd.Station",
+                    "net.connman.iwd.AccessPointDiagnostic"]
 
 def interfaces_added(path, interfaces):
     for iface, props in interfaces.items():
-- 
2.26.2

[PATCH 8/8] test: add StationDiagnostic interface to monitor-iwd

[James Prestwood]

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

---
 test/monitor-iwd | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/monitor-iwd b/test/monitor-iwd
index 707b0c39..df043bc3 100755
--- a/test/monitor-iwd
+++ b/test/monitor-iwd
@@ -66,7 +66,8 @@ relevant_ifaces = [ "net.connman.iwd.Device",
                     "net.connman.iwd.AgentManager",
                     "net.connman.iwd.KnownNetwork",
                     "net.connman.iwd.Station",
-                    "net.connman.iwd.AccessPointDiagnostic"]
+                    "net.connman.iwd.AccessPointDiagnostic",
+                    "net.connman.iwd.StationDiagnostic"]
 
 def interfaces_added(path, interfaces):
     for iface, props in interfaces.items():
-- 
2.26.2

Re: [PATCH 1/8] doc: document AccessPointDiagnostic interface

[Denis Kenzior]

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

Hi James,

On 1/20/21 12:30 PM, James Prestwood wrote:
> ---
>   doc/access-point-diagnostics.txt | 36 ++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>   create mode 100644 doc/access-point-diagnostics.txt
> 

<snip>

> +
> +Methods		dict[] GetDiagnostics()
> +

For historical reasons the preferred syntax for this is array{dict}, so I made 
this changed and applied, thanks!

Regards,
-Denis

Re: [PATCH 2/8] netdev: add netdev_get_all_stations

[Denis Kenzior]

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

Hi James,

On 1/20/21 12:30 PM, James Prestwood wrote:
> This is a nl80211 dump version of netdev_get_station aimed at
> AP mode. This will dump all stations, parse into
> netdev_station_info structs, and call the callback for each
> individual station found. Once the dump is completed the destroy
> callback is called.
> ---
>   src/netdev.c | 26 ++++++++++++++++++++++++++
>   src/netdev.h |  2 ++
>   2 files changed, 28 insertions(+)
> 

Patch 2 & 3 applied, thanks.

Regards,
-Denis

Re: [PATCH 4/8] ap: add AP diagnostic interface

[Denis Kenzior]

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

Hi James,

On 1/20/21 12:30 PM, James Prestwood wrote:
> This adds a new AccessPointDiagnostic interface. This interface
> provides similar low level functionality as StationDiagnostic, but
> for when IWD is in AP mode. This uses netdev_get_all_stations
> which will dump all stations, parse, and return each station in
> an individual callback. Once the dump is complete the destroy is
> called and all data is packaged as an array of dictionaries.
> ---
>   src/ap.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 156 insertions(+)
> 
> diff --git a/src/ap.c b/src/ap.c
> index d9e7e404..6b3fb129 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -2963,6 +2963,155 @@ static void ap_destroy_interface(void *user_data)
>   	l_free(ap_if);
>   }
>   
> +struct diagnostic_data {
> +	struct l_dbus_message *pending;
> +	struct l_dbus_message_builder *builder;
> +};
> +
> +static void ap_get_station_cb(const struct netdev_station_info *info,
> +				void *user_data)
> +{
> +	struct diagnostic_data *data = user_data;
> +	int16_t rssi;
> +
> +	/* First station info */
> +	if (!data->builder) {
> +		struct l_dbus_message *reply =
> +				l_dbus_message_new_method_return(data->pending);
> +
> +		data->builder = l_dbus_message_builder_new(reply);
> +
> +		l_dbus_message_builder_enter_array(data->builder, "a{sv}");
> +	}
> +
> +	l_dbus_message_builder_enter_array(data->builder, "{sv}");
> +	dbus_append_dict_basic(data->builder, "Address", 's',
> +					util_address_to_string(info->addr));
> +
> +	rssi = (int16_t)info->cur_rssi;
> +
> +	if (info->have_cur_rssi)
> +		dbus_append_dict_basic(data->builder, "RSSI", 'n', &rssi);
> +
> +	if (info->have_rx_mcs) {
> +		switch (info->rx_mcs_type) {
> +		case NETDEV_MCS_TYPE_HT:
> +			dbus_append_dict_basic(data->builder, "RxMode", 's',
> +						"802.11n");
> +			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
> +						&info->rx_mcs);
> +			break;
> +		case NETDEV_MCS_TYPE_VHT:
> +			dbus_append_dict_basic(data->builder, "RxMode", 's',
> +						"802.11ac");
> +			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
> +						&info->rx_mcs);
> +			break;
> +		case NETDEV_MCS_TYPE_HE:
> +			dbus_append_dict_basic(data->builder, "RxMode", 's',
> +						"802.11ax");
> +			dbus_append_dict_basic(data->builder, "RxMCS", 'y',
> +						&info->rx_mcs);
> +			break;
> +		default:
> +			break;
> +		}
> +	}

This seems to be a copy-paste from station.c, should we add a utility function 
for doing this somewhere?

> +
> +	if (info->have_tx_mcs) {
> +		switch (info->tx_mcs_type) {
> +		case NETDEV_MCS_TYPE_HT:
> +			dbus_append_dict_basic(data->builder, "TxMode", 's',
> +						"802.11n");
> +			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
> +						&info->tx_mcs);
> +			break;
> +		case NETDEV_MCS_TYPE_VHT:
> +			dbus_append_dict_basic(data->builder, "TxMode", 's',
> +						"802.11ac");
> +			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
> +						&info->tx_mcs);
> +			break;
> +		case NETDEV_MCS_TYPE_HE:
> +			dbus_append_dict_basic(data->builder, "TxMode", 's',
> +						"802.11ax");
> +			dbus_append_dict_basic(data->builder, "TxMCS", 'y',
> +						&info->tx_mcs);
> +			break;
> +		default:
> +			break;
> +		}
> +	}

Same as above.  Also seems that the basic structure is the same, only Rx/Tx 
prefix is different.  So maybe the two can be combined?

> +
> +	if (info->have_tx_bitrate)
> +		dbus_append_dict_basic(data->builder, "TxBitrate", 'u',
> +					&info->tx_bitrate);
> +
> +	if (info->have_rx_bitrate)
> +		dbus_append_dict_basic(data->builder, "RxBitrate", 'u',
> +					&info->rx_bitrate);
> +
> +	if (info->have_expected_throughput)
> +		dbus_append_dict_basic(data->builder, "ExpectedThroughput", 'u',
> +					&info->expected_throughput);
> +
> +	l_dbus_message_builder_leave_array(data->builder);
> +}
> +

<snip>

> +static void ap_setup_diagnostic_interface(struct l_dbus_interface *interface)
> +{
> +	l_dbus_interface_method(interface, "GetDiagnostics", 0,
> +				ap_dbus_get_diagnostics, "aa{sv}", "", "diagnostic");

This seems to go over 80 chars

> +}
> +

Regards,
-Denis

Re: [PATCH 5/8] client: add AccessPointDiagnostic interface definition

[Denis Kenzior]

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

Hi James,

On 1/20/21 12:30 PM, James Prestwood wrote:
> ---
>   client/dbus-proxy.h | 1 +
>   1 file changed, 1 insertion(+)
> 

Patches 5, 7 and 8 applied, thanks.

Regards,
-Denis

Re: [PATCH 6/8] client: implement "ap <wlan> show"

[Denis Kenzior]

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

Hi James,

On 1/20/21 12:30 PM, James Prestwood wrote:
> This command uses GetDiagnostics to show a list of connected
> clients and some information about them. The information
> contained for each connected station nearly maps 1:1 with the
> station diagnostics information shown in "station <wlan> show"
> apart from "ConnectedBss" which is now "Address".
> ---
>   client/ap.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 97 insertions(+)
> 

<snip>

> +static void display_client(struct l_dbus_message_iter *iter)
> +{
> +	struct l_dbus_message_iter variant;
> +	const char *key;
> +
> +	while (l_dbus_message_iter_next_entry(iter, &key, &variant)) {
> +		const char *s_value;
> +		uint32_t u_value;
> +		int16_t i_value;
> +		uint8_t y_value;
> +
> +		if (!strcmp(key, "Address") || !strcmp(key, "RxMode") ||
> +				!strcmp(key, "TxMode")) {
> +			/* String variants with no special handling */
> +
> +			l_dbus_message_iter_get_variant(&variant, "s",
> +							&s_value);
> +
> +			display("%-*s%-*s\n", 20, key, 47, s_value);
> +		} else if (!strcmp(key, "RxBitrate") ||
> +				!strcmp(key, "TxBitrate")) {
> +			/* Bitrates expressed in 100Kbit/s */
> +
> +			l_dbus_message_iter_get_variant(&variant, "u",
> +							&u_value);
> +			display("%-*s%u Kbit/s\n", 20, key, u_value * 100);
> +		} else if (!strcmp(key, "ExpectedThroughput")) {
> +			/* ExpectedThroughput expressed in Kbit/s */
> +
> +			l_dbus_message_iter_get_variant(&variant, "u",
> +							&u_value);
> +			display("%*s  %-*s%u Kbit/s\n", 0, "", 20,
> +				key, u_value);
> +		} else if (!strcmp(key, "RSSI")) {
> +			/* RSSI expressed in dBm */
> +
> +			l_dbus_message_iter_get_variant(&variant, "n",
> +							&i_value);
> +			display("%-*s%i dBm\n", 20, key, i_value);
> +		} else if (!strcmp(key, "RxMCS") || !strcmp(key, "TxMCS")) {
> +			/* MCS index's are single byte integers */
> +
> +			l_dbus_message_iter_get_variant(&variant, "y",
> +							&y_value);
> +			display("%-*s%u\n", 20, key, y_value);
> +		}

Along the lines of comment to patch 4, should the display of these attributes be 
forked out to a utility  function given that they're the same between station / 
ap for the most part?

> +	}
> +}
> +

Regards,
-Denis

Re: [PATCH 6/8] client: implement "ap <wlan> show"

[James Prestwood]

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

On Wed, 2021-01-20 at 14:16 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 1/20/21 12:30 PM, James Prestwood wrote:
> > This command uses GetDiagnostics to show a list of connected
> > clients and some information about them. The information
> > contained for each connected station nearly maps 1:1 with the
> > station diagnostics information shown in "station <wlan> show"
> > apart from "ConnectedBss" which is now "Address".
> > ---
> >   client/ap.c | 97
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 97 insertions(+)
> > 
> 
> <snip>
> 
> > +static void display_client(struct l_dbus_message_iter *iter)
> > +{
> > +	struct l_dbus_message_iter variant;
> > +	const char *key;
> > +
> > +	while (l_dbus_message_iter_next_entry(iter, &key, &variant)) {
> > +		const char *s_value;
> > +		uint32_t u_value;
> > +		int16_t i_value;
> > +		uint8_t y_value;
> > +
> > +		if (!strcmp(key, "Address") || !strcmp(key, "RxMode")
> > ||
> > +				!strcmp(key, "TxMode")) {
> > +			/* String variants with no special handling */
> > +
> > +			l_dbus_message_iter_get_variant(&variant, "s",
> > +							&s_value);
> > +
> > +			display("%-*s%-*s\n", 20, key, 47, s_value);
> > +		} else if (!strcmp(key, "RxBitrate") ||
> > +				!strcmp(key, "TxBitrate")) {
> > +			/* Bitrates expressed in 100Kbit/s */
> > +
> > +			l_dbus_message_iter_get_variant(&variant, "u",
> > +							&u_value);
> > +			display("%-*s%u Kbit/s\n", 20, key, u_value *
> > 100);
> > +		} else if (!strcmp(key, "ExpectedThroughput")) {
> > +			/* ExpectedThroughput expressed in Kbit/s */
> > +
> > +			l_dbus_message_iter_get_variant(&variant, "u",
> > +							&u_value);
> > +			display("%*s  %-*s%u Kbit/s\n", 0, "", 20,
> > +				key, u_value);
> > +		} else if (!strcmp(key, "RSSI")) {
> > +			/* RSSI expressed in dBm */
> > +
> > +			l_dbus_message_iter_get_variant(&variant, "n",
> > +							&i_value);
> > +			display("%-*s%i dBm\n", 20, key, i_value);
> > +		} else if (!strcmp(key, "RxMCS") || !strcmp(key,
> > "TxMCS")) {
> > +			/* MCS index's are single byte integers */
> > +
> > +			l_dbus_message_iter_get_variant(&variant, "y",
> > +							&y_value);
> > +			display("%-*s%u\n", 20, key, y_value);
> > +		}
> 
> Along the lines of comment to patch 4, should the display of these
> attributes be 
> forked out to a utility  function given that they're the same between
> station / 
> ap for the most part?

Yeah I'll come up with something. I think we need some kind of
name/function mapping for these since there is at least one difference
betweens station/ap (ConnectedBss/Address). Anyways, I'll do this for
iwctl and commonize the processing in IWD core as well.

> 
> > +	}
> > +}
> > +
> 
> Regards,
> -Denis