[RFCv3] Document P2P dbus interfaces

[RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

From: Andrew Zaborowski <andrew.zaborowski@intel.com>

Proposed minimum P2P interfaces for establishing basic connections.  The
device discovery results in creation of P2PPeer objects.

In the Wi-Fi Display API we are passing raw IE data because there's a
relatively big set of different values that may be encoded in them.  We
could reduce them to 2-3 bools and integers but this might limit the
client implementations feature set.
---
 doc/p2p-api.txt                | 82 ++++++++++++++++++++++++++++++++++
 doc/p2p-peer-api.txt           | 66 +++++++++++++++++++++++++++
 doc/p2p-wfd-agent-api.txt      | 34 ++++++++++++++
 doc/signal-level-agent-api.txt |  4 ++
 4 files changed, 186 insertions(+)
 create mode 100644 doc/p2p-api.txt
 create mode 100644 doc/p2p-peer-api.txt
 create mode 100644 doc/p2p-wfd-agent-api.txt

diff --git a/doc/p2p-api.txt b/doc/p2p-api.txt
new file mode 100644
index 00000000..84f862e5
--- /dev/null
+++ b/doc/p2p-api.txt
@@ -0,0 +1,82 @@
+P2P hierarchy
+=============
+
+Service		net.connman.iwd
+Interface	net.connman.iwd.P2P [Experimental]
+Object path	/{phy0,phy1,...}
+
+Methods		array(on) GetPeers()
+
+			Returns a list (possibly empty) of detected P2P peers.
+			Each record returned contains a tuple of the following
+			values.
+
+			object Object
+
+				net.connman.iwd.P2PPeer object representing
+				the peer device.
+
+			int16 SignalStrength
+
+				Peer's maximum signal strength expressed
+				in 100 * dBm.  The value is the range of 0
+				(strongest signal) to -10000 (weakest signal)
+
+		void RegisterSignalLevelAgent(object path,
+						 array(int16) levels)
+
+			Register the agent object to receive signal strength
+			level change notifications on the
+			net.connman.iwd.SignalLevelAgent interface, see
+			signal-level-agent-api.txt.  The "levels"
+			parameters decides the thresholds in dBm that will
+			generate a call to the agent's Changed
+			method whenever current RSSI crosses any of the
+			values.  The values must be passed in descending
+			order.  The number and distance between requested
+			threshold values is a compromise between resolution
+			and the frequency of system wakeups and
+			context-switches that are going to be occuring to
+			update the client's signal meter.  Only one agent
+			can be registered at any time.
+
+			Possible Errors: [service].Error.InvalidArguments
+					 [service].Error.Failed
+					 [service].Error.AlreadyExists
+					 [service].Error.NotSupported
+
+		void RegisterWFDService(object path, array(byte) payload)
+
+			Register the Wi-Fi Display service running on local
+			host with this P2P device so that device discovery
+			and connection setup phases include necessary WFD
+			information for the peers and receive peer's WFD
+			service information.  An object may be provided to
+			receive relevant WFD information about peers and
+			connections through method calls on the object's
+			net.connman.iwd.P2PWFDAgent interface.  See
+			p2p-wfd-agent-api.txt.  Only one agent can be
+			registered at any time.  The byte array provided
+			will be included in WFD Information Elements
+			present in relevant frames sent to peers.
+
+			Possible Errors: [service].Error.InvalidArguments
+					 [service].Error.AlreadyExists
+					 [service].Error.NotSupported
+
+Properties	boolean Enabled [readwrite]
+
+			Whether local P2P device is started, i.e. is scanning
+			for peers, is discoverable by peers and/or connected
+			to peer(s).
+
+		string Name [readwrite]
+
+			Sets local P2P device name as it is going to be
+			presented on other devices that we will connect to
+			or ones that discover us in scanning.
+
+		uint16 MaxConnections [readonly]
+
+			Maximum number of concurrent P2P peers that local
+			hardware is capable of connecting to.  Often 1.
diff --git a/doc/p2p-peer-api.txt b/doc/p2p-peer-api.txt
new file mode 100644
index 00000000..898f4fb9
--- /dev/null
+++ b/doc/p2p-peer-api.txt
@@ -0,0 +1,66 @@
+P2PPeer hierarchy
+=================
+
+Service		net.connman.iwd
+Interface	net.connman.iwd.P2PPeer [Experimental]
+Object path	/{phy0,phy1,...}/p2p_peers/{aa_bb_cc_dd_ee_ff}
+
+Methods		ConnectPushButton()
+
+			Connect to the P2P peer in the Push Button mode
+			using the device pointed to by the .Device property.
+			Returns when connection is complete.
+
+			Possible errors: net.connman.iwd.Aborted
+					 net.connman.iwd.Busy
+					 net.connman.iwd.Failed
+					 net.connman.iwd.NotSupported
+					 net.connman.iwd.Timeout
+					 net.connman.iwd.InProgress
+
+		ConnectPIN(string pin)
+
+			Connect to the P2P peer in PIN mode.  Returns when
+			connection is complete.
+
+			See net.connman.iwd.WiFiSimpleConfiguration.StartPIN()
+			for how the pin argument is used.
+
+			Possible errors: net.connman.iwd.Aborted
+					 net.connman.iwd.Busy
+					 net.connman.iwd.Failed
+					 net.connman.iwd.NotSupported
+					 net.connman.iwd.Timeout
+					 net.connman.iwd.InProgress
+
+		Disconnect() [optional]
+
+			If connected, disconnect from this peer.
+
+			Possible errors: net.connman.iwd.Failed
+
+Properties	string Name [readonly]
+
+			P2P Peer's display name
+
+		string DeviceCategory [readonly]
+
+			The category part of the peer's declared
+			Primary Device Type.
+
+		string DeviceSubcategory [readonly, optional]
+
+			The Sub Category part of the peer's declared
+			Primary Device Type.
+
+		object Device [readonly]
+
+			The object with a net.connman.iwd.P2P interface
+			that discovered this peer.
+
+		boolean Connected [readonly]
+
+			Whether there's currently an active connection
+			to this peer.  The property is read-only and
+			changes as a result of the Connect and
+			Disconnect methods calls.
diff --git a/doc/p2p-wfd-agent-api.txt b/doc/p2p-wfd-agent-api.txt
new file mode 100644
index 00000000..be0ee01b
--- /dev/null
+++ b/doc/p2p-wfd-agent-api.txt
@@ -0,0 +1,34 @@
+P2PWFDAgent hierarchy
+=====================
+
+Service		unique name
+Interface	net.connman.iwd.P2PWFDAgent
+Object path	freely definable
+
+Methods		void Release(object device)
+
+			This method gets called when IWD unregisters the
+			WFD service on a specific P2P device. An agent can
+			use it to do cleanup tasks. There is no need to
+			unregister the agent, because when this method
+			gets called it has already been unregistered.
+
+		void WFDPeersChanged(object device,
+					array(object, array(byte)) peers)
+
+			Called when the set of discovered WFD-capable
+			peers has changed during device discovery.
+
+			The device object has the net.connman.iwd.P2P
+			interface while the objects in the peers array
+			have net.connman.iwd.P2PPeer interfaces.  The byte
+			array included contains the reassembled payload of
+			the WFD Information Elements presented by the peer.
+
+		void NewConnection(object peer, array(byte) payload)
+
+			Called when a new P2P connection has been established
+			to a WFD-capable peer.  The peer object has the
+			net.connman.iwd.P2PPerr interface.  The byte array
+			contains the reassembled payload of the WFD
+			Information Elements presented by the peer.
diff --git a/doc/signal-level-agent-api.txt b/doc/signal-level-agent-api.txt
index 847e7ca3..1c786d96 100644
--- a/doc/signal-level-agent-api.txt
+++ b/doc/signal-level-agent-api.txt
@@ -31,3 +31,7 @@ Methods		void Release(object device)
 			0 would mean signal is received at -40 or more dBm
 			and 3 would mean below -60 dBm and might correspond
 			to 1 out of 4 bars on a UI signal meter.
+
+			The device parameter may be either a
+			net.connman.iwd.Station object or a
+			net.connman.iwd.P2PPeer object.
-- 
2.20.1

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

On 10/10/19 3:51 PM, Andrew Zaborowski wrote:
> From: Andrew Zaborowski <andrew.zaborowski@intel.com>
> 
> Proposed minimum P2P interfaces for establishing basic connections.  The
> device discovery results in creation of P2PPeer objects.
> 
> In the Wi-Fi Display API we are passing raw IE data because there's a
> relatively big set of different values that may be encoded in them.  We
> could reduce them to 2-3 bools and integers but this might limit the
> client implementations feature set.

So I'm not really a huge fan of this.  We'd still need to be able to 
understand what is inside the IE in order to verify it properly.  I'd 
err on the side of providing some attributes instead of full on binary 
IEs.

Also, some of the subelements involved should really be filled in by iwd 
itself and not the external application.  Otherwise you'll end up with 
some sort of Frankenstein where the app provides some info but we'd need 
to rewrite or modify it anyway.

> ---
>   doc/p2p-api.txt                | 82 ++++++++++++++++++++++++++++++++++
>   doc/p2p-peer-api.txt           | 66 +++++++++++++++++++++++++++
>   doc/p2p-wfd-agent-api.txt      | 34 ++++++++++++++
>   doc/signal-level-agent-api.txt |  4 ++
>   4 files changed, 186 insertions(+)
>   create mode 100644 doc/p2p-api.txt
>   create mode 100644 doc/p2p-peer-api.txt
>   create mode 100644 doc/p2p-wfd-agent-api.txt
> 
> diff --git a/doc/p2p-api.txt b/doc/p2p-api.txt
> new file mode 100644
> index 00000000..84f862e5
> --- /dev/null
> +++ b/doc/p2p-api.txt
> @@ -0,0 +1,82 @@
> +P2P hierarchy
> +=============
> +
> +Service		net.connman.iwd
> +Interface	net.connman.iwd.P2P [Experimental]
> +Object path	/{phy0,phy1,...}

So we changed the paths for 1.0 and this might need to be updated.

> +
> +Methods		array(on) GetPeers()
> +
> +			Returns a list (possibly empty) of detected P2P peers.
> +			Each record returned contains a tuple of the following
> +			values.
> +
> +			object Object
> +
> +				net.connman.iwd.P2PPeer object representing

Name this iwd.Peer?

> +				the peer device.
> +
> +			int16 SignalStrength
> +
> +				Peer's maximum signal strength expressed
> +				in 100 * dBm.  The value is the range of 0
> +				(strongest signal) to -10000 (weakest signal)
> +
> +		void RegisterSignalLevelAgent(object path,
> +						 array(int16) levels)
> +
> +			Register the agent object to receive signal strength
> +			level change notifications on the
> +			net.connman.iwd.SignalLevelAgent interface, see
> +			signal-level-agent-api.txt.  The "levels"

We moved this into the station api.  So maybe just refer to that here...

Though really, I wonder if you even want to bother with this for the 
first iteration?  I'd maybe leave this out?

> +			parameters decides the thresholds in dBm that will
> +			generate a call to the agent's Changed
> +			method whenever current RSSI crosses any of the
> +			values.  The values must be passed in descending
> +			order.  The number and distance between requested
> +			threshold values is a compromise between resolution
> +			and the frequency of system wakeups and
> +			context-switches that are going to be occuring to
> +			update the client's signal meter.  Only one agent
> +			can be registered at any time.
> +
> +			Possible Errors: [service].Error.InvalidArguments
> +					 [service].Error.Failed
> +					 [service].Error.AlreadyExists
> +					 [service].Error.NotSupported
> +
> +		void RegisterWFDService(object path, array(byte) payload)

I would maybe make this a bit more generic in case other P2P services 
are supported in the future, like file sharing, etc.

But, I'm not sure this belongs here?  P2P is a per-phy interface; do you 
really want the clients to register WiFi Display per phy?  Or just once 
and have it apply to all devices in the system, current or ones yet to 
be plugged in?  Perhaps we need a ServiceManager or add this to 
AgentManager API?

I suppose it might be in theory possible to support both a Sink and a 
Source role on different phys, but then shouldn't you be distinguishing 
between the roles somehow?

> +
> +			Register the Wi-Fi Display service running on local
> +			host with this P2P device so that device discovery
> +			and connection setup phases include necessary WFD

okay...

> +			information for the peers and receive peer's WFD
> +			service information.  An object may be provided to
> +			receive relevant WFD information about peers and
> +			connections through method calls on the object's
> +			net.connman.iwd.P2PWFDAgent interface.  See
> +			p2p-wfd-agent-api.txt.  Only one agent can be
> +			registered at any time.  The byte array provided
> +			will be included in WFD Information Elements
> +			present in relevant frames sent to peers.

So besides the IE information (which I don't really want transported in 
binary), what real use is the WFD agent?

> +
> +			Possible Errors: [service].Error.InvalidArguments
> +					 [service].Error.AlreadyExists
> +					 [service].Error.NotSupported
> +
> +Properties	boolean Enabled [readwrite]
> +
> +			Whether local P2P device is started, i.e. is scanning
> +			for peers, is discoverable by peers and/or connected
> +			to peer(s).
> +
> +		string Name [readwrite]
> +
> +			Sets local P2P device name as it is going to be
> +			presented on other devices that we will connect to
> +			or ones that discover us in scanning.
> +
> +		uint16 MaxConnections [readonly]
> +
> +			Maximum number of concurrent P2P peers that local
> +			hardware is capable of connecting to.  Often 1.

I think we've talked about this before.  Given what we now know, I would 
drop this for now.  I think the likelihood of this becoming > 1 is 
astronomically small and we shouldn't even worry about this.  This means 
we can also move some attributes / methods from the Peer interface onto 
this one.

> diff --git a/doc/p2p-peer-api.txt b/doc/p2p-peer-api.txt
> new file mode 100644
> index 00000000..898f4fb9
> --- /dev/null
> +++ b/doc/p2p-peer-api.txt
> @@ -0,0 +1,66 @@
> +P2PPeer hierarchy
> +=================
> +
> +Service		net.connman.iwd
> +Interface	net.connman.iwd.P2PPeer [Experimental]
.iwd.Peer?

> +Object path	/{phy0,phy1,...}/p2p_peers/{aa_bb_cc_dd_ee_ff}

Object path needs to be updated post 1.0

> +
> +Methods		ConnectPushButton()
> +
> +			Connect to the P2P peer in the Push Button mode
> +			using the device pointed to by the .Device property.
> +			Returns when connection is complete.

Why are you method names different from SimpleConfiguration?  in fact, 
why not just add .SimpleConfiguration interface to the same object path 
and just have clients use that?

> +
> +			Possible errors: net.connman.iwd.Aborted
> +					 net.connman.iwd.Busy
> +					 net.connman.iwd.Failed
> +					 net.connman.iwd.NotSupported
> +					 net.connman.iwd.Timeout
> +					 net.connman.iwd.InProgress
> +
> +		ConnectPIN(string pin)
> +
> +			Connect to the P2P peer in PIN mode.  Returns when
> +			connection is complete.
> +
> +			See net.connman.iwd.WiFiSimpleConfiguration.StartPIN()
> +			for how the pin argument is used.
> +
> +			Possible errors: net.connman.iwd.Aborted
> +					 net.connman.iwd.Busy
> +					 net.connman.iwd.Failed
> +					 net.connman.iwd.NotSupported
> +					 net.connman.iwd.Timeout
> +					 net.connman.iwd.InProgress
> +
> +		Disconnect() [optional]

A method can't be optional?  Anyway, I'd put this on the P2P interface 
instead.

> +
> +			If connected, disconnect from this peer.
> +
> +			Possible errors: net.connman.iwd.Failed
> +
> +Properties	string Name [readonly]
> +
> +			P2P Peer's display name
> +
> +		string DeviceCategory [readonly]
> +
> +			The category part of the peer's declared
> +			Primary Device Type.
> +
> +		string DeviceSubcategory [readonly, optional]
> +
> +			The Sub Category part of the peer's declared
> +			Primary Device Type.
> +
> +		object Device [readonly]
> +
> +			The object with a net.connman.iwd.P2P interface
> +			that discovered this peer.

Okay

> +
> +		boolean Connected [readonly]
> +
> +			Whether there's currently an active connection
> +			to this peer.  The property is read-only and
> +			changes as a result of the Connect and
> +			Disconnect methods calls.

This wording might need to be updated....

> diff --git a/doc/p2p-wfd-agent-api.txt b/doc/p2p-wfd-agent-api.txt
> new file mode 100644
> index 00000000..be0ee01b
> --- /dev/null
> +++ b/doc/p2p-wfd-agent-api.txt
> @@ -0,0 +1,34 @@
> +P2PWFDAgent hierarchy
> +=====================
> +
> +Service		unique name
> +Interface	net.connman.iwd.P2PWFDAgent
> +Object path	freely definable
> +
> +Methods		void Release(object device)
> +
> +			This method gets called when IWD unregisters the
> +			WFD service on a specific P2P device. An agent can
> +			use it to do cleanup tasks. There is no need to
> +			unregister the agent, because when this method
> +			gets called it has already been unregistered.
> +
> +		void WFDPeersChanged(object device,
> +					array(object, array(byte)) peers)
> +
> +			Called when the set of discovered WFD-capable
> +			peers has changed during device discovery.
> +
> +			The device object has the net.connman.iwd.P2P
> +			interface while the objects in the peers array
> +			have net.connman.iwd.P2PPeer interfaces.  The byte
> +			array included contains the reassembled payload of
> +			the WFD Information Elements presented by the peer.

Okay, so you want to notify the agent instead of adding a WFD specific 
interface to the Peer object.  I still wonder if we can avoid 
transporting raw IEs here... Also, how are you making sure that the 
peers are actually ones that the agent cares about?  There's no sense in 
sending Source role peer info if we're also a Source...

Can we be a source and a sink at the same time?

> +
> +		void NewConnection(object peer, array(byte) payload)
> +
> +			Called when a new P2P connection has been established
> +			to a WFD-capable peer.  The peer object has the
> +			net.connman.iwd.P2PPerr interface.  The byte array

typo

> +			contains the reassembled payload of the WFD
> +			Information Elements presented by the peer.

So I'm not entirely sure how this is useful?  The fact that we made a 
connection to a peer is already available via the Peer.Connected 
property.  Is the payload somehow different from what was sent via 
WFDPeersChanged?

> diff --git a/doc/signal-level-agent-api.txt b/doc/signal-level-agent-api.txt
> index 847e7ca3..1c786d96 100644
> --- a/doc/signal-level-agent-api.txt
> +++ b/doc/signal-level-agent-api.txt
> @@ -31,3 +31,7 @@ Methods		void Release(object device)
>   			0 would mean signal is received at -40 or more dBm
>   			and 3 would mean below -60 dBm and might correspond
>   			to 1 out of 4 bars on a UI signal meter.
> +
> +			The device parameter may be either a
> +			net.connman.iwd.Station object or a
> +			net.connman.iwd.P2PPeer object.
> 

This file no longer exists FYI.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 14 Nov 2019 at 05:42, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/10/19 3:51 PM, Andrew Zaborowski wrote:
> > Proposed minimum P2P interfaces for establishing basic connections.  The
> > device discovery results in creation of P2PPeer objects.
> >
> > In the Wi-Fi Display API we are passing raw IE data because there's a
> > relatively big set of different values that may be encoded in them.  We
> > could reduce them to 2-3 bools and integers but this might limit the
> > client implementations feature set.
>
> So I'm not really a huge fan of this.  We'd still need to be able to
> understand what is inside the IE in order to verify it properly.  I'd
> err on the side of providing some attributes instead of full on binary
> IEs.
>
> Also, some of the subelements involved should really be filled in by iwd
> itself and not the external application.  Otherwise you'll end up with
> some sort of Frankenstein where the app provides some info but we'd need
> to rewrite or modify it anyway.

I didn't plan to interpret the contents of the IE in any way.  The
idea would be to have little service-specific code, and in case of WFD
there's no need to do anything more than passing the ies.

So I see only two options, either full on binary IE passed in both
directions between iwd and the service implementation, or full on
individual values from the WFD subelements.  There's a lot of them
though, for example for a probe response you have at least:

 * 12 capability bits in a bitmap
 * RTSP tcp port
 * throughput
 * optional coupled sink status
 * optional coupled sink MAC
 * R2 support (true/false)

+ 10 or more optional values...

I just had a look at the how gnome-screencast is passing those to NM /
wpa_supplicant and they basically hardcode the IE in one place in the
code and never touch the contents...  On receival they only check if
the WFD IEs were present and don't really look at the contents either.
This is very new code though and I haven't looked at openwfd or
miraclecast in detail.

>
> > ---
> >   doc/p2p-api.txt                | 82 ++++++++++++++++++++++++++++++++++
> >   doc/p2p-peer-api.txt           | 66 +++++++++++++++++++++++++++
> >   doc/p2p-wfd-agent-api.txt      | 34 ++++++++++++++
> >   doc/signal-level-agent-api.txt |  4 ++
> >   4 files changed, 186 insertions(+)
> >   create mode 100644 doc/p2p-api.txt
> >   create mode 100644 doc/p2p-peer-api.txt
> >   create mode 100644 doc/p2p-wfd-agent-api.txt
> >
> > diff --git a/doc/p2p-api.txt b/doc/p2p-api.txt
> > new file mode 100644
> > index 00000000..84f862e5
> > --- /dev/null
> > +++ b/doc/p2p-api.txt
> > @@ -0,0 +1,82 @@
> > +P2P hierarchy
> > +=============
> > +
> > +Service              net.connman.iwd
> > +Interface    net.connman.iwd.P2P [Experimental]
> > +Object path  /{phy0,phy1,...}
>
> So we changed the paths for 1.0 and this might need to be updated.

Right.

>
> > +
> > +Methods              array(on) GetPeers()
> > +
> > +                     Returns a list (possibly empty) of detected P2P peers.
> > +                     Each record returned contains a tuple of the following
> > +                     values.
> > +
> > +                     object Object
> > +
> > +                             net.connman.iwd.P2PPeer object representing
>
> Name this iwd.Peer?

Sounds very generic, are we sure we won't have another spec that uses
"peers" too?

>
> > +                             the peer device.
> > +
> > +                     int16 SignalStrength
> > +
> > +                             Peer's maximum signal strength expressed
> > +                             in 100 * dBm.  The value is the range of 0
> > +                             (strongest signal) to -10000 (weakest signal)
> > +
> > +             void RegisterSignalLevelAgent(object path,
> > +                                              array(int16) levels)
> > +
> > +                     Register the agent object to receive signal strength
> > +                     level change notifications on the
> > +                     net.connman.iwd.SignalLevelAgent interface, see
> > +                     signal-level-agent-api.txt.  The "levels"
>
> We moved this into the station api.  So maybe just refer to that here...
>
> Though really, I wonder if you even want to bother with this for the
> first iteration?  I'd maybe leave this out?

Yeah, I wasn't going to implement it at this time.  But since you'll
have RSSI in every mode of wireless connections, AP, adhoc, ...
wouldn't it make sense to have a common interface and share code in
iwd and in the clients?

>
> > +                     parameters decides the thresholds in dBm that will
> > +                     generate a call to the agent's Changed
> > +                     method whenever current RSSI crosses any of the
> > +                     values.  The values must be passed in descending
> > +                     order.  The number and distance between requested
> > +                     threshold values is a compromise between resolution
> > +                     and the frequency of system wakeups and
> > +                     context-switches that are going to be occuring to
> > +                     update the client's signal meter.  Only one agent
> > +                     can be registered at any time.
> > +
> > +                     Possible Errors: [service].Error.InvalidArguments
> > +                                      [service].Error.Failed
> > +                                      [service].Error.AlreadyExists
> > +                                      [service].Error.NotSupported
> > +
> > +             void RegisterWFDService(object path, array(byte) payload)
>
> I would maybe make this a bit more generic in case other P2P services
> are supported in the future, like file sharing, etc.

I was thinking of having a separate agent interface and separate
register method for each service, the parameters are going to be
different... but we could also have a common register method and
another method on the service's agent to query the actual values of
the parameters (at the cost of additional round trips)

>
> But, I'm not sure this belongs here?  P2P is a per-phy interface; do you
> really want the clients to register WiFi Display per phy?  Or just once
> and have it apply to all devices in the system, current or ones yet to
> be plugged in?  Perhaps we need a ServiceManager or add this to
> AgentManager API?

Yep, perhaps a ServiceManager (P2PServiceManager?) is a better place...

>
> I suppose it might be in theory possible to support both a Sink and a
> Source role on different phys, but then shouldn't you be distinguishing
> between the roles somehow?

It is possible but luckily it's all up to the wfd implementation.
We'd only see this information if we wanted to build the WFD IEs
ourselves like you suggest but we wouldn't be doing anything more than
encoding / decoding it from the IEs.

>
> > +
> > +                     Register the Wi-Fi Display service running on local
> > +                     host with this P2P device so that device discovery
> > +                     and connection setup phases include necessary WFD
>
> okay...
>
> > +                     information for the peers and receive peer's WFD
> > +                     service information.  An object may be provided to
> > +                     receive relevant WFD information about peers and
> > +                     connections through method calls on the object's
> > +                     net.connman.iwd.P2PWFDAgent interface.  See
> > +                     p2p-wfd-agent-api.txt.  Only one agent can be
> > +                     registered at any time.  The byte array provided
> > +                     will be included in WFD Information Elements
> > +                     present in relevant frames sent to peers.
>
> So besides the IE information (which I don't really want transported in
> binary), what real use is the WFD agent?

So it would be receiving events on a new WFD connection and during
discovery it would be receiving the capabilities etc. of the
discovered peers.

>
> > +
> > +                     Possible Errors: [service].Error.InvalidArguments
> > +                                      [service].Error.AlreadyExists
> > +                                      [service].Error.NotSupported
> > +
> > +Properties   boolean Enabled [readwrite]
> > +
> > +                     Whether local P2P device is started, i.e. is scanning
> > +                     for peers, is discoverable by peers and/or connected
> > +                     to peer(s).
> > +
> > +             string Name [readwrite]
> > +
> > +                     Sets local P2P device name as it is going to be
> > +                     presented on other devices that we will connect to
> > +                     or ones that discover us in scanning.
> > +
> > +             uint16 MaxConnections [readonly]
> > +
> > +                     Maximum number of concurrent P2P peers that local
> > +                     hardware is capable of connecting to.  Often 1.
>
> I think we've talked about this before.  Given what we now know, I would
> drop this for now.  I think the likelihood of this becoming > 1 is
> astronomically small and we shouldn't even worry about this.

So I don't really have enough information to say that, the 2 P2P
capable adapters that I worked with only support one client or one GO
interface but those are only 2 adapters.

I have actually changed this since this proposal to be named
AvailableConnections and switch between 1 and 0 as the device becomes
busy.  So it has an additional use of indicating whether a connection
is in progress... the Connected property on the Peer only becomes 1
when the connection is operative.

> This means
> we can also move some attributes / methods from the Peer interface onto
> this one.

So this is a slightly different topic and I wouldn't do it.
Apparently some device will make sure they become the GO during GO
Negotiation (or as soon as P2P is enabled) and then use the invitation
procedure instead of the group formation procedure, so then they can
be connected to multiple peripherals and this seems to be an actual
use case.

MaxConnections still doesn't sound right because in theory there's no
maximum, but we still need some way to indicate if we're busy.

>
> > diff --git a/doc/p2p-peer-api.txt b/doc/p2p-peer-api.txt
> > new file mode 100644
> > index 00000000..898f4fb9
> > --- /dev/null
> > +++ b/doc/p2p-peer-api.txt
> > @@ -0,0 +1,66 @@
> > +P2PPeer hierarchy
> > +=================
> > +
> > +Service              net.connman.iwd
> > +Interface    net.connman.iwd.P2PPeer [Experimental]
> .iwd.Peer?
>
> > +Object path  /{phy0,phy1,...}/p2p_peers/{aa_bb_cc_dd_ee_ff}
>
> Object path needs to be updated post 1.0
>
> > +
> > +Methods              ConnectPushButton()
> > +
> > +                     Connect to the P2P peer in the Push Button mode
> > +                     using the device pointed to by the .Device property.
> > +                     Returns when connection is complete.
>
> Why are you method names different from SimpleConfiguration?  in fact,
> why not just add .SimpleConfiguration interface to the same object path
> and just have clients use that?

Could do this, yes, although it would have been nice to have "Connect"
in the method names.  And I'm not sure how to resolve this in the code
since the implementation of the methods will be very different from
the one in wsc.c, preferably I wouldn't duplicate setup_wsc_interface.

>
> > +
> > +                     Possible errors: net.connman.iwd.Aborted
> > +                                      net.connman.iwd.Busy
> > +                                      net.connman.iwd.Failed
> > +                                      net.connman.iwd.NotSupported
> > +                                      net.connman.iwd.Timeout
> > +                                      net.connman.iwd.InProgress
> > +
> > +             ConnectPIN(string pin)
> > +
> > +                     Connect to the P2P peer in PIN mode.  Returns when
> > +                     connection is complete.
> > +
> > +                     See net.connman.iwd.WiFiSimpleConfiguration.StartPIN()
> > +                     for how the pin argument is used.
> > +
> > +                     Possible errors: net.connman.iwd.Aborted
> > +                                      net.connman.iwd.Busy
> > +                                      net.connman.iwd.Failed
> > +                                      net.connman.iwd.NotSupported
> > +                                      net.connman.iwd.Timeout
> > +                                      net.connman.iwd.InProgress
> > +
> > +             Disconnect() [optional]
>
> A method can't be optional?

Well since the optional concept is kind of made up it just means
returning an error when the method or property doesn't apply.

> Anyway, I'd put this on the P2P interface
> instead.
>
> > +
> > +                     If connected, disconnect from this peer.
> > +
> > +                     Possible errors: net.connman.iwd.Failed
> > +
> > +Properties   string Name [readonly]
> > +
> > +                     P2P Peer's display name
> > +
> > +             string DeviceCategory [readonly]
> > +
> > +                     The category part of the peer's declared
> > +                     Primary Device Type.
> > +
> > +             string DeviceSubcategory [readonly, optional]
> > +
> > +                     The Sub Category part of the peer's declared
> > +                     Primary Device Type.
> > +
> > +             object Device [readonly]
> > +
> > +                     The object with a net.connman.iwd.P2P interface
> > +                     that discovered this peer.
>
> Okay
>
> > +
> > +             boolean Connected [readonly]
> > +
> > +                     Whether there's currently an active connection
> > +                     to this peer.  The property is read-only and
> > +                     changes as a result of the Connect and
> > +                     Disconnect methods calls.
>
> This wording might need to be updated....

You mean because we'd only have one connection?

>
> > diff --git a/doc/p2p-wfd-agent-api.txt b/doc/p2p-wfd-agent-api.txt
> > new file mode 100644
> > index 00000000..be0ee01b
> > --- /dev/null
> > +++ b/doc/p2p-wfd-agent-api.txt
> > @@ -0,0 +1,34 @@
> > +P2PWFDAgent hierarchy
> > +=====================
> > +
> > +Service              unique name
> > +Interface    net.connman.iwd.P2PWFDAgent
> > +Object path  freely definable
> > +
> > +Methods              void Release(object device)
> > +
> > +                     This method gets called when IWD unregisters the
> > +                     WFD service on a specific P2P device. An agent can
> > +                     use it to do cleanup tasks. There is no need to
> > +                     unregister the agent, because when this method
> > +                     gets called it has already been unregistered.
> > +
> > +             void WFDPeersChanged(object device,
> > +                                     array(object, array(byte)) peers)
> > +
> > +                     Called when the set of discovered WFD-capable
> > +                     peers has changed during device discovery.
> > +
> > +                     The device object has the net.connman.iwd.P2P
> > +                     interface while the objects in the peers array
> > +                     have net.connman.iwd.P2PPeer interfaces.  The byte
> > +                     array included contains the reassembled payload of
> > +                     the WFD Information Elements presented by the peer.
>
> Okay, so you want to notify the agent instead of adding a WFD specific
> interface to the Peer object.  I still wonder if we can avoid
> transporting raw IEs here... Also, how are you making sure that the
> peers are actually ones that the agent cares about?  There's no sense in
> sending Source role peer info if we're also a Source...
>
> Can we be a source and a sink at the same time?

Yeah, but I don't really see benefit in hiding the WFD sources if
we're the source, or WFD sinks if we're a sink.  It'll be simpler for
everyone if the WFD service takes care of this.

>
> > +
> > +             void NewConnection(object peer, array(byte) payload)
> > +
> > +                     Called when a new P2P connection has been established
> > +                     to a WFD-capable peer.  The peer object has the
> > +                     net.connman.iwd.P2PPerr interface.  The byte array
>
> typo
>
> > +                     contains the reassembled payload of the WFD
> > +                     Information Elements presented by the peer.
>
> So I'm not entirely sure how this is useful?  The fact that we made a
> connection to a peer is already available via the Peer.Connected
> property.  Is the payload somehow different from what was sent via
> WFDPeersChanged?

Hmm, no, it should be the same in practice, so yeah, we can maybe drop this.

The spec actually has a different subset of subelements allowed in
each frame (Associate vs. Beacon or Probe Response) but in
wpa_supplicant for example, you set one WFD IE for all possible
frames.

The bit that might change is "Available for WFD Session" now that I
think of it... but I assume when a device is not available it simply
won't be discovering or entering group formation.  But it could be a
candidate for modifying inside iwd rather than taking a value from the
DBus client.

By the way, one thing that needs to change in this proposal is how
peer discovery (scanning) is triggered and how the current state is
signalled.  Discovery is more expensive than we previously thought and
can't be automatically enabled whenever the P2P interface is enabled.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

On 11/14/19 10:59 AM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Thu, 14 Nov 2019 at 05:42, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/10/19 3:51 PM, Andrew Zaborowski wrote:
>>> Proposed minimum P2P interfaces for establishing basic connections.  The
>>> device discovery results in creation of P2PPeer objects.
>>>
>>> In the Wi-Fi Display API we are passing raw IE data because there's a
>>> relatively big set of different values that may be encoded in them.  We
>>> could reduce them to 2-3 bools and integers but this might limit the
>>> client implementations feature set.
>>
>> So I'm not really a huge fan of this.  We'd still need to be able to
>> understand what is inside the IE in order to verify it properly.  I'd
>> err on the side of providing some attributes instead of full on binary
>> IEs.
>>
>> Also, some of the subelements involved should really be filled in by iwd
>> itself and not the external application.  Otherwise you'll end up with
>> some sort of Frankenstein where the app provides some info but we'd need
>> to rewrite or modify it anyway.
> 
> I didn't plan to interpret the contents of the IE in any way.  The
> idea would be to have little service-specific code, and in case of WFD
> there's no need to do anything more than passing the ies.

So two issues:
- Passing in unchecked binary info from an unprivileged process is just 
not going to happen though.  We have to look inside anyway to sanitize it.
- Some of the sub elements are dynamic (connected bssid for example). 
So you will end up having to interpret it anyway.

> 
> So I see only two options, either full on binary IE passed in both
> directions between iwd and the service implementation, or full on
> individual values from the WFD subelements.  There's a lot of them
> though, for example for a probe response you have at least:
> 
>   * 12 capability bits in a bitmap
>   * RTSP tcp port
>   * throughput
>   * optional coupled sink status
>   * optional coupled sink MAC
>   * R2 support (true/false)
> 
> + 10 or more optional values...

So lets start with the bare minimum and see.  In the end this API is 
experimental, so we can still change things.

> 
> I just had a look at the how gnome-screencast is passing those to NM /
> wpa_supplicant and they basically hardcode the IE in one place in the
> code and never touch the contents...  On receival they only check if
> the WFD IEs were present and don't really look at the contents either.
> This is very new code though and I haven't looked at openwfd or
> miraclecast in detail.

Even more reason not to bother with passing IEs around.

<snip>

>>
>>> +
>>> +Methods              array(on) GetPeers()
>>> +
>>> +                     Returns a list (possibly empty) of detected P2P peers.
>>> +                     Each record returned contains a tuple of the following
>>> +                     values.
>>> +
>>> +                     object Object
>>> +
>>> +                             net.connman.iwd.P2PPeer object representing
>>
>> Name this iwd.Peer?
> 
> Sounds very generic, are we sure we won't have another spec that uses
> "peers" too?

I'm not aware of any.  But P2PPeer does not CamelCase well regardless. 
I'm happy to hear other ideas.

<snip>

>>> +             void RegisterSignalLevelAgent(object path,
>>> +                                              array(int16) levels)
>>> +
>>> +                     Register the agent object to receive signal strength
>>> +                     level change notifications on the
>>> +                     net.connman.iwd.SignalLevelAgent interface, see
>>> +                     signal-level-agent-api.txt.  The "levels"
>>
>> We moved this into the station api.  So maybe just refer to that here...
>>
>> Though really, I wonder if you even want to bother with this for the
>> first iteration?  I'd maybe leave this out?
> 
> Yeah, I wasn't going to implement it at this time.  But since you'll
> have RSSI in every mode of wireless connections, AP, adhoc, ...
> wouldn't it make sense to have a common interface and share code in
> iwd and in the clients?

You don't though.  For AP you have multiple clients, which rssi are you 
going to show?  Same goes for p2p-go really.  With adhoc you have an 
even weirder problem.

>>> +             void RegisterWFDService(object path, array(byte) payload)
>>
>> I would maybe make this a bit more generic in case other P2P services
>> are supported in the future, like file sharing, etc.
> 
> I was thinking of having a separate agent interface and separate
> register method for each service, the parameters are going to be
> different... but we could also have a common register method and
> another method on the service's agent to query the actual values of
> the parameters (at the cost of additional round trips)
> 

I'd just make this into a dictionary of parameters and have it depend on 
the service type.

>>
>> But, I'm not sure this belongs here?  P2P is a per-phy interface; do you
>> really want the clients to register WiFi Display per phy?  Or just once
>> and have it apply to all devices in the system, current or ones yet to
>> be plugged in?  Perhaps we need a ServiceManager or add this to
>> AgentManager API?
> 
> Yep, perhaps a ServiceManager (P2PServiceManager?) is a better place...
> 
>>
>> I suppose it might be in theory possible to support both a Sink and a
>> Source role on different phys, but then shouldn't you be distinguishing
>> between the roles somehow?
> 
> It is possible but luckily it's all up to the wfd implementation.
> We'd only see this information if we wanted to build the WFD IEs
> ourselves like you suggest but we wouldn't be doing anything more than
> encoding / decoding it from the IEs.

Then we can't do this as a global registration.  Because some 
sub-elements are dynamic it would be up to the application to update the 
IE properly.  I'm really not sure we can avoid building the WFD IE 
ourselves.

<snip.

>>> +             uint16 MaxConnections [readonly]
>>> +
>>> +                     Maximum number of concurrent P2P peers that local
>>> +                     hardware is capable of connecting to.  Often 1.
>>
>> I think we've talked about this before.  Given what we now know, I would
>> drop this for now.  I think the likelihood of this becoming > 1 is
>> astronomically small and we shouldn't even worry about this.
> 
> So I don't really have enough information to say that, the 2 P2P
> capable adapters that I worked with only support one client or one GO
> interface but those are only 2 adapters.
> 
> I have actually changed this since this proposal to be named
> AvailableConnections and switch between 1 and 0 as the device becomes
> busy.  So it has an additional use of indicating whether a connection
> is in progress... the Connected property on the Peer only becomes 1
> when the connection is operative.

Yeah, but that's the problem.  What do you actually mean by 
AvailableConnections?  Do you mean:

1.  connections on a given P2P_CLIENT or P2P_GO interface?  or
2.  number of P2P_CLIENT & P2P_GO interfaces?

If 1, then this makes no sense anyway.  For P2P_CLIENT it is always 1. 
For P2P_GO it is essentially unlimited.

For 2, I doubt you will ever find any hardware capable of doing more 
than 1 anyway.  And it is a complication I wouldn't worry about now 
regardless.  So if the v1 API is limited to a single connection and we 
later find out it is actually possible to do more, then we worry about 
it then.

> 
>> This means
>> we can also move some attributes / methods from the Peer interface onto
>> this one.
> 
> So this is a slightly different topic and I wouldn't do it.
> Apparently some device will make sure they become the GO during GO
> Negotiation (or as soon as P2P is enabled) and then use the invitation
> procedure instead of the group formation procedure, so then they can
> be connected to multiple peripherals and this seems to be an actual
> use case.

Yeah, see above.  You're talking about 1 here.  And I'd argue that 
AvailableConnections or MaxConnections still doesn't make sense in this 
context.

> 
> MaxConnections still doesn't sound right because in theory there's no
> maximum, but we still need some way to indicate if we're busy.

Exactly.  So you need to find another way.

>>> +
>>> +Methods              ConnectPushButton()
>>> +
>>> +                     Connect to the P2P peer in the Push Button mode
>>> +                     using the device pointed to by the .Device property.
>>> +                     Returns when connection is complete.
>>
>> Why are you method names different from SimpleConfiguration?  in fact,
>> why not just add .SimpleConfiguration interface to the same object path
>> and just have clients use that?
> 
> Could do this, yes, although it would have been nice to have "Connect"
> in the method names.  And I'm not sure how to resolve this in the code
> since the implementation of the methods will be very different from
> the one in wsc.c, preferably I wouldn't duplicate setup_wsc_interface.

Pretty easy I would think?  I mean you do have the netdev associated 
with it, so you can simply check the interface type.

>>> +
>>> +             Disconnect() [optional]
>>
>> A method can't be optional?
> 
> Well since the optional concept is kind of made up it just means
> returning an error when the method or property doesn't apply.

Made up?  I don't think so.  Properties are explicitly allowed to be 
optional.  Methods are not.  Check the D-Bus spec ;)  We never had any 
optional methods and never will :)

>>> +             boolean Connected [readonly]
>>> +
>>> +                     Whether there's currently an active connection
>>> +                     to this peer.  The property is read-only and
>>> +                     changes as a result of the Connect and
>>> +                     Disconnect methods calls.
>>
>> This wording might need to be updated....
> 
> You mean because we'd only have one connection?

Sorry, should have been more clear.  The Connect and maybe even 
Disconnect method calls will be named differently.  So the wording needs 
to be updated accordingly.

>>
>> Okay, so you want to notify the agent instead of adding a WFD specific
>> interface to the Peer object.  I still wonder if we can avoid
>> transporting raw IEs here... Also, how are you making sure that the
>> peers are actually ones that the agent cares about?  There's no sense in
>> sending Source role peer info if we're also a Source...
>>
>> Can we be a source and a sink at the same time?
> 
> Yeah, but I don't really see benefit in hiding the WFD sources if
> we're the source, or WFD sinks if we're a sink.  It'll be simpler for
> everyone if the WFD service takes care of this.

That goes against our core design principles and you know it :)

> 
>>
>>> +
>>> +             void NewConnection(object peer, array(byte) payload)
>>> +
>>> +                     Called when a new P2P connection has been established
>>> +                     to a WFD-capable peer.  The peer object has the
>>> +                     net.connman.iwd.P2PPerr interface.  The byte array
>>
>> typo
>>
>>> +                     contains the reassembled payload of the WFD
>>> +                     Information Elements presented by the peer.
>>
>> So I'm not entirely sure how this is useful?  The fact that we made a
>> connection to a peer is already available via the Peer.Connected
>> property.  Is the payload somehow different from what was sent via
>> WFDPeersChanged?
> 
> Hmm, no, it should be the same in practice, so yeah, we can maybe drop this.

okay

> 
> The spec actually has a different subset of subelements allowed in
> each frame (Associate vs. Beacon or Probe Response) but in
> wpa_supplicant for example, you set one WFD IE for all possible
> frames.
> 
> The bit that might change is "Available for WFD Session" now that I
> think of it... but I assume when a device is not available it simply
> won't be discovering or entering group formation.  But it could be a
> candidate for modifying inside iwd rather than taking a value from the
> DBus client.
> 
> By the way, one thing that needs to change in this proposal is how
> peer discovery (scanning) is triggered and how the current state is
> signalled.  Discovery is more expensive than we previously thought and
> can't be automatically enabled whenever the P2P interface is enabled.
> 

Funny, okay I'm curious to learn more now.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Thu, 14 Nov 2019 at 19:20, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/14/19 10:59 AM, Andrew Zaborowski wrote:
> > I didn't plan to interpret the contents of the IE in any way.  The
> > idea would be to have little service-specific code, and in case of WFD
> > there's no need to do anything more than passing the ies.
>
> So two issues:
> - Passing in unchecked binary info from an unprivileged process is just
> not going to happen though.  We have to look inside anyway to sanitize it.

Fair enough, although I think the level of sanitization would only
need to be the same as when we send out an SSID provided by an
unprivileged process... we only check it's utf-8, not too long and
such.

> - Some of the sub elements are dynamic (connected bssid for example).
> So you will end up having to interpret it anyway.

AFAIK we don't strictly need to do anything and things will work just
like with wpa_supplicant... but we could.

>
> >
> > So I see only two options, either full on binary IE passed in both
> > directions between iwd and the service implementation, or full on
> > individual values from the WFD subelements.  There's a lot of them
> > though, for example for a probe response you have at least:
> >
> >   * 12 capability bits in a bitmap
> >   * RTSP tcp port
> >   * throughput
> >   * optional coupled sink status
> >   * optional coupled sink MAC
> >   * R2 support (true/false)
> >
> > + 10 or more optional values...
>
> So lets start with the bare minimum and see.  In the end this API is
> experimental, so we can still change things.

Ok, let's do it.  I don't like that because we're again creating
barriers for client software to start supporting iwd, and adding more
work for iwd to do while doing this.

>
> >
> > I just had a look at the how gnome-screencast is passing those to NM /
> > wpa_supplicant and they basically hardcode the IE in one place in the
> > code and never touch the contents...  On receival they only check if
> > the WFD IEs were present and don't really look at the contents either.
> > This is very new code though and I haven't looked at openwfd or
> > miraclecast in detail.
>
> Even more reason not to bother with passing IEs around.
>
> <snip>
>
> >>
> >>> +
> >>> +Methods              array(on) GetPeers()
> >>> +
> >>> +                     Returns a list (possibly empty) of detected P2P peers.
> >>> +                     Each record returned contains a tuple of the following
> >>> +                     values.
> >>> +
> >>> +                     object Object
> >>> +
> >>> +                             net.connman.iwd.P2PPeer object representing
> >>
> >> Name this iwd.Peer?
> >
> > Sounds very generic, are we sure we won't have another spec that uses
> > "peers" too?
>
> I'm not aware of any.  But P2PPeer does not CamelCase well regardless.
> I'm happy to hear other ideas.
>
> <snip>
>
> >>> +             void RegisterSignalLevelAgent(object path,
> >>> +                                              array(int16) levels)
> >>> +
> >>> +                     Register the agent object to receive signal strength
> >>> +                     level change notifications on the
> >>> +                     net.connman.iwd.SignalLevelAgent interface, see
> >>> +                     signal-level-agent-api.txt.  The "levels"
> >>
> >> We moved this into the station api.  So maybe just refer to that here...
> >>
> >> Though really, I wonder if you even want to bother with this for the
> >> first iteration?  I'd maybe leave this out?
> >
> > Yeah, I wasn't going to implement it at this time.  But since you'll
> > have RSSI in every mode of wireless connections, AP, adhoc, ...
> > wouldn't it make sense to have a common interface and share code in
> > iwd and in the clients?
>
> You don't though.  For AP you have multiple clients, which rssi are you
> going to show?

That would be per client obviously, when/if information about the
clients is added.

> Same goes for p2p-go really.

With P2P we are initiating the connections, so we have to expose
information about each peer anyway.

> With adhoc you have an
> even weirder problem.
>
> >>> +             void RegisterWFDService(object path, array(byte) payload)
> >>
> >> I would maybe make this a bit more generic in case other P2P services
> >> are supported in the future, like file sharing, etc.
> >
> > I was thinking of having a separate agent interface and separate
> > register method for each service, the parameters are going to be
> > different... but we could also have a common register method and
> > another method on the service's agent to query the actual values of
> > the parameters (at the cost of additional round trips)
> >
>
> I'd just make this into a dictionary of parameters and have it depend on
> the service type.

Ok, sounds good.

>
> >>
> >> But, I'm not sure this belongs here?  P2P is a per-phy interface; do you
> >> really want the clients to register WiFi Display per phy?  Or just once
> >> and have it apply to all devices in the system, current or ones yet to
> >> be plugged in?  Perhaps we need a ServiceManager or add this to
> >> AgentManager API?
> >
> > Yep, perhaps a ServiceManager (P2PServiceManager?) is a better place...
> >
> >>
> >> I suppose it might be in theory possible to support both a Sink and a
> >> Source role on different phys, but then shouldn't you be distinguishing
> >> between the roles somehow?
> >
> > It is possible but luckily it's all up to the wfd implementation.
> > We'd only see this information if we wanted to build the WFD IEs
> > ourselves like you suggest but we wouldn't be doing anything more than
> > encoding / decoding it from the IEs.
>
> Then we can't do this as a global registration.  Because some
> sub-elements are dynamic it would be up to the application to update the
> IE properly.  I'm really not sure we can avoid building the WFD IE
> ourselves.

I think it should still be global actually, but maybe we need to
expand the API somehow.  I think it should be global because an
application like gnome-screencast is not going to know which device to
use.  But we may need to add a way for the service to say it's
connected now so that its not considered for any simultaneous
connections, until the first connection is closed.

So since our assumption is that we're always initiating our
connections (or in the future responing to an invitation, but the
response is again our initiative), we could also register services
per-connection and per-scan?  This would work for gnome-screencast,
since I believe it has its own dialog that shows you available devices
to connect to... not sure about other services.

>
> <snip.
>
> >>> +             uint16 MaxConnections [readonly]
> >>> +
> >>> +                     Maximum number of concurrent P2P peers that local
> >>> +                     hardware is capable of connecting to.  Often 1.
> >>
> >> I think we've talked about this before.  Given what we now know, I would
> >> drop this for now.  I think the likelihood of this becoming > 1 is
> >> astronomically small and we shouldn't even worry about this.
> >
> > So I don't really have enough information to say that, the 2 P2P
> > capable adapters that I worked with only support one client or one GO
> > interface but those are only 2 adapters.
> >
> > I have actually changed this since this proposal to be named
> > AvailableConnections and switch between 1 and 0 as the device becomes
> > busy.  So it has an additional use of indicating whether a connection
> > is in progress... the Connected property on the Peer only becomes 1
> > when the connection is operative.
>
> Yeah, but that's the problem.  What do you actually mean by
> AvailableConnections?  Do you mean:
>
> 1.  connections on a given P2P_CLIENT or P2P_GO interface?  or
> 2.  number of P2P_CLIENT & P2P_GO interfaces?

So this should not bleed into the API.  As long as we allow the user
to make another connection, it should be signalled to the user
somehow.  Whether we're using a separate interface or just make
another connection from our GO interface.

>
> If 1, then this makes no sense anyway.  For P2P_CLIENT it is always 1.
> For P2P_GO it is essentially unlimited.

Right, but we need to inform the user somehow whether we can make
another connection with whatever role we have currently negotiated or
because we're idle.

(and for P2P_GO we may want to set an artificial limit, it's only
unlimited in theory)

>
> For 2, I doubt you will ever find any hardware capable of doing more
> than 1 anyway.  And it is a complication I wouldn't worry about now
> regardless.  So if the v1 API is limited to a single connection and we
> later find out it is actually possible to do more, then we worry about
> it then.

So from what I've seen, it is possible, but not until we support the
GO role.  However our main usecase right now (I believe...) is a PC,
phone, tablet, rather than a peripheral device, so I think this is
definitely going to be expected from iwd, so are you sure you want to
limit the API to a single connection?

AFAIK the android API for apps doesn't have this limit for example.

>
> >
> >> This means
> >> we can also move some attributes / methods from the Peer interface onto
> >> this one.
> >
> > So this is a slightly different topic and I wouldn't do it.
> > Apparently some device will make sure they become the GO during GO
> > Negotiation (or as soon as P2P is enabled) and then use the invitation
> > procedure instead of the group formation procedure, so then they can
> > be connected to multiple peripherals and this seems to be an actual
> > use case.
>
> Yeah, see above.  You're talking about 1 here.  And I'd argue that
> AvailableConnections or MaxConnections still doesn't make sense in this
> context.
>
> >
> > MaxConnections still doesn't sound right because in theory there's no
> > maximum, but we still need some way to indicate if we're busy.
>
> Exactly.  So you need to find another way.

Hmm, maybe a 'State' property but I'm not sure how to call the
different states to make this clear...

>
> >>> +
> >>> +Methods              ConnectPushButton()
> >>> +
> >>> +                     Connect to the P2P peer in the Push Button mode
> >>> +                     using the device pointed to by the .Device property.
> >>> +                     Returns when connection is complete.
> >>
> >> Why are you method names different from SimpleConfiguration?  in fact,
> >> why not just add .SimpleConfiguration interface to the same object path
> >> and just have clients use that?
> >
> > Could do this, yes, although it would have been nice to have "Connect"
> > in the method names.  And I'm not sure how to resolve this in the code
> > since the implementation of the methods will be very different from
> > the one in wsc.c, preferably I wouldn't duplicate setup_wsc_interface.
>
> Pretty easy I would think?  I mean you do have the netdev associated
> with it, so you can simply check the interface type.

We only have a netdev after the GO Negotiation.

Esentially I'd need to have a new struct wsc, which would encapsulate either:

 * a current struct wsc for a station interface, or
 * a P2P peer that corresponds to a scan result.

Then the wsc.c PushButton method, for instance, would need to either
do its current work or delegate it to a p2p.c function and exit.

>
> >>> +
> >>> +             Disconnect() [optional]
> >>
> >> A method can't be optional?
> >
> > Well since the optional concept is kind of made up it just means
> > returning an error when the method or property doesn't apply.
>
> Made up?  I don't think so.  Properties are explicitly allowed to be
> optional.  Methods are not.  Check the D-Bus spec ;)

You mean https://dbus.freedesktop.org/doc/dbus-specification.html ?  I
think we've talked about optional properties not being considered in
the specs before...

>  We never had any
> optional methods and never will :)
>
> >>> +             boolean Connected [readonly]
> >>> +
> >>> +                     Whether there's currently an active connection
> >>> +                     to this peer.  The property is read-only and
> >>> +                     changes as a result of the Connect and
> >>> +                     Disconnect methods calls.
> >>
> >> This wording might need to be updated....
> >
> > You mean because we'd only have one connection?
>
> Sorry, should have been more clear.  The Connect and maybe even
> Disconnect method calls will be named differently.  So the wording needs
> to be updated accordingly.
>
> >>
> >> Okay, so you want to notify the agent instead of adding a WFD specific
> >> interface to the Peer object.  I still wonder if we can avoid
> >> transporting raw IEs here... Also, how are you making sure that the
> >> peers are actually ones that the agent cares about?  There's no sense in
> >> sending Source role peer info if we're also a Source...
> >>
> >> Can we be a source and a sink at the same time?
> >
> > Yeah, but I don't really see benefit in hiding the WFD sources if
> > we're the source, or WFD sinks if we're a sink.  It'll be simpler for
> > everyone if the WFD service takes care of this.
>
> That goes against our core design principles and you know it :)

I think it depends on the point of view.  You might as well want the
caller to specify what service or device class or WSC method it's
interested in and only show them matching peers.  You could even do
things like this for normal infrastructure-mode networks.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

On 11/14/19 1:37 PM, Andrew Zaborowski wrote:
> On Thu, 14 Nov 2019 at 19:20, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 11/14/19 10:59 AM, Andrew Zaborowski wrote:
>>> I didn't plan to interpret the contents of the IE in any way.  The
>>> idea would be to have little service-specific code, and in case of WFD
>>> there's no need to do anything more than passing the ies.
>>
>> So two issues:
>> - Passing in unchecked binary info from an unprivileged process is just
>> not going to happen though.  We have to look inside anyway to sanitize it.
> 
> Fair enough, although I think the level of sanitization would only
> need to be the same as when we send out an SSID provided by an
> unprivileged process... we only check it's utf-8, not too long and
> such.

Well, about the only place we do this is in Adhoc and AP code.  And the 
SSID gets sanitized by the kernel (and maybe even the firmware) as well. 
  So there are multiple layers of protection here.

But in general, we do not allow arbitrary binary blobs to be passed 
around.  That's just not our design philosophy.

> 
>> - Some of the sub elements are dynamic (connected bssid for example).
>> So you will end up having to interpret it anyway.
> 
> AFAIK we don't strictly need to do anything and things will work just
> like with wpa_supplicant... but we could.
> 

Fair enough

>>> Yeah, I wasn't going to implement it at this time.  But since you'll
>>> have RSSI in every mode of wireless connections, AP, adhoc, ...
>>> wouldn't it make sense to have a common interface and share code in
>>> iwd and in the clients?
>>
>> You don't though.  For AP you have multiple clients, which rssi are you
>> going to show?
> 
> That would be per client obviously, when/if information about the
> clients is added.

And the kernel has APIs for this?  AFAIK you don't even get this info 
without polling.  So trying to use our agent for this is not going to work.

> 
>> Same goes for p2p-go really.
> 
> With P2P we are initiating the connections, so we have to expose
> information about each peer anyway.
> 

So lets see how that works out given the above.  I think for now I would 
leave all of this out.

>> Then we can't do this as a global registration.  Because some
>> sub-elements are dynamic it would be up to the application to update the
>> IE properly.  I'm really not sure we can avoid building the WFD IE
>> ourselves.
> 
> I think it should still be global actually, but maybe we need to
> expand the API somehow.  I think it should be global because an
> application like gnome-screencast is not going to know which device to
> use.  But we may need to add a way for the service to say it's
> connected now so that its not considered for any simultaneous
> connections, until the first connection is closed.

Well, it is explicitly allowed to run multiple WFD sessions if the 
infrastructure is there, even without a P2P link.

So I generally agree this should be global, just pointing out that 
certain elements would need to be dynamic.  This also (probably?) 
precludes us from advertising a Sink role and a Source role for example. 
  And also makes things like doing simultaneous P2P_GO + P2P_CLIENT sort 
of pointless.

So overall I'd say we should ignore the possibility of multiple P2P 
iftypes.  It will be either a P2P_GO with multiple clients or a 
P2P_CLIENT and we're limited to 1.

> 
> So since our assumption is that we're always initiating our
> connections (or in the future responing to an invitation, but the
> response is again our initiative), we could also register services
> per-connection and per-scan?  This would work for gnome-screencast,
> since I believe it has its own dialog that shows you available devices
> to connect to... not sure about other services.
> 

We can always introduce a per phy agent that would override the global 
one if we really encounter a use case that required this.  Or allow 
multiple agents to be registered and if an application triggers a 
connection, then the agent registered on that application's dbus unique 
name (if any) gets preferred for any IEs broadcast.

For now I'd ignore this and assume we can only do one role at a time.

>>
>> Yeah, but that's the problem.  What do you actually mean by
>> AvailableConnections?  Do you mean:
>>
>> 1.  connections on a given P2P_CLIENT or P2P_GO interface?  or
>> 2.  number of P2P_CLIENT & P2P_GO interfaces?
> 
> So this should not bleed into the API.  As long as we allow the user
> to make another connection, it should be signalled to the user
> somehow.  Whether we're using a separate interface or just make
> another connection from our GO interface.

Then just indicate what role we have and let the application make that 
choice.  So if we're in P2P_GO mode, it can try to establish another 
connection.  And if we're in P2P_CLIENT mode, tough luck.

> 
>>
>> If 1, then this makes no sense anyway.  For P2P_CLIENT it is always 1.
>> For P2P_GO it is essentially unlimited.
> 
> Right, but we need to inform the user somehow whether we can make
> another connection with whatever role we have currently negotiated or
> because we're idle.
> 
> (and for P2P_GO we may want to set an artificial limit, it's only
> unlimited in theory)

Why? The hardware is the arbiter of this in the end.

> 
>>
>> For 2, I doubt you will ever find any hardware capable of doing more
>> than 1 anyway.  And it is a complication I wouldn't worry about now
>> regardless.  So if the v1 API is limited to a single connection and we
>> later find out it is actually possible to do more, then we worry about
>> it then.
> 
> So from what I've seen, it is possible, but not until we support the
> GO role.  However our main usecase right now (I believe...) is a PC,
> phone, tablet, rather than a peripheral device, so I think this is
> definitely going to be expected from iwd, so are you sure you want to
> limit the API to a single connection?

Ah, imprecise wording here.  By single connection I mean a single 
P2P_GO/P2P_CLIENT interface active at a time.  So if we're in P2P_GO 
mode, we can do multiple 'connections'.  But since we're not doing 
P2P_GO yet, then effectively we would only support a single active peer 
today.

> 
> AFAIK the android API for apps doesn't have this limit for example.
> 

Right, but they still expect apps to setup the topology appropriately. 
And this is maybe something you need to consider for the future: being 
able to force one mode vs the other.

> 
>>
>>>>> +
>>>>> +Methods              ConnectPushButton()
>>>>> +
>>>>> +                     Connect to the P2P peer in the Push Button mode
>>>>> +                     using the device pointed to by the .Device property.
>>>>> +                     Returns when connection is complete.
>>>>
>>>> Why are you method names different from SimpleConfiguration?  in fact,
>>>> why not just add .SimpleConfiguration interface to the same object path
>>>> and just have clients use that?
>>>
>>> Could do this, yes, although it would have been nice to have "Connect"
>>> in the method names.  And I'm not sure how to resolve this in the code
>>> since the implementation of the methods will be very different from
>>> the one in wsc.c, preferably I wouldn't duplicate setup_wsc_interface.
>>
>> Pretty easy I would think?  I mean you do have the netdev associated
>> with it, so you can simply check the interface type.
> 
> We only have a netdev after the GO Negotiation.
> 
> Esentially I'd need to have a new struct wsc, which would encapsulate either:
> 
>   * a current struct wsc for a station interface, or
>   * a P2P peer that corresponds to a scan result.
> 
> Then the wsc.c PushButton method, for instance, would need to either
> do its current work or delegate it to a p2p.c function and exit.

Then make wsc work more like struct network.  And have it register the 
interface when a station is available or on request for a p2p peer.  We 
may have to be creative.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Thu, 14 Nov 2019 at 21:36, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/14/19 1:37 PM, Andrew Zaborowski wrote:
> >>> Yeah, I wasn't going to implement it at this time.  But since you'll
> >>> have RSSI in every mode of wireless connections, AP, adhoc, ...
> >>> wouldn't it make sense to have a common interface and share code in
> >>> iwd and in the clients?
> >>
> >> You don't though.  For AP you have multiple clients, which rssi are you
> >> going to show?
> >
> > That would be per client obviously, when/if information about the
> > clients is added.
>
> And the kernel has APIs for this?  AFAIK you don't even get this info
> without polling.  So trying to use our agent for this is not going to work.

I think there's a patchset that's being rebased occasionally.  But
what I mean is that maybe that interface doc shouldn't be buried in
station-api.txt.

>
> >
> >> Same goes for p2p-go really.
> >
> > With P2P we are initiating the connections, so we have to expose
> > information about each peer anyway.
> >
>
> So lets see how that works out given the above.  I think for now I would
> leave all of this out.

Ok.

>
> >> Then we can't do this as a global registration.  Because some
> >> sub-elements are dynamic it would be up to the application to update the
> >> IE properly.  I'm really not sure we can avoid building the WFD IE
> >> ourselves.
> >
> > I think it should still be global actually, but maybe we need to
> > expand the API somehow.  I think it should be global because an
> > application like gnome-screencast is not going to know which device to
> > use.  But we may need to add a way for the service to say it's
> > connected now so that its not considered for any simultaneous
> > connections, until the first connection is closed.
>
> Well, it is explicitly allowed to run multiple WFD sessions if the
> infrastructure is there, even without a P2P link.
>
> So I generally agree this should be global, just pointing out that
> certain elements would need to be dynamic.  This also (probably?)
> precludes us from advertising a Sink role and a Source role for example.
>   And also makes things like doing simultaneous P2P_GO + P2P_CLIENT sort
> of pointless.

When talking about the API it's only a question of whether the
parameters dictionary should have to booleans, one for sink, one for
source, or one "role" variable.  We're not acting on that information
anyway, it's only so we can encode it in the IE.

>
> So overall I'd say we should ignore the possibility of multiple P2P
> iftypes.  It will be either a P2P_GO with multiple clients or a
> P2P_CLIENT and we're limited to 1.

In the code I'm currently ignoring that possibilty, but talking about
the API it wouldn't make much difference.

>
> >
> > So since our assumption is that we're always initiating our
> > connections (or in the future responing to an invitation, but the
> > response is again our initiative), we could also register services
> > per-connection and per-scan?  This would work for gnome-screencast,
> > since I believe it has its own dialog that shows you available devices
> > to connect to... not sure about other services.
> >
>
> We can always introduce a per phy agent that would override the global
> one if we really encounter a use case that required this.  Or allow
> multiple agents to be registered and if an application triggers a
> connection, then the agent registered on that application's dbus unique
> name (if any) gets preferred for any IEs broadcast.
>
> For now I'd ignore this and assume we can only do one role at a time.

Right.  But I'm thinking if there is a whole better way of doing the
service registration, i.e. per connection instead of globally.  Say
your use case is you're connecting first to a TV and then to a
keyboard, or simultaneously (as a GO).  There's no point advertising
the WFD capabilities to the keyboard device or to anyone other than
the TV, once your WFD session is running and functional.  Maybe the
ServiceManager is the wrong way to go and the Connect / PushButton
method should receive the relevant information.

>
> >>
> >> Yeah, but that's the problem.  What do you actually mean by
> >> AvailableConnections?  Do you mean:
> >>
> >> 1.  connections on a given P2P_CLIENT or P2P_GO interface?  or
> >> 2.  number of P2P_CLIENT & P2P_GO interfaces?
> >
> > So this should not bleed into the API.  As long as we allow the user
> > to make another connection, it should be signalled to the user
> > somehow.  Whether we're using a separate interface or just make
> > another connection from our GO interface.
>
> Then just indicate what role we have and let the application make that
> choice.  So if we're in P2P_GO mode, it can try to establish another
> connection.  And if we're in P2P_CLIENT mode, tough luck.

I think it would be nicer to just prefer GO automatically (maybe
without forcing it), for example by having the GO Intent default to 14
out of 15 and be configurable in main.conf.

>
> >
> >>
> >> If 1, then this makes no sense anyway.  For P2P_CLIENT it is always 1.
> >> For P2P_GO it is essentially unlimited.
> >
> > Right, but we need to inform the user somehow whether we can make
> > another connection with whatever role we have currently negotiated or
> > because we're idle.
> >
> > (and for P2P_GO we may want to set an artificial limit, it's only
> > unlimited in theory)
>
> Why? The hardware is the arbiter of this in the end.

Some dedicated access points have trouble serving 10 connections
simultaneously on one radio channel so for our off the shelf hardware
and minimal ap.c code it's going to be very difficult.

>
> >
> >>
> >> For 2, I doubt you will ever find any hardware capable of doing more
> >> than 1 anyway.  And it is a complication I wouldn't worry about now
> >> regardless.  So if the v1 API is limited to a single connection and we
> >> later find out it is actually possible to do more, then we worry about
> >> it then.
> >
> > So from what I've seen, it is possible, but not until we support the
> > GO role.  However our main usecase right now (I believe...) is a PC,
> > phone, tablet, rather than a peripheral device, so I think this is
> > definitely going to be expected from iwd, so are you sure you want to
> > limit the API to a single connection?
>
> Ah, imprecise wording here.  By single connection I mean a single
> P2P_GO/P2P_CLIENT interface active at a time.  So if we're in P2P_GO
> mode, we can do multiple 'connections'.  But since we're not doing
> P2P_GO yet, then effectively we would only support a single active peer
> today.

So I assume you're Ok having the Disconnect method on the Peer
interface, not on P2PDevice? So that our DBus clients are prepared to
support multiple connections instead of 1?

Or I wonder if we should repurpose WSC.Cancel to also disconnect a
fully established connection.  There's no need for separate Cancel and
Disconnect methods, but Disconnect sounds more correct.

>
> >
> > AFAIK the android API for apps doesn't have this limit for example.
> >
>
> Right, but they still expect apps to setup the topology appropriately.
> And this is maybe something you need to consider for the future: being
> able to force one mode vs the other.
>
> >
> >>
> >>>>> +
> >>>>> +Methods              ConnectPushButton()
> >>>>> +
> >>>>> +                     Connect to the P2P peer in the Push Button mode
> >>>>> +                     using the device pointed to by the .Device property.
> >>>>> +                     Returns when connection is complete.
> >>>>
> >>>> Why are you method names different from SimpleConfiguration?  in fact,
> >>>> why not just add .SimpleConfiguration interface to the same object path
> >>>> and just have clients use that?
> >>>
> >>> Could do this, yes, although it would have been nice to have "Connect"
> >>> in the method names.  And I'm not sure how to resolve this in the code
> >>> since the implementation of the methods will be very different from
> >>> the one in wsc.c, preferably I wouldn't duplicate setup_wsc_interface.
> >>
> >> Pretty easy I would think?  I mean you do have the netdev associated
> >> with it, so you can simply check the interface type.
> >
> > We only have a netdev after the GO Negotiation.
> >
> > Esentially I'd need to have a new struct wsc, which would encapsulate either:
> >
> >   * a current struct wsc for a station interface, or
> >   * a P2P peer that corresponds to a scan result.
> >
> > Then the wsc.c PushButton method, for instance, would need to either
> > do its current work or delegate it to a p2p.c function and exit.
>
> Then make wsc work more like struct network.  And have it register the
> interface when a station is available or on request for a p2p peer.  We
> may have to be creative.

Right, I was just thinking that there's going to be little common code
between the two usages of the WSC interface.  For the first case it's
going to be the same wsc.c code there is today, but for the latter the
3 methods will directly call into p2p.c and exit.  (specifically
PushButton, StartPin and Cancel)

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

>> And the kernel has APIs for this?  AFAIK you don't even get this info
>> without polling.  So trying to use our agent for this is not going to work.
> 
> I think there's a patchset that's being rebased occasionally.  But
> what I mean is that maybe that interface doc shouldn't be buried in
> station-api.txt.

Sure.  But until we know that this is possible this step is premature. 
Right now the signal agent logically belongs in station since that is 
the only thing using it.

>>
>> So I generally agree this should be global, just pointing out that
>> certain elements would need to be dynamic.  This also (probably?)
>> precludes us from advertising a Sink role and a Source role for example.
>>    And also makes things like doing simultaneous P2P_GO + P2P_CLIENT sort
>> of pointless.
> 
> When talking about the API it's only a question of whether the
> parameters dictionary should have to booleans, one for sink, one for
> source, or one "role" variable.  We're not acting on that information
> anyway, it's only so we can encode it in the IE.
> 

By the way, to answer my earlier question.  It is possible to be both a 
Sink and a Source at the same time:
"0b11: dual-role possible, i.e., either a WFD Source or a Primary Sink"

>> For now I'd ignore this and assume we can only do one role at a time.
> 
> Right.  But I'm thinking if there is a whole better way of doing the
> service registration, i.e. per connection instead of globally.  Say
> your use case is you're connecting first to a TV and then to a
> keyboard, or simultaneously (as a GO).  There's no point advertising
> the WFD capabilities to the keyboard device or to anyone other than

I actually wouldn't expect this to be a good idea.  Once you start 
advertising a set of IEs as a P2P GO, I'm not sure changing these is 
really something that is expected, particularly when you already have a 
peer connection established.  Changing the contents to signal some state 
is probably OK (this is done in WSC for example).  But taking out IEs 
entirely is a bit of a gray area.

See for example:

WFD Session Availability bits:
0b00: Not available for WFD Session

Or Section 5.2.1:
"If a WFD Device acts as a P2P Group Owner, the WFD Device shall insert 
one or more WFD IEs after the other information elements in the Beacon 
frames it transmits. "

I assume that at least the WFD spec wants you to twiddle the contents of 
the IEs and not remove IEs entirely.

> the TV, once your WFD session is running and functional.  Maybe the
> ServiceManager is the wrong way to go and the Connect / PushButton
> method should receive the relevant information.

And don't you need the advertising data before you even get to the point 
where you can invoke operations on the Peer interface?

>> Then just indicate what role we have and let the application make that
>> choice.  So if we're in P2P_GO mode, it can try to establish another
>> connection.  And if we're in P2P_CLIENT mode, tough luck.
> 
> I think it would be nicer to just prefer GO automatically (maybe
> without forcing it), for example by having the GO Intent default to 14
> out of 15 and be configurable in main.conf.
> 

Yes, sure.  What I'm saying though is that once we're connected, we 
should indicate the topology somehow.

>> Why? The hardware is the arbiter of this in the end.
> 
> Some dedicated access points have trouble serving 10 connections
> simultaneously on one radio channel so for our off the shelf hardware
> and minimal ap.c code it's going to be very difficult.

And?  Any limit you pick will end up being wrong for someone.

>>
>> Ah, imprecise wording here.  By single connection I mean a single
>> P2P_GO/P2P_CLIENT interface active at a time.  So if we're in P2P_GO
>> mode, we can do multiple 'connections'.  But since we're not doing
>> P2P_GO yet, then effectively we would only support a single active peer
>> today.
> 
> So I assume you're Ok having the Disconnect method on the Peer
> interface, not on P2PDevice? So that our DBus clients are prepared to
> support multiple connections instead of 1?

I think these two things are somewhat orthogonal.  We can still have 
Disconnect on the Device and not the peer.  All that would do is 
preclude us from disconnecting individual links in P2P GO mode.  But 
sure, I suppose it can remain on the Peer object.

> 
> Or I wonder if we should repurpose WSC.Cancel to also disconnect a
> fully established connection.  There's no need for separate Cancel and
> Disconnect methods, but Disconnect sounds more correct.
> 

Cancel only applies to the discovery and configuration procedure.  The 
WSC is a separate handshake.  So having a separate disconnect method is 
still needed once you get into the actual connection process.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Fri, 15 Nov 2019 at 04:27, Denis Kenzior <denkenz@gmail.com> wrote:
> >> For now I'd ignore this and assume we can only do one role at a time.
> >
> > Right.  But I'm thinking if there is a whole better way of doing the
> > service registration, i.e. per connection instead of globally.  Say
> > your use case is you're connecting first to a TV and then to a
> > keyboard, or simultaneously (as a GO).  There's no point advertising
> > the WFD capabilities to the keyboard device or to anyone other than
>
> I actually wouldn't expect this to be a good idea.  Once you start
> advertising a set of IEs as a P2P GO, I'm not sure changing these is
> really something that is expected, particularly when you already have a
> peer connection established.  Changing the contents to signal some state
> is probably OK (this is done in WSC for example).  But taking out IEs
> entirely is a bit of a gray area.

Right, but passing the WFD IE in the Association Request kind of
signals that you want to establish a WFD connection and you don't want
it for your second connection.  You can keep signalling it in your
beacons, although once the user has closed the screencasting app, I
wouldn't worry about just removing those IEs from the beacons even
though you may still be connected to the hypothetical P2P keyboard.

>
> See for example:
>
> WFD Session Availability bits:
> 0b00: Not available for WFD Session
>
> Or Section 5.2.1:
> "If a WFD Device acts as a P2P Group Owner, the WFD Device shall insert
> one or more WFD IEs after the other information elements in the Beacon
> frames it transmits. "
>
> I assume that at least the WFD spec wants you to twiddle the contents of
> the IEs and not remove IEs entirely.
>
> > the TV, once your WFD session is running and functional.  Maybe the
> > ServiceManager is the wrong way to go and the Connect / PushButton
> > method should receive the relevant information.
>
> And don't you need the advertising data before you even get to the point
> where you can invoke operations on the Peer interface?

In theory you need to advertise it in your scan.  Like I mentioned in
another email, I think the scans should also be explicitly triggered
by the apps because they're expensive, so those would also need to
know the service parameters.

Maybe that's too complicated for now and we better try a global
P2PServiceManager thing first.

>
> >> Then just indicate what role we have and let the application make that
> >> choice.  So if we're in P2P_GO mode, it can try to establish another
> >> connection.  And if we're in P2P_CLIENT mode, tough luck.
> >
> > I think it would be nicer to just prefer GO automatically (maybe
> > without forcing it), for example by having the GO Intent default to 14
> > out of 15 and be configurable in main.conf.
> >
>
> Yes, sure.  What I'm saying though is that once we're connected, we
> should indicate the topology somehow.

I think we should just indicate that we can still attempt more
connections, optimally without exposing the P2P roles.

>
> >> Why? The hardware is the arbiter of this in the end.
> >
> > Some dedicated access points have trouble serving 10 connections
> > simultaneously on one radio channel so for our off the shelf hardware
> > and minimal ap.c code it's going to be very difficult.
>
> And?  Any limit you pick will end up being wrong for someone.
>
> >>
> >> Ah, imprecise wording here.  By single connection I mean a single
> >> P2P_GO/P2P_CLIENT interface active at a time.  So if we're in P2P_GO
> >> mode, we can do multiple 'connections'.  But since we're not doing
> >> P2P_GO yet, then effectively we would only support a single active peer
> >> today.
> >
> > So I assume you're Ok having the Disconnect method on the Peer
> > interface, not on P2PDevice? So that our DBus clients are prepared to
> > support multiple connections instead of 1?
>
> I think these two things are somewhat orthogonal.  We can still have
> Disconnect on the Device and not the peer.  All that would do is
> preclude us from disconnecting individual links in P2P GO mode.  But
> sure, I suppose it can remain on the Peer object.
>
> >
> > Or I wonder if we should repurpose WSC.Cancel to also disconnect a
> > fully established connection.  There's no need for separate Cancel and
> > Disconnect methods, but Disconnect sounds more correct.
> >
>
> Cancel only applies to the discovery and configuration procedure.  The
> WSC is a separate handshake.  So having a separate disconnect method is
> still needed once you get into the actual connection process.

In any case on of the two is redundant ;)  Disconnect already aborts a
connection attempt (device discovery, group owner negotiation /
provision discovery, provisioning or the PSK connection).

Of these only the provisioning uses WSC so conceptually the WSC
interface is not a great match.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

On 11/14/19 10:02 PM, Andrew Zaborowski wrote:
> On Fri, 15 Nov 2019 at 04:27, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> For now I'd ignore this and assume we can only do one role at a time.
>>>
>>> Right.  But I'm thinking if there is a whole better way of doing the
>>> service registration, i.e. per connection instead of globally.  Say
>>> your use case is you're connecting first to a TV and then to a
>>> keyboard, or simultaneously (as a GO).  There's no point advertising
>>> the WFD capabilities to the keyboard device or to anyone other than
>>
>> I actually wouldn't expect this to be a good idea.  Once you start
>> advertising a set of IEs as a P2P GO, I'm not sure changing these is
>> really something that is expected, particularly when you already have a
>> peer connection established.  Changing the contents to signal some state
>> is probably OK (this is done in WSC for example).  But taking out IEs
>> entirely is a bit of a gray area.
> 
> Right, but passing the WFD IE in the Association Request kind of
> signals that you want to establish a WFD connection and you don't want
> it for your second connection.  You can keep signalling it in your
> beacons, although once the user has closed the screencasting app, I
> wouldn't worry about just removing those IEs from the beacons even
> though you may still be connected to the hypothetical P2P keyboard.
> 

So you want two separate registrations?  A global capability and a 
per-connection "I want to use this profile" type of thing.  It sounded 
like you only wanted the latter above.

So really, I don't see the point.  If iwd understands about WFD, then it 
simply wouldn't send any WFD IEs to a WFD-incapable device.

One can argue that the capability you propose would still be wanted in 
some weird situations.

For example, WFD allows 'Dual mode Sink & Source' only in the Beacon and 
Probe Response frames.  For Association, it needs to pick one role.

Or maybe the Keyboard in your example is actually also WFD capable.. 
But then the spec mysteriously says:

"Depending on the availability of the WFD Device for a WFD Session, the 
value for WFD Session Availability bits (B5B4) of the WFD Device 
Information field of WFD Device Information subelement within the WFD IE 
may change" in Section 4.5.2.1 "Usage of a WFD IE to establish P2P 
connection".  So who knows what they actually mean by that?  Maybe we 
can always send WFD IEs and just twiddle those bits off?

I really wouldn't worry about these bizarre cases right now until you 
have something that actually works.

>>
>> See for example:
>>
>> WFD Session Availability bits:
>> 0b00: Not available for WFD Session
>>
>> Or Section 5.2.1:
>> "If a WFD Device acts as a P2P Group Owner, the WFD Device shall insert
>> one or more WFD IEs after the other information elements in the Beacon
>> frames it transmits. "
>>
>> I assume that at least the WFD spec wants you to twiddle the contents of
>> the IEs and not remove IEs entirely.
>>
>>> the TV, once your WFD session is running and functional.  Maybe the
>>> ServiceManager is the wrong way to go and the Connect / PushButton
>>> method should receive the relevant information.
>>
>> And don't you need the advertising data before you even get to the point
>> where you can invoke operations on the Peer interface?
> 
> In theory you need to advertise it in your scan.  Like I mentioned in
> another email, I think the scans should also be explicitly triggered
> by the apps because they're expensive, so those would also need to
> know the service parameters.

Okay.  So you need to present those details then.

>> Yes, sure.  What I'm saying though is that once we're connected, we
>> should indicate the topology somehow.
> 
> I think we should just indicate that we can still attempt more
> connections, optimally without exposing the P2P roles.

I don't know why you'd want to hide that, but I'm fine either way.

>> Cancel only applies to the discovery and configuration procedure.  The
>> WSC is a separate handshake.  So having a separate disconnect method is
>> still needed once you get into the actual connection process.
> 
> In any case on of the two is redundant ;)  Disconnect already aborts a
> connection attempt (device discovery, group owner negotiation /
> provision discovery, provisioning or the PSK connection).

I think about this differently.  You have 2 phases:
   1. Simple Configuration (WiFi Direct flavor) which involves the group 
formation and wsc negotiation
   2. Actual connection establishment

I don't see why Disconnect can't just affect 2 without affecting 1.  The 
operations are on different interfaces after all.

> 
> Of these only the provisioning uses WSC so conceptually the WSC
> interface is not a great match.

Honestly, I'm not convinced that reinventing the wheel and duplicating 
the SimpleConfiguration API on the P2P Peer is any better.

Even in the classic station case WSC involves device discovery as well. 
So, in P2P, arguably everything up to the PSK connection establishment 
can be viewed as part of WSC.

It sounds like you need to send a v4 before we can go further.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Fri, 15 Nov 2019 at 06:10, Denis Kenzior <denkenz@gmail.com> wrote:
> So you want two separate registrations?  A global capability and a
> per-connection "I want to use this profile" type of thing.  It sounded
> like you only wanted the latter above.

I was rather thinking of borth Scan() and PushButton() taking the
information about services to advertise but it's a complication.

>
> So really, I don't see the point.  If iwd understands about WFD, then it
> simply wouldn't send any WFD IEs to a WFD-incapable device.
>
> One can argue that the capability you propose would still be wanted in
> some weird situations.
>
> For example, WFD allows 'Dual mode Sink & Source' only in the Beacon and
> Probe Response frames.  For Association, it needs to pick one role.
>
> Or maybe the Keyboard in your example is actually also WFD capable..
> But then the spec mysteriously says:
>
> "Depending on the availability of the WFD Device for a WFD Session, the
> value for WFD Session Availability bits (B5B4) of the WFD Device
> Information field of WFD Device Information subelement within the WFD IE
> may change" in Section 4.5.2.1 "Usage of a WFD IE to establish P2P
> connection".  So who knows what they actually mean by that?  Maybe we
> can always send WFD IEs and just twiddle those bits off?
>
> I really wouldn't worry about these bizarre cases right now until you
> have something that actually works.
>
> >>
> >> See for example:
> >>
> >> WFD Session Availability bits:
> >> 0b00: Not available for WFD Session
> >>
> >> Or Section 5.2.1:
> >> "If a WFD Device acts as a P2P Group Owner, the WFD Device shall insert
> >> one or more WFD IEs after the other information elements in the Beacon
> >> frames it transmits. "
> >>
> >> I assume that at least the WFD spec wants you to twiddle the contents of
> >> the IEs and not remove IEs entirely.
> >>
> >>> the TV, once your WFD session is running and functional.  Maybe the
> >>> ServiceManager is the wrong way to go and the Connect / PushButton
> >>> method should receive the relevant information.
> >>
> >> And don't you need the advertising data before you even get to the point
> >> where you can invoke operations on the Peer interface?
> >
> > In theory you need to advertise it in your scan.  Like I mentioned in
> > another email, I think the scans should also be explicitly triggered
> > by the apps because they're expensive, so those would also need to
> > know the service parameters.
>
> Okay.  So you need to present those details then.
>
> >> Yes, sure.  What I'm saying though is that once we're connected, we
> >> should indicate the topology somehow.
> >
> > I think we should just indicate that we can still attempt more
> > connections, optimally without exposing the P2P roles.
>
> I don't know why you'd want to hide that, but I'm fine either way.

It sounds more like the typo of information you want on a diagnostics
interface because it is not supposed to affect your connection, like
your AP's BSSID.

>
> >> Cancel only applies to the discovery and configuration procedure.  The
> >> WSC is a separate handshake.  So having a separate disconnect method is
> >> still needed once you get into the actual connection process.
> >
> > In any case on of the two is redundant ;)  Disconnect already aborts a
> > connection attempt (device discovery, group owner negotiation /
> > provision discovery, provisioning or the PSK connection).
>
> I think about this differently.  You have 2 phases:
>    1. Simple Configuration (WiFi Direct flavor) which involves the group
> formation and wsc negotiation

Well, it simply isn't part of WSC, but ok.

>    2. Actual connection establishment
>
> I don't see why Disconnect can't just affect 2 without affecting 1.  The
> operations are on different interfaces after all.

We just don't need 2 methods and I already have Disconnect working
this way (consistent with Station.Disconnect) so it'd mean just
artificially crippling it, also adding a race where your Cancel call
may work or not.

(Tecnically provisioning and the connection are both on the P2P_CLIENT
interface)

>
> >
> > Of these only the provisioning uses WSC so conceptually the WSC
> > interface is not a great match.
>
> Honestly, I'm not convinced that reinventing the wheel and duplicating
> the SimpleConfiguration API on the P2P Peer is any better.
>
> Even in the classic station case WSC involves device discovery as well.
> So, in P2P, arguably everything up to the PSK connection establishment
> can be viewed as part of WSC.
>
> It sounds like you need to send a v4 before we can go further.

Right, I'm still not sure about how discovery should be triggered but
I can go on with the code with the answers and ideas I got from you
already.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

>> I think about this differently.  You have 2 phases:
>>     1. Simple Configuration (WiFi Direct flavor) which involves the group
>> formation and wsc negotiation
> 
> Well, it simply isn't part of WSC, but ok.
> 

P2P in the end just took WSC and changed the discovery bits a bit. 
Fundamentally its still WSC in the end.  But I guess depends on how you 
think about it...

>>     2. Actual connection establishment
>>
>> I don't see why Disconnect can't just affect 2 without affecting 1.  The
>> operations are on different interfaces after all.
> 
> We just don't need 2 methods and I already have Disconnect working
> this way (consistent with Station.Disconnect) so it'd mean just
> artificially crippling it, also adding a race where your Cancel call
> may work or not.

Station.Disconnect does not affect WSC state FYI.  It can't since 
Station doesn't even know about the ongoing WSC process until after 
station_connect_network is invoked.  So in that respect we'd be 
completely consistent.

> 
> (Tecnically provisioning and the connection are both on the P2P_CLIENT
> interface)
> 

Same as Station/WSC today...

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Fri, 15 Nov 2019 at 15:53, Denis Kenzior <denkenz@gmail.com> wrote:
> >> I think about this differently.  You have 2 phases:
> >>     1. Simple Configuration (WiFi Direct flavor) which involves the group
> >> formation and wsc negotiation
> >
> > Well, it simply isn't part of WSC, but ok.
> >
>
> P2P in the end just took WSC and changed the discovery bits a bit.
> Fundamentally its still WSC in the end.  But I guess depends on how you
> think about it...
>
> >>     2. Actual connection establishment
> >>
> >> I don't see why Disconnect can't just affect 2 without affecting 1.  The
> >> operations are on different interfaces after all.
> >
> > We just don't need 2 methods and I already have Disconnect working
> > this way (consistent with Station.Disconnect) so it'd mean just
> > artificially crippling it, also adding a race where your Cancel call
> > may work or not.
>
> Station.Disconnect does not affect WSC state FYI.  It can't since
> Station doesn't even know about the ongoing WSC process until after
> station_connect_network is invoked.  So in that respect we'd be
> completely consistent.

What I meant is that Disconnect() can cancel your station connection
attempt at any point, while with Peer.Disconnect() you're going to
have to be more careful for no reason.

>
> >
> > (Tecnically provisioning and the connection are both on the P2P_CLIENT
> > interface)
> >
>
> Same as Station/WSC today...

I was just noting they're not happening on different interfaces.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

>> Station.Disconnect does not affect WSC state FYI.  It can't since
>> Station doesn't even know about the ongoing WSC process until after
>> station_connect_network is invoked.  So in that respect we'd be
>> completely consistent.
> 
> What I meant is that Disconnect() can cancel your station connection
> attempt at any point, while with Peer.Disconnect() you're going to
> have to be more careful for no reason.

You will have to explain that then, because I don't get it?  How is 
Peer.Disconnect any different?  Your 'Peer' state doesn't change to 
connecting until after WSC runs.  And if we become a GO, then things 
become even more funny since we can only send invites, not actually 
initiate a connection.  So all these methods become somewhat irrelevant. 
  In fact, I wonder if we should be removing the WSC interface when we 
become the GO and adding another one for performing invitations.

> I was just noting they're not happening on different interfaces.

Of course.  That's because P2P_CLIENT is just a station iftype under the 
hood in the kernel with some extra stuff bolted on.  But we knew that ;)

Maybe what we should do is just implement the bare minimum client side 
API, learn from it and assume that whatever API we come up with now will 
likely not survive when we start implementing GO side.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 15 Nov 2019 at 16:27, Denis Kenzior <denkenz@gmail.com> wrote:
> >> Station.Disconnect does not affect WSC state FYI.  It can't since
> >> Station doesn't even know about the ongoing WSC process until after
> >> station_connect_network is invoked.  So in that respect we'd be
> >> completely consistent.
> >
> > What I meant is that Disconnect() can cancel your station connection
> > attempt at any point, while with Peer.Disconnect() you're going to
> > have to be more careful for no reason.
>
> You will have to explain that then, because I don't get it?  How is
> Peer.Disconnect any different?  Your 'Peer' state doesn't change to
> connecting until after WSC runs.

That's needlessly complicating things for the users.  From the user's
point of view there's a device they want to connect to and use it.
They call a single method and get a connection established, and if
they need to interrupt the process then it's still single process.
You could split it in any number of ways, as before WSC there's a
bunch of other things happening, but from the user's point of view
those are details they don't want to see.  WSC is one of 4 or 5 steps
(you can make up more) in the P2P connection but there's no benefit
from making the user aware of them.

>  And if we become a GO, then things
> become even more funny since we can only send invites, not actually
> initiate a connection.

Invitation is just a technical name for a frame sequence, group
formation is another, but it's just ways to connect to a device.  Note
how Android shows the same Accept/Decline dialog whether you're using
group formation or invitation. (same or similar... I thought the
wording was the same but don't have screenshots at hand)

On a similar note:
The P2P devices I've seen don't have a physical WSC button (not
surprising), but they don't even have a virtual button or the word
Push Button anywhere in the user interfaces.  Take for example Android
connecting to a wifi-display TV, neither mentions any WSC term like
Push Button in their UIs.  The verbs are connect and accept/decline
connection (and similar).  WSC is really just a protocol used at the
network level and an implementation detail, as are the P2P roles.

>  So all these methods become somewhat irrelevant.
>   In fact, I wonder if we should be removing the WSC interface when we
> become the GO and adding another one for performing invitations.
>
> > I was just noting they're not happening on different interfaces.
>
> Of course.

Oh so this was just a typo I was replying to I guess.

>  That's because P2P_CLIENT is just a station iftype under the
> hood in the kernel with some extra stuff bolted on.  But we knew that ;)
>
> Maybe what we should do is just implement the bare minimum client side
> API, learn from it and assume that whatever API we come up with now will
> likely not survive when we start implementing GO side.

There really shouldn't be any difference to the user.

>
> Regards,
> -Denis
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

>>   And if we become a GO, then things
>> become even more funny since we can only send invites, not actually
>> initiate a connection.
> 
> Invitation is just a technical name for a frame sequence, group
> formation is another, but it's just ways to connect to a device.  Note
> how Android shows the same Accept/Decline dialog whether you're using
> group formation or invitation. (same or similar... I thought the
> wording was the same but don't have screenshots at hand)

- How are you handling the case where the Peer wants us to display our 
PIN?  And provision discovery in general?
- Invitations?
- invitation by a P2P Device (not group owner) of another P2P Device?
- The fact that GO has a bunch of stats about its clients and you 
probably want to expose that similarly to AP mode?
- The fact that GO has to handle connections from legacy clients and 
thus needs to provide legacy WPS methods?
- persistent groups, with custom or saved credentials?

To me it seems quite impractical to mix GO and client and to try and 
hide these details.  I have very strong doubts you will succeed here.

And I also think you're not avoiding exposing topology details.  For 
example, is Peer.Disconnect resulting in a disconnection of a single 
peer or group tear down?

I think you really need to send a v4.  I just don't see how you're 
handling many of the P2P concepts, much less P2P-GO specific ones in 
this one.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

On Sat, 16 Nov 2019 at 03:28, Denis Kenzior <denkenz@gmail.com> wrote:
> >>   And if we become a GO, then things
> >> become even more funny since we can only send invites, not actually
> >> initiate a connection.
> >
> > Invitation is just a technical name for a frame sequence, group
> > formation is another, but it's just ways to connect to a device.  Note
> > how Android shows the same Accept/Decline dialog whether you're using
> > group formation or invitation. (same or similar... I thought the
> > wording was the same but don't have screenshots at hand)
>
> - How are you handling the case where the Peer wants us to display our
> PIN?  And provision discovery in general?

We can add a signal for this, but this is independent of the role and
not a high priority.

Do we have a use case for this?

> - Invitations?
> - invitation by a P2P Device (not group owner) of another P2P Device?

We don't need to unless we're also going to be running on peripheral
devices, but that's not our current use case.

> - The fact that GO has a bunch of stats about its clients and you
> probably want to expose that similarly to AP mode?

We don't expose anything in AP mode?

Our main use case is to provide bluetooth-like connections to devices.

> - The fact that GO has to handle connections from legacy clients and
> thus needs to provide legacy WPS methods?

Is there a use case for this?

> - persistent groups, with custom or saved credentials?

We will need to start saving credentials because that's something
that's actually in use, and we need to do this both on the GO and
client.

>
> To me it seems quite impractical to mix GO and client and to try and
> hide these details.  I have very strong doubts you will succeed here.
>
> And I also think you're not avoiding exposing topology details.  For
> example, is Peer.Disconnect resulting in a disconnection of a single
> peer or group tear down?

Assuming that is the last peer in that group then either will work, I
don't see the difference here.

If there are more peers then obviously you don't want to break things
by disconnecting them?

>
> I think you really need to send a v4.  I just don't see how you're

Yes, we agreed on that, although I'm still missing any reasoning for
the Cancel method for example.  If you have ideas on how to handle the
following I'd also welcome them:

 * how to best signal whether the device is available to start a new connection,
   assuming you didn't like the AvailableConnections (int) property.

 * how to trigger device discovery, optimally in a way that forces the
client to stop it after
   a while, basically to make sure it's not left running when the UI
isn't active.  For
   example for our "agents" we automatically detect if they're
disconnected, here we
   probably also want the client to "own" the scan in some way.

> handling many of the P2P concepts, much less P2P-GO specific ones in
> this one.

What are some P2P-GO specific concepts that would affect the DBus API?
 As long as we provide for having multiple active connections, and we
allow for new properties on the Peer interface in the future, we
should be fine.

Best regards

Re: [RFCv3] Document P2P dbus interfaces

[Denis Kenzior]

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

Hi Andrew,

On 11/15/19 9:02 PM, Andrew Zaborowski wrote:
> On Sat, 16 Nov 2019 at 03:28, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>    And if we become a GO, then things
>>>> become even more funny since we can only send invites, not actually
>>>> initiate a connection.
>>>
>>> Invitation is just a technical name for a frame sequence, group
>>> formation is another, but it's just ways to connect to a device.  Note
>>> how Android shows the same Accept/Decline dialog whether you're using
>>> group formation or invitation. (same or similar... I thought the
>>> wording was the same but don't have screenshots at hand)
>>
>> - How are you handling the case where the Peer wants us to display our
>> PIN?  And provision discovery in general?
> 
> We can add a signal for this, but this is independent of the role and
> not a high priority.

It sort of is.  How is the user notified that some other device is 
trying to connect to us?  How do they know its time to enter the 
password or just display it on the screen?

And a signal is not enough as the UI might want to perform an 
authorization step here as well.

> 
> Do we have a use case for this?

Absolutely.

> 
>> - Invitations?
>> - invitation by a P2P Device (not group owner) of another P2P Device?
> 
> We don't need to unless we're also going to be running on peripheral
> devices, but that's not our current use case.

The latter is optional and of dubious value.  The former might be kind 
of important.

> 
>> - The fact that GO has a bunch of stats about its clients and you
>> probably want to expose that similarly to AP mode?
> 
> We don't expose anything in AP mode?
> 

AP mode isn't really finished.  We need to add this some day.

> Our main use case is to provide bluetooth-like connections to devices.
> 
>> - The fact that GO has to handle connections from legacy clients and
>> thus needs to provide legacy WPS methods?
> 
> Is there a use case for this?
> 

If you want to pass conformance tests it is.  See Table 7: GOUT Protocol 
Conformance Tests in the Wi-Fi Direct Test plan.

>> - persistent groups, with custom or saved credentials?
> 
> We will need to start saving credentials because that's something
> that's actually in use, and we need to do this both on the GO and
> client.

Fair enough.

> 
>>
>> To me it seems quite impractical to mix GO and client and to try and
>> hide these details.  I have very strong doubts you will succeed here.
>>
>> And I also think you're not avoiding exposing topology details.  For
>> example, is Peer.Disconnect resulting in a disconnection of a single
>> peer or group tear down?
> 
> Assuming that is the last peer in that group then either will work, I
> don't see the difference here.

But you may want to keep the group around.   Anyway, this is not a 
biggie but I think you're making an assumption this would never be wanted.

> 
> If there are more peers then obviously you don't want to break things
> by disconnecting them?

Sure, this one is obvious.

> 
>>
>> I think you really need to send a v4.  I just don't see how you're
> 
> Yes, we agreed on that, although I'm still missing any reasoning for
> the Cancel method for example.  If you have ideas on how to handle the

That's just how Station & WSC work today, and actually work pretty well 
compared to any other WPS API I saw.  I don't see why following the same 
pattern is so bad?  In the end the user still invokes a single method 
and gets connected.

Really, all your criticisms apply to station/wsc as well.  But then 
aborts are sort of an uncommon case anyway.

> following I'd also welcome them:
> 
>   * how to best signal whether the device is available to start a new connection,
>     assuming you didn't like the AvailableConnections (int) property.

At the moment I think exposing the role is the way to go.  But feel free 
to convince me otherwise.

> 
>   * how to trigger device discovery, optimally in a way that forces the
> client to stop it after
>     a while, basically to make sure it's not left running when the UI
> isn't active.  For
>     example for our "agents" we automatically detect if they're
> disconnected, here we
>     probably also want the client to "own" the scan in some way.

One idea would be to add a StartDiscovery / StopDiscovery methods and 
track client state.  So if a client drops off the bus, then we 
automatically stop discovery sessions initiated by it.  This could also 
be used to send parameters for the discovery.  E.g. only WFD devices. 
Each distinct dbus client (i.e. dbus unique service name) can set its 
own parameters for peers it is interested in.

However, these commands might need to be limited if we're in some 
existing connection or not.  You're the expert on the details here.

> 
>> handling many of the P2P concepts, much less P2P-GO specific ones in
>> this one.
> 
> What are some P2P-GO specific concepts that would affect the DBus API?
>   As long as we provide for having multiple active connections, and we
> allow for new properties on the Peer interface in the future, we
> should be fine.
> 

I think this was already covered.  If you can unify the API, great.  I 
think there are some nasty stumbling blocks though.

Regards,
-Denis

Re: [RFCv3] Document P2P dbus interfaces

[Andrew Zaborowski]

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

Hi Denis,

On Sat, 16 Nov 2019 at 05:12, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/15/19 9:02 PM, Andrew Zaborowski wrote:
> > On Sat, 16 Nov 2019 at 03:28, Denis Kenzior <denkenz@gmail.com> wrote:
> >>>>    And if we become a GO, then things
> >>>> become even more funny since we can only send invites, not actually
> >>>> initiate a connection.
> >>>
> >>> Invitation is just a technical name for a frame sequence, group
> >>> formation is another, but it's just ways to connect to a device.  Note
> >>> how Android shows the same Accept/Decline dialog whether you're using
> >>> group formation or invitation. (same or similar... I thought the
> >>> wording was the same but don't have screenshots at hand)
> >>
> >> - How are you handling the case where the Peer wants us to display our
> >> PIN?  And provision discovery in general?
> >
> > We can add a signal for this, but this is independent of the role and
> > not a high priority.
>
> It sort of is.  How is the user notified that some other device is
> trying to connect to us?  How do they know its time to enter the
> password or just display it on the screen?

Device to device connections weren't really my concern for this
iteration of the API, but I think we may have even talked about having
an optional agent to receive the invitations / new connections.  I
guess your use case is something like file sharing?  Note that there's
no standard service for this, you need to choose one of a hundred apps
incompatible between each other, and you need to have a compatible app
on the other end.

>
> And a signal is not enough as the UI might want to perform an
> authorization step here as well.
>
> >
> > Do we have a use case for this?
>
> Absolutely.
>
> >
> >> - Invitations?
> >> - invitation by a P2P Device (not group owner) of another P2P Device?
> >
> > We don't need to unless we're also going to be running on peripheral
> > devices, but that's not our current use case.
>
> The latter is optional and of dubious value.  The former might be kind
> of important.
>
> >
> >> - The fact that GO has a bunch of stats about its clients and you
> >> probably want to expose that similarly to AP mode?
> >
> > We don't expose anything in AP mode?
> >
>
> AP mode isn't really finished.  We need to add this some day.

Things like bandwidth usage, IP?  They should probably use the same
property names for both AP and P2P.

>
> > Our main use case is to provide bluetooth-like connections to devices.
> >
> >> - The fact that GO has to handle connections from legacy clients and
> >> thus needs to provide legacy WPS methods?
> >
> > Is there a use case for this?
> >
>
> If you want to pass conformance tests it is.  See Table 7: GOUT Protocol
> Conformance Tests in the Wi-Fi Direct Test plan.

I can't find this document right now but I assume there's a few more
things that may be not worth the effort implementing and are required
for certification.

>
> >> - persistent groups, with custom or saved credentials?
> >
> > We will need to start saving credentials because that's something
> > that's actually in use, and we need to do this both on the GO and
> > client.
>
> Fair enough.
>
> >
> >>
> >> To me it seems quite impractical to mix GO and client and to try and
> >> hide these details.  I have very strong doubts you will succeed here.
> >>
> >> And I also think you're not avoiding exposing topology details.  For
> >> example, is Peer.Disconnect resulting in a disconnection of a single
> >> peer or group tear down?
> >
> > Assuming that is the last peer in that group then either will work, I
> > don't see the difference here.
>
> But you may want to keep the group around.   Anyway, this is not a
> biggie but I think you're making an assumption this would never be wanted.

It may be wanted or it may be the default if we manage to become the
GO (the peer may in theory require the GO role).  But it may be a
config setting.

>
> >
> > If there are more peers then obviously you don't want to break things
> > by disconnecting them?
>
> Sure, this one is obvious.
>
> >
> >>
> >> I think you really need to send a v4.  I just don't see how you're
> >
> > Yes, we agreed on that, although I'm still missing any reasoning for
> > the Cancel method for example.  If you have ideas on how to handle the
>
> That's just how Station & WSC work today, and actually work pretty well
> compared to any other WPS API I saw.  I don't see why following the same
> pattern is so bad?  In the end the user still invokes a single method
> and gets connected.
>
> Really, all your criticisms apply to station/wsc as well.  But then
> aborts are sort of an uncommon case anyway.
>
> > following I'd also welcome them:
> >
> >   * how to best signal whether the device is available to start a new connection,
> >     assuming you didn't like the AvailableConnections (int) property.
>
> At the moment I think exposing the role is the way to go.  But feel free
> to convince me otherwise.

So I was rather thinking of one simple property to indicate if we can
start a new connection and/or how many.  This would combine at least:

 * our P2P role,

 * whether we're already connected (in other words whether we have as
many active connections as max allowed P2P interfaces...)

 * whether we're currently connecting/disconnecting as a P2P client
(during those operations the Connected property will be false in my
current design)

 * whether we're currently in Group Formation in which case we're not
allowed do anything as either client or GO.

>
> >
> >   * how to trigger device discovery, optimally in a way that forces the
> > client to stop it after
> >     a while, basically to make sure it's not left running when the UI
> > isn't active.  For
> >     example for our "agents" we automatically detect if they're
> > disconnected, here we
> >     probably also want the client to "own" the scan in some way.
>
> One idea would be to add a StartDiscovery / StopDiscovery methods and
> track client state.  So if a client drops off the bus, then we
> automatically stop discovery sessions initiated by it.  This could also
> be used to send parameters for the discovery.  E.g. only WFD devices.
> Each distinct dbus client (i.e. dbus unique service name) can set its
> own parameters for peers it is interested in.

Ok.

Best regards