| Summary: | Telit HE910: `+CPMS="",...` fails as "Operation Not Supported" | ||
|---|---|---|---|
| Product: | ModemManager | Reporter: | Carlo Lobrano <c.lobrano> |
| Component: | general | Assignee: | ModemManager bug user <modemmanager> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | c.lobrano |
| Version: | 1.4 | ||
| Hardware: | Other | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
Just a note, Modem Manager is at version 1.4.12 In mm_broadband_modem_lock_sms_storages(), what if self->priv->current_sms_mem1_storage == MM_SMS_STORAGE_UNKNOWN? You'd be getting "unknown" as response to mm_sms_storage_get_string(). Your assumption is that self->priv->current_sms_mem1_storage will always contain the current SMS MEM1 storage configured in the modem, and that is not really true. That variable is just to keep track of what is the currently selected memory storage, if any, but as seen by ModemManager. If the Telit modem doesn't support empty parameters and requires the current value to be re-set, we'd need to query with CPMS? first which is that value, in order to re-use it afterwards. How about preloading the current storages first with AT+CPMS? somewhere, e.g. as part of modem_messaging_load_supported_storages()? Aleksander Morgado, I think you're right. I added a check of NULL value in "modem_messaging_set_default_storage", but I didn't set the value of self->priv->current_sms_mem1_storage anywhere. I will update the patch This has been fixed in commit be317e8b80cd984149ea152c9d00c6bb814e7c88 in git master. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Observed issue: With Telit Modem HE910, all commands like `AT+CPMS="",...` fail as "Operation Not Supported":: <debug> [1448531013.833175] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): --> 'AT+CPMS="","ME","ME"<CR>' <debug> [1448531013.902282] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): <-- '<CR><LF>+CMS ERROR: 303<CR><LF>' <debug> [1448531013.933761] [mm-serial-parsers.c:364] mm_serial_parser_v1_parse(): Got failure code 303: Operation not supported <debug> [1448531013.967401] [mm-iface-modem-messaging.c:792] set_default_storage_ready(): Couldn't set default storage: 'Operation not supported' Following is my analisys and a possible patch: ModemManager tries to set default storages for MEM2 and MEM3 only with `+CPMS` providing an empty string for MEM1 argument:: <debug> [1448531013.833175] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): --> 'AT+CPMS="","ME","ME"<CR>' <debug> [1448531013.902282] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): <-- '<CR><LF>+CMS ERROR: 303<CR><LF>' <debug> [1448531013.933761] [mm-serial-parsers.c:364] mm_serial_parser_v1_parse(): Got failure code 303: Operation not supported <debug> [1448531013.967401] [mm-iface-modem-messaging.c:792] set_default_storage_ready(): Couldn't set default storage: 'Operation not supported' Some modems do support NULL arguments (considering that as a request to do not change the correspondant value), but passing an empty string should be considered invalid, since it is not a "supported value":: <debug> [1448531007.342611] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): --> 'AT+CPMS=?<CR>' <debug> [1448531007.389244] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): <-- '<CR><LF>' <debug> [1448531007.418215] [mm-port-serial-at.c:440] debug_log(): (ttyACM0): <-- '+CPMS: ("SM","ME"),("SM","ME"),("SM","ME")<CR><LF><CR><LF>OK<CR><LF>' According to the previous logs, only SM and ME are supported strings, so the command should at most be `AT+CPMS=,"ME","ME"` However, HE910 does not support NULL argument either, so I made the following changes in order to provide always the current value of MEM1, if no other value is provided, this way the code works fine with HE910 and should work with the other modems too, since it's conservative:: diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c index af24248..34407d4 100644 --- a/src/mm-broadband-modem.c +++ b/src/mm-broadband-modem.c @@ -5361,8 +5361,9 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self, ctx->previous_mem1 = self->priv->current_sms_mem1_storage; self->priv->mem1_storage_locked = TRUE; self->priv->current_sms_mem1_storage = mem1; - mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1); } + mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1); + g_assert(mem1_str != NULL); if (mem2 != MM_SMS_STORAGE_UNKNOWN) { ctx->mem2_locked = TRUE; @@ -5380,7 +5381,7 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self, if (mem2_str) cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"", - mem1_str ? mem1_str : "", + mem1_str, mem2_str); else if (mem1_str) cmd = g_strdup_printf ("+CPMS=\"%s\"", mem1_str); @@ -5434,25 +5435,32 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self, MMBroadbandModem *self = MM_BROADBAND_MODEM (_self); gchar *cmd; GSimpleAsyncResult *result; - gchar *mem_str; + gchar *mem1_str; + gchar *mem2_str; + MMSmsStorage current_mem1; + result = g_simple_async_result_new (G_OBJECT (self), callback, user_data, modem_messaging_set_default_storage); + current_mem1 = self->priv->current_sms_mem1_storage? self->priv->current_sms_mem1_storage : storage; /* Set defaults as current */ self->priv->current_sms_mem2_storage = storage; - mem_str = g_ascii_strup (mm_sms_storage_get_string (storage), -1); - cmd = g_strdup_printf ("+CPMS=\"\",\"%s\",\"%s\"", mem_str, mem_str); + mem1_str = g_ascii_strup (mm_sms_storage_get_string (current_mem1), -1); + mem2_str = g_ascii_strup (mm_sms_storage_get_string (storage), -1); + + cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"", mem1_str, mem2_str, mem2_str); mm_base_modem_at_command (MM_BASE_MODEM (self), cmd, 3, FALSE, (GAsyncReadyCallback)cpms_set_ready, result); - g_free (mem_str); + g_free (mem1_str); + g_free (mem2_str); g_free (cmd); } Let me know if I misunderstood something and/or I can improve this patch somehow. Best regards, Carlo