Re: [MM][PATCH] altair-lte: initial altair lte plugin
- From: Aleksander Morgado <aleksander lanedo com>
- To: ori inbar <ori inbar altair semi com gmail com>
- Cc: thieule chromium org, networkmanager-list gnome org
- Subject: Re: [MM][PATCH] altair-lte: initial altair lte plugin
- Date: Tue, 26 Mar 2013 12:08:28 +0100
> +{
> + MMBearerIpConfig *config;
> +
> + if (g_simple_async_result_propagate_error
(G_SIMPLE_ASYNC_RESULT (res), error))
> + return FALSE;
> +
> + config = g_simple_async_result_get_op_res_gpointer
(G_SIMPLE_ASYNC_RESULT (res));
> +
> + *ipv4_config = g_object_ref (config);
> + *ipv6_config = mm_bearer_ip_config_new ();
> + mm_bearer_ip_config_set_method (*ipv6_config,
MM_BEARER_IP_METHOD_DHCP);
Does it really support IPv6? If not, be aware that NULL can be returned
safely as IPv6 config.
yes it supports IPv6 - so that is why i created this config.
i hope i wrote it correctly.
Good. Your code was correct, but it looked kind of ugly because the IPv6
bearer config wasn't created in the same place as the IPv4 bearer config
one. With the new MMBearerConnectResult object which bundles both, and
which can be used as result in the GSimpleAsyncResult, the code should
look much better.
+/*****************************************************************************/
> +/* Load current capabilities (Modem interface) */
> +
> +static MMModemCapability
> +load_current_capabilities_finish (MMIfaceModem *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + MMModemCapability caps;
> + gchar *caps_str;
> +
> + /* Constrain the modem capabilities to LTE only.*/
> + caps = MM_MODEM_CAPABILITY_LTE;
So this is a LTE-only modem? No HSPA, no EV-DO?
correct.
Ok.
> +static void
> +load_current_bands_done (MMIfaceModem *self,
> + GAsyncResult *res,
> + GSimpleAsyncResult *operation_result)
> +{
> + GArray *bands;
> + const gchar *response;
> + const gchar *p;
> + GError *error = NULL;
> + guint32 bandval;
> + MMModemBand band;
> +
> + response = mm_base_modem_at_command_finish (MM_BASE_MODEM
(self), res, &error);
> + if (!response) {
> + mm_dbg ("Couldn't query supported bands: '%s'",
error->message);
> + g_simple_async_result_take_error (operation_result, error);
> + g_simple_async_result_complete_in_idle (operation_result);
> + g_object_unref (operation_result);
> + return;
> + }
> +
> + /*
> + * Response is "Bands: <band>,[<band>...]"
> + * we assume there should be only one band.
> + * but if there is more than one , we'll just use the last band.
> + */
Not sure how this happens. 'Current' bands must be a subset of
'Supported' bands, and looking at the code I see that supported bands is
limited to just MM_MODEM_BAND_EUTRAN_XIII. So, how is it that the
current band value can be different to that one?
this is a VZW only implementation. so i made an effort to support VZW
bands only.
I think it's not worth to do this in supported_bands, truth be told. If
there is no command to query which are the supported bands, just don't
implement load_supported_bands(), and leave it report ANY. When
'current' bands are queried, you are already making sure that they are a
subset of ANY.
>
+/*****************************************************************************/
> +/* Load access technologies (Modem interface) */
> +
> +static gboolean
> +load_access_technologies_finish (MMIfaceModem *self,
> + GAsyncResult *res,
> + MMModemAccessTechnology
*access_technologies,
> + guint *mask,
> + GError **error)
> +{
> + if (g_simple_async_result_propagate_error
(G_SIMPLE_ASYNC_RESULT (res), error))
> + return FALSE;
> +
> + *access_technologies = (MMModemAccessTechnology)
GPOINTER_TO_UINT (
> + g_simple_async_result_get_op_res_gpointer (
> + G_SIMPLE_ASYNC_RESULT (res)));
> + *mask = MM_MODEM_ACCESS_TECHNOLOGY_LTE;
> + return TRUE;
> +}
> +
> +static void
> +load_access_technologies (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GSimpleAsyncResult *operation_result;
> + MMModemAccessTechnology act;
> +
> + operation_result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> +
load_access_technologies);
> +
> +
> + act = MM_MODEM_ACCESS_TECHNOLOGY_LTE;
> +
> + g_simple_async_result_set_op_res_gpointer (operation_result,
> + GUINT_TO_POINTER
(act),
> + NULL);
> + g_simple_async_result_complete_in_idle (operation_result);
> + g_object_unref (operation_result);
Is this modem really LTE-only? Is there no way to query for the current
access tech at all? When the modem is neither registered nor searching
it will still say "LTE" in access tech...
I have tried looking into this issue.
I actually thought this function is redundant since the
access technology is already
reported in CEREG , but then i found a bug / behavior that causes CEREG
ACT not to be considered.
Let's fix it then! :)
when CEREG first notifies Home (registered) - it sends the ACT with it -
(and it will keep sending it in
unsolicited responses). but the modem manager doesn't consider this Home
state until it reads the operator data.
it is kept in some sort of "registering" state.
Yes, we make sure to report we are 'registered' only once we got the
operator code and name.
so when CEREG reports HOME+ correct ACT it is being thrown away since the
mm_iface_modem_3gpp_update_access_technologies being called
from registration_state_changed
only regards the ACT if the state is either HOME or ROAMING. but we are
in the twilight zone ("registering") which isn't
an official state.
and only after the loading of the operator code is over the state is
changed to HOME (which is done after the call to
mm_iface_modem_3gpp_update_access_technologies is over)
i think we should consider Adding this "registering" state as a valid
state and have
mm_iface_modem_3gpp_update_access_technologies accept this state for
updating access tech.
Why not just check the ctx->reloading_operator flag in the registration
context?
i also think that the periodic_access_technologies_check_enable
is redundant if we have CEREG unsolicited
to report a change of ACT.
If there is no way to query access technologies, yes, it is redundant.
If you don't implement load_access_technologies() the periodic query
shouldn't get enabled.
if we are not making the above change and since we have no other method
to get the ACT apart from CEREG - and
periodic_access_technologies_check_enable is only done in REGISTERED
state - i see no problem in reporting LTE
all the time (since we are LTE only) as the code above is doing.
let me know your thoughts.
I think we should fix the issue to allow reporting access tech while
operator code is being reloaded, and that's it. That should already help
us to report LTE when registered and UNKNOWN when not registered anywhere.
+/*****************************************************************************/
> +
> +MMBroadbandModemAltairLte *
> +mm_broadband_modem_altair_lte_new (const gchar *device,
> + const gchar **drivers,
> + const gchar *plugin,
> + guint16 vendor_id,
> + guint16 product_id)
> +{
> + return g_object_new (MM_TYPE_BROADBAND_MODEM_ALTAIR_LTE,
> + MM_BASE_MODEM_DEVICE, device,
> + MM_BASE_MODEM_DRIVERS, drivers,
> + MM_BASE_MODEM_PLUGIN, plugin,
> + MM_BASE_MODEM_VENDOR_ID, vendor_id,
> + MM_BASE_MODEM_PRODUCT_ID, product_id,
> + /*Temporarily allows only EPS network
registration status */
> + /* TODO(orii): Remove this constraint
once the CGREG bug is fixed*/
So it seems you're currently making the modem LTE-only while it really
isn't? :)
my bad - this is not temporary - we are LTE only.
Ok, please clean that up then in the next patch.
> + /* currently only VZW is supported - so we disable the scan
network feature */
> + iface->scan_networks = NULL;
> + iface->scan_networks_finish = NULL;
Is this really the way to go? Why not just leave it there?
since this is VZW only implementation currently - i don;t want to allow
scanning.
But does it do any harm? Does it break the modem? Is it just not
implemented?
--
Aleksander
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
