926372 - B2G DSDS: setup data call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug was originally cloned from Bug 922584.

We need to have some modifications for data call setting in multisim scenario. 

In the long term, it looks right we introduce a new API, maybe ConnectionManager, to take care all kinds of connections, such as 3G, Wifi. And then have a function e.g. setupDataCall(serviceId) there. See Bug 904514.

However, we need to think about another acceptable solution for v1.3 scope.

Per discussion with Ken, here's a proposal for v1.3 that no settings or API changes are needed.

Proposal: RIL still listens to 'ril.data.enabled' to control data connection. If user is willing to enable a data call, then RIL code should use the user-specified default service to setup up the data call.

In the meanwhile, we could keep discussing a long-term solution in bug 922584, such as creating a new WebAPI, ConnectionManager maybe, to manages all kinds of connections.

How does this sound?

Comment 1

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> This bug was originally cloned from Bug 922584.
> 
> We need to have some modifications for data call setting in multisim
> scenario. 
> 
> In the long term, it looks right we introduce a new API, maybe
> ConnectionManager, to take care all kinds of connections, such as 3G, Wifi.
> And then have a function e.g. setupDataCall(serviceId) there. See Bug 904514.
> 
> However, we need to think about another acceptable solution for v1.3 scope.
> 
> Per discussion with Ken, here's a proposal for v1.3 that no settings or API
> changes are needed.
> 
> Proposal: RIL still listens to 'ril.data.enabled' to control data
> connection. If user is willing to enable a data call, then RIL code should
> use the user-specified default service to setup up the data call.
> 
> In the meanwhile, we could keep discussing a long-term solution in bug
> 922584, such as creating a new WebAPI, ConnectionManager maybe, to manages
> all kinds of connections.
> 
> How does this sound?

So we are not using the method introduced in Bug 832808, RIL will just find the default service and setup data call with that service, is that correct?

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #1)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> > This bug was originally cloned from Bug 922584.
> > 
> > We need to have some modifications for data call setting in multisim
> > scenario. 
> > 
> > In the long term, it looks right we introduce a new API, maybe
> > ConnectionManager, to take care all kinds of connections, such as 3G, Wifi.
> > And then have a function e.g. setupDataCall(serviceId) there. See Bug 904514.
> > 
> > However, we need to think about another acceptable solution for v1.3 scope.
> > 
> > Per discussion with Ken, here's a proposal for v1.3 that no settings or API
> > changes are needed.
> > 
> > Proposal: RIL still listens to 'ril.data.enabled' to control data
> > connection. If user is willing to enable a data call, then RIL code should
> > use the user-specified default service to setup up the data call.
> > 
> > In the meanwhile, we could keep discussing a long-term solution in bug
> > 922584, such as creating a new WebAPI, ConnectionManager maybe, to manages
> > all kinds of connections.
> > 
> > How does this sound?
> 
> So we are not using the method introduced in Bug 832808, RIL will just find
> the default service and setup data call with that service, is that correct?

Not exactly, bug 832808 manages APN settings per sim. My proposal in comment 0 still bases on that work. But we just don't create a new setting key to indicate via which 'sim card' the data connection should be on. We only refer to the default one. Make sense?

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #2)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #1)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> > > This bug was originally cloned from Bug 922584.
> > > 
> > > We need to have some modifications for data call setting in multisim
> > > scenario. 
> > > 
> > > In the long term, it looks right we introduce a new API, maybe
> > > ConnectionManager, to take care all kinds of connections, such as 3G, Wifi.
> > > And then have a function e.g. setupDataCall(serviceId) there. See Bug 904514.
> > > 
> > > However, we need to think about another acceptable solution for v1.3 scope.
> > > 
> > > Per discussion with Ken, here's a proposal for v1.3 that no settings or API
> > > changes are needed.
> > > 
> > > Proposal: RIL still listens to 'ril.data.enabled' to control data
> > > connection. If user is willing to enable a data call, then RIL code should
> > > use the user-specified default service to setup up the data call.
> > > 
> > > In the meanwhile, we could keep discussing a long-term solution in bug
> > > 922584, such as creating a new WebAPI, ConnectionManager maybe, to manages
> > > all kinds of connections.
> > > 
> > > How does this sound?
> > 
> > So we are not using the method introduced in Bug 832808, RIL will just find
> > the default service and setup data call with that service, is that correct?
> 
> Not exactly, bug 832808 manages APN settings per sim. My proposal in comment
> 0 still bases on that work. But we just don't create a new setting key to
> indicate via which 'sim card' the data connection should be on. We only
> refer to the default one. Make sense?

Oh, my bad. You are right bug 832808 makes the platform ready already. If that's the case, then I think adding a setting entry array could help minimize developers effort.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Bug 832808 has made DSDS data connection ready on gecko. In the long term, we are considering Bug 922584 as the right way. However, in the short term, we still need gaia to send out settings value to gecko, indicating which sim card the data connection would be built upon. And this bug is for the short term solution -- gaia team setting |ril.data.enabled| value as an array for multisim.

In DSDS, if we want to use SIM0 to set up data call, the settings key and result would be:
[ril.data.enabled, [true, false]]. 
If we want to use SIM1 to set up data call, then
[ril.data.enabled, [false, true]].

And please note that we can set up only *one* data call at a time, so there could be only one *true* in the array.

Comment 5

[Ken Chang[:kenkai]]

Arthur, I wonder if you could take this bug.

Comment 6

[Kai-Chih Hu [:khu]]

Arthur, may I know what's the target milestone in your mind for this bug? Thanks.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Stealing!

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)

> In DSDS, if we want to use SIM0 to set up data call, the settings key and
> result would be:
> [ril.data.enabled, [true, false]]. 
> If we want to use SIM1 to set up data call, then
> [ril.data.enabled, [false, true]].

IHMO this change can be avoided. This change could lead to errors. I am aware that gecko is ready for this approach but I wanted to share my thoughts. See below please.

> And please note that we can set up only *one* data call at a time, so there
> could be only one *true* in the array.

If we can only set up a data call at a time IMHO it makes no sense to have an array for `ril.data.enabled` values corresponding to the two different ICC cards. If we already have a `dom.telephony.defaultServiceId` for the ICC card we use for data calls (assuming `dom.telephony.defaultServiceId` let us know the ICC card for MO data calls) this setting could be used in the RIL plumbing to figure out what ICC card must be used when the user sets `ril.data.enabled` to true. What do you think?

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> 
> > In DSDS, if we want to use SIM0 to set up data call, the settings key and
> > result would be:
> > [ril.data.enabled, [true, false]]. 
> > If we want to use SIM1 to set up data call, then
> > [ril.data.enabled, [false, true]].
> 
> IHMO this change can be avoided. This change could lead to errors. I am
> aware that gecko is ready for this approach but I wanted to share my
> thoughts. See below please.
> 

Thanks for the comment, jaoo :)

> > And please note that we can set up only *one* data call at a time, so there
> > could be only one *true* in the array.
> 
> If we can only set up a data call at a time IMHO it makes no sense to have
> an array for `ril.data.enabled` values corresponding to the two different
> ICC cards. If we already have a `dom.telephony.defaultServiceId` for the ICC
> card we use for data calls (assuming `dom.telephony.defaultServiceId` let us
> know the ICC card for MO data calls) this setting could be used in the RIL
> plumbing to figure out what ICC card must be used when the user sets
> `ril.data.enabled` to true. What do you think?

Your comment is exactly the one I originally had, see comment #0. And I wanted to have an easy and reasonable solution for v1.3, so I had a second proposal, i.e. array value. However, the following user story makes me change my mind again that your proposal here (and my original idea) is indeed better. :)

Confirmed by UX that we'd need to take care of the situation user changes the data primary sim when data connection on sim1 is on. That is, when sim1 detects the fact that default sim changes to sim2, sim1 needs to disconnect data; for sim2, it needs to set up data call when the previous one is disconnected. 

However, the disadvantage of using settings to control data enabling is... gaia cannot correctly tell whether the data connection upon sim1 has been successfully turned off. That said, gaia would have no idea when is the time to set up datacall upon sim2. 

In consideration of this critical user story and your concern, I agree that we should resolve this issue based on your proposal. 

In short, we need to add one more settings key 'ril.data.defaultServiceId' and in RIL, whenever 'ril.data.enabled' and 'ril.data.defaultServiceId' change, each RadioInterface object needs to check if itself is the chosen default one.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Jose, from Hsinyi's comment 9, changes need to be made on gecko side now. Are you going to take this or should I start working on this? Thanks.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Need info for Comment 10.

Comment 12

[Ken Chang[:kenkai]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> Need info for Comment 10.
Jessica, you are supposed to handle the modification for gecko side and please file a bug for this. Thanks! :)

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #10)

Sorry for the being late commenting here.

Well, I fully agree Hsin-Yi's reply in comment #9.

> Jose, from Hsinyi's comment 9, changes need to be made on gecko side now.
> Are you going to take this or should I start working on this? Thanks.

I'm afraid I'm too busy with other stuff and cannot take care of gecko side changes. Go for it if you want. We can track this work here in this bug and we will take care of changes in Gaia side in bug 928297 if needed. Does it sounds good to you? Thanks.

Comment 14

[Kai-Chih Hu [:khu]]

Jessica, can you help on this bug? Thanks.

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Kevin Hu [:khu] from comment #14)
> Jessica, can you help on this bug? Thanks.

Yes, it's mine. :)
Sorry for the late feedback.

Comment 16

[Kai-Chih Hu [:khu]]

Since all depended 1.3+ bugs are all resolved, I think we can close this bug now, and leave the 1.4 bugs opened. Thank you!

Comment 17

[Ken Chang[:kenkai]]

(In reply to Kevin Hu [:khu] from comment #16)
> Since all depended 1.3+ bugs are all resolved, I think we can close this bug
> now, and leave the 1.4 bugs opened. Thank you!
Kevin, can you please make sure if this bug is what you want to close.

Comment 18

[Kai-Chih Hu [:khu]]

(In reply to Ken Chang from comment #17)
> (In reply to Kevin Hu [:khu] from comment #16)
> > Since all depended 1.3+ bugs are all resolved, I think we can close this bug
> > now, and leave the 1.4 bugs opened. Thank you!
> Kevin, can you please make sure if this bug is what you want to close.

Looks like bug 933203 is still under discussion. Open this bug for now, until we have a conclusion on bug 933203. Thanks.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch.

We handle DSDS data call in the following way:
RadioInterface(s) will listen to "ril.data.defaultServiceId" setting changes. When "ril.data.defaultServiceId" value changes, each RadioInterface will check if it is the one switch to or switch away from. In the latter case, RadioInterface will deactivate its data call if it is connected; in the former case, RadioInterface will wait for an active data call to be disconnected or setup data call right away.
When "ril.data.enabled" value changes, each RadioInterface will act only if it is the default one.

Comment 20

[Ken Chang[:kenkai]]

Sorry to jump in. We have reserved the "default" type in apnSettings for this scenario. Should we create another method for this?
In the original multi-SIM design, we can only set a APN as "default" type for specific SIM. That means we can establish default data call on this specific SIM and we don't need to do any change in radiointerfacelayer.js

Comment 21

[Ken Chang[:kenkai]]

(In reply to Ken Chang from comment #20)
> Sorry to jump in. We have reserved the "default" type in apnSettings for
> this scenario. Should we create another method for this?
> In the original multi-SIM design, we can only set a APN as "default" type
> for specific SIM. That means we can establish default data call on this
> specific SIM and we don't need to do any change in radiointerfacelayer.js
Per discussing with Hsinyi, this method isn't suit for current user story. Please ignore this comment.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 830748 [details] [diff] [review]
proposed patch.

Review of attachment 830748 [details] [diff] [review]:
-----------------------------------------------------------------

It looks really good.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2264,5 @@
>            // effect immediately before the setting get enabled.
>            if (this._sntp.isExpired()) {
>              this._sntp.request();
>            }
>          }

nit: add an extra line.

@@ +2268,5 @@
>          }
> +        // DSDS: setup pending data connection when switching the default id
> +        // for data call.
> +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,
> +        // cause the type is removed from TILNetworkInterface.connectedTypes

nit: s/TILNetworkInterface/RILNetworkInterface/

@@ +2417,5 @@
> +            break;
> +          }
> +          this.updateRILNetworkInterface();
> +          break;
> +        }

nit: add an extra line.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v2.

- Add "handleDataClientIdChange()" function for readability
- Fire "network-interface-state-changed" notification before unregistering network interface
- Bail out if "ril.data.enabled" value has not changed
(I think this is a regression.. when disabling airplane mode, we are receiving two "ril.data.enabled" with the same value, which makes dataCallSettings.oldEnabled and dataCallSettings.enabled the same value, then after PS is registered, data is not reconnected cause there is no change)
- Address review comment 22: add extra lines and fix typo

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: [debug] test case

This is just for reference, the test case works only in emulator-jb.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 832828 [details] [diff] [review]
proposed patch, v2.

Looks great! Thanks for the test case which is awesome :)

Comment 26

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> Comment on attachment 832828 [details] [diff] [review]
> proposed patch, v2.
> 
> Looks great! Thanks for the test case which is awesome :)

Thank you Hsinyi!
Are we going to land this or are we going to wait until we have tested on real devices?

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> > Comment on attachment 832828 [details] [diff] [review]
> > proposed patch, v2.
> > 
> > Looks great! Thanks for the test case which is awesome :)
> 
> Thank you Hsinyi!
> Are we going to land this or are we going to wait until we have tested on
> real devices?

I tested this with emulator and did some hack to test more. The test result works as expected. I think we could let go! Thanks.

Comment 28

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #27)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> > > Comment on attachment 832828 [details] [diff] [review]
> > > proposed patch, v2.
> > > 
> > > Looks great! Thanks for the test case which is awesome :)
> > 
> > Thank you Hsinyi!
> > Are we going to land this or are we going to wait until we have tested on
> > real devices?
> 
> I tested this with emulator and did some hack to test more. The test result
> works as expected. I think we could let go! Thanks.

Got it, thanks!

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #28)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #27)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> > > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> > > > Comment on attachment 832828 [details] [diff] [review]
> > > > proposed patch, v2.
> > > > 
> > > > Looks great! Thanks for the test case which is awesome :)
> > > 
> > > Thank you Hsinyi!
> > > Are we going to land this or are we going to wait until we have tested on
> > > real devices?
> > 
> > I tested this with emulator and did some hack to test more. The test result
> > works as expected. I think we could let go! Thanks.
> 
> Got it, thanks!

The patch (v2) doesn't apply cleanly. Could you please provide a newly rebased one? Thanks!

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: final patch. r=hsinyi

Rebased and add r=hsinyi.

Let's wait till the try results are out:
https://tbpl.mozilla.org/?tree=Try&rev=0901310673be

Comment 31

[Jessica Jong [:jessica] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=0901310673be

All green! \o/

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9e6d13b8283b

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e6d13b8283b