Closed Bug 1182953 Opened 6 years ago Closed 6 years ago

[Bluetooth][HFP] Implement Voice Recognition Activation(VRA)

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.2r+, b2g-v2.2r fixed)

RESOLVED FIXED
feature-b2g 2.2r+
Tracking Status
b2g-v2.2r --- fixed

People

(Reporter: wiwang, Assigned: wiwang, NeedInfo)

References

Details

Attachments

(1 file, 2 obsolete files)

Implement HFP 1.6 Voice Recognition Activation(VRA). 
(Refer to HFP 1.6 - section 4.25)

"The HF, via the AT+BVRA command, or the AG autonomously, may activate/deactivate the voice recognition function resident in the AG. Beyond the audio routing and voice recognition activation capabilities, the rest of the voice recognition functionality is implementation dependent."

I am going to implement this including passing the related Bluetooth PTS test.

Example use cases: Gaia application can use this to activate voice recognition, or carkit can deactivate if it has its own voice recognition functionality.

Furthermore:
In this implementation, we should consider the potential security issues which were stated in following bug:

1091417 – Investigate to design Bluetooth SCO connection (bluetooth voice call audio) API for WebRTC/VoIP/Voice Dialing use cases
https://bugzilla.mozilla.org/show_bug.cgi?id=1091417
Duplicate of this bug: 991552
Hi Shawn,

Attached patch:
- add HFP VRA support
- fix a typo in BluetoothDaemonHelpers.cpp
- pass PTS test TC_AG_VRA_BV_01_I, TC_AG_VRD_BV_01_I

Could you help to review?
Thanks!
Attachment #8660539 - Flags: review?(shuang)
Comment on attachment 8660539 [details] [diff] [review]
Patch (for branch 2.2r): Bug 1182953: Add HFP Voice Recognition (VRA) support

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1419,5 @@
> +BluetoothHfpManager::VoiceRecognitionNotification(
> +  BluetoothHandsfreeVoiceRecognitionState aState, const nsAString& aBdAddr)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  nsAutoString name;

nit: Add a new line after MOZ_ASSERT

@@ +1427,5 @@
> +
> +  if (aState == HFP_VOICE_RECOGNITION_STOPPED) {
> +    if (mVoiceRecognitionStarted) {
> +      mVoiceRecognitionStarted = false;
> +      DisconnectSco();

Please check current SCO state.

@@ +1431,5 @@
> +      DisconnectSco();
> +      name.AssignLiteral("stop");
> +    }
> +  } else if (aState == HFP_VOICE_RECOGNITION_STARTED) {
> +    if ((FindFirstCall(nsITelephonyService::CALL_STATE_CONNECTED) == 0) &&

Add comment to explain.

@@ +1434,5 @@
> +  } else if (aState == HFP_VOICE_RECOGNITION_STARTED) {
> +    if ((FindFirstCall(nsITelephonyService::CALL_STATE_CONNECTED) == 0) &&
> +        !mVoiceRecognitionStarted) {
> +      mVoiceRecognitionStarted = true;
> +      ConnectSco();

Please check current SCO state.

@@ +1443,5 @@
> +    return;
> +  }
> +
> +  if (!BroadcastSystemMessage(type, BluetoothValue(name))) {
> +    SendResponse(HFP_AT_RESPONSE_ERROR);

You should return directly, otherwise you will send AT_ERROR and AT_OK.
Attachment #8660539 - Flags: review?(shuang) → review-
Hi Wesley,

I observed that a Bluetooth PTS test case "TC_AG_VRA_BV_02_I" will not pass unless VR can be explicitly enabled using UI of Firefox OS(instead of using HF device's request in background)

That is, we may need a sort of Gaia app which can enable VR to cooperate for this verification case,
we need to discuss this.

Similarly,
another test case "TC_AG_VRA_BI_01_I" will need the same UI functionality to correctly pass test.
(Currently this case will pass, in a wrong way)
Flags: needinfo?(whuang)
Flags: needinfo?(shuang)
Hi Shawn,

Revised patch:
- fix nits.
- Per our offline discussion, remain the way we use ConnectSco/DisconnectSco since these function will check state in their own.

Thanks!
Attachment #8660539 - Attachment is obsolete: true
Attachment #8661191 - Flags: review?(shuang)
Comment on attachment 8661191 [details] [diff] [review]
Patch (for branch 2.2r)(v2): Bug 1182953: Add HFP Voice Recognition (VRA) support

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

Hi
r=me, but one more thing to do. See my comments.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1438,5 @@
> +      ConnectSco();
> +      name.AssignLiteral("start");
> +    }
> +  } else {
> +    BT_LOGR("Invalid state");

What happened if it's invalid state? Why not replying AT_ERROR here?
Attachment #8661191 - Flags: review?(shuang) → review+
(In reply to Will Wang [:WillWang] from comment #4)
> Hi Wesley,
> 
> I observed that a Bluetooth PTS test case "TC_AG_VRA_BV_02_I" will not pass
> unless VR can be explicitly enabled using UI of Firefox OS(instead of using
> HF device's request in background)
> 
> That is, we may need a sort of Gaia app which can enable VR to cooperate for
> this verification case,
> we need to discuss this.
> 
> Similarly,
> another test case "TC_AG_VRA_BI_01_I" will need the same UI functionality to
> correctly pass test.
> (Currently this case will pass, in a wrong way)

Hi Wesley,
This is about Bluetooth certification. Bluetooth certification assumes the phone shall have voice recognition feature (that implied 'Voice Recognition' application shall be ready). Do you know the plan regarding voice recognition?
Flags: needinfo?(shuang)
If it's for 2.2R, we don't have such plan at Mozilla end to add VR applications.
Flags: needinfo?(whuang)
Hi Young-Jin,

You may want to be informed per comment 4.
Flags: needinfo?(cockoo.lee)
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Comment on attachment 8661191 [details] [diff] [review]
> Patch (for branch 2.2r)(v2): Bug 1182953: Add HFP Voice Recognition (VRA)
> support
> 
> Review of attachment 8661191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi
> r=me, but one more thing to do. See my comments.
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1438,5 @@
> > +      ConnectSco();
> > +      name.AssignLiteral("start");
> > +    }
> > +  } else {
> > +    BT_LOGR("Invalid state");
> 
> What happened if it's invalid state? Why not replying AT_ERROR here?

I think we don't need to replay AT_ERROR to HF device, since:
1. It's an additional error handling for invalid VR state, which actually should not be sent from HF device.(undefined behavior in HFP spec). Another similar but different situation is, HFP spec request AG to reply AT_ERROR when AG don't support VRA.
2. Current android implementation[1] won't reply AT_ERROR for this case, either.

What do you think?
If you also accept my point, I am going to request checkin for branch 2.2r ,thanks!

[1] http://androidxref.com/5.1.1_r6/xref/packages/apps/Bluetooth/src/com/android/bluetooth/hfp/HeadsetStateMachine.java#2156
Flags: needinfo?(shuang)
(In reply to Will Wang [:WillWang] from comment #10)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > Comment on attachment 8661191 [details] [diff] [review]
> > Patch (for branch 2.2r)(v2): Bug 1182953: Add HFP Voice Recognition (VRA)
> > support
> > 
> > Review of attachment 8661191 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi
> > r=me, but one more thing to do. See my comments.
> > 
> > ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> > @@ +1438,5 @@
> > > +      ConnectSco();
> > > +      name.AssignLiteral("start");
> > > +    }
> > > +  } else {
> > > +    BT_LOGR("Invalid state");
> > 
> > What happened if it's invalid state? Why not replying AT_ERROR here?
> 
> I think we don't need to replay AT_ERROR to HF device, since:
> 1. It's an additional error handling for invalid VR state, which actually
> should not be sent from HF device.(undefined behavior in HFP spec). Another
> similar but different situation is, HFP spec request AG to reply AT_ERROR
> when AG don't support VRA.
> 2. Current android implementation[1] won't reply AT_ERROR for this case,
> either.
> 
> What do you think?
> If you also accept my point, I am going to request checkin for branch 2.2r
> ,thanks!
> 
> [1]
> http://androidxref.com/5.1.1_r6/xref/packages/apps/Bluetooth/src/com/android/
> bluetooth/hfp/HeadsetStateMachine.java#2156

That really depends on how bluetoothd and bluedroid handle invalid BVRA AT command. Your assumption is there are other cases.

enum BluetoothHandsfreeVoiceRecognitionState {                                  
  HFP_VOICE_RECOGNITION_STOPPED,                                                
  HFP_VOICE_RECOGNITION_STARTED                                                 
}; 

I don't think you need to handle other cases. You should delete that else statement.
Flags: needinfo?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> (In reply to Will Wang [:WillWang] from comment #10)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > > Comment on attachment 8661191 [details] [diff] [review]
> > > Patch (for branch 2.2r)(v2): Bug 1182953: Add HFP Voice Recognition (VRA)
> > > support
> > > 
> > > Review of attachment 8661191 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Hi
> > > r=me, but one more thing to do. See my comments.
> > > 
> > > ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> > > @@ +1438,5 @@
> > > > +      ConnectSco();
> > > > +      name.AssignLiteral("start");
> > > > +    }
> > > > +  } else {
> > > > +    BT_LOGR("Invalid state");
> > > 
> > > What happened if it's invalid state? Why not replying AT_ERROR here?
> > 
> > I think we don't need to replay AT_ERROR to HF device, since:
> > 1. It's an additional error handling for invalid VR state, which actually
> > should not be sent from HF device.(undefined behavior in HFP spec). Another
> > similar but different situation is, HFP spec request AG to reply AT_ERROR
> > when AG don't support VRA.
> > 2. Current android implementation[1] won't reply AT_ERROR for this case,
> > either.
> > 
> > What do you think?
> > If you also accept my point, I am going to request checkin for branch 2.2r
> > ,thanks!
> > 
> > [1]
> > http://androidxref.com/5.1.1_r6/xref/packages/apps/Bluetooth/src/com/android/
> > bluetooth/hfp/HeadsetStateMachine.java#2156
> 
> That really depends on how bluetoothd and bluedroid handle invalid BVRA AT
> command. Your assumption is there are other cases.

After investigation, for the bluedroid part, I found that it does check[1] out of range AT command and handles this in two ways[2]:
1. Pass unknown AT commands to upper application (if this feature is enabled)
2. Send ERROR AT command back to HF
(This handling is for all AT commands, not only for VRA.)

That is, gecko bluetooth may still get invalid BVRA state since bluetooth stack will not always reply ERROR back to HF.
Therefore, for more stability, I think we better do this reply once the invalid AT command is propagated to profile layer. (except, bluetooth daemon can handle this kind of error at all situations)



[1] http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/bta/ag/bta_ag_at.c#150
[2] http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/bta/ag/bta_ag_cmd.c#1293
Flags: needinfo?(shuang)
> After investigation, for the bluedroid part, I found that it does check[1]
> out of range AT command and handles this in two ways[2]:
> 1. Pass unknown AT commands to upper application (if this feature is enabled)
Per offline discussion, for this case, bluetooth stack will pass unknown AT command using another callback[1] instead of original event(say, VRA) callback.
Therefore the VRA handling function in gecko will not receive the invalid state.

> 2. Send ERROR AT command back to HF
> (This handling is for all AT commands, not only for VRA.)

[1] http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/bta/ag/bta_ag_cmd.c#1300
Flags: needinfo?(shuang)
Revised patch
- remove unnecessary error handling case.

Hi Shawn,

Could you help to review?
Thanks!
Attachment #8667834 - Flags: review?(shuang)
Attachment #8661191 - Attachment is obsolete: true
Comment on attachment 8667834 [details] [diff] [review]
Patch (for branch 2.2r)(v3): Bug 1182953: Add HFP Voice Recognition (VRA) support

Carry r+ from previous patch.
Attachment #8667834 - Flags: review?(shuang) → review+
blocking-b2g: --- → 2.2r?
Keywords: checkin-needed
Set feature-b2g 2.2r+ for required bluetooth feature VRA.
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/764e4309d7a1
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
See Also: → 1220067
Blocks: b2g-hfp-16
You need to log in before you can comment on or make changes to this bug.