1137088 - B2G RIL: move data call related handling out of ril_worker

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

I'd like to move data call related handling out of ril_worker, ril_worker should only be responsible of passing ril request results or unsolicited messages to |RadioInterface|.

My idea is, for setup/deactivate data call, |DataCall| will have callback functions to handle the necessary subsequent actions. And for unsolicited data call list changes, |DataConnectionHandler| will pass the message to each |DataCall| and each |DataCall| should decide what to do (deactivate, update, etc.)

Comment 1

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

In generally speaking, I personal think this is the right direction to go :)

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

I have added callback functions to setupDataCall and deactivateDataCall in |DataCall|, after |DataCall| process it, it will notify |DataConnectionHandler| to do the necessary actions.
For unsolicited data call list change, |DataConnectionHandler| will receive the message first, it will parse the data call list and notify each |DataCall| if necessary.

Edgar, may I have your review on this? Thanks.

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8572988 [details] [diff] [review]
patch, v1.

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

Thanks for doing this, Jessica. Please see my comments below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1337,5 @@
> +    for (let i = 0; i < currentDataCalls.length; i++) {
> +      let currentDataCall = currentDataCalls[i];
> +      if (currentDataCall && currentDataCall.linkInfo.cid != null &&
> +          currentDataCall.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +        this.debug("Expected data call missing, must have been DISCONNECTED.");

if (DEBUG) ...
And also print the details of |currentDataCall|.

@@ +1373,2 @@
>     */
> +  handleDataCallChanged: function(updatedDataCall) {

Could it be merged with |handleDataCallListChanged|?
e.g. update datacall first then check for all data calls are disconnected.

@@ +1379,4 @@
>          this.allDataDisconnected()) {
>        if (gRadioEnabledController.isDeactivatingDataCalls()) {
>          if (DEBUG) {
>            this.debug("All data connections are disconnected.");

s/data connections/data calls/

@@ +2758,5 @@
> +    this.state = dataCall.state;
> +
> +    // Notify DataConnectionHandler about data call connected.
> +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> +    connHandler.handleDataCallChanged(this);

Why it needs to notify connHandler regarding to the datacall is changed, won't it be notified by "datacalllistchanged"?

@@ +2780,5 @@
>      }
>  
> +    // Notify DataConnectionHandler about data call disconnected.
> +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> +    connHandler.handleDataCallChanged(this);

Ditto. Why it needs to notify connHandler regarding to the datacall is changed, won't it be notified by "datacalllistchanged"?

@@ +2809,1 @@
>              this.deactivate();

Why deactivate data call when the interface or address in linkinfo is changed?

@@ +2844,5 @@
> +    this.state = updatedDataCall.state;
> +
> +    // Notify DataConnectionHandler about data call changed.
> +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> +    connHandler.handleDataCallChanged(this);

Ditto. Could handleDataCallChanged and handleDataCallListChanged be merged together?
Then we probably don't need to notify back to connHandler here.

::: dom/system/gonk/ril_worker.js
@@ +5273,5 @@
>    }
> +
> +  let Buf = this.context.Buf;
> +  let version = Buf.readInt32();
> +  let num = Buf.readInt32();

Just add some comments for ignored field, don't have to introduce a unused variable for it.

e.g.

// Version of data call ....
Buf.readInt32();
// Number of data calls...
Buf.readInt32();

@@ +5839,5 @@
>  
>  RilObject.prototype[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST(length, options) {
> +  if (!options.rilMessageType) {
> +    // This is an unsolicited data call list changed.
> +    options.rilMessageType = "datacalllistchanged";

I guess this is for UNSOLICITED_DATA_CALL_LIST_CHANGED, if so how about assigning option.rilMessageType to "datacalllistchanged" in caller?

e.g.
this[REQUEST_DATA_CALL_LIST](length, {
  rilRequestError: ERROR_SUCCESS,
  rilMessageType: "datacalllistchanged"
});

@@ +5846,3 @@
>    if (options.rilRequestError) {
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +    this.sendChromeMessage(options);

It seems not necessary to sendChromeMessage to upper layer for error case, because REQUEST_DATA_CALL_LIST is only triggered inside ril_worker to query and notify data call list.

@@ +6641,5 @@
>    }
>  
>    this.initRILState();
> +  // rild might have restarted, ensure data call list.
> +  this.getDataCallList();

Looks like you use different way to handle data call for the rild restart case. But the radio still will be turned off soon in line#6649. My question is does it cause any problem if we turn off radio without deactivating data calls?

Comment 5

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review!

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> Comment on attachment 8572988 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8572988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this, Jessica. Please see my comments below.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1337,5 @@
> > +    for (let i = 0; i < currentDataCalls.length; i++) {
> > +      let currentDataCall = currentDataCalls[i];
> > +      if (currentDataCall && currentDataCall.linkInfo.cid != null &&
> > +          currentDataCall.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > +        this.debug("Expected data call missing, must have been DISCONNECTED.");
> 
> if (DEBUG) ...
> And also print the details of |currentDataCall|.

Will do.

> 
> @@ +1373,2 @@
> >     */
> > +  handleDataCallChanged: function(updatedDataCall) {
> 
> Could it be merged with |handleDataCallListChanged|?
> e.g. update datacall first then check for all data calls are disconnected.

Hmm, the idea of |handleDataCallListChanged| is to compare the data calls that come from the network and the our current data calls, to see if any data call is missing (data call change is handled in each |DataCall|), so |handleDataCallListChanged| takes an array of data calls as argument. If we merge these two functions, it would be hard to tell whether any of the data calls is missing or redundant.
Please let me know if I missed anything.

> 
> @@ +1379,4 @@
> >          this.allDataDisconnected()) {
> >        if (gRadioEnabledController.isDeactivatingDataCalls()) {
> >          if (DEBUG) {
> >            this.debug("All data connections are disconnected.");
> 
> s/data connections/data calls/

Will do.

> 
> @@ +2758,5 @@
> > +    this.state = dataCall.state;
> > +
> > +    // Notify DataConnectionHandler about data call connected.
> > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > +    connHandler.handleDataCallChanged(this);
> 
> Why it needs to notify connHandler regarding to the datacall is changed,
> won't it be notified by "datacalllistchanged"?

According to ril.h [1], a success data call setup will be followed by a RIL_UNSOL_DATA_CALL_LIST_CHANGED, so yes, it will be notified by "datacalllistchanged". However, I'd like DataCall to handle first and then notify back only if there is any change. It can be in pair with DISCONNECTED case as well.

> 
> @@ +2780,5 @@
> >      }
> >  
> > +    // Notify DataConnectionHandler about data call disconnected.
> > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > +    connHandler.handleDataCallChanged(this);
> 
> Ditto. Why it needs to notify connHandler regarding to the datacall is
> changed, won't it be notified by "datacalllistchanged"?

According to ril.h [2], a RIL_UNSOL_DATA_CALL_LIST_CHANGED is not expected to be issued after data call deactivation, so we need to notify back here.

> 
> @@ +2809,1 @@
> >              this.deactivate();
> 
> Why deactivate data call when the interface or address in linkinfo is
> changed?

This logic was from ril_worker, and I remember we imitated Android's behavior. I think it is because interface name change and or address missing is a big effect, so it would be better to reset the data call.

> 
> @@ +2844,5 @@
> > +    this.state = updatedDataCall.state;
> > +
> > +    // Notify DataConnectionHandler about data call changed.
> > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > +    connHandler.handleDataCallChanged(this);
> 
> Ditto. Could handleDataCallChanged and handleDataCallListChanged be merged
> together?
> Then we probably don't need to notify back to connHandler here.

Same reason as above.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +5273,5 @@
> >    }
> > +
> > +  let Buf = this.context.Buf;
> > +  let version = Buf.readInt32();
> > +  let num = Buf.readInt32();
> 
> Just add some comments for ignored field, don't have to introduce a unused
> variable for it.
> 
> e.g.
> 
> // Version of data call ....
> Buf.readInt32();
> // Number of data calls...
> Buf.readInt32();

Will do.

> 
> @@ +5839,5 @@
> >  
> >  RilObject.prototype[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST(length, options) {
> > +  if (!options.rilMessageType) {
> > +    // This is an unsolicited data call list changed.
> > +    options.rilMessageType = "datacalllistchanged";
> 
> I guess this is for UNSOLICITED_DATA_CALL_LIST_CHANGED, if so how about
> assigning option.rilMessageType to "datacalllistchanged" in caller?
> 
> e.g.
> this[REQUEST_DATA_CALL_LIST](length, {
>   rilRequestError: ERROR_SUCCESS,
>   rilMessageType: "datacalllistchanged"
> });
> 

Sounds good!

> @@ +5846,3 @@
> >    if (options.rilRequestError) {
> > +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> > +    this.sendChromeMessage(options);
> 
> It seems not necessary to sendChromeMessage to upper layer for error case,
> because REQUEST_DATA_CALL_LIST is only triggered inside ril_worker to query
> and notify data call list.

Currently, getDataCallList() is used only in ril_worker only, but I'm afraid one day DataConnectionHandler will need to call this. I can check for options.rilMessageType first before calling sendChromeMessage.

> 
> @@ +6641,5 @@
> >    }
> >  
> >    this.initRILState();
> > +  // rild might have restarted, ensure data call list.
> > +  this.getDataCallList();
> 
> Looks like you use different way to handle data call for the rild restart
> case. But the radio still will be turned off soon in line#6649. My question
> is does it cause any problem if we turn off radio without deactivating data
> calls?

In this case rild has already restarted, so the underlying behavior is unexpected, all data calls may be cleared already. From my test, when this function here is called, it returns empty datacalls, so it is just to notify DataConnectionHandler that data calls are not available anymore.


[1] https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#1497-1499
[2] https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#1832-1834

Comment 6

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > Comment on attachment 8572988 [details] [diff] [review]
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1373,2 @@
> > >     */
> > > +  handleDataCallChanged: function(updatedDataCall) {
> > 
> > Could it be merged with |handleDataCallListChanged|?
> > e.g. update datacall first then check for all data calls are disconnected.
> 
> Hmm, the idea of |handleDataCallListChanged| is to compare the data calls
> that come from the network and the our current data calls, to see if any
> data call is missing (data call change is handled in each |DataCall|), so
> |handleDataCallListChanged| takes an array of data calls as argument. If we
> merge these two functions, it would be hard to tell whether any of the data
> calls is missing or redundant.
> Please let me know if I missed anything.

My original thought is that DataConnectionHandler already knows there are some changes in data call (by receives "datacalllistchanged" message), and so |handleDataCallListChanged| probably could do finishDeactivatingDataCalls stuff (what handleDataCallChanged does) after all data calls are updated. 
But yes, we still have to take activating and deactivating into consideration.

> > @@ +2758,5 @@
> > > +    this.state = dataCall.state;
> > > +
> > > +    // Notify DataConnectionHandler about data call connected.
> > > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > > +    connHandler.handleDataCallChanged(this);
> > 
> > Why it needs to notify connHandler regarding to the datacall is changed,
> > won't it be notified by "datacalllistchanged"?
> 
> According to ril.h [1], a success data call setup will be followed by a
> RIL_UNSOL_DATA_CALL_LIST_CHANGED, so yes, it will be notified by
> "datacalllistchanged". However, I'd like DataCall to handle first and then
> notify back only if there is any change. It can be in pair with DISCONNECTED
> case as well.
> 
> > 
> > @@ +2780,5 @@
> > >      }
> > >  
> > > +    // Notify DataConnectionHandler about data call disconnected.
> > > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > > +    connHandler.handleDataCallChanged(this);
> > 
> > Ditto. Why it needs to notify connHandler regarding to the datacall is
> > changed, won't it be notified by "datacalllistchanged"?
> 
> According to ril.h [2], a RIL_UNSOL_DATA_CALL_LIST_CHANGED is not expected
> to be issued after data call deactivation, so we need to notify back here.

I see. 

> > 
> > @@ +2809,1 @@
> > >              this.deactivate();
> > 
> > Why deactivate data call when the interface or address in linkinfo is
> > changed?
> 
> This logic was from ril_worker, and I remember we imitated Android's
> behavior. I think it is because interface name change and or address missing
> is a big effect, so it would be better to reset the data call.

Ah, I remember that, thanks for the explanation.
 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +5839,5 @@
> > >  
> > >  RilObject.prototype[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST(length, options) {
> > > +  if (!options.rilMessageType) {
> > > +    // This is an unsolicited data call list changed.
> > > +    options.rilMessageType = "datacalllistchanged";
> > 
> > I guess this is for UNSOLICITED_DATA_CALL_LIST_CHANGED, if so how about
> > assigning option.rilMessageType to "datacalllistchanged" in caller?
> > 
> > e.g.
> > this[REQUEST_DATA_CALL_LIST](length, {
> >   rilRequestError: ERROR_SUCCESS,
> >   rilMessageType: "datacalllistchanged"
> > });
> > 
> 
> Sounds good!
> 
> > @@ +5846,3 @@
> > >    if (options.rilRequestError) {
> > > +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> > > +    this.sendChromeMessage(options);
> > 
> > It seems not necessary to sendChromeMessage to upper layer for error case,
> > because REQUEST_DATA_CALL_LIST is only triggered inside ril_worker to query
> > and notify data call list.
> 
> Currently, getDataCallList() is used only in ril_worker only, but I'm afraid
> one day DataConnectionHandler will need to call this. I can check for
> options.rilMessageType first before calling sendChromeMessage.

You are right.

> 
> > 
> > @@ +6641,5 @@
> > >    }
> > >  
> > >    this.initRILState();
> > > +  // rild might have restarted, ensure data call list.
> > > +  this.getDataCallList();
> > 
> > Looks like you use different way to handle data call for the rild restart
> > case. But the radio still will be turned off soon in line#6649. My question
> > is does it cause any problem if we turn off radio without deactivating data
> > calls?
> 
> In this case rild has already restarted, so the underlying behavior is
> unexpected, all data calls may be cleared already. From my test, when this
> function here is called, it returns empty datacalls, so it is just to notify
> DataConnectionHandler that data calls are not available anymore.

I'm afraid that rild returns empty datacalls is just because it has an unsynced state with modem. :(
Could you help to use linux shell to check the interface status when rild restart? Maybe we could find some clues on this.
Thank you.

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > > Comment on attachment 8572988 [details] [diff] [review]
> > > ::: dom/system/gonk/RadioInterfaceLayer.js
> > > @@ +1373,2 @@
> > > >     */
> > > > +  handleDataCallChanged: function(updatedDataCall) {
> > > 
> > > Could it be merged with |handleDataCallListChanged|?
> > > e.g. update datacall first then check for all data calls are disconnected.
> > 
> > Hmm, the idea of |handleDataCallListChanged| is to compare the data calls
> > that come from the network and the our current data calls, to see if any
> > data call is missing (data call change is handled in each |DataCall|), so
> > |handleDataCallListChanged| takes an array of data calls as argument. If we
> > merge these two functions, it would be hard to tell whether any of the data
> > calls is missing or redundant.
> > Please let me know if I missed anything.
> 
> My original thought is that DataConnectionHandler already knows there are
> some changes in data call (by receives "datacalllistchanged" message), and
> so |handleDataCallListChanged| probably could do finishDeactivatingDataCalls
> stuff (what handleDataCallChanged does) after all data calls are updated. 
> But yes, we still have to take activating and deactivating into
> consideration.
> 
> > > @@ +2758,5 @@
> > > > +    this.state = dataCall.state;
> > > > +
> > > > +    // Notify DataConnectionHandler about data call connected.
> > > > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > > > +    connHandler.handleDataCallChanged(this);
> > > 
> > > Why it needs to notify connHandler regarding to the datacall is changed,
> > > won't it be notified by "datacalllistchanged"?
> > 
> > According to ril.h [1], a success data call setup will be followed by a
> > RIL_UNSOL_DATA_CALL_LIST_CHANGED, so yes, it will be notified by
> > "datacalllistchanged". However, I'd like DataCall to handle first and then
> > notify back only if there is any change. It can be in pair with DISCONNECTED
> > case as well.
> > 
> > > 
> > > @@ +2780,5 @@
> > > >      }
> > > >  
> > > > +    // Notify DataConnectionHandler about data call disconnected.
> > > > +    let connHandler = gDataConnectionManager.getConnectionHandler(this.clientId);
> > > > +    connHandler.handleDataCallChanged(this);
> > > 
> > > Ditto. Why it needs to notify connHandler regarding to the datacall is
> > > changed, won't it be notified by "datacalllistchanged"?
> > 
> > According to ril.h [2], a RIL_UNSOL_DATA_CALL_LIST_CHANGED is not expected
> > to be issued after data call deactivation, so we need to notify back here.
> 
> I see. 
> 
> > > 
> > > @@ +2809,1 @@
> > > >              this.deactivate();
> > > 
> > > Why deactivate data call when the interface or address in linkinfo is
> > > changed?
> > 
> > This logic was from ril_worker, and I remember we imitated Android's
> > behavior. I think it is because interface name change and or address missing
> > is a big effect, so it would be better to reset the data call.
> 
> Ah, I remember that, thanks for the explanation.
>  
> > > ::: dom/system/gonk/ril_worker.js
> > > @@ +5839,5 @@
> > > >  
> > > >  RilObject.prototype[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST(length, options) {
> > > > +  if (!options.rilMessageType) {
> > > > +    // This is an unsolicited data call list changed.
> > > > +    options.rilMessageType = "datacalllistchanged";
> > > 
> > > I guess this is for UNSOLICITED_DATA_CALL_LIST_CHANGED, if so how about
> > > assigning option.rilMessageType to "datacalllistchanged" in caller?
> > > 
> > > e.g.
> > > this[REQUEST_DATA_CALL_LIST](length, {
> > >   rilRequestError: ERROR_SUCCESS,
> > >   rilMessageType: "datacalllistchanged"
> > > });
> > > 
> > 
> > Sounds good!
> > 
> > > @@ +5846,3 @@
> > > >    if (options.rilRequestError) {
> > > > +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> > > > +    this.sendChromeMessage(options);
> > > 
> > > It seems not necessary to sendChromeMessage to upper layer for error case,
> > > because REQUEST_DATA_CALL_LIST is only triggered inside ril_worker to query
> > > and notify data call list.
> > 
> > Currently, getDataCallList() is used only in ril_worker only, but I'm afraid
> > one day DataConnectionHandler will need to call this. I can check for
> > options.rilMessageType first before calling sendChromeMessage.
> 
> You are right.
> 
> > 
> > > 
> > > @@ +6641,5 @@
> > > >    }
> > > >  
> > > >    this.initRILState();
> > > > +  // rild might have restarted, ensure data call list.
> > > > +  this.getDataCallList();
> > > 
> > > Looks like you use different way to handle data call for the rild restart
> > > case. But the radio still will be turned off soon in line#6649. My question
> > > is does it cause any problem if we turn off radio without deactivating data
> > > calls?
> > 
> > In this case rild has already restarted, so the underlying behavior is
> > unexpected, all data calls may be cleared already. From my test, when this
> > function here is called, it returns empty datacalls, so it is just to notify
> > DataConnectionHandler that data calls are not available anymore.
> 
> I'm afraid that rild returns empty datacalls is just because it has an
> unsynced state with modem. :(
> Could you help to use linux shell to check the interface status when rild
> restart? Maybe we could find some clues on this.
> Thank you.

I have tested with Flame, when rild restarts, network interface will go down even if we don't turn the radio off. If we send 'DEACTIVATE_DATA_CALL' in this case, rild will return 'Generic Failure' cause it can't find a matching cid.

Comment 8

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #7)
> I have tested with Flame, when rild restarts, network interface will go down
> even if we don't turn the radio off. If we send 'DEACTIVATE_DATA_CALL' in
> this case, rild will return 'Generic Failure' cause it can't find a matching
> cid.

Cool, thanks for this information.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Edgar, I have addressed your review comments, may I have your review again? Thanks.

Comment 10

[Edgar Chen [:edgar]]

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

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

Looks good to me, thank you.

Comment 11

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d858c897dceb&exclusion_profile=false

test_mobile_data_connection.js times out, investigating...

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Fixed careless mistakes :(

Comment 13

[Jessica Jong [:jessica] (inactive)]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=826c10fd9abc

mochitest-9 is a known failure. I also found that sometimes we never get the result of some tests, need to retrigger the job.

Comment 14

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/744789f751de

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/744789f751de