[PATCH rev 2] Improve support for network time on Huawei modules
Aleksander Morgado
aleksander at aleksander.es
Tue Aug 5 07:39:20 PDT 2014
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...).
>
>> > }
>> >
>> > 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.
>
>> > +
>> > + /* 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 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.
>> > +/*****************************************************************************/
>> > +/* 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.
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list