Fixing bug 93135 (AT+CPMS error with Telit modems)

[Aleksander Morgado]

On Mon, Mar 7, 2016 at 10:07 AM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> I am working on a fix for Bug 93135 - Telit HE910: `+CPMS="",...` fails as
> "Operation Not Supported".
> The problem is that Telit modems does not support empty strings as +CPMS
> parameter,
> while in MM the empty parameter is used in order to keep the current memory
> configuration for
> that field (like AT+CPMS="","ME")
>
> I followed the hint given by Aleksander in a previous e-mail in order to
> have mem1 set to the current
> supported storage, if not set otherwise, in place of being unknown
>
>> [...] what I would do, is to provide in MMBroadbandModemClass a new
>> async virtual method to "load_current_sms_mem_storage()" (or something
>> like that). The logic in MMBroadbandModem which checks which are the
>> supported storages could then have a new step to query which is the
>> current storage (which we only need to do once), all this in the
>> MMBroadbandModem class.
>
> however the same kind of AT command is emitted in sms_store function
> (mm-sms-base.c), where
> mm_broadband_modem_lock_sms_storages is called with mem1 explicitly set to
> UNKNOWN:
>
>     mm_broadband_modem_lock_sms_storages (
>         MM_BROADBAND_MODEM (self->priv->modem),
>         MM_SMS_STORAGE_UNKNOWN, /* none required for mem1 */
>         ctx->storage,
>         (GAsyncReadyCallback)store_lock_sms_storages_ready,
>         ctx);
>
> I do not get why none is required for mem1 in this case and I would like to
> know if can be considered
> safe to use the value of "current_sms_mem1_storage" when the local value of
> mem1 in
> mm_broadband_modem_lock_sms_storages is UNKNOWN

"mem1" is for reading/listing/deleting messages; "mem2" is for
storing/sending. In different operations we may want to lock different
mems. In your specific question, none is required for mem1 in
sms_store() because we're storing only, so we don't need to select
explicitly a given storage for mem1 (which is why we give the empty
field in mem1). If Telit modems don't support the mem1 field being
empty, what you need to provide instead of an empty string is the
current mem1 value, so that it effectively doesn't get changed.

That's, IIRC, why I was suggesting the load_current_sms_mem_storage():
you run it once at the beginning (e.g. once we have listed the
supported memory storages) and from then on we keep track of which one
is selected in all the next operations we do, so that
"current_sms_mem1_storage" has always the correct value (and so that
you can use "current_sms_mem1_storage" instead of an empty string).

Currently the only moment in which "current_sms_mem1_storage" may not
have the correct value is upon boot, before
mm_broadband_modem_lock_sms_storages() is called with a non-UNKNOWN
"mem1" parameter.

E.g. we currently do this:

current_sms_mem1_storage = UNKNOWN
mm_broadband_modem_lock_sms_storages (mem1=SM)
   current_sms_mem1_storage = SM
mm_broadband_modem_lock_sms_storages (mem1=ME)
   current_sms_mem1_storage = ME
...

If we never called mm_broadband_modem_lock_sms_storages() with a mem1
parameter different to UNKNOWN, then current_sms_mem1_storage would
always be UNKNOWN, and you don't want that. What I think we want is:

current_sms_mem1_storage = UNKNOWN
load_current_sms_mem_storage()
   current_sms_mem1_storage = SM  <--- as read from the modem
mm_broadband_modem_lock_sms_storages (mem1=SM)
   current_sms_mem1_storage = SM
mm_broadband_modem_lock_sms_storages (mem1=ME)
   current_sms_mem1_storage = ME
...

And now that I see it... this may not be an issue after all, because
we do try to list all SMS messages during the Messaging interface
enabling step, and that means that we're automatically calling
mm_broadband_modem_lock_sms_storages() with different mem1 values...
Could you validate that that is what's happening?

Have you tried to implement the fix by just re-using the
"current_sms_mem1_storage" value instead of an empty string in
AT+CPMS? Did it work?

-- 
Aleksander
https://aleksander.es