[PATCH 2/2] Improve support for network time on Huawei modules
Ben Chan
benchan at chromium.org
Mon Jul 28 17:41:25 PDT 2014
Some drive-by comments. Aleksander / Dan may provide more feedback.
On Mon, Jul 28, 2014 at 4:35 PM, David McCullough
<david.mccullough at accelecon.com> wrote:
>
>
> CDMA modems already had this support, extended to work with 3G
> and LTE modem modules like the MU609 and MU909.
>
> Unfortunately we can only check the network time once we have an active
> network. The errors for unsupported commands and no active network are
> the same (error 100).
> ---
> plugins/huawei/mm-broadband-modem-huawei.c | 309 +++++++++++++++++++++++------
> 1 file changed, 249 insertions(+), 60 deletions(-)
>
> diff --git a/plugins/huawei/mm-broadband-modem-huawei.c b/plugins/huawei/mm-broadband-modem-huawei.c
> index 9cbbfb8..64061d4 100644
> --- a/plugins/huawei/mm-broadband-modem-huawei.c
> +++ b/plugins/huawei/mm-broadband-modem-huawei.c
> @@ -74,6 +74,12 @@ typedef enum {
> FEATURE_SUPPORTED
> } FeatureSupport;
>
> +typedef enum {
> + TIME_SUPPORT_TRY_ALL,
> + TIME_SUPPORT_USE_NWTIME,
> + TIME_SUPPORT_USE_TIME
> +} TimeSupport;
> +
> struct _MMBroadbandModemHuaweiPrivate {
> /* Regex for signal quality related notifications */
> GRegex *rssi_regex;
> @@ -109,6 +115,8 @@ struct _MMBroadbandModemHuaweiPrivate {
> FeatureSupport syscfgex_support;
> FeatureSupport prefmode_support;
>
> + TimeSupport time_support;
> +
> MMModemLocationSource enabled_sources;
>
> GArray *syscfg_supported_modes;
> @@ -2840,74 +2848,267 @@ get_detailed_registration_state (MMIfaceModemCdma *self,
> /*****************************************************************************/
> /* Load network time (Time interface) */
>
> -static gchar *
> -modem_time_load_network_time_finish (MMIfaceModemTime *self,
> - GAsyncResult *res,
> - GError **error)
> +static void
> +modem_nwtime_parse (const gchar *response,
[Ben] Perhaps refactor this function to mm-modem-helpers-huawei.c and
add some unit test.
> + gchar **iso8601p,
[Ben] Arguments should be aligned.
> + MMNetworkTimezone **tzp,
> + GError **error)
> {
> - const gchar *response;
> GRegex *r;
> GMatchInfo *match_info = NULL;
> GError *match_error = NULL;
> - guint year, month, day, hour, minute, second;
> - gchar *result = NULL;
> -
> - response = mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> - if (!response)
> - return NULL;
> + guint year = 0, month = 0, day = 0, hour = 0, minute = 0, second = 0, dt = 0;
> + gint tz = 0;
>
> - /* Already in ISO-8601 format, but verify just to be sure */
> - r = g_regex_new ("\\^TIME:\\s*(\\d+)/(\\d+)/(\\d+)\\s*(\\d+):(\\d+):(\\d*)$", 0, 0, NULL);
> + r = g_regex_new ("\\^NWTIME:\\s*(\\d+)/(\\d+)/(\\d+),(\\d+):(\\d+):(\\d*)([\\-\\+\\d]+),(\\d+)$", 0, 0, NULL);
> g_assert (r != NULL);
>
> if (!g_regex_match_full (r, response, -1, 0, 0, &match_info, &match_error)) {
> if (match_error) {
> g_propagate_error (error, match_error);
> - g_prefix_error (error, "Could not parse ^TIME results: ");
> + g_prefix_error (error, "Could not parse ^NWTIME results: ");
> } else {
> g_set_error_literal (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> - "Couldn't match ^TIME reply");
> + "Couldn't match ^NWTIME reply");
> }
> - } else {
> - /* Remember that g_match_info_get_match_count() includes match #0 */
> - g_assert (g_match_info_get_match_count (match_info) >= 7);
> -
> - if (mm_get_uint_from_match_info (match_info, 1, &year) &&
> - mm_get_uint_from_match_info (match_info, 2, &month) &&
> - mm_get_uint_from_match_info (match_info, 3, &day) &&
> - mm_get_uint_from_match_info (match_info, 4, &hour) &&
> - mm_get_uint_from_match_info (match_info, 5, &minute) &&
> - mm_get_uint_from_match_info (match_info, 6, &second)) {
[Ben] match_info is created even if g_regex_match_full returns FALSE.
so it's leaked here
> + return;
> + }
> + /* Remember that g_match_info_get_match_count() includes match #0 */
> + g_assert (g_match_info_get_match_count (match_info) >= 9);
> +
> + if (mm_get_uint_from_match_info (match_info, 1, &year) &&
> + mm_get_uint_from_match_info (match_info, 2, &month) &&
> + mm_get_uint_from_match_info (match_info, 3, &day) &&
> + mm_get_uint_from_match_info (match_info, 4, &hour) &&
> + mm_get_uint_from_match_info (match_info, 5, &minute) &&
> + mm_get_uint_from_match_info (match_info, 6, &second) &&
> + mm_get_int_from_match_info (match_info, 7, &tz) &&
> + mm_get_uint_from_match_info (match_info, 8, &dt)) {
> + /* adjust year */
> + if (year < 100)
> + year += 2000;
> + /*
> + * tz = timezone offset in 15 minute intervals
> + * dt = daylight adjustment, 0 = none, 1 = 1 hour, 2 = 2 hours
> + * other values are marked reserved.
> + */
> + if (iso8601p) {
> /* Return ISO-8601 format date/time string */
> - result = g_strdup_printf ("%04d/%02d/%02d %02d:%02d:%02d",
> - year, month, day, hour, minute, second);
> + *iso8601p = mm_new_iso8601_time (year, month, day, hour,
> + minute, second,
> + TRUE, (tz * 15) + (dt * 60));
> + }
> + if (tzp) {
> + *tzp = mm_network_timezone_new ();
> + mm_network_timezone_set_offset (*tzp, tz * 15);
> + mm_network_timezone_set_dst_offset (*tzp, dt * 60);
> + }
> + } else {
> + g_set_error_literal (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to parse ^NWTIME reply");
[Ben] match_info is leaked here as well.
>
> + return;
> + }
> +
> + if (match_info)
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> +
> + return;
> +}
> +
> +static void
> +modem_time_parse (const gchar *response,
> + gchar **iso8601p,
> + MMNetworkTimezone **tzp,
> + GError **error)
> +{
> + GRegex *r;
> + GMatchInfo *match_info = NULL;
> + GError *match_error = NULL;
> + guint year, month, day, hour, minute, second;
> +
> + /* Already in ISO-8601 format, but verify just to be sure */
> + r = g_regex_new ("\\^TIME:\\s*(\\d+)/(\\d+)/(\\d+)\\s*(\\d+):(\\d+):(\\d*)$", 0, 0, NULL);
> + g_assert (r != NULL);
> +
> + if (!g_regex_match_full (r, response, -1, 0, 0, &match_info, &match_error)) {
> + if (match_error) {
> + g_propagate_error (error, match_error);
> + g_prefix_error (error, "Could not parse ^TIME results: ");
> } else {
> g_set_error_literal (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> - "Failed to parse ^TIME reply");
> + "Couldn't match ^TIME reply");
[Ben] match_info is leaked here.
> }
> + return;
> + }
> + /* Remember that g_match_info_get_match_count() includes match #0 */
> + g_assert (g_match_info_get_match_count (match_info) >= 7);
> +
> + if (mm_get_uint_from_match_info (match_info, 1, &year) &&
> + mm_get_uint_from_match_info (match_info, 2, &month) &&
> + mm_get_uint_from_match_info (match_info, 3, &day) &&
> + mm_get_uint_from_match_info (match_info, 4, &hour) &&
> + mm_get_uint_from_match_info (match_info, 5, &minute) &&
> + mm_get_uint_from_match_info (match_info, 6, &second)) {
> + /* Return ISO-8601 format date/time string */
> + if (iso8601p)
> + *iso8601p = mm_new_iso8601_time (year, month, day, hour,
> + minute, second, FALSE, 0);
> + if (tzp) {
> + /* not implemented/available for this modem ?
> + * *tzp = mm_network_timezone_new ();
> + * mm_network_timezone_set_offset (*tzp, 0); no TZ
> + * mm_network_timezone_set_dst_offset (*tzp, dt * 60);
> + */
> + *tzp = NULL;
> + }
> + } else {
> + g_set_error_literal (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to parse ^TIME reply");
[Ben] match_info is leaked here.
> + return;
> }
>
> if (match_info)
> g_match_info_free (match_info);
> g_regex_unref (r);
> - return result;
> +
> + return;
> +}
> +
> +static gchar *
> +modem_time_load_network_time_finish (MMIfaceModemTime *_self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> + gchar *iso8601p = NULL;
> + GVariant *cmd_result;
> + const gchar *response;
> +
> + cmd_result = mm_base_modem_at_sequence_full_finish (MM_BASE_MODEM (_self),
> + res, NULL, error);
> + if (!cmd_result) {
> + /* We'll assume we can retry a bit later */
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_RETRY,
> + "Retry");
> + return NULL;
> + }
> +
> + response = g_variant_get_string (cmd_result, NULL);
> +
> + if (self->priv->time_support == TIME_SUPPORT_USE_NWTIME)
> + modem_nwtime_parse(response, &iso8601p, NULL, error);
> + else if (self->priv->time_support == TIME_SUPPORT_USE_TIME)
> + modem_time_parse(response, &iso8601p, NULL, error);
> +
> + return iso8601p;
> }
>
> +static MMNetworkTimezone *
> +modem_time_load_network_timezone_finish (MMIfaceModemTime *_self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> + MMNetworkTimezone *tz = NULL;
> + GVariant *cmd_result;
> + const gchar *response;
> +
> + cmd_result = mm_base_modem_at_sequence_full_finish (MM_BASE_MODEM (_self),
> + res, NULL, error);
> + if (!cmd_result) {
> + /* We'll assume we can retry a bit later */
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_RETRY,
> + "Retry");
> + return NULL;
> + }
> +
> + response = g_variant_get_string (cmd_result, NULL);
> +
> + if (self->priv->time_support == TIME_SUPPORT_USE_NWTIME)
> + modem_nwtime_parse(response, NULL, &tz, error);
[Ben] Add a space after modem_nwtime_parse
> + else if (self->priv->time_support == TIME_SUPPORT_USE_TIME)
> + modem_time_parse(response, NULL, &tz, error);
[Ben] Add a space after modem_time_parse
> +
> + return tz;
> +}
> +
> +static gboolean
> +modem_check_time_reply (MMBaseModem *_self,
> + gpointer none,
> + const gchar *command,
> + const gchar *response,
> + gboolean last_command,
> + const GError *error,
> + GVariant **result,
> + GError **result_error)
> +{
> + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> +
> + if (!error) {
> + if (strstr (response, "^NWTIME")) {
> + self->priv->time_support = TIME_SUPPORT_USE_NWTIME;
> + *result = g_variant_new_string(response);
[Ben] space after g_variant_new_string
> + } else if (strstr (response, "^TIME")) {
> + self->priv->time_support = TIME_SUPPORT_USE_TIME;
> + *result = g_variant_new_string(response);
[Ben] space after g_variant_new_string
> + }
> + }
> +
> + /* Stop sequence if we have an answer */
> + return self->priv->time_support != TIME_SUPPORT_TRY_ALL ? TRUE : FALSE;
> +}
> +
> +static const MMBaseModemAtCommand time_cmd_sequence_all[] = {
> + { "^NWTIME?", 3, FALSE, modem_check_time_reply }, /* 3GPP/LTE */
> + { "^TIME", 3, FALSE, modem_check_time_reply }, /* CDMA */
> + { NULL }
> +};
> +
> +static const MMBaseModemAtCommand time_cmd_sequence_nwtime[] = {
> + { "^NWTIME?", 3, FALSE, modem_check_time_reply }, /* 3GPP/LTE */
> + { NULL }
> +};
> +
> +static const MMBaseModemAtCommand time_cmd_sequence_time[] = {
> + { "^TIME", 3, FALSE, modem_check_time_reply }, /* CDMA */
> + { NULL }
> +};
> +
> static void
> -modem_time_load_network_time (MMIfaceModemTime *self,
> +modem_time_load_network_time_or_zone (MMIfaceModemTime *_self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - mm_base_modem_at_command (MM_BASE_MODEM (self),
> - "^TIME",
> - 3,
> - FALSE,
> - callback,
> - user_data);
> + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> + const MMBaseModemAtCommand *seq;
> +
> + if (self->priv->time_support == TIME_SUPPORT_USE_NWTIME)
> + seq = time_cmd_sequence_nwtime;
> + else if (self->priv->time_support == TIME_SUPPORT_USE_TIME)
> + seq = time_cmd_sequence_time;
> + else
> + seq = time_cmd_sequence_all;
> +
> + mm_base_modem_at_sequence (
> + MM_BASE_MODEM (_self),
> + seq,
> + NULL, /* response_processor_context */
> + NULL, /* response_processor_context_free */
> + callback,
> + user_data);
> }
>
> /*****************************************************************************/
> @@ -3491,22 +3692,11 @@ modem_time_check_support_finish (MMIfaceModemTime *self,
> GAsyncResult *res,
> GError **error)
> {
> - return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error);
> -}
> -
> -static void
> -modem_time_check_ready (MMBroadbandModem *self,
> - GAsyncResult *res,
> - GSimpleAsyncResult *simple)
> -{
> - GError *error = NULL;
> -
> - mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, &error);
> - if (error)
> - g_simple_async_result_take_error (simple, error);
> -
> - g_simple_async_result_complete (simple);
> - g_object_unref (simple);
> + /* We need to assume Huawei devices always support fetching the time
> + * since they report error 100 if not yet connected to the network.
> + * Do our best to runtime determine the access method later.
> + */
> + return TRUE;
> }
>
> static void
> @@ -3521,13 +3711,8 @@ modem_time_check_support (MMIfaceModemTime *self,
> user_data,
> modem_time_check_support);
>
> - /* Only CDMA devices support this at the moment */
> - mm_base_modem_at_command (MM_BASE_MODEM (self),
> - "^TIME",
> - 3,
> - TRUE,
> - (GAsyncReadyCallback)modem_time_check_ready,
> - result);
> + g_simple_async_result_complete_in_idle (result);
> + g_object_unref (result);
> }
>
> /*****************************************************************************/
> @@ -3714,6 +3899,8 @@ mm_broadband_modem_huawei_init (MMBroadbandModemHuawei *self)
> self->priv->syscfg_support = FEATURE_SUPPORT_UNKNOWN;
> self->priv->syscfgex_support = FEATURE_SUPPORT_UNKNOWN;
> self->priv->prefmode_support = FEATURE_SUPPORT_UNKNOWN;
> +
> + self->priv->time_support = TIME_SUPPORT_TRY_ALL;
> }
>
> static void
> @@ -3845,8 +4032,10 @@ iface_modem_time_init (MMIfaceModemTime *iface)
> {
> iface->check_support = modem_time_check_support;
> iface->check_support_finish = modem_time_check_support_finish;
> - iface->load_network_time = modem_time_load_network_time;
> + iface->load_network_time = modem_time_load_network_time_or_zone;
> iface->load_network_time_finish = modem_time_load_network_time_finish;
> + iface->load_network_timezone = modem_time_load_network_time_or_zone;
> + iface->load_network_timezone_finish = modem_time_load_network_timezone_finish;
> }
>
> static void
> --
> 1.8.2.rc0
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
More information about the ModemManager-devel
mailing list