Closed Bug 993290 Opened 10 years ago Closed 10 years ago

[Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TCA_BV_04_I

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
2.0 S1 (9may)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: ericcc, Assigned: yrliou)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached file TC_AG_TCA_BV04_I.zip
### ENV
Nexus 4/Bluedroid
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-nexus-4/2014/03/2014-03-25-16-02-01/


### STR
1. PTS 5.0
2. TC_AG_TCA_BV04_I 

### Expected
Pass 

### Actual
Test case : TC_AG_TCA_BV_04_I started
	- SDP Service record for PTS: 'Handsfree HF' successfully registered
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- AT: SPP connect succeeded
	- AT: WARNING: IUT does not report support for AT+CHLD=3 in its +CHLD response, but TSPC_HFP_2_12c is set to TRUE
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- HCI: Audio Connection enabled
	- AT: Outgoing call established
	- FATAL ERROR (MTC): Call setup by tester failed
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
Final Verdict : Fail
Summary: [Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TCA_BV04_I → [Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TCA_BV_04_I
Blocks: 986297
Assignee: nobody → joliu
This is caused by the bug that we didn't respond OK after HF sending ATDdd..dd; request.
PTS will not proceed to send AT+CHUP request because it is still waiting for the OK response.
Comment on attachment 8403763 [details] [diff] [review]
Bug 993290 : Send OK response to HF after receiving ATDdd..dd;

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

The patch looks good to me. However after taking a look at function BluetoothHfpManager::ProcessDialCall(), I think it's a good time to do the cleanup. If you may not be available to do this, please file a followup. :)
Hi Eric,
I have done some modification on ProcessDialCall() for better readability.
Please let me know if you have any comments on this patch.

Besides, I want to point out one thing in the patch in advance.
Although it might be weird to send OK for missing ';' case in ATD>xxx and ATDxxx at first glance, I choose not to change it to ERROR right now for the consistency between HFP implementation for bluez and bluedroid. 
(In bluez/BluetoothHfpManager.cpp, there are many other cases we choose to respond OK while we encounter invalid value while parsing AT commands.)
Since HFP spec didn't specify this case, I think it should be fine to send either OK or ERROR in this case.
Attachment #8403763 - Attachment is obsolete: true
Attachment #8403763 - Flags: review?(echou)
Attachment #8404433 - Flags: review?(echou)
Comment on attachment 8404433 [details] [diff] [review]
Patch1(v2) Bug 993290: Respond OK after receiving ATDxxx and improve readability on ProcessDialCall().

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

Hi Jocelyn,

Please see my comments below. Thanks! :)

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +607,2 @@
>  
>  void BluetoothHfpManager::ProcessDialCall(char *aNumber)

AFAIK, there are only 3 cases would be handled in this function which is based on the format of |aNumber|:

1. Empty string      - Redial
2. ">xxxx;"          - Memory dial
3. "xxxxxxxxxxx;"    - Normal dial

Instead of inserting comments everywhere in such a small function, I think put a summary at the beginning of the function might not be a bad idea. Something like:

  // The argument |aNumber| may be presented as 3 different formats and all
  // represent different corresponding commands:
  // (1) Empty value - Redial
  // (2) Ends with a ';', and
  //   (2-1) Starts with a '>'     - Memory Dial (e.g. ">2;")
  //   (2-2) NOT starts with a '>' - Normal Dial (e.g. "886222334455#3;")

@@ +613,5 @@
>    if (message.IsEmpty()) {
>      // Redial: BLDN
>      mDialingRequestProcessed = false;
>      BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_DIALER,
>                          NS_LITERAL_STRING("BLDN"));

I have an idea: early return for Redial case.

In this function, aNumber

@@ +615,5 @@
>      mDialingRequestProcessed = false;
>      BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_DIALER,
>                          NS_LITERAL_STRING("BLDN"));
>    } else {
> +    // Memory dial: >ATDxxx; or Standard dial: ATDxxx;

The format of memory dial should be "ATD>xxx;"

@@ +620,2 @@
>      nsAutoCString newMsg("ATD");
> +    int end = message.FindChar(';');

I just checked what value would be contained in |aNumber|. According to [1], Bluedroid does the format-checking for us, which means that we can assume the last character in |aNumber| would be ';', which follows HFP spec. If so, then maybe we can put an assertion here instead of worrying about this too much. After applying this change, it may be another chance for us to refactor more than this patch does.

[1] http://androidxref.com/4.2_r1/xref/external/bluetooth/bluedroid/bta/ag/bta_ag_cmd.c#890

@@ +629,2 @@
>        mDialingRequestProcessed = false;
>        newMsg += message;

I wonder why we do

  |newMsg += message;|

for memory dial and

  |newMsg += nsDependentCSubstring(message, 0, end);|

for normal dialing.

Please see if we can use the same mechanism for both cases. There is a string helper function called "StringHead()" which may be helpful.

@@ +642,1 @@
>    if (!mDialingRequestProcessed) {

I think this if-check could be integrated into other parts of this function.
Sorry, I forgot to complete one sentence in comment 7. 

@@ +613,5 @@
>    if (message.IsEmpty()) {
>      // Redial: BLDN
>      mDialingRequestProcessed = false;
>      BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_DIALER,
>                          NS_LITERAL_STRING("BLDN"));

I have an idea: early return for Redial case.

Early return makes sense to me because it would make the other parts of code become more easy to be refined.
Hi Eric,

After checking bta_ag_cmd.c, I removed the missing ';' handling
since bluedroid already handle this case and will not call ProcessDialCall() 
callback if there is no ';' for ATDxxx; and ATD>xxx;.

I have refined the function again after removing ';' check.
Please kindly review my new patch.

Thanks,
Jocelyn
Attachment #8404433 - Attachment is obsolete: true
Attachment #8404433 - Flags: review?(echou)
Attachment #8406632 - Flags: review?(echou)
Comment on attachment 8406632 [details] [diff] [review]
Patch1(v3) Bug 993290: Respond OK after receiving ATDxxx and improve readability on ProcessDialCall()

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

r=me with the only problem fixed. Thanks for being patient. :)

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +627,2 @@
>      nsAutoCString newMsg("ATD");
> +    newMsg += StringHead(message, message.Length());

Shouldn't the 2nd argument be message.Length() - 1 to strip the trailing ';'?
Attachment #8406632 - Flags: review?(echou) → review+
Keywords: checkin-needed
(In reply to jocelyn [:jocelyn] from comment #11)
> Created attachment 8408018 [details]
> [final] Patch1 Bug 993290: Respond OK after receiving ATDxxx and improve
> readability on ProcessDialCall()
The patch conflicts with bug 993280's fix. Please help rebase on it. Thanks.
Keywords: checkin-needed
Keywords: checkin-needed
Set 1.4? as this bug fails bluetooth certification HFP test on bluedroid.
Set depends on 993280 for avoiding conflict.
blocking-b2g: --- → 1.4?
Depends on: 993280
https://hg.mozilla.org/mozilla-central/rev/1388fb71fc64
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
blocking-b2g: 1.4? → 1.4+
You need to log in before you can comment on or make changes to this bug.