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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Will Wang [:WillWang]]

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

Comment 2

[Will Wang [:WillWang]]

Attachment: Patch (for branch 2.2r): Bug 1182953: Add HFP Voice Recognition (VRA) support

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!

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 4

[Will Wang [:WillWang]]

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)

Comment 5

[Will Wang [:WillWang]]

Attachment: Patch (for branch 2.2r)(v2): Bug 1182953: Add HFP Voice Recognition (VRA) support

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!

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

If it's for 2.2R, we don't have such plan at Mozilla end to add VR applications.

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Young-Jin,

You may want to be informed per comment 4.

Comment 10

[Will Wang [:WillWang]]

(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

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 12

[Will Wang [:WillWang]]

(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

Comment 13

[Will Wang [:WillWang]]

> 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

Comment 14

[Will Wang [:WillWang]]

Attachment: Patch (for branch 2.2r)(v3): Bug 1182953: Add HFP Voice Recognition (VRA) support

Revised patch
- remove unnecessary error handling case.

Hi Shawn,

Could you help to review?
Thanks!

Comment 15

[Will Wang [:WillWang]]

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.

Comment 16

[Ben Tian (inactive)]

Set feature-b2g 2.2r+ for required bluetooth feature VRA.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/764e4309d7a1