Closed Bug 791329 Opened 7 years ago Closed 7 years ago

No microphone output through headphones when connected to a call

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
blocking-basecamp +

People

(Reporter: tchung, Assigned: mchen)

References

Details

(Whiteboard: [LOE:M])

Attachments

(2 files, 4 obsolete files)

When using a set of headphones with a mic, there is no audio outbound when connected to a call.  The receiving party can't hear you talk.

Environment: 
- 9-14-2012 otoro build
-  <project name="fake-dalvik" path="dalvik" remote="b2g" revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce"/>
- <project name="gaia" path="gaia" remote="b2g" revision="bc3eacff55cd46ac307e3c251760d2c21fb9c75b"/>
- <project name="releases-mozilla-central" path="gecko" remote="mozilla" revision="f415ee0ecf8829b6cb37659a80b1780acd64c41c"/>
- <project name="gonk-misc" path="gonk-misc" remote="b2g" revision="ec56d3ede37a051321ea7c1973b5c29ef53a88db"/>
- <project name="rilproxy" path="rilproxy" remote="b2g" revision="32106d4ea635ebe17a1610b643b398db639b8b97"/>

Repro:
1) connect a call to another phone
2) plug your headset in with a microphone (i have apple iphone earbuds)
3) talk through the mic on your headphones
4) Verify the receiving caller cannot hear you speak

Expected:
- microphone output works with headphone on call

Actual:
- microphone output is busted
The kernel seems have problem when detecting the headset with mic.
I checked dmesg when I inserting the headset with mic, I got 

<3>[09-16 09:41:18.172] [88: khsclntd][YXS]key_code 0x82, key_parm 0x0
<3>[09-16 09:41:18.172] [88: khsclntd][YXS]key_code 0x84, key_parm 0xff
<3>[09-16 09:41:18.622] [88: khsclntd][YXS]key_code 0x84, key_parm 0x0
<3>[09-16 09:41:18.622] [88: khsclntd][YXS]hs_tv[0] = 1347788478.181234
<3>[09-16 09:41:18.622] [88: khsclntd][YXS]hs_tv[1] = 1347788478.630178
<3>[09-16 09:41:18.622] [88: khsclntd][YXS]diff = 0, 448944
<3>[09-16 09:41:18.622] [88: khsclntd][YXS]This is Headset without MIC
Assignee: nobody → mchen
Two Issues here:

1. AudioManager didn't recognize the headset with or without mic.
2. Kernel driver can't detect the headset with mic.

Will update patch for issue 1 first then wait for Vendor on issue 2.
People will expect these types of headsets to work so let's block on this issue.
blocking-basecamp: ? → +
Whiteboard: [LOE:M]
Attached patch Part 1: GonkSwitch (obsolete) — Splinter Review
Modify the GonkSwitch for headphone status to support 
  state 1 - headset (headphone with mic)
  state 2 - headphone (without mic)
Attachment #662447 - Flags: feedback?(mwu)
Attached patch Part2: AudioManager (obsolete) — Splinter Review
Modify audio manager to recognize headset state for headset or headphone.
Attachment #662448 - Flags: feedback?(mwu)
Part 3: Audio HAL

Vendor reported that they do some customization on audio.primary.msm7627a.so so we need use that one from Otoro device. But currently we use the one from QAEP site.
(customization: 1. detecting mic on audio hal. 2. new SND DEVICE called HEADSET_MONO) 

This one will discuss with :mwu then update to github. (not only otoro but also m4)
The ODM partner also noticed us that the headset (with mic, 4 rings) for Otoro must uses the version of ground on 4th ring (European) not on 3th ring (USA). Or input device will be internal mic not the one on headset.
Comment on attachment 662447 [details] [diff] [review]
Part 1: GonkSwitch

dhylands, would you be able to review this?
Attachment #662447 - Flags: feedback?(mwu) → feedback?(dhylands)
(In reply to Marco Chen [:mchen] from comment #6)
> Vendor reported that they do some customization on audio.primary.msm7627a.so
> so we need use that one from Otoro device. But currently we use the one from
> QAEP site.
> (customization: 1. detecting mic on audio hal. 2. new SND DEVICE called
> HEADSET_MONO) 
> 
> This one will discuss with :mwu then update to github. (not only otoro but
> also m4)

r=me on replacing it. Go ahead and land it whenever you want.
Comment on attachment 662447 [details] [diff] [review]
Part 1: GonkSwitch

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

Looks good. I noticed that the file could probably use a whitespace cleanup (trailing whitespace on the ends of the lines), but that should probably be done as a separate patch.

::: hal/gonk/GonkSwitch.cpp
@@ +189,5 @@
> +class SwitchHandlerHeadphone: public SwitchHandler
> +{
> +public:
> +  SwitchHandlerHeadphone(const char* aDevPath) :
> +            SwitchHandler(aDevPath, SWITCH_HEADPHONES)

nit: indentation

@@ +202,5 @@
> +  {
> +    MOZ_ASSERT(aState);
> +
> +    return aState[0] == '0' ? SWITCH_STATE_OFF :
> +                     (aState[0] == '1' ? SWITCH_STATE_HEADSET : SWITCH_STATE_HEADPHONE);

nit: indentation
Attachment #662447 - Flags: feedback?(dhylands) → feedback+
Attached patch Part 1: GonkSwitch WIPv2 (obsolete) — Splinter Review
Remove spaces and adjust indentation.
Attachment #662447 - Attachment is obsolete: true
Attachment #662786 - Flags: review?(dhylands)
Comment on attachment 662786 [details] [diff] [review]
Part 1: GonkSwitch WIPv2

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

Looks good.
Attachment #662786 - Flags: review?(dhylands) → review+
Comment on attachment 662448 [details] [diff] [review]
Part2: AudioManager

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

::: dom/system/gonk/AudioManager.cpp
@@ +64,5 @@
> +    AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_WIRED_HEADPHONE,
> +            AUDIO_POLICY_DEVICE_STATE_AVAILABLE, "");
> +    sHeadsetState |= AUDIO_DEVICE_OUT_WIRED_HEADPHONE;
> +  } else if (aState == SWITCH_STATE_OFF) {
> +    AudioSystem::setDeviceConnectionState(static_cast<audio_devices_t>(sHeadsetState),

Can we use AUDIO_DEVICE_OUT_WIRED_HEADSET | AUDIO_DEVICE_OUT_WIRED_HEADPHONE instead of sHeadsetState?

@@ +104,5 @@
> +      status_t (*)(audio_devices_t, audio_policy_dev_state_t, const char*)
> +      >(AudioSystem::setDeviceConnectionState)) {
> +      InternalSetAudioRoutesICS(aEvent.status());
> +    }
> +    else if (static_cast<

This file puts else on the same line as }.

We should probably have an InternalSetAudioRoutes function which determines the right function to call. That'll reduce duplication of detection code.

@@ +256,5 @@
>    return NS_OK;
>  }
>  
>  void
>  AudioManager::SetAudioRoute(int aRoutes) {

We should be able to inline this function into InternalSetAudioRoutesGB and make things clearer.
Attached patch Part2: AudioManager WIPv2 (obsolete) — Splinter Review
> Can we use AUDIO_DEVICE_OUT_WIRED_HEADSET | AUDIO_DEVICE_OUT_WIRED_HEADPHONE
> instead of sHeadsetState?

I checked and the answer is negative.
Refer to AudioPolicyManager.cpp line 338, it will check the only one device can be connect/disconnect at a time.

Others follow the mwu's comment and thanks for suggestion.
Attachment #662448 - Attachment is obsolete: true
Attachment #662448 - Flags: feedback?(mwu)
Attachment #663307 - Flags: review?(mwu)
Attachment #663307 - Attachment is patch: true
Comment on attachment 663307 [details] [diff] [review]
Part2: AudioManager WIPv2

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

::: dom/system/gonk/AudioManager.cpp
@@ +65,5 @@
> +            AUDIO_POLICY_DEVICE_STATE_AVAILABLE, "");
> +    sHeadsetState |= AUDIO_DEVICE_OUT_WIRED_HEADPHONE;
> +  } else if (aState == SWITCH_STATE_OFF) {
> +    AudioSystem::setDeviceConnectionState(static_cast<audio_devices_t>(sHeadsetState),
> +            AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");

So if setDeviceConnectionState can't set more than one device at a time, and we manage to do a transition from HEADSET to HEADPHONE or vice versa, then SWITCH_STATE_OFF would only turn off one of the devices, right?

The HEADSET to HEADPHONE transition would also turn both devices on at the same time, which doesn't seem correct. Not sure how likely this scenario is, but it should be possible to guard against it.

@@ +73,5 @@
> +
> +static void
> +InternalSetAudioRoutesGB(SwitchState aState)
> +{
> +  audio_io_handle_t handle = 

trailing space
(In reply to Michael Wu [:mwu] from comment #15)
> So if setDeviceConnectionState can't set more than one device at a time, and
> we manage to do a transition from HEADSET to HEADPHONE or vice versa, then
> SWITCH_STATE_OFF would only turn off one of the devices, right?
> 
> The HEADSET to HEADPHONE transition would also turn both devices on at the
> same time, which doesn't seem correct. Not sure how likely this scenario is,
> but it should be possible to guard against it.

The phone devices has one pole for headset/headphone only. So the transition between them will be "headset <-> off <-> headphone".
There is no transition between headset/headpone directly.
In my opinion, there is no case we need to guard agains it.

> @@ +73,5 @@
> > +
> > +static void
> > +InternalSetAudioRoutesGB(SwitchState aState)
> > +{
> > +  audio_io_handle_t handle = 
> 
> trailing space

My fault, thanks for reminding.
Attachment #663307 - Flags: review?(mwu) → review+
Add reviewer for checkin-needed
Attachment #662786 - Attachment is obsolete: true
Add reviewer for checkin-needed
Attachment #663307 - Attachment is obsolete: true
Keywords: checkin-needed
For verification notes, except the patch here it still need

a. update android-device-otoro repository and execute extract_files.sh (update audio hal).
   Call build.sh for system.img then flash it.

b. If your ground ping is on third ring of headset, it will be detected as headphone (no mic) even it is a headset. You need a headset with ground pin on 4th ring (ex: the specific headset for otoro).
Blocks: 794598
Verified fix on otoro daily 10-15-2010 build.  i can make audible on a headset with a microphone.

  <project name="fake-dalvik" path="dalvik" remote="b2g" revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce"/>
  <project name="gaia" path="gaia" remote="b2g" revision="2b4f33c3fc2873681b53943a721a287116d1f313"/>
  <project name="releases-mozilla-central" path="gecko" remote="mozilla" revision="1ec4bdea9de7c0567810fd09cb55b02304b3186d"/>
  <project name="gonk-misc" path="gonk-misc" remote="b2g" revision="4fb0902d5e0683e3aa8a71e0ad3b27c5b51682ba"/>
  <project name="rilproxy" path="rilproxy" remote="b2g" revision="32106d4ea635ebe17a1610b643b398db639b8b97"/>
  <project name="librecovery" path="librecovery" remote="b2g" revision="1355ac6c5db1777baf6ed7a9f398ea2f30f85175"/>
Status: RESOLVED → VERIFIED
Blocks: 812607
You need to log in before you can comment on or make changes to this bug.