The branch itself looks fine to me; just some things that came up while I was reviewing the commits...


 * Why do we store modem (non-current) caps in the NetworkManager modem objects? Shouldn't that go away? Or are we expecting to be able to change current caps through NM? (don't think we should).


 * In nm-device-modem.c, the NM_DEVICE_TYPE_DESC property is now set to "Broadband" also for the old MM modems; is that intended? Type description is an internal string used within NM, shouldn't it be something like "Old" or something?

if (NM_IS_MODEM_OLD (modem)) {
    nm_modem_old_get_capabilities (NM_MODEM_OLD (modem), ...);
    type_desc = "Broadband";
    ip_iface = nm_modem_get_data_port (modem);
}


 * If the modem is both GSM/UMTS and CDMA/EVDO but not LTE, CDMA connection settings get preference:

/* Otherwise, prefer CDMA on the theory that if the modem has CDMA/EVDO
 * that's most likely what the user will be using.
 */
if (priv->caps & NM_DEVICE_MODEM_CAPABILITY_CDMA_EVDO)
  return complete_connection_cdma (...);

Thinking in the new MM here, shouldn't we have some way of telling ModemManager which kind of connection setup we want in these cases, instead of assuming one or the other is preferred? I'm thinking in a new key/par for CreateBearer() and Simple.Connect(). Something like "connection-type" (3GGP or 3GPP2). In the new MM, if a modem has both 3GPP2 and 3GPP caps (whether it's LTE or not), 3GPP connection setup is preferred IIRC. Another option is to base the decision on the input parameters we get, e.g. if APN is given, always launch 3GPP connection logic, otherwise CDMA connection logic.

>    mobile: merge NMModemCdma into NMModemOld

the "merge NMModemCdma" patch also removes nm-modem-gsm.c from POTFILES.in

create_connect_properties() is a little goofy; it should check for the presence of s_cdma first, before creating the properties hash. (I suppose it makes a bit more sense after the next patch...)

The "Otherwise, prefer CDMA" comment in nm-modem-old.c:complete_connection() doesn't make sense at this point in the patchset.

"nm_modem_old_new" :)


>     mobile: merge NMModemGsm into NMModemOld

On failure, stage1_prepare_done() and stage1_enable_done() call translate_mm_error() even in the CDMA case, which they didn't before. Is that intentional?

>	/* If the modem is only 3GPP and the connection is not 3GPP, error */
>	if (!(priv->caps & NM_DEVICE_MODEM_CAPABILITY_CDMA_EVDO)) {

wrong capability

(In reply to comment #1)
>  * Why do we store modem (non-current) caps in the NetworkManager modem
> objects? Shouldn't that go away? Or are we expecting to be able to change
> current caps through NM? (don't think we should).

I think that was the original idea, it should probably be dropped now and we only care about current capabilities.

>  * In nm-device-modem.c, the NM_DEVICE_TYPE_DESC property is now set to
> "Broadband" also for the old MM modems; is that intended? Type description is
> an internal string used within NM, shouldn't it be something like "Old" or
> something?

That was intentional since now the old MM code doesn't care whether the device is CDMA or 3GPP, and old MM may need to support CDMA+LTE in the future, hopefully not.  I suppose I can hack the capabilities stuff back in.

>  * If the modem is both GSM/UMTS and CDMA/EVDO but not LTE, CDMA connection
> settings get preference:
> 
> /* Otherwise, prefer CDMA on the theory that if the modem has CDMA/EVDO
>  * that's most likely what the user will be using.
>  */
> if (priv->caps & NM_DEVICE_MODEM_CAPABILITY_CDMA_EVDO)
>   return complete_connection_cdma (...);
> 
> Thinking in the new MM here, shouldn't we have some way of telling ModemManager
> which kind of connection setup we want in these cases, instead of assuming one
> or the other is preferred? I'm thinking in a new key/par for CreateBearer() and
> Simple.Connect(). Something like "connection-type" (3GGP or 3GPP2). In the new
> MM, if a modem has both 3GPP2 and 3GPP caps (whether it's LTE or not), 3GPP
> connection setup is preferred IIRC. Another option is to base the decision on
> the input parameters we get, e.g. if APN is given, always launch 3GPP
> connection logic, otherwise CDMA connection logic.

Yeah, we should do this.  Remember the "default subscription APN" though, which the operator will use if you don't specify one.  More operators are starting to support it, though last I knew it was not a majority.  So we may want to start a 3GPP bearer with an APN value of "".  But that still works with your idea of doing 3GPP if APN is given, CDMA otherwise.

I think doing auto-detect of bearer type is probably best, we have these possibilities:

1) no APN given, modem registered with CDMA => CDMA bearer
2) no APN given, modem registered with 3GPP or LTE => error
3) APN given, modem registered with CDMA => 3GPP bearer, assume LTE could be used if the modem finds it?
4) APN given, modem registered with 3GPP or LTE => 3GPP bearer

The only trick case is #3, where the modem is on a CDMA network but could handover to LTE.  We can also detect whether this is possible by checking whether the modem has eHRPD enabled, if eHRPD is *not* enabled, the modem cannot handoff between CDMA and LTE.

(In reply to comment #2)
> >    mobile: merge NMModemCdma into NMModemOld
> 
> the "merge NMModemCdma" patch also removes nm-modem-gsm.c from POTFILES.in

Grr, I suppose I can change that :)

> create_connect_properties() is a little goofy; it should check for the presence
> of s_cdma first, before creating the properties hash. (I suppose it makes a bit
> more sense after the next patch...)

Yeah, that was the reason.

> The "Otherwise, prefer CDMA" comment in nm-modem-old.c:complete_connection()
> doesn't make sense at this point in the patchset.

Yeah, because I did the whole thing and then split it up, though I'll change this.

> "nm_modem_old_new" :)

That is kinda odd, though I couldn't think of anything other than "old".  I suppose we could use "nm_modem_mm06" if you'd like?

> >     mobile: merge NMModemGsm into NMModemOld
> 
> On failure, stage1_prepare_done() and stage1_enable_done() call
> translate_mm_error() even in the CDMA case, which they didn't before. Is that
> intentional?

Yeah, it shouldn't be a problem, and we should actually try to translate that error if we can even for CDMA.

> >	/* If the modem is only 3GPP and the connection is not 3GPP, error */
> >	if (!(priv->caps & NM_DEVICE_MODEM_CAPABILITY_CDMA_EVDO)) {
> 
> wrong capability

Not quite, that hunk was supposed to return the same error as we used to if we ever support 56k dialup connections in the future.  But I suppose for 56k dialup connections, they'll never work on WWAN modems.  So I'll change that check.  The corresponding CDMA check needs to stay because the device might also have LTE capability, and we need to ensure 3GPP connections work on LTE-capable CDMA devices.

Hm. OK, I was confused by the fact that you're checking CDMA_EVDO in both cases, but on second look, you're checking them differently in the two cases.

Still quite confusing. With a few more macros you could do something like:

  if (IS_CDMA_ONLY (caps) && !valid_cdma)
    return NOT_CDMA;
  else if (IS_GSM_ONLY (caps) && !valid_gsm)
    return NOT_GSM;
  else
    return TRUE;

I just got rid of all the confusion, re-pushed the branch with the changes.

If the device is multi-mode, and the connection failed the CDMA checks, and failed the GSM checks, then it doesn't matter a ton what error we return here, because it can't be used with this modem anyway.  So I just killed that part and did:

/* Validate 3GPP */
if (priv->caps & CAPS_3GPP) {
	if (valid_gsm)
		return TRUE;

	g_set_error (error, NM_MODEM_ERROR, NM_MODEM_ERROR_CONNECTION_NOT_GSM,
		         "The connection was not a GSM/UMTS/LTE connection.");
	return FALSE;
}

That could result in the NOT_GSM error being returned if the modem is multi-mode and the connection has neither GSM or CDMA settings, but we don't really care that much I guess.

Does the re-push look OK?

I'm attaching a couple of patches to this bugreport, which can be applied on top of dcbw/old-mm-consolidate...

Created attachment 245901 [details] [review]
Fix setting current caps

Not sure how this one got missing before...

Created attachment 245902 [details] [review]
Consolidate loading capabilities

Also, I've been looking at how to deprecate 'ModemCapabilities', but not sure what to do. The property is also exposed via the NMDeviceModem in libnm-glib, so part of the NM API... should I just mark the property & getter method as deprecated, and internally make ModemCapabilities=0? Or ModemCapabilities=CurrentCapabilities? Or maybe just don't touch the property as it doesn't really harm?

(In reply to comment #6)
> Does the re-push look OK?

looks good to me

(In reply to comment #10)
> Also, I've been looking at how to deprecate 'ModemCapabilities', but not sure
> what to do. The property is also exposed via the NMDeviceModem in libnm-glib,
> so part of the NM API... should I just mark the property & getter method as
> deprecated, and internally make ModemCapabilities=0? Or
> ModemCapabilities=CurrentCapabilities? Or maybe just don't touch the property
> as it doesn't really harm?

Hmm, is there harm in leaving it alone since it's just informational and it can't be changed through NM?

Merged to master wtih Aleksander's two patches (fixed for a few whitespace issues).

(In reply to comment #12)
> (In reply to comment #10)
> > Also, I've been looking at how to deprecate 'ModemCapabilities', but not sure
> > what to do. The property is also exposed via the NMDeviceModem in libnm-glib,
> > so part of the NM API... should I just mark the property & getter method as
> > deprecated, and internally make ModemCapabilities=0? Or
> > ModemCapabilities=CurrentCapabilities? Or maybe just don't touch the property
> > as it doesn't really harm?
> 
> Hmm, is there harm in leaving it alone since it's just informational and it
> can't be changed through NM?

Shouldn't harm no, so let's leave it for now.