Hi Robertino,

On 11/05/2010 12:55 PM, Robertino Benis wrote:
> ---
>  doc/assistedgps-manager-api.txt |  114 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 114 insertions(+), 0 deletions(-)
>  create mode 100644 doc/assistedgps-manager-api.txt
> 
> diff --git a/doc/assistedgps-manager-api.txt b/doc/assistedgps-manager-api.txt
> new file mode 100644
> index 0000000..b09c8b1
> --- /dev/null
> +++ b/doc/assistedgps-manager-api.txt
> @@ -0,0 +1,114 @@
> +AgpsManager hierarchy
> +=====================
> +
> +Service		org.ofono
> +Interface	org.ofono.AgpsManager

So in general I'm currently against introducing this API as oFono
official API.  I suggest prefixing this with an IFX specific identifier.
 Maybe modem.ifx.AgpsManager.

> +Object path	[variable prefix]/{modem0,modem1,...}
> +
> +Methods		dict GetProperties()
> +
> +			Returns properties for the modem object. See
> +			the properties section for available properties.
> +
> +			Possible Errors: [service].Error.InvalidArguments
> +
> +		void SetProperty(string name, variant value)
> +
> +			Changes the value of the specified property. Only
> +			properties that are listed as read-write are
> +			changeable. On success a PropertyChanged signal
> +			will be emitted.
> +
> +			Possible Errors: [service].Error.InvalidArguments
> +					 [service].Error.DoesNotExist
> +
> +		void SendLCSFrame(string frametype, string framedata)

oFono is strict CamelCase, even for abbreviations.  You do this
correctly above (AgpsManager) but not here.  Please be consistent.  And
it might be a good idea to not have abbreviations at all.  What does LCS
stand for in this case?

> +
> +			Send a LCS position protocol frame to the Mobile
> +			Network. The LCS frame typically represents a
> +			Position Response.
> +
> +			Valid frametypes are:
> +				rrlp_measure_position_response
> +				rrc_measurement_report
> +
> +			The raw frame data is formatted as the concatenated
> +			sequence of the two digit hexadecimal representation
> +			of each of its octets. Example: "00FC2345"

Sending hex-encoded data screams as being non-portable and AT command
modem specific.  Is this really your intent here?

> +
> +		void RequestFineTimeInjection(string rat, uint16 pulselength)
> +
> +			Request modem to generate a fine time injection
> +			pulse. pulselength is the duration of the pulse
> +			expressed in radio frames.
> +
> +			rat specifies the access technology used to derive
> +			the pulse from and can be "gsm" or "umts".
> +			If the requested access technology is not currently
> +			in use an error is returned.

It seems to me that making the upper layers pass in the rat as a
parameter to this function is a pretty bad idea.  You should either have
the atom figure this out if it can (e.g. by using netreg atom) or have
the driver take care of this directly.

> +
> +Signals		PropertyChanged(string name, variant value)
> +
> +			This signal indicates a changed value of the given
> +			property.
> +
> +		IncomingLCSFrame(string frametypes, string framedata)
> +
> +			LCS positioning protocol frame received from the
> +			Mobile Network.
> +
> +			Valid frametypes for the LCS frame are:
> +				rrlp_assistance_data
> +				rrlp_measure_position_request
> +				rrc_assistance_data_delivery
> +				rrc_measurement_control
> +
> +			Note that position/measurement requests can include
> +			assistance data as well.
> +
> +			The raw frame data is formatted as the concatenated
> +			sequence of the two digit hexadecimal representation
> +			of each of its octets. Example: "00FC2345"

Same comment as above about LCS naming and hex-encoded strings.

> +
> +		FineTimeInjectionNotification(dict radioframenumber)
> +
> +			Notification about fine time injection pulse
> +			generated by modem. The radioframenumber dict
> +			is defined as follow:
> +

Why is this returned in a separate signal as opposed to a reply to the
RequestFineTimeInjection method call?

> +			string AccessTechnology
> +				"gsm" or "umts"
> +			
> +			uint32 TdmaFrameNumber (gsm only)
> +				range 0 - 2715647 (2048*26*51)
> +
> +			uint16 TdmaTimeslot (gsm only)
> +				range 0 - 7
> +
> +			uint16 TimeslotBit (gsm only)
> +				range 0 - 156
> +
> +			uint16 TimingAdvance (gsm only)
> +				range 0 - 63
> +
> +			uint16 BcchArfcn (gsm only)
> +				range 0 - 1023
> +
> +			uint16 Bsic (gsm only)
> +				range 0 - 64
> +
> +			uint16 Sfn (umts only)
> +				range 0 - 4095
> +
> +			string RrcState (umts only)
> +				"cell_dch", "cell_fach", "cell_pch" or
> +				"ura_pch"
> +			
> +			uint16 RoundTripTime (umts only)
> +				range 0 - 32766
> +
> +
> +Properties	boolean LcsEnabled [readwrite]
> +
> +			If LcsEnabled is False, then no LCS positioning
> +			protocol frames are received.

In general it is a bad idea to have properties with abbreviations in
them.  What does LCS stand for?

Regards,
-Denis