subject:"Re\: \[PATCH 4\/5\] cinterion\: support AT\^SGPSC capable modems"

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

2017-05-30 Thread Aleksander Morgado

On 17/05/17 15:24, Aleksander Morgado wrote:
> The AT^SGPSS command provides an easy way to just start/stop GPS, but
> unfortunately it isn't supported by all Cinterion modems.
> 
> The AT^SGPSC command instead is more widely available but it requires
> several steps to start and stop the different elements of the GPS
> receiver.
> 
> Implement support for both, preferring AT^SGPSSS if available and
> falling back to AT^SGPSC otherwise.
> ---

This has been merged to git master.

>  plugins/cinterion/mm-common-cinterion.c | 209 
> +++-
>  1 file changed, 182 insertions(+), 27 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-common-cinterion.c 
> b/plugins/cinterion/mm-common-cinterion.c
> index ce337bd9..6e839c8d 100644
> --- a/plugins/cinterion/mm-common-cinterion.c
> +++ b/plugins/cinterion/mm-common-cinterion.c
> @@ -36,6 +36,7 @@ typedef enum {
>  typedef struct {
>  MMModemLocationSource enabled_sources;
>  FeatureSupportsgpss_support;
> +FeatureSupportsgpsc_support;
>  } LocationContext;
>  
>  static void
> @@ -59,6 +60,7 @@ get_location_context (MMBaseModem *self)
>  ctx = g_slice_new (LocationContext);
>  ctx->enabled_sources = MM_MODEM_LOCATION_SOURCE_NONE;
>  ctx->sgpss_support = FEATURE_SUPPORT_UNKNOWN;
> +ctx->sgpsc_support = FEATURE_SUPPORT_UNKNOWN;
>  
>  g_object_set_qdata_full (
>  G_OBJECT (self),
> @@ -131,6 +133,30 @@ mm_common_cinterion_location_load_capabilities_finish 
> (MMIfaceModemLocation  *se
>  static void probe_gps_features (GTask *task);
>  
>  static void
> +sgpsc_test_ready (MMBaseModem  *self,
> +  GAsyncResult *res,
> +  GTask*task)
> +{
> +LocationContext *location_ctx;
> +
> +location_ctx = get_location_context (self);
> +if (!mm_base_modem_at_command_finish (self, res, NULL))
> +location_ctx->sgpsc_support = FEATURE_NOT_SUPPORTED;
> +else {
> +/* ^SGPSC supported! */
> +location_ctx->sgpsc_support = FEATURE_SUPPORTED;
> +/* It may happen that the modem was started with GPS already 
> enabled, or
> + * maybe ModemManager got rebooted and it was left enabled before. 
> We'll
> + * make sure that it is disabled when we initialize the modem. */
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"Engine\",\"0\"",  3, FALSE, NULL, NULL);
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"Power/Antenna\",\"off\"", 3, FALSE, NULL, NULL);
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"NMEA/Output\",\"off\"",   3, FALSE, NULL, NULL);
> +}
> +
> +probe_gps_features (task);
> +}
> +
> +static void
>  sgpss_test_ready (MMBaseModem  *self,
>GAsyncResult *res,
>GTask*task)
> @@ -143,6 +169,11 @@ sgpss_test_ready (MMBaseModem  *self,
>  else {
>  /* ^SGPSS supported! */
>  location_ctx->sgpss_support = FEATURE_SUPPORTED;
> +
> +/* Flag ^SGPSC as unsupported, even if it may be supported, so that 
> we
> + * only use one set of commands to enable/disable GPS. */
> +location_ctx->sgpsc_support = FEATURE_NOT_SUPPORTED;
> +
>  /* It may happen that the modem was started with GPS already 
> enabled, or
>   * maybe ModemManager got rebooted and it was left enabled before. 
> We'll
>   * make sure that it is disabled when we initialize the modem. */
> @@ -169,8 +200,14 @@ probe_gps_features (GTask *task)
>  return;
>  }
>  
> +/* Need to check if SGPSC supported... */
> +if (location_ctx->sgpsc_support == FEATURE_SUPPORT_UNKNOWN) {
> +mm_base_modem_at_command (self, "AT^SGPSC=?", 3, TRUE, 
> (GAsyncReadyCallback) sgpsc_test_ready, task);
> +return;
> +}
> +
>  /* All GPS features probed, check if GPS supported */
> -if (location_ctx->sgpss_support == FEATURE_SUPPORTED) {
> +if (location_ctx->sgpss_support == FEATURE_SUPPORTED || 
> location_ctx->sgpsc_support == FEATURE_SUPPORTED) {
>  mm_dbg ("GPS commands supported: GPS capabilities enabled");
>  ctx->sources |= (MM_MODEM_LOCATION_SOURCE_GPS_NMEA |
>   MM_MODEM_LOCATION_SOURCE_GPS_RAW  |
> @@ -244,6 +281,9 @@ mm_common_cinterion_location_load_capabilities 
> (MMIfaceModemLocation *self,
>  typedef enum {
>  DISABLE_LOCATION_GATHERING_GPS_STEP_FIRST,
>  DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSS,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ENGINE,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ANTENNA,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_OUTPUT,
>  DISABLE_LOCATION_GATHERING_GPS_STEP_LAST,
>  } DisableLocationGatheringGpsStep;
>  
> @@ -251,6 +291,7 @@ typedef struct {
>  MMModemLocationSourcesource;
>  DisableLocationGatheringGpsStep  gps_step;
>  GError

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

2017-05-30 Thread Aleksander Morgado

On Tue, May 30, 2017 at 8:27 PM, Dan Williams  wrote:
>> @@ -429,13 +536,15 @@ mm_common_cinterion_disable_location_gathering 
>> (MMIfaceModemLocation  *self,
>>  typedef enum {
>>  ENABLE_LOCATION_GATHERING_GPS_STEP_FIRST,
>>  ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSS,
>> +ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_OUTPUT,
>> +ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ANTENNA,
>> +ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ENGINE,
>>  ENABLE_LOCATION_GATHERING_GPS_STEP_LAST,
>>  } EnableLocationGatheringGpsStep;
>>
>>  typedef struct {
>>  MMModemLocationSource  source;
>>  EnableLocationGatheringGpsStep gps_step;
>> -gboolean   sgpss_success;
>>  } EnableLocationGatheringContext;
>>
>>  static void
>> @@ -454,28 +563,35 @@ mm_common_cinterion_enable_location_gathering_finish 
>> (MMIfaceModemLocation  *sel
>>
>>  static void enable_location_gathering_context_gps_step (GTask *task);
>>
>> -static void
>> -enable_sgpss_ready (MMBaseModem  *self,
>> -GAsyncResult *res,
>> -GTask*task)
>> +static gboolean
>> +enable_location_gathering_context_gps_step_next_cb (GTask *task)
>>  {
>>  EnableLocationGatheringContext *ctx;
>> -GError *error = NULL;
>>
>>  ctx = (EnableLocationGatheringContext *) g_task_get_task_data (task);
>
> Is there a way we can check a GCancellable here?  If the modem gets
> removed while we're enabling GPS this might call back with a dead
> object.

We've discussed this in IRC already and we'll go on with the patch as
it is for now. The rationale for this is that the GTask already holds
a full reference of the modem object, so a valid reference will be
available until the whole async method finishes. If the modem goes
away in the middle of the sequence, we'll just end up doing some dummy
timeouts (retrying for Engine = 1) and then just fail the async
method, see e.g. the following logs:

ModemManager[4839]:  [1496171739.271335] Setting up location
sources: '3gpp-lac-ci, gps-nmea'
ModemManager[4839]:  [1496171739.271399] Location '3gpp-lac-ci'
gathering is already enabled...
ModemManager[4839]:  [1496171739.271415] Location 'gps-raw'
gathering is already disabled...
ModemManager[4839]:  [1496171739.271427] Location
'gps-unmanaged' gathering is already disabled...
ModemManager[4839]:  [1496171739.271443] Need to enable the
following location sources: 'gps-nmea'
ModemManager[4839]:  [1496171739.271637] (ttyACM1) device open
count is 2 (open)
ModemManager[4839]:  [1496171739.271694] (ttyACM1): -->
'AT^SGPSC="NMEA/Output","on"'
ModemManager[4839]:  [1496171739.331314] (ttyACM1): <--
'^SGPSC: "Nmea/Output","on"OK'
ModemManager[4839]:  [1496171739.331410] (ttyACM1) device open
count is 1 (close)
ModemManager[4839]:  [1496171739.431720] (ttyACM1) device open
count is 2 (open)
ModemManager[4839]:  [1496171739.431815] (ttyACM1): -->
'AT^SGPSC="Power/Antenna","on"'
ModemManager[4839]:  [1496171739.495599] (ttyACM1): <--
'^SGPSC: "Power/Antenna","on"OK'
ModemManager[4839]:  [1496171739.495722] (ttyACM1) device open
count is 1 (close)
ModemManager[4839]:  [1496171740.133830] (ttyACM0) unexpected
port hangup!
ModemManager[4839]:  [1496171740.133888] (ttyACM0) forced to close port
ModemManager[4839]:  [1496171740.133918] (ttyACM0) device open
count is 0 (close)
ModemManager[4839]:  [1496171740.133939] (ttyACM0) closing serial port...
ModemManager[4839]:  [1496171740.165232] (ttyACM0) serial port closed
ModemManager[4839]:  [1496171740.167708] (ttyACM1) unexpected
port hangup!
ModemManager[4839]:  [1496171740.167770] (ttyACM1) forced to close port
ModemManager[4839]:  [1496171740.167791] (ttyACM1) device open
count is 0 (close)
ModemManager[4839]:  [1496171740.167811] (ttyACM1) closing serial port...
ModemManager[4839]:  [1496171740.188124] (ttyACM1) serial port closed
ModemManager[4839]:  [1496171740.188506] Removing device 'USB3'
ModemManager[4839]:  [1496171740.188722] [device USB3]
unexported modem from path '/org/freedesktop/ModemManager1/Modem/0'
ModemManager[4839]:  [1496171740.189045] (ttyACM2) forced to close port
ModemManager[4839]:  [1496171741.497337] GPS Engine setup failed (1/3)
ModemManager[4839]:  [1496171743.499526] GPS Engine setup failed (2/3)
ModemManager[4839]:  [1496171745.500241] GPS Engine setup failed (3/3)
ModemManager[4839]:  [1496171745.500593] Modem (Cinterion)
'USB3' completely disposed

The last lines show how the GPS Engine setup retries are happening
every 2s, until the last 3/3 where the async task finishes, the GTask
gets disposed, and along with the GTask the last modem object
reference.

In order to avoid the retries when we know that the modem is already
gone (something that also happens in other logic bits doing retries,
like the unlock check retries), I'll try to revive the idea of having
a "device wide cancellable" that we can attach to every async
operation (e.g. even to the GTask themselves), so that we

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

2017-05-30 Thread Dan Williams

On Wed, 2017-05-17 at 15:24 +0200, Aleksander Morgado wrote:
> The AT^SGPSS command provides an easy way to just start/stop GPS, but
> unfortunately it isn't supported by all Cinterion modems.
> 
> The AT^SGPSC command instead is more widely available but it requires
> several steps to start and stop the different elements of the GPS
> receiver.
> 
> Implement support for both, preferring AT^SGPSSS if available and
> falling back to AT^SGPSC otherwise.
> ---
>  plugins/cinterion/mm-common-cinterion.c | 209 
> +++-
>  1 file changed, 182 insertions(+), 27 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-common-cinterion.c 
> b/plugins/cinterion/mm-common-cinterion.c
> index ce337bd9..6e839c8d 100644
> --- a/plugins/cinterion/mm-common-cinterion.c
> +++ b/plugins/cinterion/mm-common-cinterion.c
> @@ -36,6 +36,7 @@ typedef enum {
>  typedef struct {
>  MMModemLocationSource enabled_sources;
>  FeatureSupportsgpss_support;
> +FeatureSupportsgpsc_support;
>  } LocationContext;
>  
>  static void
> @@ -59,6 +60,7 @@ get_location_context (MMBaseModem *self)
>  ctx = g_slice_new (LocationContext);
>  ctx->enabled_sources = MM_MODEM_LOCATION_SOURCE_NONE;
>  ctx->sgpss_support = FEATURE_SUPPORT_UNKNOWN;
> +ctx->sgpsc_support = FEATURE_SUPPORT_UNKNOWN;
>  
>  g_object_set_qdata_full (
>  G_OBJECT (self),
> @@ -131,6 +133,30 @@ mm_common_cinterion_location_load_capabilities_finish 
> (MMIfaceModemLocation  *se
>  static void probe_gps_features (GTask *task);
>  
>  static void
> +sgpsc_test_ready (MMBaseModem  *self,
> +  GAsyncResult *res,
> +  GTask*task)
> +{
> +LocationContext *location_ctx;
> +
> +location_ctx = get_location_context (self);
> +if (!mm_base_modem_at_command_finish (self, res, NULL))
> +location_ctx->sgpsc_support = FEATURE_NOT_SUPPORTED;
> +else {
> +/* ^SGPSC supported! */
> +location_ctx->sgpsc_support = FEATURE_SUPPORTED;
> +/* It may happen that the modem was started with GPS already 
> enabled, or
> + * maybe ModemManager got rebooted and it was left enabled before. 
> We'll
> + * make sure that it is disabled when we initialize the modem. */
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"Engine\",\"0\"",  3, FALSE, NULL, NULL);
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"Power/Antenna\",\"off\"", 3, FALSE, NULL, NULL);
> +mm_base_modem_at_command (MM_BASE_MODEM (self), 
> "AT^SGPSC=\"NMEA/Output\",\"off\"",   3, FALSE, NULL, NULL);
> +}
> +
> +probe_gps_features (task);
> +}
> +
> +static void
>  sgpss_test_ready (MMBaseModem  *self,
>    GAsyncResult *res,
>    GTask*task)
> @@ -143,6 +169,11 @@ sgpss_test_ready (MMBaseModem  *self,
>  else {
>  /* ^SGPSS supported! */
>  location_ctx->sgpss_support = FEATURE_SUPPORTED;
> +
> +/* Flag ^SGPSC as unsupported, even if it may be supported, so that 
> we
> + * only use one set of commands to enable/disable GPS. */
> +location_ctx->sgpsc_support = FEATURE_NOT_SUPPORTED;
> +
>  /* It may happen that the modem was started with GPS already 
> enabled, or
>   * maybe ModemManager got rebooted and it was left enabled before. 
> We'll
>   * make sure that it is disabled when we initialize the modem. */
> @@ -169,8 +200,14 @@ probe_gps_features (GTask *task)
>  return;
>  }
>  
> +/* Need to check if SGPSC supported... */
> +if (location_ctx->sgpsc_support == FEATURE_SUPPORT_UNKNOWN) {
> +mm_base_modem_at_command (self, "AT^SGPSC=?", 3, TRUE, 
> (GAsyncReadyCallback) sgpsc_test_ready, task);
> +return;
> +}
> +
>  /* All GPS features probed, check if GPS supported */
> -if (location_ctx->sgpss_support == FEATURE_SUPPORTED) {
> +if (location_ctx->sgpss_support == FEATURE_SUPPORTED || 
> location_ctx->sgpsc_support == FEATURE_SUPPORTED) {
>  mm_dbg ("GPS commands supported: GPS capabilities enabled");
>  ctx->sources |= (MM_MODEM_LOCATION_SOURCE_GPS_NMEA |
>   MM_MODEM_LOCATION_SOURCE_GPS_RAW  |
> @@ -244,6 +281,9 @@ mm_common_cinterion_location_load_capabilities 
> (MMIfaceModemLocation *self,
>  typedef enum {
>  DISABLE_LOCATION_GATHERING_GPS_STEP_FIRST,
>  DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSS,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ENGINE,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ANTENNA,
> +DISABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_OUTPUT,
>  DISABLE_LOCATION_GATHERING_GPS_STEP_LAST,
>  } DisableLocationGatheringGpsStep;
>  
> @@ -251,6 +291,7 @@ typedef struct {
>  MMModemLocationSourcesource;
>  DisableLocationGatheringGpsStep  gps_step;
>  GError  *sgpss_error;
> +GError

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

Re: [PATCH 4/5] cinterion: support AT^SGPSC capable modems

3 matches

Site Navigation

Mail list logo

Footer information