| Message ID | 20220622194142.213135-2-jesse@twosheds.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] dpp: Add State and Role properties to dbus API | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-ci-gitlint | success | GitLint |
Hi Jesse, On Wed, 2022-06-22 at 15:41 -0400, Jesse Lentz wrote: > Document the State and Role properties of the DeviceProvisioning API. > --- > doc/device-provisioning-api.txt | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/doc/device-provisioning-api.txt b/doc/device- > provisioning-api.txt > index 0aba2557..c1d706ec 100644 > --- a/doc/device-provisioning-api.txt > +++ b/doc/device-provisioning-api.txt > @@ -56,3 +56,13 @@ Methods string StartEnrollee() > net.connman.iwd.NotCo > nfigured > net.connman.iwd.NotSu > pported > net.connman.iwd.Busy > + > +Properties string State [readonly, optional] > + > + Reflects the DPP state. Possible values are > "presence", > + "authenticating", and "configuring". I think a "stopped" state would also be needed. > + > + string Role [readonly, optional] > + > + Reflects the DPP role. Possible values are > "enrollee" > + and "configurator" I don't think you actually need this one. The role should be implied by however you started DPP: StartEnrollee() -> enrollee StartConfigurator() -> configurator ConfigureEnrollee() -> configurator Thanks, James
Hi James, Thanks for your feedback. > > +Properties string State [readonly, optional] > > + > > + Reflects the DPP state. Possible values are > > "presence", > > + "authenticating", and "configuring". > > I think a "stopped" state would also be needed. As a user of the API, I can infer that DPP is inactive if "State" is undefined. A non-optional State property with an explicit "inactive" value should provide equivalent information; would you prefer the latter approach? > > + string Role [readonly, optional] > > + > > + Reflects the DPP role. Possible values are > > "enrollee" > > + and "configurator" > > I don't think you actually need this one. The role should be implied by > however you started DPP: > > StartEnrollee() -> enrollee > StartConfigurator() -> configurator > ConfigureEnrollee() -> configurator One use case that I'm trying to address is when a user launches the front-end app while DPP is already active. There are two ways that I can think of to handle this: 1) The front-end stops and re-starts DPP if the user requests it. This only requires iwd to provide State, so that the front-end can see if a previous invocation must be stopped. 2) iwd provides sufficient information via properties to restore the previous invocation: State, Role, and URI. I originally had option 2 in mind, but if DPP invocations are intended to be transient, then perhaps option 1 would be more appropriate. Let me know what your thoughts are, and I'll send a revised patch. Thanks. Sincerely, Jesse
Hi Jesse, On Thu, 2022-06-23 at 12:30 -0400, Jesse Lentz wrote: > Hi James, > > Thanks for your feedback. > > > > +Properties string State [readonly, optional] > > > + > > > + Reflects the DPP state. Possible values > > > are > > > "presence", > > > + "authenticating", and "configuring". > > > > I think a "stopped" state would also be needed. > > As a user of the API, I can infer that DPP is inactive if "State" is > undefined. A non-optional State property with an explicit "inactive" > value should provide equivalent information; would you prefer the > latter > approach? Yeah, I think having an explicit value is best. I think it makes it easier to handle for the consumer of the API. > > > > + string Role [readonly, optional] > > > + > > > + Reflects the DPP role. Possible values > > > are > > > "enrollee" > > > + and "configurator" > > > > I don't think you actually need this one. The role should be > > implied by > > however you started DPP: > > > > StartEnrollee() -> enrollee > > StartConfigurator() -> configurator > > ConfigureEnrollee() -> configurator > > One use case that I'm trying to address is when a user launches the > front-end app while DPP is already active. There are two ways that I > can > think of to handle this: Ah, ok this is fine then. Having the property would be the simplest way to handle this case. > > 1) The front-end stops and re-starts DPP if the user requests it. > This > only requires iwd to provide State, so that the front-end can see if > a > previous invocation must be stopped. > > 2) iwd provides sufficient information via properties to restore the > previous invocation: State, Role, and URI. > > I originally had option 2 in mind, but if DPP invocations are > intended > to be transient, then perhaps option 1 would be more appropriate. Let > me > know what your thoughts are, and I'll send a revised patch. Out of curiosity do you need these properties purely for displaying to the user? If so I wonder if they shoud go on a new interface, e.g. DeviceProvisioningDiagnostics. We have a similar concept with AccessPointDiagnostics and StationDiagnostics. These interfaces hold extra information that isn't really needed to use IWD generally, but more debugging or displaying information in a UI. These seem to fit more into that category than needed for DPP in general. > > Thanks. > > Sincerely, > Jesse
James, > Yeah, I think having an explicit value is best. I think it makes it > easier to handle for the consumer of the API. Sounds good. > > 1) The front-end stops and re-starts DPP if the user requests it. > > This > > only requires iwd to provide State, so that the front-end can see if > > a > > previous invocation must be stopped. > > > > 2) iwd provides sufficient information via properties to restore the > > previous invocation: State, Role, and URI. > > > > I originally had option 2 in mind, but if DPP invocations are > > intended > > to be transient, then perhaps option 1 would be more appropriate. Let > > me > > know what your thoughts are, and I'll send a revised patch. > > Out of curiosity do you need these properties purely for displaying to > the user? If so I wonder if they shoud go on a new interface, e.g. > DeviceProvisioningDiagnostics. > > We have a similar concept with AccessPointDiagnostics and > StationDiagnostics. These interfaces hold extra information that isn't > really needed to use IWD generally, but more debugging or displaying > information in a UI. These seem to fit more into that category than > needed for DPP in general. The uses that I have in mind are: 1) Notify the user when a device is being configured (by monitoring State) 2) Determine whether to show the user a "Start" button or a "Stop" button. 3) Display a QR code and a label showing the user whether credentials are being given or received. The benefit of properties over method return values for (2) and (3) is that the front-end app gets to be stateless, so that the user could quit and re-start the app and still see the QR code. Thanks, and let me know what your thoughts are. Sincerely, Jesse
Hi Jesse, On 6/23/22 11:30, Jesse Lentz wrote: > Hi James, > > Thanks for your feedback. > >>> +Properties string State [readonly, optional] >>> + >>> + Reflects the DPP state. Possible values are >>> "presence", >>> + "authenticating", and "configuring". >> >> I think a "stopped" state would also be needed. > > As a user of the API, I can infer that DPP is inactive if "State" is > undefined. A non-optional State property with an explicit "inactive" > value should provide equivalent information; would you prefer the latter > approach? Inferring the state is fine if you can do this reliably. One problem with optional properties is that once they transition from 'defined' -> 'undefined' there is no PropertiesChanged signal that is sent (or rather you have to look into the invalidated_properties array of that signal). We have been designing our APIs with the assumption that many bindings wouldn't be using this invalidated_properties reliably and added another point of reference. For example, Station.ConnectedNetwork is invalidated when Station.State goes into the 'disconnected' state and thus not explicitly 'changed'. But the DBus client can infer that Station.ConnectedNetwork is 'undefined' at that point. But maybe bindings have improved over the years and we should just rely on invalidated_properties being taken into account properly? If so, that would be a good argument to make 'State' optional. Regards, -Denis
Hi Denis, > Inferring the state is fine if you can do this reliably. One problem with > optional properties is that once they transition from 'defined' -> 'undefined' > there is no PropertiesChanged signal that is sent (or rather you have to look > into the invalidated_properties array of that signal). > > We have been designing our APIs with the assumption that many bindings wouldn't > be using this invalidated_properties reliably and added another point of reference. > > For example, Station.ConnectedNetwork is invalidated when Station.State goes > into the 'disconnected' state and thus not explicitly 'changed'. But the DBus > client can infer that Station.ConnectedNetwork is 'undefined' at that point. > > But maybe bindings have improved over the years and we should just rely on > invalidated_properties being taken into account properly? If so, that would be > a good argument to make 'State' optional. Hmm, I wasn't aware of that. I can only comment on the GLib binding, which works reliably as far as I can tell - it supplies both changed and invalidated properties to the user-registered callback, which gets called in response to both changed and to invalidated properties. But if other bindings handle it incorrectly, then I suppose there's no compelling reason to make it an optional property. Thanks. Sincerely, Jesse
Hi Jesse, On 6/23/22 14:07, Jesse Lentz wrote: > Hi Denis, > >> Inferring the state is fine if you can do this reliably. One problem with >> optional properties is that once they transition from 'defined' -> 'undefined' >> there is no PropertiesChanged signal that is sent (or rather you have to look >> into the invalidated_properties array of that signal). >> >> We have been designing our APIs with the assumption that many bindings wouldn't >> be using this invalidated_properties reliably and added another point of reference. >> >> For example, Station.ConnectedNetwork is invalidated when Station.State goes >> into the 'disconnected' state and thus not explicitly 'changed'. But the DBus >> client can infer that Station.ConnectedNetwork is 'undefined' at that point. >> >> But maybe bindings have improved over the years and we should just rely on >> invalidated_properties being taken into account properly? If so, that would be >> a good argument to make 'State' optional. > > Hmm, I wasn't aware of that. I can only comment on the GLib binding, > which works reliably as far as I can tell - it supplies both changed and > invalidated properties to the user-registered callback, which gets > called in response to both changed and to invalidated properties. > Ok, good to know that this works well. > But if other bindings handle it incorrectly, then I suppose there's no > compelling reason to make it an optional property. Maybe I shouldn't blame the bindings as much as the documentation. This wasn't a widely explained feature at the time we started, and this project is getting pretty old. But yes, maybe it is easier to just make this property mandatory while all others can be made optional. Regards, -Denis
diff --git a/doc/device-provisioning-api.txt b/doc/device-provisioning-api.txt index 0aba2557..c1d706ec 100644 --- a/doc/device-provisioning-api.txt +++ b/doc/device-provisioning-api.txt @@ -56,3 +56,13 @@ Methods string StartEnrollee() net.connman.iwd.NotConfigured net.connman.iwd.NotSupported net.connman.iwd.Busy + +Properties string State [readonly, optional] + + Reflects the DPP state. Possible values are "presence", + "authenticating", and "configuring". + + string Role [readonly, optional] + + Reflects the DPP role. Possible values are "enrollee" + and "configurator"