Closed Bug 842252 Opened 11 years ago Closed 11 years ago

[B2G][Setting] Change data call setting architecture in Gaia.

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: kchang, Assigned: johnhu)

References

Details

(Whiteboard: [TAIPEI_FND_TRACKING])

Attachments

(1 file)

pull request for 842252
86 bytes, text/html
arthurcc
: review+
fcampo
: review+
jaoo
: review+
Details 
For getting all necessary data of data call, RIL is changing the data call setting architecture. Gaia also have to do corresponding modifications.
We add a new key, "ril.data.apnSetting",to contain all information making data call need. Here is the example for data format,
lock.set("ril.data.apnSettings",
          {apnSetting0: 
            {apn: "Internet", 
             user: "", 
             passwd: "", 
             httpProxyHost: "192.168.0.1", 
             httpProxyPort: "8080", 
             types: "data"
            },
           apnSetting1: 
            {apn: "emome", 
             user: "emome", 
             passwd: "emome", 
             httpProxyHost: "192.168.0.1", 
             httpProxyPort: "8080", 
             types: "mms,supl",
            }
          }, null);
No longer blocks: 837488
Depends on: 837488
We need to change the data format for reusing this design in multi-SIM fuction.
{"0": {apnSetting0: {apn: "Internet", 
                     user: "", 
                     passwd: "", 
                     httpProxyHost: "", 
                     httpProxyPort: "", 
                     type: ["default"],
                     },
       apnSetting1: {apn: "emome", 
                     user: "", 
                     passwd: "", 
                     httpProxyHost: "", 
                     httpProxyPort: "", 
                     type: ["mms","supl"],
                     }}}
Depends on: 850555
No longer depends on: 837488
After discussing with Vicamo, the following is the new format. 
[ // It is for sim1.
  [ 
    {
       apn: "internet", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["default"],
    },
    {
       apn: "emome", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["mms","supl"],
    }
  ],
  // It is for sim2.
  [ 
    {
       apn: "internet", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["dun"],
    },
    {
       apn: "fetne01", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["mms","supl","default"],
    }
  ]
]
Whiteboard: [TAIPEI_FND_TRACKING]
Arthur,

Could you take this bug or mentoring John to work on it?
Assignee: schung → arthur.chen
Flags: needinfo?(arthur.chen)
I'm not familiar with this part. I'll do some surveys to see if it is okay for John to work on it.
Flags: needinfo?(arthur.chen)
Just a waring here. If we are changing the data call settings architecture we must change the way of getting the APN settings from the `apn.json` database through the operator variant code (see [1]).

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant/operator_variant.js
Blocks: 871442
Assignee: arthur.chen → johu
1. add new APN Settings and keep old settings
2. make settings app save setting in both new and old version. old version is for backward compatible.
Attachment #749777 - Flags: review?(arthur.chen)
Comment on attachment 749777 [details]
pull request for 842252

Fernando, please help review the changes made on FTU.
Jose, please help review operator_variants.js.

Thanks!
Attachment #749777 - Flags: review?(josea.olivera)
Attachment #749777 - Flags: review?(fernando.campo)
No longer blocks: 871442
Comment on attachment 749777 [details]
pull request for 842252

Good patch, John! Thanks! r=me.
Attachment #749777 - Flags: review?(arthur.chen) → review+
Comment on attachment 749777 [details]
pull request for 842252

See comments in github please. I'd like to receive the review request again once the review comments get addressed. Thanks!
Attachment #749777 - Flags: review?(josea.olivera)
Comment on attachment 749777 [details]
pull request for 842252

jaoo, 

I had change the implementation on operator_variant.js. Please take a look at.
Attachment #749777 - Flags: review?(josea.olivera)
Comment on attachment 749777 [details]
pull request for 842252

See comments in github please. I'd like to receive the review request again once the review comments get addressed. Thanks!
Attachment #749777 - Flags: review?(josea.olivera)
Jose, 

To replying your first comment:

1. why just first simcard: 
This is because the UX and underlying API only supports one simcard currently. That's the reason we only put APN settings for first simcard. We will open another bug for changing single simcard to multiple simcard. But the APN settings architecture will still be the same.

2. why not mmsc, mmsproxy, mmsport
For the view of ril, they shares the same settings if APN name is the same, and mmsc, mmsproxy, mmsport is only for MMS service, not for ril layer.
Flags: needinfo?(josea.olivera)
Comment on attachment 749777 [details]
pull request for 842252

Sorry this one fell off my list. 

Changes on FTU look fine, double thanks for remembering about the tests :D
Attachment #749777 - Flags: review?(fernando.campo) → review+
Ken, 

I need your opinions on pull request discuss of https://github.com/mozilla-b2g/gaia/pull/9784

Thanks.
Flags: needinfo?(kchang)
Hey guys, let's discuss a bit about what to do in Gaia right now about this bug. The new APN settings architecture (I really like Ken's idea about how that architecture should be) will be needed once we have multi-SIM devices and the RIL plumbing bits
get ready. Moreover (AFAIK) we don't have yet the corresponding UX design to deal with the multi-SIM device support. I guess you guys want to land bug 850555 ASAP but IHMO we should try to avoid -as much as possible- changes in Gaia that are not currently needed. At the moment the old APN settings architecture, as I have seen in the patch in this bug, coexists with the new one and IHMO we should avoid that as well.

If you guys really want to move forward with this bug and land this bug that's fine I just wanted to expose my thoughts here.

Jonh, thanks for the patch. I did quickly a WIP patch [1] for this bug to see if something much simplier was possible. It might need some improvements/corrections. Feel free to take a look and use here what you need.

[1] https://github.com/jaoo/gaia/commit/6141dd3dd33918bf7d46670ba42dd42883764674
Flags: needinfo?(josea.olivera)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15)
> IHMO we should try to avoid -as much as possible- changes in
> Gaia that are not currently needed. At the moment the old APN settings
> architecture, as I have seen in the patch in this bug, coexists with the new
> one and IHMO we should avoid that as well.

I second that. And to be completely honest, I’m a bit surprised that the module owners haven’t been involved in the discussion for such a big change. The current APN architecture has proven its efficiency (we had a lot of issues with the former one…) and I’d be relieved if we could keep it as long as possible — e.g. until we get the full picture of the multi-SIM use case.

Can we please have a closer look at Jaoo’s approach? He’s familiar with the code base, he has a very good view of the whole APN picture (both back-end and front-end) and he’s very aware of what we need to be able to fetch all the data from the existing databases (Gnome + Android + custom data).
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15)
(In reply to Fabien Cazenave [:kaze] from comment #16)
Hi, thanks for your opinions. This architecture isn't only for multi-sim(bug 850555) but also for bug 837488 (shared APN settings). We had found some coexistence problems in shared APN settings(For example, data and MMS use the same APN. Can we send MMS when data option isn't enable?) in old design and done a lot changes in data call architecture of RIL(bug 837488) to solve those problems. However, the new apn settings is not ready in Gaia now, we land those patches. That is why we are eager to land this patch. Although the UX isn't ready now, we still can be based on current UX design to have those fixes. Moreover, if we keep waiting for UX being ready, we will shorten the development and verification time. And you can also check the apn.json, the new architecture of apnSettings is very similar with apn.json. I think this change are in the right direction.
Indeed, we should have a good communication with module owner.(I didn't know the the owner of Gaia data call setting before, but I know now.) And I hope that it isn't too late to communicate it now.
Flags: needinfo?(kchang)
(In reply to Ken Chang from comment #17)
Oops! Correct sentence.
> those problems. However, the new apn settings is not ready in Gaia now, we
> land those patches. That is why we are eager to land this patch. Although
those problems. However, the new apn settings is not ready in Gaia now, we
cannot land those patches. That is why we are eager to land this patch. Although
Jose, 

IMHO, I had put some comments on your pull request. Most of them are the reason I use different approach. Thanks for your patch.
Jaoo, 

I had modified a new pull request based on your opinions. Please help me check if there is anything improper.
Ken, 

Is it possible to change the name of apn seting fields to be similar to apn.json file?? like:

    {
       apn: "internet", 
       user: "", 
       password: "", 
       proxy: "", 
       port: "", 
       types: ["default"],
    } 

That will make things simpler
Flags: needinfo?(kchang)
(In reply to John Hu [:johnhu] from comment #21)
> Ken, 
> 
> Is it possible to change the name of apn seting fields to be similar to
> apn.json file?? like:
> 
>     {
>        apn: "internet", 
>        user: "", 
>        password: "", 
>        proxy: "", 
>        port: "", 
>        types: ["default"],
>     } 
> 
> That will make things simpler
Yes, you can do that.
Flags: needinfo?(kchang)
Sorry, another change is "types" to type.


    {
       apn: "internet", 
       user: "", 
       password: "", 
       proxy: "", 
       port: "", 
       type: ["default"],
    }
John, I left some comments in the PR review. A couple of things First one, do not override the APN settings in the fields when the user enters the subpanel please, leave -do not change- the settings in the fields and the preselected APN in the subpanel. Second one, you must continue handling the settings under the `ril.mms.{mmsc, mmsproxy, mmsport}` keys. If the user choose a different APN for MMS in the Setting app you must update the values under the `ril.mms.{mmsc, mmsproxy, mmsport}` keys. If you don't do that the mmsc, mmsproxy, mmsport settings retrieved by the MMS plumbings would be the ones set by the operator variant logic on boot. You are only updating the values for the
`ril.{data,mms,supl}.{carrier,apn,user,passwd,httpProxyHost,httpProxyPort}` keys.

The overall approach would be to save under the `ril.data.apnSettings` key the APN settings for the one that the user selects in the setting app according to the new APN settings architecture and let the RIL uses those settings to set up the data call.

I've applied both patches (bugs 850555 and 842252) It seems that It doesn't work correctly. The value of `ril.data.apnSettings` sometimes's wrong because It is not correctly updated in the setting app.

Btw, are you planning guys to uplift bug 850555 and this one to v1-train gaia branch?
Jaoo, I had read your comments. Thanks for that. I already change the patch according to your comments: 1. add the handling of mmsc, mmsproxy, mmsport, and carrier, 2. not remove the override code on subpanel.

BTW, if you applied the patch after 2013-05-23 04:00:31 PDT, I had changed the setting name according your comments. So, you may find "apnSetting.types is undefined" in console and nothing works. I will sync with Ken to have the correct path for both.
(In reply to John Hu [:johnhu] from comment #23)
> Sorry, another change is "types" to type.
> 
> 
>     {
>        apn: "internet", 
>        user: "", 
>        password: "", 
>        proxy: "", 
>        port: "", 
>        type: ["default"],
>     }

Note, in backend implementation, we already use 'types.' I prefer |types| to |type| since it can be plural. But if 'types' indeed brings some difficult in gaia implementation, then please kindly make sure the consistency.
We want to use this kind of settings just because apn.json use the same field name. If you guys prefer types instead of type, I can make the change from type to types (I had made the change to this pull request). The type field also need to be processed for preventing duplication type in the same APN settings, so it may not be difficult to us.
(In reply to John Hu [:johnhu] from comment #27)
> We want to use this kind of settings just because apn.json use the same
> field name. If you guys prefer types instead of type, I can make the change
> from type to types (I had made the change to this pull request). The type
> field also need to be processed for preventing duplication type in the same
> APN settings, so it may not be difficult to us.
Please use the |types|. thanks
blocking-b2g: --- → leo?
Comment on attachment 749777 [details]
pull request for 842252

Jose, 

I had changed the PR with your suggestion code. Thanks for that. Please review it again.
Attachment #749777 - Flags: review?(josea.olivera)
Triage - leo-'ing as this does not seem to be a required change for 1.1's feature set.
Leo, can you renominate with reasons if you believe this is required?
blocking-b2g: leo? → ---
(In reply to John Hu [:johnhu] from comment #29)
> Comment on attachment 749777 [details]
> pull request for 842252
> 
> Jose, 
> 
> I had changed the PR with your suggestion code. Thanks for that. Please
> review it again.

Jonh, Ken, see my comments on github please. We are almost there.
As jaoo pinged me on github to raise a suggestion, let’s discuss it here.

Quick sum-up: we currently have one APN set per connection type (data, mms, supl), which is not optimal in the (frequent) cases where the same APN is used for several connection types -- see bug 837488.  A new data architecture is proposed to solve this (bug 850555): the point is to merge APN sets that share the same apn/username/password.

Now the question is, where should we merge these APNs? This bug thinks of merging APNs in Gaia; but could we merge this APNs in Gecko instead?  It would allow us to keep Gaia and the settings DB unchanged: no Gaia/Gecko interdependency, and we won’t break the 3G during the transition.  (FTR, that’s what Android does)

IMHO, doing the merge in Gaia without changing the UI does not make much sense: we’d still keep the user-facing complexity of having separate APNs, and for developers the Settings app is easier to deal with when the UI reflects the setting names closely.

Long story short:
 • if the Settings *UI* can be simplified by merging these APNs sets on the Gaia side, I think it’s worth waiting for some UX wireframes (including the multi-SIM case) before modifying Gaia;
 • if we can’t wait for UX wireframes, I’d prefer to merge the APNs on the Gecko side.

Vicamo, what do you think?

(pinging UX for the wireframes…)
Flags: needinfo?(fdjabri)
Handing the need info over to Neo in Taipei as he is now the UX owner for this area.
Flags: needinfo?(fdjabri) → needinfo?(nhsieh)
Hi Everyone, 
As far as I know, There are 7 types APN setting in the world. Refer to iPhone and Android APN settings, I saw they merge APN settings and change them in a flexible way. Means If the operator has separated APN to MMS and Internet, System will show 2 items. And If the operator has merged APNS, System will show 1 items. 
 
I think at v1.1 we don't need to change UI or UX right now. Since Ken will merge them from Gecko layer. I think that is a good start. I will discuss this question with more UX designers for future version.
Flags: needinfo?(nhsieh)
Comment on attachment 749777 [details]
pull request for 842252

Please review this PR. Thanks.
Comment on attachment 749777 [details]
pull request for 842252

Thanks John, please see my comments in github. Some changes are still needed, nothing difficult. Thanks.
Attachment #749777 - Flags: review?(josea.olivera) → review-
Jose,

I had changed them in this PR. And some of these changes may not be needed, like type and types. I also replied your comment in this PR. Let us discuss them before next review. Thanks.
(In reply to John Hu [:johnhu] from comment #37)
> Jose,
> 
> I had changed them in this PR. And some of these changes may not be needed,
> like type and types. I also replied your comment in this PR. Let us discuss
> them before next review. Thanks.

John, see the discussion on github. You were right about all my comments. I'm wondering if it could be possible to update bug 850555 to check whether everything is right before giving the formal r+ flags here. So ni? to Ken.
Flags: needinfo?(kchang)
Jose, 

Sure, it's a nice step to do it. I will love to wait your feedback after applying both patches.
Ideally, changing the data call setting should get us rid of the kind of hack we did in bug 882095.
This bug blocks a leo+ bug, bug 837488.
blocking-b2g: --- → leo?
Depends on: 837488
Flags: needinfo?(kchang)
Jose, the 837488 is r+ and I had verified the John's patch.
(In reply to Ken Chang from comment #42)
> Jose, the 837488 is r+ and I had verified the John's patch.

I'll test John's patch on top of 837488 applied in birch. I'll keep you updated. Thanks.
John, the patch does not apply on top of latest gaia master branch. I'll rebase it for my test.
(In reply to Ken Chang from comment #42)
> Jose, the 837488 is r+ and I had verified the John's patch.

Yep, Seems to work correctly. Let's wait John to rebase the patch and ask for review at me again. I'll be happy to give r=me. Thanks for your work in bug 837488.
Jose, 

I had rebase and update the patch, please review it. 

I found there is a conflict with bug 882059. It says those codes need to be refined after this bug is landed. I had read the bug 882059. Do I need to do it in this patch, or we should open another bug for it??
(In reply to John Hu [:johnhu] from comment #46)
> I found there is a conflict with bug 882059. It says those codes need to be
> refined after this bug is landed. I had read the bug 882059. Do I need to do
> it in this patch, or we should open another bug for it??

Please, file another bug for a follow-up for bug 882059. Thanks!
Bug 887659 is create for a follow-up for bug 882095.
Comment on attachment 749777 [details]
pull request for 842252

Change the flag to r?
Attachment #749777 - Flags: review- → review?(josea.olivera)
Comment on attachment 749777 [details]
pull request for 842252

r=me with the comments in the PR. I guess this could land right now even without waiting bug 837488 to land.

John, thanks for your work and my apologizes for the long discussion and so many changes in the code.
Attachment #749777 - Flags: review?(josea.olivera) → review+
Thanks, Jose. never mind it. This process makes me understand APN more.

landed to master: https://github.com/mozilla-b2g/gaia/commit/ecc78307f2d2dc51dd207af0534cac26347999be
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
The bug 837488 isn't r+ bug, so this bug is also not r+ bug. I remove the leo?.
blocking-b2g: leo? → ---
(In reply to Ken Chang from comment #52)
> The bug 837488 isn't r+ bug, so this bug is also not r+ bug. I remove the
> leo?.

Oops! Typo, what I mean is, "the bug 837488 isn't leo+ bug, so this bug, 842252, is also not leo+ bug."
The reason of this bugs is nominated as leo+ is because the bug 837488 is a leo+ bug.
But the bug 837488 isn't leo+ bug, so this bug, 842252, is also not leo+ bug. 
And this bug is a improvement for V1.2 or later version, I don't think we should include it now.
That is why I remove leo+ from this bug.
(In reply to Ken Chang from comment #53)
> (In reply to Ken Chang from comment #52)
> Oops! Typo, what I mean is, "the bug 837488 isn't leo+ bug, so this bug,
> 842252, is also not leo+ bug."

Ken, John, do you think we should back out the patch landed for this bug in Gaia until bug 837488 lands?
blocking-b2g: --- → leo?
Flags: needinfo?(kchang)
Flags: needinfo?(johu)
blocking-b2g: leo? → ---
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #54)
> Ken, John, do you think we should back out the patch landed for this bug in
> Gaia until bug 837488 lands?
If Gaia still send the old apn settings to RIL, we won't back out the patch.
Flags: needinfo?(kchang)
If we have any side effects on this bug, we should do it. I don't know if we have it or not...
Flags: needinfo?(johu)
The apnSettings = [[]] when I used this patch to do the test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry about this case.

I had tested with known simcards, like CHT, TWM, etc. This case only occurs when simcard has unknown carrier. We use apn.json file to build default settings. If carrier is not known by apn.json, the apnSettings will be [[]].

Because this patch is landed, I will create another patch for it.
According to comment 58, close this bug.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Blocks: 890862
Blocks: 908564
Blocks: 901227
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: