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

Tue May 30 19:35:03 UTC 2017

On Tue, May 30, 2017 at 8:27 PM, Dan Williams <dcbw at redhat.com> 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]: <debug> [1496171739.271335] Setting up location
sources: '3gpp-lac-ci, gps-nmea'
ModemManager[4839]: <debug> [1496171739.271399] Location '3gpp-lac-ci'
gathering is already enabled...
ModemManager[4839]: <debug> [1496171739.271415] Location 'gps-raw'
gathering is already disabled...
ModemManager[4839]: <debug> [1496171739.271427] Location
'gps-unmanaged' gathering is already disabled...
ModemManager[4839]: <debug> [1496171739.271443] Need to enable the
following location sources: 'gps-nmea'
ModemManager[4839]: <debug> [1496171739.271637] (ttyACM1) device open
count is 2 (open)
ModemManager[4839]: <debug> [1496171739.271694] (ttyACM1): -->
'AT^SGPSC="NMEA/Output","on"<CR>'
ModemManager[4839]: <debug> [1496171739.331314] (ttyACM1): <--
'<CR><LF>^SGPSC: "Nmea/Output","on"<CR><LF><CR><LF><CR><LF>OK<CR><LF>'
ModemManager[4839]: <debug> [1496171739.331410] (ttyACM1) device open
count is 1 (close)
ModemManager[4839]: <debug> [1496171739.431720] (ttyACM1) device open
count is 2 (open)
ModemManager[4839]: <debug> [1496171739.431815] (ttyACM1): -->
'AT^SGPSC="Power/Antenna","on"<CR>'
ModemManager[4839]: <debug> [1496171739.495599] (ttyACM1): <--
'<CR><LF>^SGPSC: "Power/Antenna","on"<CR><LF><CR><LF>OK<CR><LF>'
ModemManager[4839]: <debug> [1496171739.495722] (ttyACM1) device open
count is 1 (close)
ModemManager[4839]: <debug> [1496171740.133830] (ttyACM0) unexpected
port hangup!
ModemManager[4839]: <debug> [1496171740.133888] (ttyACM0) forced to close port
ModemManager[4839]: <debug> [1496171740.133918] (ttyACM0) device open
count is 0 (close)
ModemManager[4839]: <debug> [1496171740.133939] (ttyACM0) closing serial port...
ModemManager[4839]: <debug> [1496171740.165232] (ttyACM0) serial port closed
ModemManager[4839]: <debug> [1496171740.167708] (ttyACM1) unexpected
port hangup!
ModemManager[4839]: <debug> [1496171740.167770] (ttyACM1) forced to close port
ModemManager[4839]: <debug> [1496171740.167791] (ttyACM1) device open
count is 0 (close)
ModemManager[4839]: <debug> [1496171740.167811] (ttyACM1) closing serial port...
ModemManager[4839]: <debug> [1496171740.188124] (ttyACM1) serial port closed
ModemManager[4839]: <debug> [1496171740.188506] Removing device 'USB3'
ModemManager[4839]: <debug> [1496171740.188722] [device USB3]
unexported modem from path '/org/freedesktop/ModemManager1/Modem/0'
ModemManager[4839]: <debug> [1496171740.189045] (ttyACM2) forced to close port
ModemManager[4839]: <debug> [1496171741.497337] GPS Engine setup failed (1/3)
ModemManager[4839]: <debug> [1496171743.499526] GPS Engine setup failed (2/3)
ModemManager[4839]: <debug> [1496171745.500241] GPS Engine setup failed (3/3)
ModemManager[4839]: <debug> [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 cancel that
cancellable when the modem is invalid and any async operation that may
be ongoing can also error out as Cancelled, see e.g. these old
never-merged commits:
https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?h=aleksander/cancellations&id=7bbc6dbbf6b79b7605a506257a2a7195692d91a5
https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?h=aleksander/cancellations&id=d34008221a59f2402fa1695a07a1535c965eaee2
https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?h=aleksander/cancellations&id=89d5291af5c2362453dcf4759258ce03d508cfd1

-- 
Aleksander
https://aleksander.es