[PATCH rev 2] Improve support for network time on Huawei modules
David McCullough
david.mccullough at accelecon.com
Tue Aug 5 04:56:31 PDT 2014
Aleksander Morgado wrote the following:
> Hey!
>
> Some comments inline.
Thanks, most of it looks good to me, some clarifications below.
>
> On Tue, Aug 5, 2014 at 12:43 PM, David McCullough
> <david.mccullough at accelecon.com> wrote:
> >
> > Second revision of Huawei nwtime support. Takes on feedback from the
> > mailing list including helpers, some basic tests and use of the ^NTCT
> > command to determine network time support (^NWTIME). The helper tests
> > need expanding once the approach is confirmed as acceptable.
> >
> > Signed-off-by: David McCullough <david.mccullough at accelecon.com>
> > ---
> > plugins/huawei/mm-broadband-modem-huawei.c | 173 ++++++++++++++---------
> > plugins/huawei/mm-modem-helpers-huawei.c | 145 +++++++++++++++++++
> > plugins/huawei/mm-modem-helpers-huawei.h | 16 +++
> > plugins/huawei/tests/test-modem-helpers-huawei.c | 129 +++++++++++++++++
> > 4 files changed, 394 insertions(+), 69 deletions(-)
> >
> > diff --git a/plugins/huawei/mm-broadband-modem-huawei.c b/plugins/huawei/mm-broadband-modem-huawei.c
> > index 9cbbfb8..84d0215 100644
> > --- a/plugins/huawei/mm-broadband-modem-huawei.c
> > +++ b/plugins/huawei/mm-broadband-modem-huawei.c
> > @@ -108,6 +108,8 @@ struct _MMBroadbandModemHuaweiPrivate {
> > FeatureSupport syscfg_support;
> > FeatureSupport syscfgex_support;
> > FeatureSupport prefmode_support;
> > + FeatureSupport time_support;
> > + FeatureSupport nwtime_support;
> >
> > MMModemLocationSource enabled_sources;
> >
> > @@ -2840,74 +2842,69 @@ get_detailed_registration_state (MMIfaceModemCdma *self,
> > /*****************************************************************************/
> > /* Load network time (Time interface) */
> >
> > +static MMNetworkTimezone *
> > +modem_time_load_network_timezone_finish (MMIfaceModemTime *_self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> > + MMNetworkTimezone *tz = NULL;
> > + const gchar *response;
> > + gboolean ret = FALSE;
> > +
> > + response = mm_base_modem_at_command_finish (MM_BASE_MODEM (_self), res, error);
> > + if (!response)
> > + return NULL;
>
> How about adding this? This method must only be called when Time
> support is available.
> g_assert (self->priv->nwtime_support == FEATURE_SUPPORTED ||
> self->priv->time_support == FEATURE_SUPPORTED);
ok.
> > +
> > + if (self->priv->nwtime_support == FEATURE_SUPPORTED)
> > + ret = mm_huawei_parse_nwtime_response (response, NULL, &tz, error);
> > + else if (self->priv->time_support == FEATURE_SUPPORTED)
> > + ret = mm_huawei_parse_time_response (response, NULL, &tz, error);
> > + return ret ? tz : NULL;
>
> Why not just this, and avoid the "ret" variable?
> return tz;
I guess I was ensuring that we returned NULL on error and did not rely on
the parse functions also ensuring tz on error.
Happy to switch to this if you think that is not needed.
> > +}
> > +
> > +
> > static gchar *
> > -modem_time_load_network_time_finish (MMIfaceModemTime *self,
> > +modem_time_load_network_time_finish (MMIfaceModemTime *_self,
> > GAsyncResult *res,
> > GError **error)
> > {
> > + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> > const gchar *response;
> > - GRegex *r;
> > - GMatchInfo *match_info = NULL;
> > - GError *match_error = NULL;
> > - guint year, month, day, hour, minute, second;
> > - gchar *result = NULL;
> > + gchar *iso8601 = NULL;
> > + gboolean ret = FALSE;
> >
> > - response = mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> > + response = mm_base_modem_at_command_finish (MM_BASE_MODEM (_self), res, error);
> > if (!response)
> > return NULL;
> >
> > - /* 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,
> > - "Couldn't match ^TIME 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)) {
> > - /* Return ISO-8601 format date/time string */
> > - result = g_strdup_printf ("%04d/%02d/%02d %02d:%02d:%02d",
> > - year, month, day, hour, minute, second);
> > - } else {
> > - g_set_error_literal (error,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "Failed to parse ^TIME reply");
> > - }
> > - }
> > -
> > - if (match_info)
> > - g_match_info_free (match_info);
> > - g_regex_unref (r);
> > - return result;
>
> Same here, I would include:
> g_assert (self->priv->nwtime_support == FEATURE_SUPPORTED ||
> self->priv->time_support == FEATURE_SUPPORTED);
>
> > + if (self->priv->nwtime_support == FEATURE_SUPPORTED)
> > + ret = mm_huawei_parse_nwtime_response (response, &iso8601, NULL, error);
> > + else if (self->priv->time_support == FEATURE_SUPPORTED)
> > + ret = mm_huawei_parse_time_response (response, &iso8601, NULL, error);
> > + return ret ? iso8601 : NULL;
>
> And again, just this, avoiding the ret variable:
Same resoning as above.
> return iso8601;
>
> > }
> >
> > 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);
> > + const char *command = NULL;
> > + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> > +
> > + if (self->priv->nwtime_support == FEATURE_SUPPORTED)
> > + command = "^NWTIME?";
> > + else if (self->priv->time_support == FEATURE_SUPPORTED)
> > + command = "^TIME";
> > +
> > + if (command)
> > + mm_base_modem_at_command (MM_BASE_MODEM (self),
> > + command,
> > + 3,
> > + FALSE,
> > + callback,
> > + user_data);
>
> This is an async method, and therefore it must *always* have an async
> method call inside. If (!command) nothing will be executed and the
> async method will never be completed. BUT, yes, this method will only
> be called when Time interface support is in place, and therefore it
> will logically always have either self->priv->nwtime_support or
> self->priv->time_support set to FEATURE_SUPPORTED. In order to
> emphasize that, I would therefore rewrite the code so that, instead we
> have:
>
> g_assert (command != NULL);
> mm_base_modem_at_command (MM_BASE_MODEM (self), command, 3....);
>
> This is, no if (command), a g_assert instead.
No problems, I wasn't happy with the possibility that nothing would happen.
> > }
> >
> > /*****************************************************************************/
> > @@ -3487,28 +3484,63 @@ enable_location_gathering (MMIfaceModemLocation *self,
> > /* Check support (Time interface) */
> >
> > static gboolean
> > -modem_time_check_support_finish (MMIfaceModemTime *self,
> > +modem_time_check_support_finish (MMIfaceModemTime *_self,
> > GAsyncResult *res,
> > GError **error)
> > {
> > - return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error);
> > + MMBroadbandModemHuawei *self = MM_BROADBAND_MODEM_HUAWEI (_self);
> > + if (self->priv->nwtime_support == FEATURE_SUPPORTED)
> > + return TRUE;
> > + if (self->priv->time_support == FEATURE_SUPPORTED)
> > + return TRUE;
> > + return FALSE;
>
> Just a side note in case anyone wondered, these check_support()
> methods are allowed to return FALSE without error being set.
Hey, I got something right :-) Actually I am pretty sure there are
existing example that just return TRUE or FALSE.
> > }
> >
> > static void
> > -modem_time_check_ready (MMBroadbandModem *self,
> > +modem_time_check_ready (MMBaseModem *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);
> > -
> > + (void)mm_base_modem_at_sequence_finish (self, res, NULL, &error);
> > + g_clear_error (&error);
>
> If you're not interested on the result nor the error, you can just do:
> mm_base_modem_at_sequence_finish (self, res, NULL, NULL);
Sounds good. I wasn't real sure if I cared about the error or not here,
but I don't think we do, we just want to know about the support and those
cases have all be taken care of.
> > g_simple_async_result_complete (simple);
> > g_object_unref (simple);
> > }
> >
> > +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, "^NTCT"))
> > + self->priv->nwtime_support = FEATURE_SUPPORTED;
> > + else if (strstr (response, "^TIME"))
> > + self->priv->time_support = FEATURE_SUPPORTED;
> > + } else {
> > + if (strstr (command, "^NTCT"))
> > + self->priv->nwtime_support = FEATURE_NOT_SUPPORTED;
> > + else if (strstr (command, "^TIME"))
> > + self->priv->time_support = FEATURE_NOT_SUPPORTED;
> > + }
> > +
> > + return FALSE;
> > +}
> > +
> > +static const MMBaseModemAtCommand time_cmd_sequence[] = {
> > + { "^NTCT?", 3, FALSE, modem_check_time_reply }, /* 3GPP/LTE */
> > + { "^TIME", 3, FALSE, modem_check_time_reply }, /* CDMA */
> > + { NULL }
> > +};
> > +
> > static void
> > modem_time_check_support (MMIfaceModemTime *self,
> > GAsyncReadyCallback callback,
> > @@ -3521,13 +3553,12 @@ 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);
> > + mm_base_modem_at_sequence (MM_BASE_MODEM (self),
> > + time_cmd_sequence,
> > + NULL, /* response_processor_context */
> > + NULL, /* response_processor_context_free */
> > + (GAsyncReadyCallback)modem_time_check_ready,
> > + result);
> > }
> >
> > /*****************************************************************************/
> > @@ -3714,6 +3745,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->nwtime_support = FEATURE_SUPPORT_UNKNOWN;
> > + self->priv->time_support = FEATURE_SUPPORT_UNKNOWN;
> > }
> >
> > static void
> > @@ -3845,8 +3878,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
> > diff --git a/plugins/huawei/mm-modem-helpers-huawei.c b/plugins/huawei/mm-modem-helpers-huawei.c
> > index 656c6de..7dd554b 100644
> > --- a/plugins/huawei/mm-modem-helpers-huawei.c
> > +++ b/plugins/huawei/mm-modem-helpers-huawei.c
> > @@ -1033,3 +1033,148 @@ mm_huawei_parse_syscfgex_response (const gchar *response,
> > g_strfreev (split);
> > return NULL;
> > }
> > +
> > +/*****************************************************************************/
> > +/* ^NWTIME response parser */
> > +
> > +gboolean mm_huawei_parse_nwtime_response (const gchar *response,
> > + gchar **iso8601p,
> > + MMNetworkTimezone **tzp,
> > + GError **error)
> > +{
> > + GRegex *r;
> > + GMatchInfo *match_info = NULL;
> > + GError *match_error = NULL;
> > + guint year = 0, month = 0, day = 0, hour = 0, minute = 0, second = 0, dt = 0;
> > + gint tz = 0;
> > + gboolean ret = TRUE;
>
> I would start with "ret = FALSE", and only set it to TRUE if we're
> building correct output to set in 'tzp' or 'iso8601p'.
Agreed.
> > +
> > + 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 ^NWTIME results: ");
> > + } else {
> > + g_set_error_literal (error,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Couldn't match ^NWTIME reply");
> > + }
> > +
> > + ret = FALSE;
> > + } else {
> > + /* 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 */
> > + *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");
> > + ret = FALSE;
> > + }
> > + }
> > +
> > + if (match_info)
> > + g_match_info_free (match_info);
> > + g_regex_unref (r);
> > +
> > + return ret;
> > +}
> > +
> > +/*****************************************************************************/
> > +/* ^TIME response parser */
> > +
> > +gboolean mm_huawei_parse_time_response (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;
> > + gboolean ret = TRUE;
>
> Same here, I would start with "ret = FALSE", and only set it to TRUE
> if we're building correct output to set in 'tzp' or 'iso8601p'.
Agreed.
> > +
> > + /* 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,
> > + "Couldn't match ^TIME reply");
> > + }
> > + ret = FALSE;
> > + } 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)) {
> > + /* 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;
>
> This is not worth a TRUE response; it should be returning FALSE and an
> error set to e.g. MM_CORE_ERROR_UNSUPPORTED. Doing this, we make sure
> that TRUE is returned *only* when the requested outputs are filled in.
> If you do this by setting the error here; also make sure you:
> if (iso8601p)
> g_free (*iso8601p);
>
> The implementation in the huawei plugin will actually request either
> time or timezone, so it should never happen that you really need to
> free iso8601p, but well, just in case. If we ever end up needing
> requesting both time and timezone at the same time then it may be
> actually worth to split the parsers, one for time and another one for
> timezone. So I really wouldn't mind never allowing asking for both at
> the same time:
>
> g_assert (iso8601p || tzp); /* at least one */
> g_assert (!(iso8601p && tzp)); /* never both */
>
> If you add these to the beginning of the parsers, you can actually
> forget the extra free() I'm talking about; your choice.
I was keeping the helper able to handle any combination of inputs including
no return values (ie., just returns TRUE or FALSE if the string parses
correctly.
I am also enabling timezone support on the CDMA modems by defining the
functions for it, yet it currently has no way to report it that I know of
(I only have the previous code version to judge the CDMA support from,
perhps it actually has TZ support)..
Pretty sure the above method means that on the CDMAS modem it will always
report 'not available' for the timezone parameters, but it will not
generate errors.
I am happy to follow whatever ou guys feel is correct behaviour for modem
manager.
The best option would be to "not" support timezone fetching on the CDMA
modems, but I am not sure how that should be done since we cannot return
to enable the load_network_timezone/load_network_timezone_finish functions
later.
> > + }
> > + } else {
> > + g_set_error_literal (error,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Failed to parse ^TIME reply");
> > + ret = FALSE;
> > + }
> > + }
> > +
> > + if (match_info)
> > + g_match_info_free (match_info);
> > + g_regex_unref (r);
> > +
> > + return ret;
> > +}
> > +
> > diff --git a/plugins/huawei/mm-modem-helpers-huawei.h b/plugins/huawei/mm-modem-helpers-huawei.h
> > index 05894f3..e225e90 100644
> > --- a/plugins/huawei/mm-modem-helpers-huawei.h
> > +++ b/plugins/huawei/mm-modem-helpers-huawei.h
> > @@ -113,4 +113,20 @@ const MMHuaweiSyscfgexCombination *mm_huawei_parse_syscfgex_response (const gcha
> > const GArray *supported_mode_combinations,
> > GError **error);
> >
> > +/*****************************************************************************/
> > +/* ^NWTIME response parser */
> > +
> > +gboolean mm_huawei_parse_nwtime_response (const gchar *response,
> > + gchar **iso8601p,
> > + MMNetworkTimezone **tzp,
> > + GError **error);
> > +
> > +/*****************************************************************************/
> > +/* ^TIME response parser */
> > +
> > +gboolean mm_huawei_parse_time_response (const gchar *response,
> > + gchar **iso8601p,
> > + MMNetworkTimezone **tzp,
> > + GError **error);
> > +
> > #endif /* MM_MODEM_HELPERS_HUAWEI_H */
> > diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c b/plugins/huawei/tests/test-modem-helpers-huawei.c
> > index b74821f..7e79106 100644
> > --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
> > +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
> > @@ -1002,6 +1002,133 @@ test_syscfgex_response (void)
> > }
> >
> > /*****************************************************************************/
> > +/* Test ^NWTIME responses */
> > +
> > +typedef struct {
> > + const gchar *str;
> > + gboolean ret;
> > + gboolean test_iso8601;
> > + gboolean test_tz;
> > + gchar *iso8601;
> > + gint32 offset;
> > + gint32 dst_offset;
> > + gint32 leap_seconds;
> > +} NwtimeTest;
> > +
> > +#define NWT_UNKNOWN MM_NETWORK_TIMEZONE_LEAP_SECONDS_UNKNOWN
> > +
> > +static const NwtimeTest nwtime_tests[] = {
> > + { "^NWTIME: 14/08/05,04:00:21+40,00", TRUE, FALSE, FALSE,
> > + "2014-08-05T04:00:21+10:00", 600, 0, NWT_UNKNOWN },
> > + { "^NWTIME: 14/08/05,04:00:21+40,00", TRUE, TRUE, FALSE,
> > + "2014-08-05T04:00:21+10:00", 600, 0, NWT_UNKNOWN },
> > + { "^NWTIME: 14/08/05,04:00:21+40,00", TRUE, FALSE, TRUE,
> > + "2014-08-05T04:00:21+10:00", 600, 0, NWT_UNKNOWN },
> > + { "^NWTIME: 14/08/05,04:00:21+40,00", TRUE, TRUE, TRUE,
> > + "2014-08-05T04:00:21+10:00", 600, 0, NWT_UNKNOWN },
> > + { "^NWTIME: XX/XX/XX,XX:XX:XX+XX,XX", FALSE, FALSE, FALSE,
> > + NULL, NWT_UNKNOWN, NWT_UNKNOWN, NWT_UNKNOWN },
> > + { "^TIME: XX/XX/XX,XX:XX:XX+XX,XX", FALSE, FALSE, FALSE,
> > + NULL, NWT_UNKNOWN, NWT_UNKNOWN, NWT_UNKNOWN },
> > + { "^TIME: 14/08/05,04:00:21+40,00", FALSE, FALSE, FALSE,
> > + NULL, NWT_UNKNOWN, NWT_UNKNOWN, NWT_UNKNOWN },
> > + { NULL, FALSE, FALSE, FALSE, NULL, NWT_UNKNOWN, NWT_UNKNOWN, NWT_UNKNOWN }
> > +};
> > +
> > +static void
> > +test_nwtime (void)
> > +{
> > + guint i;
> > +
> > + for (i = 0; nwtime_tests[i].str; i++) {
> > + GError *error = NULL;
> > + gchar *iso8601;
> > + MMNetworkTimezone *tz;
> > + gboolean ret;
> > +
> > + iso8601 = NULL;
> > + tz = NULL;
> > +
> > + ret = mm_huawei_parse_nwtime_response (nwtime_tests[i].str,
> > + nwtime_tests[i].test_iso8601 ? &iso8601 : NULL,
> > + nwtime_tests[i].test_tz ? &tz : NULL,
> > + &error);
> > +
> > + g_assert (ret == nwtime_tests[i].ret);
> > +
> > + if (nwtime_tests[i].test_iso8601)
> > + g_assert_cmpstr (nwtime_tests[i].iso8601, ==, iso8601);
> > +
> > + if (nwtime_tests[i].test_tz) {
> > + g_assert (nwtime_tests[i].offset == mm_network_timezone_get_offset (tz));
> > + g_assert (nwtime_tests[i].dst_offset == mm_network_timezone_get_dst_offset (tz));
> > + g_assert (nwtime_tests[i].leap_seconds == mm_network_timezone_get_leap_seconds (tz));
> > + }
> > +
> > + if (iso8601)
> > + g_free (iso8601);
> > + }
> > +}
> > +
> > +/*****************************************************************************/
> > +/* Test ^TIME responses */
> > +
> > +typedef struct {
> > + const gchar *str;
> > + gboolean ret;
> > + gboolean test_iso8601;
> > + gboolean test_tz;
> > + gchar *iso8601;
> > +} TimeTest;
> > +
> > +static const TimeTest time_tests[] = {
> > + { "^TIME: XX/XX/XX,XX:XX:XX+XX,XX", FALSE, FALSE, FALSE,
> > + NULL },
> > +#if 0 /* no examples for this yet */
> > + { "^TIME: ???????????????????????", TRUE, FALSE, FALSE,
> > + "2014-08-05T03:49:36+10:00" },
> > + { "^TIME: ???????????????????????", TRUE, TRUE, FALSE,
> > + "2014-08-05T03:49:36+10:00" },
> > + { "^TIME: ???????????????????????", TRUE, FALSE, TRUE,
> > + "2014-08-05T03:49:36+10:00" },
> > + { "^TIME: ???????????????????????", TRUE, TRUE, TRUE,
> > + "2014-08-05T03:49:36+10:00" },
> > +#endif
>
>
> Is it that we don't have any examples, or that they were not added yet?
I don't have any examples. I can make some up based on the code but I
figure it would be better to have some real modem output to use.
There are a lot more things that could be tested here, I was just holding
off on filling in lots of tests until I knew the approach was sound and
likely to be used going forward ;-)
> > + { NULL, FALSE, FALSE, FALSE, NULL }
> > +};
> > +
> > +static void
> > +test_time (void)
> > +{
> > + guint i;
> > +
> > + for (i = 0; time_tests[i].str; i++) {
> > + GError *error = NULL;
> > + gchar *iso8601;
> > + MMNetworkTimezone *tz;
> > + gboolean ret;
> > +
> > + iso8601 = NULL;
> > + tz = NULL;
> > +
> > + ret = mm_huawei_parse_time_response (time_tests[i].str,
> > + time_tests[i].test_iso8601 ? &iso8601 : NULL,
> > + time_tests[i].test_tz ? &tz : NULL,
> > + &error);
> > +
> > + g_assert (ret == time_tests[i].ret);
> > +
> > + if (time_tests[i].test_iso8601)
> > + g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
> > +
> > + g_assert (tz == NULL); /* TZ is not supported */
> > +
> > + if (iso8601)
> > + g_free (iso8601);
> > + }
> > +}
> > +
> > +/*****************************************************************************/
> >
> > void
> > _mm_log (const char *loc,
> > @@ -1039,6 +1166,8 @@ int main (int argc, char **argv)
> > g_test_add_func ("/MM/huawei/syscfg/response", test_syscfg_response);
> > g_test_add_func ("/MM/huawei/syscfgex", test_syscfgex);
> > g_test_add_func ("/MM/huawei/syscfgex/response", test_syscfgex_response);
> > + g_test_add_func ("/MM/huawei/nwtime", test_nwtime);
> > + g_test_add_func ("/MM/huawei/time", test_time);
> >
> > return g_test_run ();
> > }
> > --
> > 1.8.2.rc0
> >
>
> --
> Aleksander
> https://aleksander.es
--
David McCullough, david.mccullough at accelecon.com, Ph: 0410 560 763
More information about the ModemManager-devel
mailing list