[PATCH rev 2] Improve support for network time on Huawei modules
David McCullough
david.mccullough at accelecon.com
Tue Aug 5 22:54:13 PDT 2014
Aleksander Morgado wrote the following:
> Hey hey,
>
> >
> >> > +
> >> > + 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.
> >
>
> The logic should return:
> * if tz != NULL, return tz.
> * if tz == NULL, return NULL AN set error != NULL.
>
> I guess this issue comes with how the parsers are developed. If
> returning TRUE by the parser means that "there were no errors but the
> outputs may have not been set" (as it's the case with this tz when
> loading timezone is not supported), then I think it's a bit cryptic.
> That is why I was suggesting to return TRUE if and only if all the
> requested outputs are set correctly, and otherwise FALSE will be
> returned AND error set AND all other outputs set to NULL. That is
> usually the way APIs are implemented in GLib based apps.
>
> So, from my POV, the thing to achieve is to make sure that the parsers
> will either set all outputs requested, or otherwise return a proper
> GError. If that is done like that, then you can safely just return
> "tz" here as you'll know that if tz is NULL the parser had to set the
> GError.
>
> Actually, with your current parser, I believe that there may be a case
> in which you're returning a NULL tz and no error set. If you look at
> load_network_timezone_ready(), you'll see that when not getting an
> 'error' we directly assume that tz is not NULL (i.e. in the last line,
> we try to unref() it). Most of the APIs in ModemManager work like
> that, which makes it much easier to understand (except for the
> check_support() one that I noted earlier...).
Will be doen for next pass.
> >> >
> >> > 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.
> >
>
> Well, if you think the error is meaningful, you can also always return
> it. The only difference between returning just FALSE or FALSE+error in
> the feature interface checks is that the error message will be logged.
Not meaniful, so will be completely ignored now.
> >> > +
> >> > + /* 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.
>
>
> Well... that means that you may be asking for TZ data and your parser
> parsed the string correctly but the string didn't have TZ data, so you
> would be returning TRUE but with a NULL tz? that's overly
> complicated...
>
> I'd say forget about the result of parsing the string and think of
> what the caller wants:
> * If I'm asking for TZ and I got TZ, return TRUE. Otherwise (either
> error, or just no TZ available) return FALSE, a NULL tz, and GError
> set.
> * If I'm asking for Time and I got Time, return TRUE. Otherwise
> (either error, or just no Time available) return FALSE, a NULL Time,
> and GError set.
> * If I'm asking for Time and TZ and I got both Time and TZ, return
> TRUE. Otherwise (either error, or just no Time or TZ available) return
> FALSE, a NULL Time, a NULL TZ, and GError set.
>
> If we do it like that, we're effectively just completely skipping the
> need of knowing whether the method returned TRUE or FALSE, btw, it
> could very well just be returning void. The presence of the outputs
> (either TZ, Time or both) would mean the method went OK, and otherwise
> (if no outputs) then GError must have been set.
I will keep the boolean just in case it aids any future use, but otherwise
all of the above will be done.
> > 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.
> >
>
> How about completely removing the timezone polling timeout
> (ctx->network_timezone_poll_id) if the actual method loading the
> timezone reports MM_CORE_ERROR_UNSUPPORTED? This would mean that we
> enable the polling when we enable the modem, but once it fails once
> with UNSUPPORTED, we just remove the polling and it will never be
> fired up again.
Turns out the poll will turn off if its gets an error other than "RETRY".
Still doesn't prevent the tiemzone from being tried again later, not sure
if that can/will happen nore if it matters.
> >> > +/*****************************************************************************/
> >> > +/* 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 ;-)
> >
>
> Ah, ok. Yeah, I would add some tests even if you don't have real strings.
I will blow it out some more post API updates.
Cheers,
Davidm
--
David McCullough, david.mccullough at accelecon.com, Ph: 0410 560 763
More information about the ModemManager-devel
mailing list