Re: [PATCH v3 7/7] gps: add documentation

[Rafael Ignacio Zurita <rafael.zurita@profusion.mobi>]

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

On Wed, Jan 26, 2011 at 07:47:46PM -0200, Gustavo F. Padovan wrote:
> Hi Rafael,
> 
> * Rafael Ignacio Zurita <rafael.zurita@profusion.mobi> [2011-01-26 12:32:54 -0300]:
> 
> > ---
> >  Makefile.am                  |    3 +-
> >  doc/gps-api.txt              |   49 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+), 1 deletions(-)
> >  create mode 100644 doc/gps-api.txt
> >  mode change 100755 => 100644 test/test-location-reporting
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 49bf8f3..2ebf9e6 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -399,7 +399,8 @@ doc_files = doc/overview.txt doc/ofono-paper.txt doc/release-faq.txt \
> >  			doc/phonebook-api.txt doc/radio-settings-api.txt \
> >  			doc/sim-api.txt doc/stk-api.txt \
> >  			doc/audio-settings-api.txt doc/text-telephony-api.txt \
> > -			doc/calypso-modem.txt doc/message-api.txt
> > +			doc/calypso-modem.txt doc/message-api.txt \
> > +			doc/gps-api.txt
> 
> What about call this location-report-api.txt?

Yes, that could be more consistent.

> 
> >  
> >  
> >  test_scripts = test/backtrace \
> > diff --git a/doc/gps-api.txt b/doc/gps-api.txt
> > new file mode 100644
> > index 0000000..85b0669
> > --- /dev/null
> > +++ b/doc/gps-api.txt
> > @@ -0,0 +1,49 @@
> > +Location Reporting hierarchy
> > +=================
> > +
> > +Service		org.ofono
> > +Interface	org.ofono.LocationReporting
> > +Object path	[variable prefix]/{modem0,modem1,...}
> > +
> > +Methods		dict GetProperties()
> > +
> > +			Returns all Gps properties. See the
> > +			properties section for available properties.
> > +
> > +			Possible Errors: [service].Error.InProgress
> > +					 [service].Error.Failed
> > +
> > +		void RegisterAgent(object path)
> > +
> > +			Registers an agent which will be called with the
> > +			gps file descriptor.
> > +
> > +		void UnregisterAgent(object path)
> > +
> > +			Unregisters an agent.
> > +
> 
> You are missing Enable() and Disable() methods here, you can't just enable the
> GPS device when registering the Agent.

Why no? I followed the comments for the new API :
http://lists.ofono.org/pipermail/ofono/2011-January/007477.html
which suggests :

An external client registers an Agent with this
interface.  Internally this triggers the driver function to bring up the
GPS device and return the file descriptor to the core.  The core takes
care of calling the Agent with the file descriptor using DBus FD passing
mechanisms.

> 
> > +Properties	string Type [readonly]
> > +
> > +			Holds the type of the device (e.g. "nmea")
> 
> Then you also need a Enabled property here.
> 
> > +
> > +LocationReportingAgent Hierarchy [experimental]
> > +===============
> > +
> > +Service		unique name
> > +Interface	org.ofono.LocationReportingAgent
> > +Object path	freely definable
> > +
> > +Methods		void ReceiveGpsFileDescriptor(int32 fd)
> 
> I think you get rid of this method an make Enable() return the file descriptor
> you want. Input from Marcel and Denis here would be good.

If so then you would like to see the previous proposal, my version 2 of
the gps patches, which does you like.


Rafael Zurita