> > +{
> > + 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
_______________________________________________
networkmanager-list mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/networkmanager-list
