Re: [PATCH 5/5 v2] cinterion: retry GPS Engine activation up to 3 times

2017-05-30 Thread Aleksander Morgado 
On 20/05/17 09:57, Aleksander Morgado wrote:
> The default setup with 100ms between GPS commands doesn't seem to be
> always enough for the Engine activation step:
> 
>[1495016625.392972] (ttyACM1): --> 'AT^SGPSC="NMEA/Output","on"'
>[1495016625.503885] (ttyACM1): <-- '^SGPSC: 
> "Nmea/Output","on"OK'
>[1495016625.607650] (ttyACM1): --> 'AT^SGPSC="Power/Antenna","on"'
>[1495016625.697862] (ttyACM1): <-- '^SGPSC: 
> "Power/Antenna","on"OK'
>[1495016625.809393] (ttyACM1): --> 'AT^SGPSC="Engine","1"'
>[1495016625.895970] (ttyACM1): <-- '+CME ERROR: 767'
> 
> We now setup up to 3 retries for the Engine activation step before
> returning an error, and we also update to 2000ms the wait time before
> the Engine activation command is run.
> ---
> 
> Hey,
> 
> This update just changes the default wait time before issuing the Engine=1 
> command, instead of 500ms we now use 2000ms. I've done several tests and most 
> of the times the Engine is correctly activated after the first 2s wait time, 
> and some other times it requires one retry. Never seen it require more than 
> one retry, but I guess it doesn't harm if we leave the max 3 retry attempts.
> 

This has been merged to git master.

> ---
>  plugins/cinterion/mm-common-cinterion.c | 43 
> -
>  1 file changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-common-cinterion.c 
> b/plugins/cinterion/mm-common-cinterion.c
> index 6e839c8d..3f2b3994 100644
> --- a/plugins/cinterion/mm-common-cinterion.c
> +++ b/plugins/cinterion/mm-common-cinterion.c
> @@ -533,6 +533,15 @@ mm_common_cinterion_disable_location_gathering 
> (MMIfaceModemLocation  *self,
>  
> /*/
>  /* Enable location gathering (Location interface) */
> 
> +/* We will retry the SGPSC command that enables the Engine */
> +#define MAX_SGPSC_ENGINE_RETRIES 3
> +
> +/* Cinterion asks for 100ms some time between GPS commands, but we'll give up
> + * to 2000ms before setting the Engine configuration as 100ms didn't seem 
> always
> + * enough (we would get +CME ERROR: 767 errors reported). */
> +#define GPS_COMMAND_TIMEOUT_DEFAULT_MS  100
> +#define GPS_COMMAND_TIMEOUT_ENGINE_MS  2000
> +
>  typedef enum {
>  ENABLE_LOCATION_GATHERING_GPS_STEP_FIRST,
>  ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSS,
> @@ -545,6 +554,7 @@ typedef enum {
>  typedef struct {
>  MMModemLocationSource  source;
>  EnableLocationGatheringGpsStep gps_step;
> +guint  sgpsc_engine_retries;
>  } EnableLocationGatheringContext;
> 
>  static void
> @@ -564,16 +574,10 @@ mm_common_cinterion_enable_location_gathering_finish 
> (MMIfaceModemLocation  *sel
>  static void enable_location_gathering_context_gps_step (GTask *task);
> 
>  static gboolean
> -enable_location_gathering_context_gps_step_next_cb (GTask *task)
> +enable_location_gathering_context_gps_step_schedule_cb (GTask *task)
>  {
> -EnableLocationGatheringContext *ctx;
> -
> -ctx = (EnableLocationGatheringContext *) g_task_get_task_data (task);
> -
> -/* We jump to the next step */
> -ctx->gps_step++;
> +/* Run the scheduled step */
>  enable_location_gathering_context_gps_step (task);
> -
>  return G_SOURCE_REMOVE;
>  }
> 
> @@ -582,16 +586,33 @@ enable_sgpsc_or_sgpss_ready (MMBaseModem  *self,
>   GAsyncResult *res,
>   GTask*task)
>  {
> -GError *error = NULL;
> +EnableLocationGatheringContext *ctx;
> +GError *error = NULL;
> +
> +ctx = (EnableLocationGatheringContext *) g_task_get_task_data (task);
> 
>  if (!mm_base_modem_at_command_finish (self, res, )) {
> +/* The GPS setup may sometimes report "+CME ERROR 767" when enabling 
> the
> + * Engine; so we'll run some retries of the same command ourselves. 
> */
> +if (ctx->gps_step == 
> ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ENGINE) {
> +ctx->sgpsc_engine_retries++;
> +mm_dbg ("GPS Engine setup failed (%u/%u)", 
> ctx->sgpsc_engine_retries, MAX_SGPSC_ENGINE_RETRIES);
> +if (ctx->sgpsc_engine_retries < MAX_SGPSC_ENGINE_RETRIES) {
> +g_clear_error ();
> +goto schedule;
> +}
> +}
>  g_task_return_error (task, error);
>  g_object_unref (task);
>  return;
>  }
> 
> -/* Cinterion asks for 100ms between GPS commands... */
> -g_timeout_add (100, (GSourceFunc) 
> enable_location_gathering_context_gps_step_next_cb, task);
> +/* Go on to next step */
> +ctx->gps_step++;
> +
> +schedule:
> +g_timeout_add (ctx->gps_step == 
> ENABLE_LOCATION_GATHERING_GPS_STEP_SGPSC_ENGINE ? 
> GPS_COMMAND_TIMEOUT_ENGINE_MS : GPS_COMMAND_TIMEOUT_DEFAULT_MS,
> +   (GSourceFunc) 
> enable_location_gathering_context_gps_step_schedule_cb, task);
>

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 2/5] cinterion: flag PLS8 GPS data port

2017-05-30 Thread Aleksander Morgado 
On 17/05/17 15:24, Aleksander Morgado wrote:
> ---
>  plugins/cinterion/77-mm-cinterion-port-types.rules | 4 
>  1 file changed, 4 insertions(+)
> 

This has been merged to git master.

> diff --git a/plugins/cinterion/77-mm-cinterion-port-types.rules 
> b/plugins/cinterion/77-mm-cinterion-port-types.rules
> index f43f0b8e..7c63a1aa 100644
> --- a/plugins/cinterion/77-mm-cinterion-port-types.rules
> +++ b/plugins/cinterion/77-mm-cinterion-port-types.rules
> @@ -7,6 +7,10 @@ GOTO="mm_cinterion_port_types_end"
>  LABEL="mm_cinterion_port_types"
>  SUBSYSTEMS=="usb", ATTRS{bInterfaceNumber}=="?*", 
> ENV{.MM_USBIFNUM}="$attr{bInterfaceNumber}"
>  
> +# PHS8
>  ATTRS{idVendor}=="1e2d", ATTRS{idProduct}=="0053", ENV{.MM_USBIFNUM}=="01", 
> ENV{ID_MM_CINTERION_PORT_TYPE_GPS}="1"
>  
> +# PLS8
> +ATTRS{idVendor}=="1e2d", ATTRS{idProduct}=="0061", ENV{.MM_USBIFNUM}=="04", 
> ENV{ID_MM_CINTERION_PORT_TYPE_GPS}="1"
> +
>  LABEL="mm_cinterion_port_types_end"
> 


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 3/5] cinterion: setup GPS port only if GPS support detected

2017-05-30 Thread Aleksander Morgado 
On 17/05/17 15:24, Aleksander Morgado wrote:
> i.e. if AT^SGPSS not supported, we don't even add traces handler to
> the GPS data port.
> ---

This has been merged to git master.

>  plugins/cinterion/mm-broadband-modem-cinterion.c   |  14 ---
>  .../cinterion/mm-broadband-modem-qmi-cinterion.c   |  16 
>  plugins/cinterion/mm-common-cinterion.c| 103 
> ++---
>  plugins/cinterion/mm-common-cinterion.h|   2 -
>  4 files changed, 47 insertions(+), 88 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c 
> b/plugins/cinterion/mm-broadband-modem-cinterion.c
> index 4a4e18bf..fc9a3356 100644
> --- a/plugins/cinterion/mm-broadband-modem-cinterion.c
> +++ b/plugins/cinterion/mm-broadband-modem-cinterion.c
> @@ -1586,18 +1586,6 @@ after_sim_unlock (MMIfaceModem *self,
>  }
>  
>  
> /*/
> -/* Setup ports (Broadband modem class) */
> -
> -static void
> -setup_ports (MMBroadbandModem *self)
> -{
> -/* Call parent's setup ports first always */
> -MM_BROADBAND_MODEM_CLASS 
> (mm_broadband_modem_cinterion_parent_class)->setup_ports (self);
> -
> -mm_common_cinterion_setup_gps_port (self);
> -}
> -
> -/*/
>  /* Create Bearer (Modem interface) */
>  
>  typedef struct {
> @@ -1868,11 +1856,9 @@ static void
>  mm_broadband_modem_cinterion_class_init (MMBroadbandModemCinterionClass 
> *klass)
>  {
>  GObjectClass *object_class = G_OBJECT_CLASS (klass);
> -MMBroadbandModemClass *broadband_modem_class = MM_BROADBAND_MODEM_CLASS 
> (klass);
>  
>  g_type_class_add_private (object_class, sizeof 
> (MMBroadbandModemCinterionPrivate));
>  
>  /* Virtual methods */
>  object_class->finalize = finalize;
> -broadband_modem_class->setup_ports = setup_ports;
>  }
> diff --git a/plugins/cinterion/mm-broadband-modem-qmi-cinterion.c 
> b/plugins/cinterion/mm-broadband-modem-qmi-cinterion.c
> index 05f92e4f..2410d091 100644
> --- a/plugins/cinterion/mm-broadband-modem-qmi-cinterion.c
> +++ b/plugins/cinterion/mm-broadband-modem-qmi-cinterion.c
> @@ -35,18 +35,6 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemQmiCinterion, 
> mm_broadband_modem_qmi_cin
>  G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_LOCATION, 
> iface_modem_location_init))
>  
>  
> /*/
> -/* Setup ports (Broadband modem class) */
> -
> -static void
> -setup_ports (MMBroadbandModem *self)
> -{
> -/* Call parent's setup ports first always */
> -MM_BROADBAND_MODEM_CLASS 
> (mm_broadband_modem_qmi_cinterion_parent_class)->setup_ports (self);
> -
> -mm_common_cinterion_setup_gps_port (self);
> -}
> -
> -/*/
>  
>  MMBroadbandModemQmiCinterion *
>  mm_broadband_modem_qmi_cinterion_new (const gchar *device,
> @@ -85,8 +73,4 @@ iface_modem_location_init (MMIfaceModemLocation *iface)
>  static void
>  mm_broadband_modem_qmi_cinterion_class_init 
> (MMBroadbandModemQmiCinterionClass *klass)
>  {
> -MMBroadbandModemClass *broadband_modem_class = MM_BROADBAND_MODEM_CLASS 
> (klass);
> -
> -/* Virtual methods */
> -broadband_modem_class->setup_ports = setup_ports;
>  }
> diff --git a/plugins/cinterion/mm-common-cinterion.c 
> b/plugins/cinterion/mm-common-cinterion.c
> index 91af530f..ce337bd9 100644
> --- a/plugins/cinterion/mm-common-cinterion.c
> +++ b/plugins/cinterion/mm-common-cinterion.c
> @@ -16,6 +16,7 @@
>  
>  #include "mm-common-cinterion.h"
>  #include "mm-base-modem-at.h"
> +#include "mm-log.h"
>  
>  static MMIfaceModemLocation *iface_modem_location_parent;
>  
> @@ -70,6 +71,38 @@ get_location_context (MMBaseModem *self)
>  }
>  
>  
> /*/
> +/* GPS trace received */
> +
> +static void
> +trace_received (MMPortSerialGps *port,
> +const gchar *trace,
> +MMIfaceModemLocation *self)
> +{
> +/* Helper to debug GPS location related issues. Don't depend on a real 
> GPS
> + * fix for debugging, just use some random values to update */
> +#if 0
> +if (g_str_has_prefix (trace, "$GPGGA")) {
> +GString *str;
> +GDateTime *now;
> +
> +now = g_date_time_new_now_utc ();
> +str = g_string_new ("");
> +g_string_append_printf (str,
> +
> "$GPGGA,%02u%02u%02u,4807.038,N,01131.000,E,1,08,0.9,545.4,M,46.9,M,,*47",
> +g_date_time_get_hour (now),
> +g_date_time_get_minute (now),
> +g_date_time_get_second (now));
> +mm_iface_modem_location_gps_update (self, str->str);
> +g_string_free (str, TRUE);
> +g_date_time_unref (now);
> +

Re: [PATCH 1/5] cinterion,location: refactor enable/disable and capabilities checks

2017-05-30 Thread Aleksander Morgado 
On 17/05/17 15:24, Aleksander Morgado wrote:
> When checking for location capabilities, we will make sure AT^SGPSS is
> supported and if it isn't we won't report GPS capabilities.
> 
> The location enable and disable paths are refactored to make it easier
> to add possible new GPS commands to use instead of AT^SGPSS, if this
> isn't supported (e.g. in the PLS8 devices).

This has been merged to git master.

> ---
>  plugins/cinterion/mm-common-cinterion.c | 534 
> +---
>  1 file changed, 350 insertions(+), 184 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-common-cinterion.c 
> b/plugins/cinterion/mm-common-cinterion.c
> index 6e4da70c..91af530f 100644
> --- a/plugins/cinterion/mm-common-cinterion.c
> +++ b/plugins/cinterion/mm-common-cinterion.c
> @@ -26,8 +26,15 @@ static GQuark cinterion_location_context_quark;
>  
>  
> /*/
>  
> +typedef enum {
> +FEATURE_SUPPORT_UNKNOWN,
> +FEATURE_NOT_SUPPORTED,
> +FEATURE_SUPPORTED,
> +} FeatureSupport;
> +
>  typedef struct {
>  MMModemLocationSource enabled_sources;
> +FeatureSupportsgpss_support;
>  } LocationContext;
>  
>  static void
> @@ -50,6 +57,7 @@ get_location_context (MMBaseModem *self)
>  /* Create context and keep it as object data */
>  ctx = g_slice_new (LocationContext);
>  ctx->enabled_sources = MM_MODEM_LOCATION_SOURCE_NONE;
> +ctx->sgpss_support = FEATURE_SUPPORT_UNKNOWN;
>  
>  g_object_set_qdata_full (
>  G_OBJECT (self),
> @@ -61,133 +69,234 @@ get_location_context (MMBaseModem *self)
>  return ctx;
>  }
>  
> -
>  
> /*/
>  /* Location capabilities loading (Location interface) */
>  
> +typedef struct {
> +MMModemLocationSource sources;
> +} LoadCapabilitiesContext;
> +
> +static void
> +load_capabilities_context_free (LoadCapabilitiesContext *ctx)
> +{
> +g_slice_free (LoadCapabilitiesContext, ctx);
> +}
> +
>  MMModemLocationSource
> -mm_common_cinterion_location_load_capabilities_finish (MMIfaceModemLocation 
> *self,
> -   GAsyncResult *res,
> -   GError **error)
> +mm_common_cinterion_location_load_capabilities_finish (MMIfaceModemLocation  
> *self,
> +   GAsyncResult  
> *res,
> +   GError   
> **error)
>  {
> -if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), 
> error))
> +gssize aux;
> +
> +if ((aux = g_task_propagate_int (G_TASK (res), error)) < 0)
>  return MM_MODEM_LOCATION_SOURCE_NONE;
>  
> -return (MMModemLocationSource) GPOINTER_TO_UINT 
> (g_simple_async_result_get_op_res_gpointer (
> - 
> G_SIMPLE_ASYNC_RESULT (res)));
> +return (MMModemLocationSource) aux;
> +}
> +
> +static void probe_gps_features (GTask *task);
> +
> +static void
> +sgpss_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->sgpss_support = FEATURE_NOT_SUPPORTED;
> +else
> +location_ctx->sgpss_support = FEATURE_SUPPORTED;
> +probe_gps_features (task);
> +}
> +
> +static void
> +probe_gps_features (GTask *task)
> +{
> +LoadCapabilitiesContext *ctx;
> +MMBaseModem *self;
> +LocationContext *location_ctx;
> +
> +ctx = (LoadCapabilitiesContext *) g_task_get_task_data (task);
> +self = MM_BASE_MODEM (g_task_get_source_object (task));
> +location_ctx = get_location_context (self);
> +
> +/* Need to check if SGPSS supported... */
> +if (location_ctx->sgpss_support == FEATURE_SUPPORT_UNKNOWN) {
> +mm_base_modem_at_command (self, "AT^SGPSS=?", 3, TRUE, 
> (GAsyncReadyCallback) sgpss_test_ready, task);
> +return;
> +}
> +
> +/* All GPS features probed, check if GPS supported */
> +if (location_ctx->sgpss_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  |
> + MM_MODEM_LOCATION_SOURCE_GPS_UNMANAGED);
> +} else
> +mm_dbg ("No GPS command supported: no GPS capabilities");
> +
> +g_task_return_int (task, (gssize) ctx->sources);
> +g_object_unref (task);
>  }
>  
>  static void
>  parent_load_capabilities_ready (MMIfaceModemLocation *self,
> -GAsyncResult *res,
> -

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: Enabling GPS support for the Cinterion PLS8 devices

2017-05-30 Thread Dan Williams 
On Wed, 2017-05-17 at 15:23 +0200, Aleksander Morgado wrote:
> Hey Dan, Matthew & everyone,
> 
> The following patches implement support for GPS capabilities in the
> Cinterion PLS8-X, PLS8-E and similar devices, plus some additional
> improvements in how GPS support is handled.
> 
> The logic is based on Matthew's original logic included in the patch
> he submitted here:
> https://lists.freedesktop.org/archives/modemmanager-devel/2016-August
> /003381.html
> 
> Comments?

Other than the GCancellable thing for the enable idle callback, looks
good to me.

Dan
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

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 

[PATCH v2 4/4] cinterion: use ^SIND unsolicited messages for access tech reporting

2017-05-30 Thread Aleksander Morgado 
If the modem supports ^SIND psinfo reporting, we enable the URC and
flag the access technology polling unsupported, so that we only update
access technology via the +CIEV URCs.

E.g.:

(ttyACM1): --> 'AT^SIND="psinfo",1'
(ttyACM1): <-- '^SIND: psinfo,1,10OK'
Reporting initial access technologies...
Modem /org/freedesktop/ModemManager1/Modem/0: access technology changed 
(unknown -> hsdpa, hsupa)
...
(ttyACM1): <-- '+CIEV: psinfo,4'
Modem /org/freedesktop/ModemManager1/Modem/0: access technology changed 
(hsdpa, hsupa -> edge)
...
---
 plugins/cinterion/mm-broadband-modem-cinterion.c | 470 ---
 plugins/cinterion/mm-modem-helpers-cinterion.c   |  33 ++
 plugins/cinterion/mm-modem-helpers-cinterion.h   |   5 +
 3 files changed, 366 insertions(+), 142 deletions(-)

diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c 
b/plugins/cinterion/mm-broadband-modem-cinterion.c
index 5cdb0107..befd0041 100644
--- a/plugins/cinterion/mm-broadband-modem-cinterion.c
+++ b/plugins/cinterion/mm-broadband-modem-cinterion.c
@@ -61,9 +61,6 @@ typedef enum {
 } FeatureSupport;

 struct _MMBroadbandModemCinterionPrivate {
-/* Flag to know if we should try AT^SIND or not to get psinfo */
-gboolean sind_psinfo;
-
 /* Command to go into sleep mode */
 gchar *sleep_mode_cmd;

@@ -80,8 +77,12 @@ struct _MMBroadbandModemCinterionPrivate {
 GArray *cnmi_supported_ds;
 GArray *cnmi_supported_bfr;

-/*Flags for SWWAN support*/
+/* +CIEV 'psinfo' indications */
+GRegex *ciev_psinfo_regex;
+
+/* Flags for feature support checks */
 FeatureSupport swwan_support;
+FeatureSupport sind_psinfo_support;
 };

 /*/
@@ -572,185 +573,354 @@ modem_power_off (MMIfaceModem *self,
 }

 /*/
-/* ACCESS TECHNOLOGIES */
+/* Access technologies polling */

 static gboolean
-load_access_technologies_finish (MMIfaceModem *self,
- GAsyncResult *res,
- MMModemAccessTechnology *access_technologies,
- guint *mask,
- GError **error)
+load_access_technologies_finish (MMIfaceModem *self,
+ GAsyncResult *res,
+ MMModemAccessTechnology  *access_technologies,
+ guint*mask,
+ GError  **error)
 {
-if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), 
error))
+gssize val;
+
+if ((val = g_task_propagate_int (G_TASK (res), error)) < 0)
 return FALSE;

-*access_technologies = (MMModemAccessTechnology) GPOINTER_TO_UINT (
-g_simple_async_result_get_op_res_gpointer (
-G_SIMPLE_ASYNC_RESULT (res)));
+*access_technologies = (MMModemAccessTechnology) val;
 *mask = MM_MODEM_ACCESS_TECHNOLOGY_ANY;
 return TRUE;
 }

 static void
-smong_query_ready (MMBroadbandModemCinterion *self,
+smong_query_ready (MMBaseModem  *self,
GAsyncResult *res,
-   GSimpleAsyncResult *operation_result)
+   GTask*task)
 {
 const gchar *response;
 GError  *error = NULL;
 MMModemAccessTechnology  access_tech;

-response = mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, 
);
-if (!response) {
-/* Let the error be critical. */
-g_simple_async_result_take_error (operation_result, error);
-} else if (!mm_cinterion_parse_smong_response (response, _tech, 
)) {
-/* We'll reset here the flag to try to use SIND/psinfo the next time */
-self->priv->sind_psinfo = TRUE;
-g_simple_async_result_take_error (operation_result, error);
-} else {
-/* We'll default to use SMONG then */
-self->priv->sind_psinfo = FALSE;
-g_simple_async_result_set_op_res_gpointer (operation_result, 
GUINT_TO_POINTER (access_tech), NULL);
+response = mm_base_modem_at_command_finish (self, res, );
+if (!response || !mm_cinterion_parse_smong_response (response, 
_tech, ))
+g_task_return_error (task, error);
+else
+g_task_return_int (task, (gssize) access_tech);
+g_object_unref (task);
+}
+
+static void
+load_access_technologies (MMIfaceModem*_self,
+  GAsyncReadyCallback  callback,
+  gpointer user_data)
+{
+MMBroadbandModemCinterion *self = MM_BROADBAND_MODEM_CINTERION (_self);
+GTask *task;
+
+task = g_task_new (self, NULL, callback, user_data);
+
+/* Abort access technology polling if ^SIND psinfo URCs are enabled */
+if (self->priv->sind_psinfo_support == FEATURE_SUPPORTED) {
+

[PATCH v2 2/4] iface-modem: if bands, capabilities or modes change, refresh signal

2017-05-30 Thread Aleksander Morgado 
We also remove the explicit refresh request from the Cinterion plugin,
as this is a generic action applicable to all modems that require
polling for signal quality and/or access technology.
---
 plugins/cinterion/mm-broadband-modem-cinterion.c | 10 ++
 src/mm-iface-modem.c | 16 +---
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c 
b/plugins/cinterion/mm-broadband-modem-cinterion.c
index d829fe15..5cdb0107 100644
--- a/plugins/cinterion/mm-broadband-modem-cinterion.c
+++ b/plugins/cinterion/mm-broadband-modem-cinterion.c
@@ -852,11 +852,8 @@ allowed_access_technology_update_ready 
(MMBroadbandModemCinterion *self,
 mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, );
 if (error)
 g_task_return_error (task, error);
-else {
-/* Request immediate signal update */
-mm_iface_modem_refresh_signal (MM_IFACE_MODEM (self));
+else
 g_task_return_boolean (task, TRUE);
-}
 g_object_unref (task);
 }

@@ -1139,11 +1136,8 @@ scfg_set_ready (MMBaseModem *self,
 if (!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, ))
 /* Let the error be critical */
 g_simple_async_result_take_error (operation_result, error);
-else {
-/* Request immediate signal update */
-mm_iface_modem_refresh_signal (MM_IFACE_MODEM (self));
+else
 g_simple_async_result_set_op_res_gboolean (operation_result, TRUE);
-}

 g_simple_async_result_complete (operation_result);
 g_object_unref (operation_result);
diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
index 223f2785..7f040d1d 100644
--- a/src/mm-iface-modem.c
+++ b/src/mm-iface-modem.c
@@ -2058,8 +2058,12 @@ set_current_capabilities_ready (MMIfaceModem *self,

 if (!MM_IFACE_MODEM_GET_INTERFACE (self)->set_current_capabilities_finish 
(self, res, ))
 g_dbus_method_invocation_take_error (ctx->invocation, error);
-else
+else {
+/* Capabilities updated: explicitly refresh signal and access 
technology */
+mm_iface_modem_refresh_signal (self);
 mm_gdbus_modem_complete_set_current_capabilities (ctx->skeleton, 
ctx->invocation);
+}
+
 handle_set_current_capabilities_context_free (ctx);
 }

@@ -2436,8 +2440,11 @@ handle_set_current_bands_ready (MMIfaceModem *self,

 if (!mm_iface_modem_set_current_bands_finish (self, res, ))
 g_dbus_method_invocation_take_error (ctx->invocation, error);
-else
+else {
+/* Bands updated: explicitly refresh signal and access technology */
+mm_iface_modem_refresh_signal (self);
 mm_gdbus_modem_complete_set_current_bands (ctx->skeleton, 
ctx->invocation);
+}

 handle_set_current_bands_context_free (ctx);
 }
@@ -2752,8 +2759,11 @@ handle_set_current_modes_ready (MMIfaceModem *self,

 if (!mm_iface_modem_set_current_modes_finish (self, res, ))
 g_dbus_method_invocation_take_error (ctx->invocation, error);
-else
+else {
+/* Modes updated: explicitly refresh signal and access technology */
+mm_iface_modem_refresh_signal (self);
 mm_gdbus_modem_complete_set_current_modes (ctx->skeleton, 
ctx->invocation);
+}

 handle_set_current_modes_context_free (ctx);
 }
--
2.13.0
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Allow plugins to specify that signal quality/access tech polling isn't needed (v2)

2017-05-30 Thread Aleksander Morgado 
Hey Dan and everyone,

This patch series contains the following updates w.r.t. the v1 patches:

 * When a explicit signal refresh is requested, setup the initial fast-polling 
configuration (this was suggested in the review of PATCH 2/4, but the fix has 
been applied in PATCH v2 1/4).
 * In the Cinterion plugin, Use G_N_ELEMENTS() when iterating ports array 
setting up unsolicited message handlers.
 * In the Cinterion plugin, check support for SIND/psinfo and if it isn't 
supported, don't bother enabling or disabling those unsolicited messages any 
more.

[PATCH v2 1/4] iface-modem: consolidate signal quality and access tech
[PATCH v2 2/4] iface-modem: if bands, capabilities or modes change,
[PATCH v2 3/4] port-serial-at: always prepend unsolicited message
[PATCH v2 4/4] cinterion: use ^SIND unsolicited messages for access tech

Comments?

--
Aleksander
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

[PATCH v2 1/4] iface-modem: consolidate signal quality and access tech polling

2017-05-30 Thread Aleksander Morgado 
Plugins have two ways to update signal quality and access technology
values: via unsolicited messages or via polling periodically.

Instead of keeping separate contexts for polling signal quality and
access technology values, we setup a common timeout to trigger
both. This allows us to simplify in which case the explicit update is
required, whenever one is needed to be explicitly updated, the other
one should also be.

The logic now also allows plugins to return an UNSUPPORTED error in
either load_signal_quality() and/or load_access_technologies() to tell
the interface logic that the polling of the specific item shouldn't be
performed (e.g. if the updates are expected via unsolicited messages).

If both signal quality and access technology polling is flagged as
disabled, we totally disable the polling logic internally.

The new SignalCheckContext is bound to the lifetime of the object so
that we can keep the value of the supported flags until the object is
destroyed.
---
 plugins/cinterion/mm-broadband-modem-cinterion.c |   8 +-
 src/mm-iface-modem-3gpp.c|   7 +-
 src/mm-iface-modem.c | 589 ---
 src/mm-iface-modem.h |   6 +-
 4 files changed, 312 insertions(+), 298 deletions(-)

diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c 
b/plugins/cinterion/mm-broadband-modem-cinterion.c
index 186bf80b..d829fe15 100644
--- a/plugins/cinterion/mm-broadband-modem-cinterion.c
+++ b/plugins/cinterion/mm-broadband-modem-cinterion.c
@@ -853,8 +853,8 @@ allowed_access_technology_update_ready 
(MMBroadbandModemCinterion *self,
 if (error)
 g_task_return_error (task, error);
 else {
-/* Request immediate access tech update */
-mm_iface_modem_refresh_access_technologies (MM_IFACE_MODEM (self));
+/* Request immediate signal update */
+mm_iface_modem_refresh_signal (MM_IFACE_MODEM (self));
 g_task_return_boolean (task, TRUE);
 }
 g_object_unref (task);
@@ -1140,8 +1140,8 @@ scfg_set_ready (MMBaseModem *self,
 /* Let the error be critical */
 g_simple_async_result_take_error (operation_result, error);
 else {
-/* Request immediate access tech update */
-mm_iface_modem_refresh_access_technologies (MM_IFACE_MODEM (self));
+/* Request immediate signal update */
+mm_iface_modem_refresh_signal (MM_IFACE_MODEM (self));
 g_simple_async_result_set_op_res_gboolean (operation_result, TRUE);
 }

diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
index 8b93550a..bcd12e85 100644
--- a/src/mm-iface-modem-3gpp.c
+++ b/src/mm-iface-modem-3gpp.c
@@ -307,8 +307,11 @@ run_registration_checks_ready (MMIfaceModem3gpp *self,
 current_registration_state == 
MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING_SMS_ONLY ||
 current_registration_state == 
MM_MODEM_3GPP_REGISTRATION_STATE_HOME_CSFB_NOT_PREFERRED ||
 current_registration_state == 
MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING_CSFB_NOT_PREFERRED) {
-/* Request immediate access tech update */
-mm_iface_modem_refresh_access_technologies (MM_IFACE_MODEM 
(ctx->self));
+/* Request immediate access tech and signal update: we may have changed
+ * from home to roaming or viceversa, both registered states, so there
+ * wouldn't be an explicit refresh triggered from the modem interface 
as
+ * the modem never got un-registered during the sequence. */
+mm_iface_modem_refresh_signal (MM_IFACE_MODEM (ctx->self));
 mm_dbg ("Modem is currently registered in a 3GPP network");
 g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
 register_in_network_context_complete_and_free (ctx);
diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
index ecb351a0..223f2785 100644
--- a/src/mm-iface-modem.c
+++ b/src/mm-iface-modem.c
@@ -29,21 +29,20 @@
 #include "mm-log.h"
 #include "mm-context.h"

-#define SIGNAL_QUALITY_RECENT_TIMEOUT_SEC60
-#define SIGNAL_QUALITY_INITIAL_CHECK_TIMEOUT_SEC 3
-#define SIGNAL_QUALITY_CHECK_TIMEOUT_SEC 30
-#define ACCESS_TECHNOLOGIES_CHECK_TIMEOUT_SEC30
+#define SIGNAL_QUALITY_RECENT_TIMEOUT_SEC 60

-#define STATE_UPDATE_CONTEXT_TAG  "state-update-context-tag"
-#define SIGNAL_QUALITY_UPDATE_CONTEXT_TAG 
"signal-quality-update-context-tag"
-#define SIGNAL_QUALITY_CHECK_CONTEXT_TAG  
"signal-quality-check-context-tag"
-#define ACCESS_TECHNOLOGIES_CHECK_CONTEXT_TAG 
"access-technologies-check-context-tag"
-#define RESTART_INITIALIZE_IDLE_TAG   "restart-initialize-tag"
+#define SIGNAL_CHECK_INITIAL_RETRIES  5
+#define SIGNAL_CHECK_INITIAL_TIMEOUT_SEC  3
+#define SIGNAL_CHECK_TIMEOUT_SEC  30
+
+#define STATE_UPDATE_CONTEXT_TAG  "state-update-context-tag"
+#define SIGNAL_QUALITY_UPDATE_CONTEXT_TAG "signal-quality-update-context-tag"
+#define SIGNAL_CHECK_CONTEXT_TAG

[PATCH v2 3/4] port-serial-at: always prepend unsolicited message handlers

2017-05-30 Thread Aleksander Morgado 
The generic modem has several URC handlers for generic messages, like
+CIEV. But, there may be cases where the plugins themselves want to
provide their own URC handlers to be run before the generic one.

This patch enables this setup by always prepending the new URC
handlers, so that the last ones added are processed before the ones
already in the list. So if a plugin does its own unsolicited
messages setup, they can run first the parent setup and then their
own.

The only thing to consider is that the regex provided by the plugin
must be specific enough not to match the specific messages required
by the parent implementation.
---
 src/mm-port-serial-at.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/mm-port-serial-at.c b/src/mm-port-serial-at.c
index 8e2aa2aa..fb2174da 100644
--- a/src/mm-port-serial-at.c
+++ b/src/mm-port-serial-at.c
@@ -207,9 +207,12 @@ mm_port_serial_at_add_unsolicited_msg_handler 
(MMPortSerialAt *self,
 if (handler->notify)
 handler->notify (handler->user_data);
 } else {
+/* The new handler is always PREPENDED, so that e.g. plugins can 
provide
+ * more specific matches for URCs that are also handled by the generic
+ * plugin. */
 handler = g_slice_new (MMAtUnsolicitedMsgHandler);
-self->priv->unsolicited_msg_handlers = g_slist_append 
(self->priv->unsolicited_msg_handlers, handler);
 handler->regex = g_regex_ref (regex);
+self->priv->unsolicited_msg_handlers = g_slist_prepend 
(self->priv->unsolicited_msg_handlers, handler);
 }

 handler->callback = callback;
--
2.13.0
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 4/4] cinterion: use ^SIND unsolicited messages for access tech reporting

2017-05-30 Thread Aleksander Morgado 
On Mon, May 22, 2017 at 7:15 PM, Aleksander Morgado
 wrote:
>>> +static void
>>> +set_unsolicited_events_handlers (MMBroadbandModemCinterion *self,
>>> + gboolean   enable)
>>> +{
>>> +MMPortSerialAt *ports[2];
>>> +guint   i;
>>> +
>>> +ports[0] = mm_base_modem_peek_port_primary   (MM_BASE_MODEM (self));
>>> +ports[1] = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
>>> +
>>> +/* Enable unsolicited events in given port */
>>> +for (i = 0; i < 2; i++) {
>>
>> G_N_ELEMENTS()?
>>
>
> Ah, yeah, we could do that. This was copy pasted from some other
> plugin, so if you don't mind I'll push a separate patch fixing the
> same thing in several places.

Pushed already a patch to git master changing this in all plugins that
were not using G_N_ELEMENTS.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 2/2] ublox: use send-delay=0 by default

2017-05-30 Thread Aleksander Morgado 
On Tue, May 23, 2017 at 6:39 PM, Aleksander Morgado
 wrote:
> Looks like the u-blox SARA-U260 requires this, see:
> https://lists.freedesktop.org/archives/modemmanager-devel/2017-May/004670.html
> ---

Pushed to git master.


>  plugins/ublox/mm-broadband-modem-ublox.c | 31 ++-
>  plugins/ublox/mm-plugin-ublox.c  |  1 +
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/ublox/mm-broadband-modem-ublox.c 
> b/plugins/ublox/mm-broadband-modem-ublox.c
> index 64dd9dec..1ccb4be8 100644
> --- a/plugins/ublox/mm-broadband-modem-ublox.c
> +++ b/plugins/ublox/mm-broadband-modem-ublox.c
> @@ -899,6 +899,32 @@ modem_create_bearer (MMIfaceModem*self,
>  }
>
>  
> /*/
> +/* Setup ports (Broadband modem class) */
> +
> +static void
> +setup_ports (MMBroadbandModem *self)
> +{
> +MMPortSerialAt *ports[2];
> +guint   i;
> +
> +/* Call parent's setup ports first always */
> +MM_BROADBAND_MODEM_CLASS 
> (mm_broadband_modem_ublox_parent_class)->setup_ports (self);
> +
> +ports[0] = mm_base_modem_peek_port_primary   (MM_BASE_MODEM (self));
> +ports[1] = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
> +
> +/* Configure AT ports */
> +for (i = 0; i < G_N_ELEMENTS (ports); i++) {
> +if (!ports[i])
> +continue;
> +
> +g_object_set (ports[i],
> +  MM_PORT_SERIAL_SEND_DELAY, (guint64) 0,
> +  NULL);
> +}
> +}
> +
> +/*/
>
>  MMBroadbandModemUblox *
>  mm_broadband_modem_ublox_new (const gchar  *device,
> @@ -962,7 +988,10 @@ iface_modem_init (MMIfaceModem *iface)
>  static void
>  mm_broadband_modem_ublox_class_init (MMBroadbandModemUbloxClass *klass)
>  {
> -GObjectClass *object_class = G_OBJECT_CLASS (klass);
> +GObjectClass  *object_class = G_OBJECT_CLASS (klass);
> +MMBroadbandModemClass *broadband_modem_class = MM_BROADBAND_MODEM_CLASS 
> (klass);
>
>  g_type_class_add_private (object_class, sizeof 
> (MMBroadbandModemUbloxPrivate));
> +
> +broadband_modem_class->setup_ports = setup_ports;
>  }
> diff --git a/plugins/ublox/mm-plugin-ublox.c b/plugins/ublox/mm-plugin-ublox.c
> index a1592d51..d4a29236 100644
> --- a/plugins/ublox/mm-plugin-ublox.c
> +++ b/plugins/ublox/mm-plugin-ublox.c
> @@ -75,6 +75,7 @@ mm_plugin_create (void)
>  MM_PLUGIN_ALLOWED_VENDOR_STRINGS, 
> vendor_strings,
>  MM_PLUGIN_ALLOWED_AT, TRUE,
>  MM_PLUGIN_CUSTOM_AT_PROBE,
> custom_at_probe,
> +MM_PLUGIN_SEND_DELAY, 0,
>  NULL));
>  }
>
> --
> 2.12.2
>



-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 1/2] altair,hso,samsung: setup send-delay=0 for probing

2017-05-30 Thread Aleksander Morgado 
On Tue, May 23, 2017 at 6:39 PM, Aleksander Morgado
 wrote:
> The Altair, Option HSO and Samsung plugins all use the send-delay=0
> setting once the modem object has been created. For consistency, use
> the same setting during port probing.

Pushed to git master.

> ---
>  plugins/altair/mm-plugin-altair-lte.c | 1 +
>  plugins/option/mm-plugin-hso.c| 1 +
>  plugins/samsung/mm-plugin-samsung.c   | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/plugins/altair/mm-plugin-altair-lte.c 
> b/plugins/altair/mm-plugin-altair-lte.c
> index 7f6a7c0b..2338502b 100644
> --- a/plugins/altair/mm-plugin-altair-lte.c
> +++ b/plugins/altair/mm-plugin-altair-lte.c
> @@ -83,6 +83,7 @@ mm_plugin_create (void)
>MM_PLUGIN_CUSTOM_AT_PROBE, custom_at_probe,
>MM_PLUGIN_ALLOWED_SINGLE_AT,   TRUE,
>MM_PLUGIN_SEND_LF, TRUE,
> +  MM_PLUGIN_SEND_DELAY,  0,
>NULL));
>  }
>
> diff --git a/plugins/option/mm-plugin-hso.c b/plugins/option/mm-plugin-hso.c
> index 3633cfd4..de42c457 100644
> --- a/plugins/option/mm-plugin-hso.c
> +++ b/plugins/option/mm-plugin-hso.c
> @@ -186,6 +186,7 @@ mm_plugin_create (void)
>MM_PLUGIN_ALLOWED_AT, TRUE,
>MM_PLUGIN_ALLOWED_QCDM,   TRUE,
>MM_PLUGIN_CUSTOM_INIT,_init,
> +  MM_PLUGIN_SEND_DELAY, 0,
>NULL));
>  }
>
> diff --git a/plugins/samsung/mm-plugin-samsung.c 
> b/plugins/samsung/mm-plugin-samsung.c
> index 157d286c..9e6aaf94 100644
> --- a/plugins/samsung/mm-plugin-samsung.c
> +++ b/plugins/samsung/mm-plugin-samsung.c
> @@ -65,6 +65,7 @@ mm_plugin_create (void)
>MM_PLUGIN_ALLOWED_SUBSYSTEMS,  subsystems,
>MM_PLUGIN_ALLOWED_PRODUCT_IDS, products,
>MM_PLUGIN_ALLOWED_AT,  TRUE,
> +  MM_PLUGIN_SEND_DELAY,  0,
>NULL));
>  }
>
> --
> 2.12.2
>



-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: AT send delay set to 0 in port probings

2017-05-30 Thread Aleksander Morgado 
Hi Brent,

On Tue, May 30, 2017 at 4:11 PM, Brent Sink  wrote:
> How did you apply the patches I sent? Did you "save" the emails and
>
>> apply them with "git am"? or did you manually modify the files with
>> the changes seen in the patches? I ask because the \x90\xf0 looks like
>> some UTF-8 multibyte sequence that shouldn't have been there.
>
> So it seems there were some leftover files in buildroot from an older
> version of ModemManager (I believe it was 1.6.2), which resulted in some of
> the errors I was seeing.  I have struggled getting autogen.sh to configure
> everything correctly for buildroot, so I had copied all of the configure and
> Makefile.in files from 1.6.2 into the 1.7.0 branch so that it would build
> correctly.  This must have caused some issues... :)  I ended up building
> ModemManager on the embedded device to avoid any potential cross compiling
> or linking issues.
>
> I have verified that the patches are good, so you can merge them into master
> when you are ready.  I think I will need to add a few more AT commands so
> that it initializes the SARA-U260 properly, so I'll see if I can figure that
> out this week.  Thanks for the help!

Thanks for the follow up, will merge the patches now.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel