Re: [MM][PATCH v2] altair-lte: initial altair lte plugin

[Aleksander Morgado <aleksander lanedo com>]

Hey,

> > > +/*****************************************************************************/
> > > +/* Facility locks status loading (3GPP interface) */
> > > +
> > > +typedef struct {
> > > +    MMBroadbandModem *self;
> > > +    GSimpleAsyncResult *result;
> > > +    guint current;
> > > +    MMModem3gppFacility facilities;
> > > +    MMModem3gppFacility locks;
> > > +} LoadEnabledFacilityLocksContext;
> > > +
> > > +static void get_next_facility_lock_status (LoadEnabledFacilityLocksContext *ctx);
> > > +
> > > +static void
> > > +load_enabled_facility_locks_context_complete_and_free (LoadEnabledFacilityLocksContext *ctx)
> > > +{
> > > +    g_simple_async_result_complete (ctx->result);
> > > +    g_object_unref (ctx->result);
> > > +    g_object_unref (ctx->self);
> > > +    g_free (ctx);
> > > +}
> > > +
> > > +static MMModem3gppFacility
> > > +modem_3gpp_load_enabled_facility_locks_finish (MMIfaceModem3gpp *self,
> > > +                                               GAsyncResult *res,
> > > +                                               GError **error)
> > > +{
> > > +    if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error))
> > > +        return MM_MODEM_3GPP_FACILITY_NONE;
> > > +
> > > +    return ((MMModem3gppFacility) GPOINTER_TO_UINT (
> > > +                g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res))));
> > > +}
> > > +
> > > +static void
> > > +clck_single_query_ready (MMBaseModem *self,
> > > +                         GAsyncResult *res,
> > > +                         LoadEnabledFacilityLocksContext *ctx)
> > > +{
> > > +    const gchar *response;
> > > +    gboolean enabled = FALSE;
> > > +
> > > +    response = mm_base_modem_at_command_finish (self, res, NULL);
> > > +    if (response &&
> > > +        mm_3gpp_parse_clck_write_response (response, &enabled) &&
> > > +        enabled) {
> > > +        ctx->locks |= (1 << ctx->current);
> > > +    } else {
> > > +        /* On errors, we'll just assume disabled */
> > > +        ctx->locks &= ~(1 << ctx->current);
> > > +    }
> > > +
> > > +    /* And go on with the next one */
> > > +    ctx->current++;
> > > +    get_next_facility_lock_status (ctx);
> > > +}
> > > +
> > > +static void
> > > +get_next_facility_lock_status (LoadEnabledFacilityLocksContext *ctx)
> > > +{
> > > +    guint i;
> > > +
> > > +    for (i = ctx->current; i < sizeof (MMModem3gppFacility) * 8; i++) {
> > > +        guint32 facility = 1 << i;
> > > +
> > > +        /* Found the next one to query! */
> > > +        if (ctx->facilities & facility) {
> > > +            gchar *cmd;
> > > +
> > > +            /* Keep the current one */
> > > +            ctx->current = i;
> > > +
> > > +            /* Query current */
> > > +            cmd = g_strdup_printf ("+CLCK=\"%s\",2",
> > > +                                   mm_3gpp_facility_to_acronym (facility));
> > > +            mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> > > +                                      cmd,
> > > +                                      3,
> > > +                                      FALSE,
> > > +                                      (GAsyncReadyCallback)clck_single_query_ready,
> > > +                                      ctx);
> > > +            g_free (cmd);
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > > +    /* No more facilities to query, all done */
> > > +    g_simple_async_result_set_op_res_gpointer (ctx->result,
> > > +                                               GUINT_TO_POINTER (ctx->locks),
> > > +                                               NULL);
> > > +    load_enabled_facility_locks_context_complete_and_free (ctx);
> > > +}
> > > +
> > > +static void
> > > +modem_3gpp_load_enabled_facility_locks (MMIfaceModem3gpp *self,
> > > +                                        GAsyncReadyCallback callback,
> > > +                                        gpointer user_data)
> > > +{
> > > +    LoadEnabledFacilityLocksContext *ctx;
> > > +
> > > +    ctx = g_new (LoadEnabledFacilityLocksContext, 1);
> > > +    ctx->self = g_object_ref (self);
> > > +    ctx->result = g_simple_async_result_new (G_OBJECT (self),
> > > +                                             callback,
> > > +                                             user_data,
> > > +                                             modem_3gpp_load_enabled_facility_locks);
> > > +    ctx->facilities = MM_MODEM_3GPP_FACILITY_SIM;
> >
> > What's the problem with the generic method doing "AT+CLCK=?"? Why not
> > ask for which are the supported facilities and then query each of them
> > (which is what the generic method is doing)?
> >
> > If you only really need to report about the SIM lock, you better
> > simplify the logic of this method, you don't need to have a
> > ctx->facilities and then query each enabled facility in the flags mask,
> > as there will only be one. So just a single AT+CLCK="xx",2 will be needed.
> >
> > If the problem is that you don't want certain facilities to be checked,
> > you can set the new "MM_IFACE_MODEM_3GPP_IGNORED_FACILITY_LOCKS" to
> > contain a mask of flags with the facilities to ignore. See commits
> > d9cf4fe91 and e33fc37e.
>
> I'm trying to save time at boot up.
> I will look into the ignored flag.
> Thanks.

Don't just remove stuff to save time, please, unless the specific
commands being run really take more time than needed. If the plugin
needs to be generic enough, clients need to rely on MM to provide always
the same set of functionality, removing features just to speed it up a
little doesn't seem a good idea.


> > > +static void
> > > +iface_modem_init (MMIfaceModem *iface)
> > > +{
> > > +    /* the modem is powered up at startup - no need to waste
> > > +     * on power query and power up commands */
> > > +    iface->load_power_state = NULL;
> > > +    iface->load_power_state_finish = NULL;
> > > +    iface->modem_power_up = NULL;
> > > +    iface->modem_power_up_finish = NULL;
> > > +
> >
> > This is not right. If you do provide power_down, you need to provide
> > power_up. Users can request low-power mode themselves via DBus, so we
> > also need to allow them to power-up the modem back. If the module is
> > *always* powered up when enabled, you can skip load_power_state, but
> > modem_power_up really needs to be implemented.
>
> Understood, Again I was trying to save time . I will add the power up.

Yeah, please, this one is pretty important.


> > > +static void
> > > +mm_broadband_modem_altair_lte_class_init (MMBroadbandModemAltairLteClass *klass)
> > > +{
> > > +    GObjectClass *object_class = G_OBJECT_CLASS (klass);
> > > +    MMBroadbandModemClass *broadband_modem_class = MM_BROADBAND_MODEM_CLASS (klass);
> > > +
> > > +    g_type_class_add_private (object_class, sizeof (MMBroadbandModemAltairLtePrivate));
> > > +
> > > +    broadband_modem_class->setup_ports = setup_ports;
> > > +
> > > +    /* no need to send any special commands to initialize */
> > > +    broadband_modem_class->initialization_started = NULL;
> > > +    broadband_modem_class->initialization_started_finish = NULL;
> > > +    broadband_modem_class->initialization_stopped = NULL;
> >
> > Wait, but you *do* need the parent implementation of these methods, so
> > don't set them to NULL.
>
> Actually I don't want any initialization run on the port at all.
> So that's why I set them to null , so the parent wouldn't run as well.

The initialization started/stopped methods take care of making sure that
the primary port is kept open during the whole initialization sequence,
you really need to have those.


-- 
Aleksander