Add support for Qualcomm's device.

[peat-psuwit]

This add some essential code for oFono to work with LG L90 Dual (and maybe other Qualcomm's multi-sim device) correctly. This code cannot utilize 2nd sim slot yet.

[tonyespy]

Thanks for the pull-request. It's a reasonably well thought out approach.

That said, I have a couple of global comments, as well as some comments on the code itself.

First, our ofono rilmodem code is currently based on AOSP 4.4.2, whereas as some of the new RIL requests added by this pull-request, including the critical SET_UICC_SUBSCRIPTION request have been introduced in AOSP 5. As such, I wonder if it's worth adding a device plugin ( see plugins/mtk.c ), and using a vendor quirk for set subscription behavior, as it will fail on devices running an rild < 5. Using this approach would also allow you to start to build proper support for your device's dual-modem support.

Second, could you please re-format your commit messages to use prefixes ( eg. "include:", "gril:", ... ). Please take a look at some of the existing commits in our repository.

[tonyespy]

GET_DATA_CALL_PROFILE doesn't appear to be a standard request defined by any version of AOSP's ril.h header. If it's a custom OEM request, then it should be added in an OEM-specific header. Also, as it's not used in any of your new code, I would just leave it out of this change.

[tonyespy]

This is only defined in AOSP v5. See my global comment about this. It also might be a good idea to add a comment indicating this ( either RIL version or AOSP version ).

[alfonsosanchezbeato]

Note also that in lollipop the number for this request is 122. See:
http://androidxref.com/5.1.0_r1/xref/hardware/ril/include/telephony/ril.h

[tonyespy]

Neither SET_DATA_SUBS or SIM_TRANSMIT_BASIC appear to be standard requests defined by any version of AOSP's ril.h header. If they are custom OEM requests, then they should be added in an OEM-specific header. Also, as it's not used in any of your new code, I would just leave them out of this change.

[tonyespy]

SIM_OPEN_CHANNEL and SIM_CLOSE_CHANNEL are only defined in AOSP v5. See my global comment about this. It also might be a good idea to add comments indicating this ( either RIL version or AOSP version ).

[tonyespy]

Neither SIM_TRANSMIT_CHANNEL or SIM_GET_ATR appear to be standard requests defined by any version of AOSP's ril.h header. If they are custom OEM requests, then they should be added in an OEM-specific header. Also, as it's not used in any of your new code, I would just leave them out of this change.

[tonyespy]

The closing brace should be on the same line as the 'else'. Ex.

} else {

[tonyespy]

Since -1 is still a possible return value from sim_select_uicc_subscription(), it still can be used an array index, which leads to undefined behavior. We should catch this, log it as an error, and not allow it to be used.

[tonyespy]

First, shouldn't this be RIL_APPTYPE_SIM | USIM? RUIM is used by CDMA phone.

I'm also not sure why you're check app_type twice or why you'd set selected_app in the default case of the second check?

[peat-psuwit]

This is algorithm used in CM [1]. It prefers USIM | RUIM over SIM | CSIM. (That second app_type check replaces selected_app with current one if current selected_app isn't USIM | RUIM) As there isn't CDMA support in oFono yet, it may be make sense to just log that and skip that app.
[1] https://github.com/CyanogenMod/android_frameworks_opt_telephony-msim/blob/cm-11.0/frameworks/src/com/codeaurora/telephony/msim/SubscriptionManager.java#L401-L450

[alfonsosanchezbeato]

A few additional comments on my side:

• As Tony said, some of the requests you have added are not really from AOSP, but Qualcomm-specific. Even more, I have taken a look at latest ril.h in codeaurora (=Qualcomm Android), and those requests are there, but the numbers have changed, probably because they are taking new AOSP requests and they displace theirs when needed. See:
  https://www.codeaurora.org/cgit/quic/la/platform/hardware/ril/tree/include/telephony/ril.h?h=LA.BF64.1.2.1_rb2.7
  So the changes can be said to be specific to your concrete device. The best thing to in this case is to define a vendor specific quirk, say OFONO_RIL_VENDOR_LG_L90_DUAL, and add it to drivers/rilmodem/vendor.h. You need to define then a plugin for your modem, which in this case will be fairly easy as you can use all functions from the ril plugin (you can use plugins/infineon.c as an example).
  One or another driver is loaded depending on environment variables: see plugins/rildev.c and /etc/init/ofono.override and /etc/init/rild.conf upstart jobs on the phone). You can define the requests in a similar way to drivers/infineonmodem/infineon_constants.h or drivers/mtkmodem/mtk_constants.h. And after that you can use a quirk in drivers/rilmodem/sim.c so you send SET_UICC_SUBSCRIPTION only when your plugin is the one being used.
• Do you have a pointer to the ril.h you used for defining the constants? Just curious :-)
• Please take a look to doc/coding-style.txt and format your PR accordingly. oFono uses the kernel coding guidelines as base, for instance: use tabs, tabs == 8 whitespaces, line length < 80, etc.

[peat-psuwit]

For ril.h constants, I just copy them from CM 11's version of ril.h, as that works for my device. I though it's some update from somewhere so it seemed to make sense to include them all. But, as you point out, it may be more appropriate to just include the needed constants only.

[alfonsosanchezbeato]

Just for the record, latest HEAD in CM repo defines

#define RIL_REQUEST_SET_UICC_SUBSCRIPTION 122

(as in AOSP) so as a minimum, using 115 instead of 122 must be a vendor quirk. Note also that most commits to CM's ril.h come from codeaurora guys.

[peat-psuwit]

Ok, then it's probably Qualcomm's quirk in Android 4.4. But if I move the constant to modem-specific header, then where should I put the g_ril_set_uicc_subscription function?

BTW, how should I change the code and re-submit? New pull request?

[alfonsosanchezbeato]

It is fine to implement g_ril_set_uicc_subscription in gril as it is defined by AOSP 5. However, the constant used for the request in g_ril_send should be defined as vendor-specific in a similar way as the constants in mtk_constants.h. And when using it in rilmodem/sim.c you should check the vendor.

You can rebase and resubmit this PR or use a new one, whatever which is easier for you.

[peat-psuwit]

Resubmit this PR? You mean force-pushing new commit into this branch?

[alfonsosanchezbeato]

@peat-psuwit yes, I normally do a "git rebase -i" to have a clean history and then a forced push when re-submitting. But, again, whatever works best for you. You can close this PR and create a new one if you prefer.

[peat-psuwit]

Ok, new version is pushed. I hope I does it right.

[alfonsosanchezbeato]

You created this file so you can (C) it yourself

[alfonsosanchezbeato]

I would add here a callback function to check for message->error and call g_ril_print_response_no_args() (see for instance mtkmodem/voicecall.c:mtk_set_indication_cb()), so we know when the call failed.

[alfonsosanchezbeato]

line > 80

[alfonsosanchezbeato]

You probably need to send SET_UICC_SUBSCRIPTION only once, not after each SIM_STATUS request. I think you could check sim_data::app_index to see if it is initialized or not (it is set by configure_active_app()). You would need to initialize that variable to -1 when sim_data is created.

[peat-psuwit]

I only send that request if search_index is -1 (actually UINT_MAX, which is more than num_apps), not every time the SIM status is replied.

[alfonsosanchezbeato]

Ok, you are right, the modem should send a value different to -1 once the application is selected.

[peat-psuwit]

@tonyespy this line is 2 characters longer then 80 characters. I'm not sure how to cut the word?

[tonyespy]

It's actually only one character over 80, as the trailing line breaks aren't counted.

Take a look at the doc/coding-style.txt, as it has some examples of how this could be corrected.

The simplest would be to just wrap the 2nd parameter:

               search_index = sim_select_uicc_subscription(sim,
                                                             status);

Note - all tabs are used, this puts the wrapped link just at the 80 char limit.

Or, you could wrap after the "=", like this:

                    search_index =
                            sim_select_uicc_subscription(sim, status);

But I think the first is preferable.

[alfonsosanchezbeato]

No double empty lines, please

[alfonsosanchezbeato]

ofono uses only tabs for indenting

[alfonsosanchezbeato]

line > 80

[alfonsosanchezbeato]

Overall, looks pretty good to me. @tonyespy ?

[peat-psuwit]

New version updated with @alfonsosanchezbeato suggestion and rebased to current master.

[tonyespy]

So this looks pretty good to me, although I have a couple of comments:

1. The copyright for plugins/qcom-msim.c is 'Copyright 2014 Canonical', and should be '2015 Ratchanan Srirattanamet' instead.

2. I would also probably add additional copyright statements to rilmodem/sim.c and grilrequest.c.

3. I'm not sure I like the last commit, which says in the commit message "This assumes that Qualcomm's multi-sim device behave similar to AOSP device". Can't this be tested and the assumption confirmed?

[peat-psuwit]

I'll edit copyright for plugins/qcom-msim.c as you suggest. But I'm not so sure if I can add a copyright statement on my rilmodem/sim.c and grilrequest.c's addition. (Isn't there some kind of CLA?)

For the last commit, I forgot to test that because I don't have PIN locking enabled. Let me test that and I'll edit the commit message in the next patchset.

[tonyespy]

I'll double check with Alfonso re: the copyright. AFAIK, there is no CLA for ofono. I think the general rule of thumb is whether or not the patch is considered significant or not.

Also please do test the last commit. We don't like to merge code based on assumptions...

[peat-psuwit]

OK, after a simple checking with my phone with this patch, PIN locking seems to work fine. So I'll definitely edit the commit message. But to reduce clutter on this pull request, I'll wait until I have clarity about copyright, and will re-submit these commit again.

[tonyespy]

There is no CLA, and the change you've submitted is complex enough to warrant an additional copyright statement if you want to add it, but I think the choice is yours.

[peat-psuwit]

Ok, new version uploaded. Copyright statement is added. PIN handling's commit is edited. And the commit is rebased to the new master.

[tonyespy]

I hate to reject this again, but as I was doing a final review I noticed that some of these comments had been moved inside of this if block, and noticed that some of the lines are pretty long, so I checked... And at least fives lines in the first comment, and two in the next comment are all over 80 characters long. Sorry to be a stickler, but we try to stick to the coding style doc ( see /doc/coding-style.txt ).

[peat-psuwit]

As they are indented a lot, I tough they seem to look better when they're a few characters longer. But if that's matter, I'll try to do the word cutting again.

[tonyespy]

Well, the comments had been left in their original position, this wouldn't have been a problem.

The style guide says it's OK to exceed if it makes the code really ugly otherwise, but as we're talking about comment here, these should be fixed.

[alfonsosanchezbeato]

LGTM. @peat-psuwit, thanks a lot for the patch and for your patience while following the review process