986362 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[shawn ku [:sku]]

Currently, emulator can only support id/dir/state/mode/multi/number (see [1]). It is not enough to simulate the inbound call with numberPresentation/name/namePresentation case (see [2]).

  To have more comprehensive support on inbound call, it is better to have those variables supported in emulator.

// Emulator log.
D AT      : AT< +CLCC: 1,1,4,0,0,"1234",129

// Tarako log
D/AT      (   96): Channel1: AT< +CLCC: 1,1,4,0,0,"xxxxxxxxxx",129

// Unagi log
D/RILC    (  112): Reply to RIL --> call[0] : Incoming, index 1, toa 129, isMpty 0, isMT 1, als 0, isVoice 1, isVoicePrivacy 0
D/RILC    (  112):                            num xxxxxxxxxx, num presentation 0, name , name presentation 0

[1] - https://github.com/android/platform_external_qemu/blob/master/telephony/android_modem.h#L151
[2] - https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L1887

Comment 1

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator.

Comment 2

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator.

Comment 3

[shawn ku [:sku]]

Comment on attachment 8396098 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator.

cancel r? due to comment on github.

Comment 4

[shawn ku [:sku]]

Comment on attachment 8396099 [details] [review]
Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator.

cancel r? due to comment on github.

Comment 5

[shawn ku [:sku]]

Hi Vicamo:
 Sorry I can not have a f2f talk to you due to I am on Biz trip.

I face a problem when implement a seperated AT+CNAP request.
Please see line 802 and 805 at below link.

https://github.com/chenghuk/platform_hardware_ril/compare/mozilla-b2g:master...986362#diff-03be6fb09746d8e0f942e2df6b791eb0R803

I can confirm beefore line 805, p_call->number is still valid (that's said 1234), however, ril_worker.js will got a weird number (that's said 12\u0013).

The reason might be I try to re-use p_response for both AT+CLCC and AT+CNAP.
I also have to invoke "at_response_free(p_response);" to free p_response for both CLCC/CNAP. or there will be memory leakage.

Is there any suggetion by checking the link above?, or should I provide a full patch (including qemu part) for you to check?

Thank you very much.
Shawn

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to shawn ku [:sku] from comment #5)
> Hi Vicamo:
>  Sorry I can not have a f2f talk to you due to I am on Biz trip.

No problem.

> I face a problem when implement a seperated AT+CNAP request.
> Please see line 802 and 805 at below link.
> 
> https://github.com/chenghuk/platform_hardware_ril/compare/mozilla-b2g:master.
> ..986362#diff-03be6fb09746d8e0f942e2df6b791eb0R803
> 
> I can confirm beefore line 805, p_call->number is still valid (that's said
> 1234), however, ril_worker.js will got a weird number (that's said 12\u0013).

Because |p_call->number| is actually pointing to an address allocated for one of the ATLine, and |at_response_free(p_response);| frees all the allocated memory for a ATResponse, inclusive of all the ATLines.

> The reason might be I try to re-use p_response for both AT+CLCC and AT+CNAP.
> I also have to invoke "at_response_free(p_response);" to free p_response for
> both CLCC/CNAP. or there will be memory leakage.

From 3GPP TS 27.007 sub clause 7.30 "Calling name identification presentation +CNAP"

  When the presentation of the CNI at the TE is enabled (and CNI is provided),
  +CNAP: <name>[,<CNI validity>] response is returned after every RING result
  code sent from TA to TE.

Table 59D also tells you that its query form response is undefined.

So we should have:

  (qemu <<>> rild)
  >> RING
  >> +CNAP: <name>[,<CNI validity>]
  << AT+CLCC
  >> +CLCC: ....

At the time +CNAP unsolicited AT response comes in, we keep its content and insert them into ATResponse of GET_CURRENT_CALLS.

However, this also raise a question in my mind: will CNAP info remains forever if second incoming call arrives?

  (qemu <<>> rild)
  >> RING
  >> +CNAP: <name>[,<CNI validity>]
  << AT+CLCC
  >> +CLCC: ....
  << ATA
  >> RING
  >> +CNAP: <name>[,<CNI validity>]
  << AT+CLCC
  >> +CLCC: ....
  >> +CLCC: ....

In oFono [1], RING or +CRING triggers a voluntarily AT+CLCC poll.  oFono then keeps the calls list along with the unsolicited CNAP info in ofono daemon, so it always returns calls list with correct attributes.

AOSP's reference rild doesn't keep calls list. At least for now.  Can we assume the CNAP infos in the returned GET_CURRENT_CALLS are static and last until the calls are destroyed?  AOSP source code might have the answer.

[1] http://git.kernel.org/cgit/network/ofono/ofono.git/tree/drivers/atmodem/voicecall.c#n851

> Is there any suggetion by checking the link above?, or should I provide a
> full patch (including qemu part) for you to check?

Besides, CLCC response has format <id>,<dir>,<stat>,<mode>,<mpty>[,<number>,<type>[,<alpha>[,<priority>[,<CLI validity>]]]] but your commit has "index,isMT,state,mode,isMpty(,number,TOA,numberPresentation)?"

Comment 7

[shawn ku [:sku]]

Vicamo,thanks for your reply.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> (In reply to shawn ku [:sku] from comment #5)
> > Hi Vicamo:
> >  Sorry I can not have a f2f talk to you due to I am on Biz trip.
> 
> No problem.
> 
> > I face a problem when implement a seperated AT+CNAP request.
> > Please see line 802 and 805 at below link.
> > 
> > https://github.com/chenghuk/platform_hardware_ril/compare/mozilla-b2g:master.
> > ..986362#diff-03be6fb09746d8e0f942e2df6b791eb0R803
> > 
> > I can confirm beefore line 805, p_call->number is still valid (that's said
> > 1234), however, ril_worker.js will got a weird number (that's said 12\u0013).
> 
> Because |p_call->number| is actually pointing to an address allocated for
> one of the ATLine, and |at_response_free(p_response);| frees all the
> allocated memory for a ATResponse, inclusive of all the ATLines.
> 

I know this, all I am trying to do is to get rid of this problem.:)

> > The reason might be I try to re-use p_response for both AT+CLCC and AT+CNAP.
> > I also have to invoke "at_response_free(p_response);" to free p_response for
> > both CLCC/CNAP. or there will be memory leakage.
> 
> From 3GPP TS 27.007 sub clause 7.30 "Calling name identification
> presentation +CNAP"
> 
>   When the presentation of the CNI at the TE is enabled (and CNI is
> provided),
>   +CNAP: <name>[,<CNI validity>] response is returned after every RING result
>   code sent from TA to TE.
> 
> Table 59D also tells you that its query form response is undefined.
> 
> So we should have:
> 
>   (qemu <<>> rild)
>   >> RING
>   >> +CNAP: <name>[,<CNI validity>]
>   << AT+CLCC
>   >> +CLCC: ....
> 
> At the time +CNAP unsolicited AT response comes in, we keep its content and
> insert them into ATResponse of GET_CURRENT_CALLS.
> 
> However, this also raise a question in my mind: will CNAP info remains
> forever if second incoming call arrives?

No, it should not.

My thought is:
RING -> CNAP -> CLCC
1. CNAP will come after CRING, rild just append it to RIL_CALL object (which is id based), and report UNSOL_CALL_STATE_CHANGE to upper layer.

or

2. modem has both CLCC and CNAP info, notify rild, ril to query those info, and report UNSOL_CALL_STATE_CHANGE to upper layer.

No matter case 1 or 2, CNAP only affect the "current" call, not second incoming call.

> 
>   (qemu <<>> rild)
>   >> RING
>   >> +CNAP: <name>[,<CNI validity>]
>   << AT+CLCC
>   >> +CLCC: ....
>   << ATA
>   >> RING
>   >> +CNAP: <name>[,<CNI validity>]
>   << AT+CLCC
>   >> +CLCC: ....
>   >> +CLCC: ....
> 
> In oFono [1], RING or +CRING triggers a voluntarily AT+CLCC poll.  oFono
> then keeps the calls list along with the unsolicited CNAP info in ofono
> daemon, so it always returns calls list with correct attributes.
> 
> AOSP's reference rild doesn't keep calls list. At least for now.  Can we
> assume the CNAP infos in the returned GET_CURRENT_CALLS are static and last
> until the calls are destroyed?  AOSP source code might have the answer.

should be as you said.

> 
> [1]
> http://git.kernel.org/cgit/network/ofono/ofono.git/tree/drivers/atmodem/
> voicecall.c#n851
> 
> > Is there any suggetion by checking the link above?, or should I provide a
> > full patch (including qemu part) for you to check?
> 
> Besides, CLCC response has format
> <id>,<dir>,<stat>,<mode>,<mpty>[,<number>,<type>[,<alpha>[,<priority>[,<CLI
> validity>]]]] but your commit has
> "index,isMT,state,mode,isMpty(,number,TOA,numberPresentation)?"

Yes, spec. defines 
[+CLCC: <ccid1>,<dir>,<stat>,<mode>,<mpty>[,<number>
,<type>[,<alpha>[,<priority>[,<CLI validity>]]]]
[<CR><LF>+CLCC: <ccid2>,<dir>,<stat>,<mode>,<mpty>[,
<number>,<type>[,<alpha>[,<priority>[,<CLI validity>
]]]]

Should I also implement alpha, priority (both set to default) in this patch?
if yes, I will try to make this part up.

Thanks again.
Shawn

Comment 8

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

(In reply to shawn ku [:sku] from comment #7)
> > 
> >   (qemu <<>> rild)
> >   >> RING
> >   >> +CNAP: <name>[,<CNI validity>]
> >   << AT+CLCC
> >   >> +CLCC: ....
> >   << ATA
> >   >> RING
> >   >> +CNAP: <name>[,<CNI validity>]
> >   << AT+CLCC
> >   >> +CLCC: ....
> >   >> +CLCC: ....
> > 
> > In oFono [1], RING or +CRING triggers a voluntarily AT+CLCC poll.  oFono
> > then keeps the calls list along with the unsolicited CNAP info in ofono
> > daemon, so it always returns calls list with correct attributes.
> > 
> > AOSP's reference rild doesn't keep calls list. At least for now.  Can we
> > assume the CNAP infos in the returned GET_CURRENT_CALLS are static and last
> > until the calls are destroyed?  AOSP source code might have the answer.
> 
> should be as you said.
> 

Should be the case as Vicamo said.

CdmaConnection.java checks "name" and "namePresentation" coming from the response of getCurrentCalls every time. That definitely implies CNAP in the response of getCurrentCalls lasts till a call is released. I think we would need to keep the call list in rild if we all agree we should follow the spec of +CNAP.

Comment 9

[shawn ku [:sku]]

We might need to consider cache CNAP + callID in reference RILD since we still need it when MT call get connected, but not clean CNAP cache when +CRING/+CNAP.

Ex: 1st MT call get connected with CNAP + 2nd MT call case w/o CNAP.
CNAP for 1st MT will be gone when 2nd MT is waiting or answered.
 

// 3GPP spec.
By TS 27.007 7.30 Calling name identification presentation +CNAP
... It is manufacturer specific if this response is used when normal voice call is answered.

By TS 23.096 Figure 3b, CNAP can be send to device when call setup/connected.

Comment 10

[shawn ku [:sku]]

I am not sure if my emulator problem or generic one.
It looks like emulator can have multiple incoming calls at the same time. (this will not be happened in real network)


If we need to cache CANP + callID on reference rild, this part need to be aware of.


03-27 22:28:45.085    38    94 D AT      : AT< RING
03-27 22:28:45.085    38    94 D AT      : AT< +CNAP: "test",0
03-27 22:28:45.456    38    94 D AT      : AT< +CLCC: 1,1,4,0,0,"1234",129,"",2,0
...
03-27 22:28:49.316    38    94 D AT      : AT< RING
03-27 22:28:49.326    38    94 D AT      : AT< +CNAP: "",0
03-27 22:28:49.406    38    94 D AT      : AT< +CLCC: 1,1,4,0,0,"1234",129,"",2,0
03-27 22:28:49.406    38    94 D AT      : AT< +CLCC: 2,1,4,0,0,"1234",129,"",2,0

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to shawn ku [:sku] from comment #10)
> I am not sure if my emulator problem or generic one.
> It looks like emulator can have multiple incoming calls at the same time.
> (this will not be happened in real network)

Yes, that's a known problem.

> If we need to cache CANP + callID on reference rild, this part need to be
> aware of.

I think we can pretend it doesn't exist for now :)

Comment 12

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

(In reply to shawn ku [:sku] from comment #9)
> We might need to consider cache CNAP + callID in reference RILD since we
> still need it when MT call get connected, but not clean CNAP cache when
> +CRING/+CNAP.
> 
> Ex: 1st MT call get connected with CNAP + 2nd MT call case w/o CNAP.
> CNAP for 1st MT will be gone when 2nd MT is waiting or answered.
>  
> 
> // 3GPP spec.
> By TS 27.007 7.30 Calling name identification presentation +CNAP
> ... It is manufacturer specific if this response is used when normal voice
> call is answered.
> 
> By TS 23.096 Figure 3b, CNAP can be send to device when call setup/connected.

I guess I will need CNAP in bug 975778, a cdma call waiting case. That means, there would be no "callId" for the mapping. Not sure if we want to consider the use case in this bug. If not, I am also fine to refactor the mapping part in bug 975772, but still want to share the information to you :)

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> (In reply to shawn ku [:sku] from comment #10)

> Yes, that's a known problem.
> 
> > If we need to cache CANP + callID on reference rild, this part need to be
> > aware of.
> 
> I think we can pretend it doesn't exist for now :)

Agree we can live with ignoring the multi-incoming call problem in this bug.

Comment 13

[shawn ku [:sku]]

For CANP cache, we can have name/namePresentation by checking INCOMING or WAITING state to fill up correct CNAP info. And report it via RIL_CALL object to gecko.

However, it might be an issue after multi-calls get connected.
Ex:
1. For 1 Active + 1 Waiting case, there will be CRING -> CNAP -> CLCC for waiting call, 
the CNAP should be only valid for waiting call (First CNAP info is gone due to new CRING/CNAP)

And,
After Waiting call be answered, is there way for us to update cached CANP to correct connections? (two calls alive)

Any idea/suggestion on this part?

(By comment 9, CNAP could be received at incoming call setup or call connected stage, and it can be last until that connection ended).

Comment 14

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

(In reply to shawn ku [:sku] from comment #13)
> For CANP cache, we can have name/namePresentation by checking INCOMING or
> WAITING state to fill up correct CNAP info. And report it via RIL_CALL
> object to gecko.
> 

But the thing in CDMA network is, there won't be an additional RIL_CALL object for the waiting call. Hmmm, maybe it's not an issue, since in CDMA call waiting case the CNAP doesn't need to be cached. We just need to make sure the CNAP info is sent via unsol_cdma_callwaiting event.


> However, it might be an issue after multi-calls get connected.
> Ex:
> 1. For 1 Active + 1 Waiting case, there will be CRING -> CNAP -> CLCC for
> waiting call, 
> the CNAP should be only valid for waiting call (First CNAP info is gone due
> to new CRING/CNAP)
> 
> And,
> After Waiting call be answered, is there way for us to update cached CANP to
> correct connections? (two calls alive)
> 
> Any idea/suggestion on this part?
> 
> (By comment 9, CNAP could be received at incoming call setup or call
> connected stage, and it can be last until that connection ended).

Comment 15

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v2.

Comment 16

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v2.

Comment 17

[shawn ku [:sku]]

Comment on attachment 8404545 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v2.

clear r? due to defects (comment on github) was found by Vicamo.
Need revision first.

Thanks, Vicamo!!!

Comment 18

[shawn ku [:sku]]

Comment on attachment 8404547 [details] [review]
Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v2.

clear r? due to defects (comment on github) was found by Vicamo.
Need revision first.

Thanks, Vicamo!!!

Comment 19

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v3.

Comment 20

[shawn ku [:sku]]

Attachment: 8404547: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v3.

Comment 21

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v3.

Comment 22

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v4.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8407286 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v4.

Two nits to be addressed.  Thank you.

Comment 24

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v5. r=Vicamo.

Comment 25

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v4.

Comment 26

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v6.

Comment 27

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v5..

Comment 28

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v5.

Comment 29

[shawn ku [:sku]]

Comment on attachment 8408111 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v6.

re-check flow first

Comment 30

[shawn ku [:sku]]

Comment on attachment 8408123 [details] [review]
Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v5.

re-check flow first

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8408111 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v6.

Please use `strchr` instead of non-portable `strsep`.

Comment 32

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v7.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8410016 [details] [review]
Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v7.

Thank you :)

Comment 34

[shawn ku [:sku]]

Attachment: Bug 986362: Part 1 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v7. r=Vicamo.

Comment 35

[shawn ku [:sku]]

Attachment: Bug 986362: Part 2 - B2G Emulator: Support numberPresentation/name/namePresentation in emulator. v5. r=Vicamo.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

platform_external_qemu/master: https://github.com/mozilla-b2g/platform_external_qemu/commit/85f9690323b235f4dcf2901ea2240d3c60fc22a0

platform_hardware_ril/master: https://github.com/mozilla-b2g/platform_hardware_ril/commit/0292e64ef8451df104dcf9ac3b2c6749b81684dd

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

I thought we're still waiting for HsinYi's test cases.  Next time, please attach test cases along with your emulator pull requests.

Comment 38

[shawn ku [:sku]]

Sorry to be that rush.
I will be more careful next time.

PS:
Should I request backout first?