[PATCH 1/8] doc: diagnostic DBus interface definition

[PATCH 1/8] doc: diagnostic DBus interface definition

[James Prestwood]

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

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

diff --git a/doc/diagnostics.txt b/doc/diagnostics.txt
new file mode 100644
index 00000000..2354d316
--- /dev/null
+++ b/doc/diagnostics.txt
@@ -0,0 +1,36 @@
+Station diagnostic hierarchy
+=================
+
+Service		net.connman.iwd
+Interface	net.connman.iwd.StationDiagnostic
+Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
+
+Methods		dict GetDiagnostics()
+
+			Get all diagnostic information for this interface. The
+			diagnostics are contained in a single dictionary. Values
+			here are generally low level and not meant for general
+			purpose applications which could get by with the
+			existing Station interface or values which are volatile
+			and change too frequently to be represented as
+			properties. The values in the dictionary may come and
+			go depending on the state of IWD.
+
+			Below is a list of possible diagnostic dictionary
+			values:
+
+			ConnectedBss - MAC address of currently connected BSS.
+
+			RSSI -	The RSSI of the currently connected BSS.
+
+			RxRate - Receive rate in 100kbit/s
+
+			RxMCS - Receiving MCS index
+
+			TxRate - Transmission rate in 100kbit/s
+
+			TxMCS - Transmitting MCS index
+
+			Possible errors: net.connman.iwd.Busy
+					 net.connman.iwd.Failed
+					 net.connman.iwd.NotConnected
-- 
2.26.2

[PATCH 2/8] netdev: add netdev_get_station

[James Prestwood]

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

This adds a generalized API for GET_STATION. This API handles
calling and parsing the results into a new structure,
netdev_station_info. This results structure will hold any
data needed by consumers of netdev_get_station.

For now only the RSSI is parsed as this is already being
done for RSSI polling/events. Looking further more info will
be added such as rx/tx rates and estimated throughput.
---
 src/netdev.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/netdev.h |  12 ++++++
 2 files changed, 115 insertions(+)

diff --git a/src/netdev.c b/src/netdev.c
index 3f78afbf..3cda1c48 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -132,6 +132,11 @@ struct netdev {
 	void *set_powered_user_data;
 	netdev_destroy_func_t set_powered_destroy;
 
+	uint32_t get_station_cmd_id;
+	netdev_get_station_cb_t get_station_cb;
+	void *get_station_data;
+	netdev_destroy_func_t get_station_destroy;
+
 	struct watchlist station_watches;
 
 	struct l_io *pae_io;  /* for drivers without EAPoL over NL80211 */
@@ -636,6 +641,11 @@ static void netdev_free(void *data)
 		netdev->mac_change_cmd_id = 0;
 	}
 
+	if (netdev->get_station_cmd_id) {
+		l_genl_family_cancel(nl80211, netdev->get_station_cmd_id);
+		netdev->get_station_cmd_id = 0;
+	}
+
 	if (netdev->events_ready)
 		WATCHLIST_NOTIFY(&netdev_watches, netdev_watch_func_t,
 					netdev, NETDEV_WATCH_EVENT_DEL);
@@ -4022,6 +4032,99 @@ done:
 	return 0;
 }
 
+static bool netdev_parse_sta_info(struct l_genl_attr *attr,
+					struct netdev_station_info *info)
+{
+	uint16_t type, len;
+	const void *data;
+
+	while (l_genl_attr_next(attr, &type, &len, &data)) {
+		switch (type) {
+		case NL80211_STA_INFO_SIGNAL_AVG:
+			if (len != 1)
+				return false;
+
+			info->cur_rssi = *(const int8_t *) data;
+
+			break;
+		}
+	}
+
+	return true;
+}
+
+static void netdev_get_station_cb(struct l_genl_msg *msg, void *user_data)
+{
+	struct netdev *netdev = user_data;
+	struct l_genl_attr attr, nested;
+	uint16_t type, len;
+	const void *data;
+	struct netdev_station_info info;
+
+	netdev->get_station_cmd_id = 0;
+
+	if (!l_genl_attr_init(&attr, msg))
+		goto parse_error;
+
+	while (l_genl_attr_next(&attr, &type, &len, &data)) {
+		switch (type) {
+		case NL80211_ATTR_STA_INFO:
+			if (!l_genl_attr_recurse(&attr, &nested))
+				goto parse_error;
+
+			if (!netdev_parse_sta_info(&nested, &info))
+				goto parse_error;
+
+			break;
+		}
+	}
+
+	if (netdev->get_station_cb)
+		netdev->get_station_cb(netdev, &info, netdev->get_station_data);
+
+	return;
+
+parse_error:
+	if (netdev->get_station_cb)
+		netdev->get_station_cb(netdev, NULL, netdev->get_station_data);
+}
+
+static void netdev_get_station_destroy(void *user_data)
+{
+	struct netdev *netdev = user_data;
+
+	netdev->get_station_cmd_id = 0;
+
+	if (netdev->get_station_destroy)
+		netdev->get_station_destroy(netdev->get_station_data);
+}
+
+int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
+			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);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_MAC, ETH_ALEN, mac);
+
+	netdev->get_station_cmd_id = l_genl_family_send(nl80211, msg,
+						netdev_get_station_cb, netdev,
+						netdev_get_station_destroy);
+	if (!netdev->get_station_cmd_id)
+		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 65fdbaaf..5cf38076 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -114,6 +114,14 @@ typedef void (*netdev_station_watch_func_t)(struct netdev *netdev,
 					const uint8_t *mac, bool added,
 					void *user_data);
 
+struct netdev_station_info {
+	int8_t cur_rssi;
+};
+
+typedef void (*netdev_get_station_cb_t)(struct netdev *netdev,
+					struct netdev_station_info *info,
+					void *user_data);
+
 struct wiphy *netdev_get_wiphy(struct netdev *netdev);
 const uint8_t *netdev_get_address(struct netdev *netdev);
 uint32_t netdev_get_ifindex(struct netdev *netdev);
@@ -174,6 +182,10 @@ int netdev_neighbor_report_req(struct netdev *netdev,
 int netdev_set_rssi_report_levels(struct netdev *netdev, const int8_t *levels,
 					size_t levels_num);
 
+int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
+			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);
 
 struct netdev *netdev_find(int ifindex);
-- 
2.26.2

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

[Denis Kenzior]

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

Hi James,

On 1/11/21 11:12 AM, James Prestwood wrote:
> This adds a generalized API for GET_STATION. This API handles
> calling and parsing the results into a new structure,
> netdev_station_info. This results structure will hold any
> data needed by consumers of netdev_get_station.
> 
> For now only the RSSI is parsed as this is already being
> done for RSSI polling/events. Looking further more info will
> be added such as rx/tx rates and estimated throughput.
> ---
>   src/netdev.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/netdev.h |  12 ++++++
>   2 files changed, 115 insertions(+)
> 

<snip>

> +int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
> +			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);
> +	l_genl_msg_append_attr(msg, NL80211_ATTR_MAC, ETH_ALEN, mac);
> +
> +	netdev->get_station_cmd_id = l_genl_family_send(nl80211, msg,
> +						netdev_get_station_cb, netdev,
> +						netdev_get_station_destroy);
> +	if (!netdev->get_station_cmd_id)
> +		return -EIO;

I think you're leaking msg here

> +
> +	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 65fdbaaf..5cf38076 100644
> --- a/src/netdev.h
> +++ b/src/netdev.h
> @@ -114,6 +114,14 @@ typedef void (*netdev_station_watch_func_t)(struct netdev *netdev,
>   					const uint8_t *mac, bool added,
>   					void *user_data);
>   
> +struct netdev_station_info {
> +	int8_t cur_rssi;
> +};
> +
> +typedef void (*netdev_get_station_cb_t)(struct netdev *netdev,
> +					struct netdev_station_info *info,

const struct netdev_...?

> +					void *user_data);
> +
>   struct wiphy *netdev_get_wiphy(struct netdev *netdev);
>   const uint8_t *netdev_get_address(struct netdev *netdev);
>   uint32_t netdev_get_ifindex(struct netdev *netdev);
> @@ -174,6 +182,10 @@ int netdev_neighbor_report_req(struct netdev *netdev,
>   int netdev_set_rssi_report_levels(struct netdev *netdev, const int8_t *levels,
>   					size_t levels_num);
>   
> +int netdev_get_station(struct netdev *netdev, const uint8_t *mac,

Do you want to add a convenience method to grab the current station?  The mac 
parameter is for cases where you might have multiple ones, like AP.

> +			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);
>   
>   struct netdev *netdev_find(int ifindex);
> 

Regards,
-Denis

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

[James Prestwood]

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

Hi Denis,

> >   struct wiphy *netdev_get_wiphy(struct netdev *netdev);
> >   const uint8_t *netdev_get_address(struct netdev *netdev);
> >   uint32_t netdev_get_ifindex(struct netdev *netdev);
> > @@ -174,6 +182,10 @@ int netdev_neighbor_report_req(struct netdev
> > *netdev,
> >   int netdev_set_rssi_report_levels(struct netdev *netdev, const
> > int8_t *levels,
> >   					size_t levels_num);
> >   
> > +int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
> 
> Do you want to add a convenience method to grab the current
> station?  The mac 
> parameter is for cases where you might have multiple ones, like AP.

I could, although its not too bad just passing in handshake->aa... But
if you want a current station API and a separate one for AP I don't
mind either way.

> 
> > +			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);
> >   
> >   struct netdev *netdev_find(int ifindex);
> > 
> 
> Regards,
> -Denis

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

[Denis Kenzior]

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

Hi James,

>>> +int netdev_get_station(struct netdev *netdev, const uint8_t *mac,
>>
>> Do you want to add a convenience method to grab the current
>> station?  The mac
>> parameter is for cases where you might have multiple ones, like AP.
> 
> I could, although its not too bad just passing in handshake->aa... But
> if you want a current station API and a separate one for AP I don't
> mind either way.
> 

I was just thinking that you probably can keep this method and just add a 
wrapper (netdev_get_current_station or something) that would just call 
netdev_get_station appropriately.  But I'm fine either way.

Regards,
-Denis

[PATCH 3/8] netdev: update RSSI polling to use netdev_get_station

[James Prestwood]

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

---
 src/netdev.c | 61 +++++++++-------------------------------------------
 1 file changed, 10 insertions(+), 51 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 3cda1c48..1e54acec 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -124,7 +124,6 @@ struct netdev {
 	uint8_t cur_rssi_level_idx;
 	int8_t cur_rssi;
 	struct l_timeout *rssi_poll_timeout;
-	uint32_t rssi_poll_cmd_id;
 	uint8_t set_mac_once[6];
 
 	uint32_t set_powered_cmd_id;
@@ -379,47 +378,16 @@ static void netdev_set_rssi_level_idx(struct netdev *netdev)
 	netdev->cur_rssi_level_idx = new_level;
 }
 
-static void netdev_rssi_poll_cb(struct l_genl_msg *msg, void *user_data)
+static void netdev_rssi_poll_cb(struct netdev *netdev,
+				struct netdev_station_info *info,
+				void *user_data)
 {
-	struct netdev *netdev = user_data;
-	struct l_genl_attr attr, nested;
-	uint16_t type, len;
-	const void *data;
-	bool found;
 	uint8_t prev_rssi_level_idx = netdev->cur_rssi_level_idx;
 
-	netdev->rssi_poll_cmd_id = 0;
-
-	if (!l_genl_attr_init(&attr, msg))
-		goto done;
-
-	found = false;
-	while (l_genl_attr_next(&attr, &type, &len, &data)) {
-		if (type != NL80211_ATTR_STA_INFO)
-			continue;
-
-		found = true;
-		break;
-	}
-
-	if (!found || !l_genl_attr_recurse(&attr, &nested))
+	if (!info)
 		goto done;
 
-	found = false;
-	while (l_genl_attr_next(&nested, &type, &len, &data)) {
-		if (type != NL80211_STA_INFO_SIGNAL_AVG)
-			continue;
-
-		if (len != 1)
-			continue;
-
-		found = true;
-		netdev->cur_rssi = *(const int8_t *) data;
-		break;
-	}
-
-	if (!found)
-		goto done;
+	netdev->cur_rssi =  info->cur_rssi;
 
 	/*
 	 * Note we don't have to handle LOW_SIGNAL_THRESHOLD here.  The
@@ -441,16 +409,12 @@ done:
 static void netdev_rssi_poll(struct l_timeout *timeout, void *user_data)
 {
 	struct netdev *netdev = user_data;
-	struct l_genl_msg *msg;
-
-	msg = l_genl_msg_new_sized(NL80211_CMD_GET_STATION, 64);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &netdev->index);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_MAC, ETH_ALEN,
-							netdev->handshake->aa);
 
-	netdev->rssi_poll_cmd_id = l_genl_family_send(nl80211, msg,
-							netdev_rssi_poll_cb,
-							netdev, NULL);
+	if (netdev_get_station(netdev, netdev->handshake->aa,
+				netdev_rssi_poll_cb, NULL, NULL) < 0) {
+		/* Some problem occurred, rearm timer and try again */
+		l_timeout_modify(netdev->rssi_poll_timeout, 6);
+	}
 }
 
 /* To be called whenever operational or rssi_levels_num are updated */
@@ -472,11 +436,6 @@ static void netdev_rssi_polling_update(struct netdev *netdev)
 
 		l_timeout_remove(netdev->rssi_poll_timeout);
 		netdev->rssi_poll_timeout = NULL;
-
-		if (netdev->rssi_poll_cmd_id) {
-			l_genl_family_cancel(nl80211, netdev->rssi_poll_cmd_id);
-			netdev->rssi_poll_cmd_id = 0;
-		}
 	}
 }
 
-- 
2.26.2

[PATCH 4/8] netdev: parse rates in netdev_get_station

[James Prestwood]

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

---
 src/netdev.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/netdev.h |  4 ++++
 2 files changed, 62 insertions(+)

diff --git a/src/netdev.c b/src/netdev.c
index 1e54acec..6fb33847 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3991,11 +3991,50 @@ done:
 	return 0;
 }
 
+static bool netdev_parse_bitrate(struct l_genl_attr *attr, uint32_t *rate_out,
+					uint8_t *mcs_out)
+{
+	uint16_t type, len;
+	const void *data;
+	uint32_t rate = 0;
+	uint8_t mcs = 0;
+
+	while (l_genl_attr_next(attr, &type, &len, &data)) {
+		switch (type) {
+		case NL80211_RATE_INFO_BITRATE32:
+			if (len != 4)
+				return false;
+
+			l_debug("parsing bitrate");
+			rate = *(const uint32_t *) data;
+
+			break;
+
+		case NL80211_RATE_INFO_MCS:
+			if (len != 1)
+				return false;
+
+			mcs = *(const uint8_t *) data;
+
+			break;
+		}
+	}
+
+	if (!rate || !mcs)
+		return false;
+
+	*rate_out = rate;
+	*mcs_out = mcs;
+
+	return true;
+}
+
 static bool netdev_parse_sta_info(struct l_genl_attr *attr,
 					struct netdev_station_info *info)
 {
 	uint16_t type, len;
 	const void *data;
+	struct l_genl_attr nested;
 
 	while (l_genl_attr_next(attr, &type, &len, &data)) {
 		switch (type) {
@@ -4005,6 +4044,25 @@ static bool netdev_parse_sta_info(struct l_genl_attr *attr,
 
 			info->cur_rssi = *(const int8_t *) data;
 
+			break;
+		case NL80211_STA_INFO_RX_BITRATE:
+			if (!l_genl_attr_recurse(attr, &nested))
+				return false;
+
+			if (!netdev_parse_bitrate(&nested, &info->rx_bitrate,
+							&info->rx_mcs))
+				return false;
+
+			break;
+
+		case NL80211_STA_INFO_TX_BITRATE:
+			if (!l_genl_attr_recurse(attr, &nested))
+				return false;
+
+			if (!netdev_parse_bitrate(&nested, &info->tx_bitrate,
+							&info->tx_mcs))
+				return false;
+
 			break;
 		}
 	}
diff --git a/src/netdev.h b/src/netdev.h
index 5cf38076..c5d1ff53 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -116,6 +116,10 @@ typedef void (*netdev_station_watch_func_t)(struct netdev *netdev,
 
 struct netdev_station_info {
 	int8_t cur_rssi;
+	uint32_t rx_bitrate;
+	uint8_t rx_mcs;
+	uint32_t tx_bitrate;
+	uint8_t tx_mcs;
 };
 
 typedef void (*netdev_get_station_cb_t)(struct netdev *netdev,
-- 
2.26.2

[PATCH 5/8] dbus: add diagnostic interface definition

[James Prestwood]

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

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

diff --git a/src/dbus.h b/src/dbus.h
index 045b1bcb..3e2018d8 100644
--- a/src/dbus.h
+++ b/src/dbus.h
@@ -39,6 +39,7 @@
 #define IWD_P2P_PEER_INTERFACE "net.connman.iwd.p2p.Peer"
 #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_BASE_PATH "/net/connman/iwd"
 #define IWD_AGENT_MANAGER_PATH IWD_BASE_PATH
-- 
2.26.2

Re: [PATCH 5/8] dbus: add diagnostic interface definition

[Denis Kenzior]

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

Hi James,

On 1/11/21 11:12 AM, James Prestwood wrote:
> ---
>   src/dbus.h | 1 +
>   1 file changed, 1 insertion(+)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 6/8] netdev: parse expected throughput in netdev_get_station

[James Prestwood]

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

---
 src/netdev.c | 8 ++++++++
 src/netdev.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/src/netdev.c b/src/netdev.c
index 6fb33847..ba5912fd 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -4064,6 +4064,14 @@ static bool netdev_parse_sta_info(struct l_genl_attr *attr,
 				return false;
 
 			break;
+
+		case NL80211_STA_INFO_EXPECTED_THROUGHPUT:
+			if (len != 4)
+				return false;
+
+			info->expected_throughput = *(const uint32_t *)data;
+
+			break;
 		}
 	}
 
diff --git a/src/netdev.h b/src/netdev.h
index c5d1ff53..afff33ab 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -120,6 +120,7 @@ struct netdev_station_info {
 	uint8_t rx_mcs;
 	uint32_t tx_bitrate;
 	uint8_t tx_mcs;
+	uint32_t expected_throughput;
 };
 
 typedef void (*netdev_get_station_cb_t)(struct netdev *netdev,
-- 
2.26.2

[PATCH 7/8] station: create StationDiagnostic interface

[James Prestwood]

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

This interface sits aside the regular station interface but
provides low level connection details for diagnostic and
testing purposes.
---
 src/station.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/src/station.c b/src/station.c
index 1e3706af..8cdd5443 100644
--- a/src/station.c
+++ b/src/station.c
@@ -77,6 +77,7 @@ struct station {
 	struct l_dbus_message *hidden_pending;
 	struct l_dbus_message *disconnect_pending;
 	struct l_dbus_message *scan_pending;
+	struct l_dbus_message *get_station_pending;
 	struct signal_agent *signal_agent;
 	uint32_t dbus_scan_id;
 	uint32_t quick_scan_id;
@@ -3333,6 +3334,9 @@ static struct station *station_create(struct netdev *netdev)
 
 	l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
 					IWD_STATION_INTERFACE, station);
+	l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
+					IWD_STATION_DIAGNOSTIC_INTERFACE,
+					station);
 
 	if (netconfig_enabled)
 		station->netconfig = netconfig_new(netdev_get_ifindex(netdev));
@@ -3455,6 +3459,111 @@ static void station_destroy_interface(void *user_data)
 	station_free(station);
 }
 
+/*
+ * Helper to append a dictionary value. This will only work for basic types.
+ */
+static void append_dict(struct l_dbus_message_builder *builder,
+			const char *name, const char *type, const void *data)
+{
+	l_dbus_message_builder_enter_dict(builder, "sv");
+	l_dbus_message_builder_append_basic(builder, 's', name);
+	l_dbus_message_builder_enter_variant(builder, type);
+	l_dbus_message_builder_append_basic(builder, type[0], data);
+	l_dbus_message_builder_leave_variant(builder);
+	l_dbus_message_builder_leave_dict(builder);
+}
+
+static void station_build_diagnostic_dict(struct station *station,
+					struct l_dbus_message *msg,
+					struct netdev_station_info *info)
+{
+	struct l_dbus_message_builder *builder =
+					l_dbus_message_builder_new(msg);
+	int16_t rssi = (int16_t)info->cur_rssi;
+
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+
+	append_dict(builder, "ConnectedBss", "s", util_address_to_string(
+						station->connected_bss->addr));
+	append_dict(builder, "RSSI", "n", &rssi);
+	append_dict(builder, "TxBitrate", "u", &info->tx_bitrate);
+	append_dict(builder, "TxMCS", "y", &info->tx_mcs);
+	append_dict(builder, "RxBitrate", "u", &info->rx_bitrate);
+	append_dict(builder, "RxMCS", "y", &info->rx_mcs);
+	append_dict(builder, "ExpectedThroughput", "u",
+				&info->expected_throughput);
+
+	l_dbus_message_builder_leave_array(builder);
+	l_dbus_message_builder_finalize(builder);
+	l_dbus_message_builder_destroy(builder);
+}
+
+static void station_get_diagnostic_cb(struct netdev *netdev,
+					struct netdev_station_info *info,
+					void *user_data)
+{
+	struct station *station = user_data;
+	struct l_dbus_message *reply;
+
+	if (!info) {
+		reply = dbus_error_aborted(station->get_station_pending);
+		goto done;
+	}
+
+	reply = l_dbus_message_new_method_return(station->get_station_pending);
+
+	station_build_diagnostic_dict(station, reply, info);
+
+done:
+	dbus_pending_reply(&station->get_station_pending, reply);
+}
+
+static void station_get_diagnostic_destroy(void *user_data)
+{
+	struct station *station = user_data;
+	struct l_dbus_message *reply;
+
+	if (station->get_station_pending) {
+		reply = l_dbus_message_new_method_return(
+						station->get_station_pending);
+		dbus_pending_reply(&station->get_station_pending, reply);
+	}
+}
+
+static struct l_dbus_message *station_get_diagnostics(struct l_dbus *dbus,
+						struct l_dbus_message *message,
+						void *user_data)
+{
+	struct station *station = user_data;
+
+	/*
+	 * At this time all values depend on a connected state.
+	 */
+	if (station->state != STATION_STATE_CONNECTED)
+		return dbus_error_not_connected(message);
+
+	if (netdev_get_station(station->netdev, station->connected_bss->addr,
+				station_get_diagnostic_cb, station,
+				station_get_diagnostic_destroy) < 0)
+		return dbus_error_busy(message);
+
+	station->get_station_pending = l_dbus_message_ref(message);
+
+	return NULL;
+}
+
+static void station_setup_diagnostic_interface(
+					struct l_dbus_interface *interface)
+{
+	l_dbus_interface_method(interface, "GetDiagnostics", 0,
+				station_get_diagnostics, "a{sv}", "",
+				"diagnostics");
+}
+
+static void station_destroy_diagnostic_interface(void *user_data)
+{
+}
+
 static void station_netdev_watch(struct netdev *netdev,
 				enum netdev_watch_event event, void *userdata)
 {
@@ -3483,6 +3592,11 @@ static int station_init(void)
 	l_dbus_register_interface(dbus_get_bus(), IWD_STATION_INTERFACE,
 					station_setup_interface,
 					station_destroy_interface, false);
+	l_dbus_register_interface(dbus_get_bus(),
+					IWD_STATION_DIAGNOSTIC_INTERFACE,
+					station_setup_diagnostic_interface,
+					station_destroy_diagnostic_interface,
+					false);
 
 	if (!l_settings_get_uint(iwd_get_config(), "General",
 					"ManagementFrameProtection",
@@ -3521,6 +3635,8 @@ static int station_init(void)
 
 static void station_exit(void)
 {
+	l_dbus_unregister_interface(dbus_get_bus(),
+					IWD_STATION_DIAGNOSTIC_INTERFACE);
 	l_dbus_unregister_interface(dbus_get_bus(), IWD_STATION_INTERFACE);
 	netdev_watch_remove(netdev_watch);
 	l_queue_destroy(station_list, NULL);
-- 
2.26.2

Re: [PATCH 7/8] station: create StationDiagnostic interface

[Denis Kenzior]

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

Hi James,

On 1/11/21 11:12 AM, James Prestwood wrote:
> This interface sits aside the regular station interface but
> provides low level connection details for diagnostic and
> testing purposes.
> ---
>   src/station.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
> 

<snip>

> @@ -3455,6 +3459,111 @@ static void station_destroy_interface(void *user_data)
>   	station_free(station);
>   }
>   
> +/*
> + * Helper to append a dictionary value. This will only work for basic types.
> + */
> +static void append_dict(struct l_dbus_message_builder *builder,
> +			const char *name, const char *type, const void *data)
> +{
> +	l_dbus_message_builder_enter_dict(builder, "sv");
> +	l_dbus_message_builder_append_basic(builder, 's', name);
> +	l_dbus_message_builder_enter_variant(builder, type);
> +	l_dbus_message_builder_append_basic(builder, type[0], data);
> +	l_dbus_message_builder_leave_variant(builder);
> +	l_dbus_message_builder_leave_dict(builder);
> +}

Maybe this belongs in src/dbus.[ch] somewhere.

<snip>

> +static void station_get_diagnostic_cb(struct netdev *netdev,
> +					struct netdev_station_info *info,
> +					void *user_data)
> +{
> +	struct station *station = user_data;
> +	struct l_dbus_message *reply;
> +
> +	if (!info) {
> +		reply = dbus_error_aborted(station->get_station_pending);
> +		goto done;
> +	}
> +
> +	reply = l_dbus_message_new_method_return(station->get_station_pending);
> +
> +	station_build_diagnostic_dict(station, reply, info);

Not sure why you break this into a separate step?

> +
> +done:
> +	dbus_pending_reply(&station->get_station_pending, reply);
> +}
> +
> +static void station_get_diagnostic_destroy(void *user_data)
> +{
> +	struct station *station = user_data;
> +	struct l_dbus_message *reply;
> +
> +	if (station->get_station_pending) {
> +		reply = l_dbus_message_new_method_return(
> +						station->get_station_pending);

Hmm, an empty message?  Maybe use dbus_error_aborted instead?  See how device.c 
does this for example.

> +		dbus_pending_reply(&station->get_station_pending, reply);
> +	}
> +}
> +
> +static struct l_dbus_message *station_get_diagnostics(struct l_dbus *dbus,
> +						struct l_dbus_message *message,
> +						void *user_data)
> +{
> +	struct station *station = user_data;
> +
> +	/*
> +	 * At this time all values depend on a connected state.
> +	 */
> +	if (station->state != STATION_STATE_CONNECTED)
> +		return dbus_error_not_connected(message);
> +
> +	if (netdev_get_station(station->netdev, station->connected_bss->addr,
> +				station_get_diagnostic_cb, station,
> +				station_get_diagnostic_destroy) < 0)
> +		return dbus_error_busy(message);

Maybe use dbus_error_from_errno instead?

> +
> +	station->get_station_pending = l_dbus_message_ref(message);
> +
> +	return NULL;
> +}
> +

<snip>

Regards,
-Denis

[PATCH 8/8] test: add a script for GetDiagnostics

[James Prestwood]

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

---
 test/get-diagnostics | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 test/get-diagnostics

diff --git a/test/get-diagnostics b/test/get-diagnostics
new file mode 100755
index 00000000..c508c58c
--- /dev/null
+++ b/test/get-diagnostics
@@ -0,0 +1,37 @@
+#!/usr/bin/python3
+
+import sys
+import dbus
+
+# Map dict keys to units. Units can be a function/lambda which should return the
+# entire value with units as a string.
+unit_map = {
+    "ConnectedBss" : None,
+    "RSSI" : "dBm",
+    "RxBitrate" : lambda k : str(100 * int(k)) + ' Kbps',
+    "RxMCS" : lambda i : str(int(i)),
+    "TxBitrate" : lambda k : str(100 * int(k)) + ' Kbps',
+    "TxMCS" : lambda i : str(int(i)),
+    "ExpectedThroughput" : "Kbps"
+}
+
+if (len(sys.argv) != 2):
+    print("Usage: %s <device>" % (sys.argv[0]))
+    sys.exit(1)
+
+bus = dbus.SystemBus()
+device = dbus.Interface(bus.get_object("net.connman.iwd", sys.argv[1]),
+                                    "net.connman.iwd.StationDiagnostic")
+diagnostics = device.GetDiagnostics()
+
+for key, value in diagnostics.items():
+    if key in unit_map:
+        units = unit_map[key]
+        if units is None:
+            units = ''
+        elif callable(units):
+            value = units(value)
+            units = ''
+
+        print('%s : %s %s' % (key, value, units))
+
-- 
2.26.2

Re: [PATCH 1/8] doc: diagnostic DBus interface definition

[Denis Kenzior]

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

Hi James,

On 1/11/21 11:12 AM, James Prestwood wrote:
> ---
>   doc/diagnostics.txt | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>   create mode 100644 doc/diagnostics.txt
> 

I added an [experimental] tag to this and applied, thanks.

Regards,
-Denis