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)
Tracking
(blocking-b2g:1.4+, 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)
681.32 KB,
application/zip
|
Details | |
2.62 KB,
patch
|
Details | Diff | Splinter Review |
### 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
Reporter | ||
Comment 1•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → joliu
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8403763 -
Flags: review?(echou)
Comment 5•10 years ago
|
||
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. :)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8406632 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
try server: https://tbpl.mozilla.org/?tree=Try&rev=0bcdf065bce4
Attachment #8408018 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
(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
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8408022 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/1388fb71fc64
Keywords: checkin-needed
Assignee | ||
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1388fb71fc64
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 18•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2156310d45b9
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Reporter | ||
Comment 19•10 years ago
|
||
### Build for verification https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-nexus-4/2014/04/2014-04-28-00-02-06/
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•