852497 - [tara][fugu]Support sprd FM radio

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Vendcom, defect)

Description

[James Zhang (Spreadtrum)]

When enter FM app, app crash.
Gecko use qcom chipset hard code noew, but tara device use brcm FM chipset.

Comment 1

[James Zhang (Spreadtrum)]

Attachment: brcm fm patch

Comment 2

[Jason Smith [:jsmith]]

Can you a crash stack URL here from crash data?

Comment 3

[James Zhang (Spreadtrum)]

Hi Jason, 
Our tara device use brcm FM chipset, not qcom chipset. 
So qcom FM hard code will cause our device crash.
Gecko need use board macro to config different chipset.

Comment 4

[James Zhang (Spreadtrum)]

Add styang and kkuo

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Dear james, 
Can you intergrate your FM volume control function call into android audio hal library?

Comment 6

[Xinhe Yan]

Dear Randy,
There were two functions need be pay close attention. AudioSystem::setStreamVolumeIndex & AudioSystem::setFmVolume. AudioSystem::setFmVolume is a qualcomm private function which other chip manufacturers does not support. 
AudioSystem::setStreamVolumeIndex can not set FM volume in sprd device because all the operation of FM in sprd are defined in jni level not in android audio hal.

My suggestion is move FM volume control implements to GonkFMRadio.cpp. Vendors can give chip or device specific implement.

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Dear Xinhe, 
As my experience, even use the non-qualcomm fm chip solution, we still add those volume control/routing control into audio system /audio hal/ audio policy manager. Becasue we can let the routing control/volume control logic into the same place. That means we can share the same framework part and implement the hw depend function on hal.

Comment 8

[Xinhe Yan]

Attachment: sprd fm patch

This is a sprd version of GonkFMRadio.cpp

Comment 9

[Xinhe Yan]

Dear Randy,
I already realize FM volume control in android audio modules.So FM will not restart in tara. But FM module still not work in tara because there is no FM driver in gecko hal for tara. I post a patch for tara. Someone need to review this patch. 

And I was consider how to choose FM driver in gecko. In android sometimes use macro to choose device. I don not know whether this work in gecko.

Comment 10

[James Zhang (Spreadtrum)]

Hi Randy & Viral,

   Pls review Xinhe's new commit, it's a new file for tara brcm FM. Thanks.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Michael,
For this case, should we use the build flag to support two kind of fm chip or use the dynamic module loading ?

Comment 12

[Marcelo Poli]

On Unagi, I've experienced crashes that locks the phone. The only way to unlock it is to remove the battery.
It seems that the white noise (no FM station) hangs everything.

Phone: Unagi
B2G: 1.1.0.0-prerelease
Platform: 18.0
Build ID: 20130418070305
Git Commit: 2013-04-05 21:17:35

Comment 13

[Michael Wu [:mwu]]

Wrong bug. This bug is for supporting FM radio on the tara device.

Comment 14

[James Zhang (Spreadtrum)]

Hi all,

   Do you have any schedule to support tara's FM?

Comment 15

[Marco Chen [:mchen]]

(In reply to James Zhang from comment #14)
Hi James,

If you think your patch is ready then you can ask the review from Justin Lebar who is the peer of HAL module and author of FM stuff.

By the way, I think at least you need to add a define in configure.in and use that to change the CPPSRCS in makefile.in.

Comment 16

[James Zhang (Spreadtrum)]

Xinhe, please commit the new SPRD FM patch base on v1.1 for Justin's review.

Comment 17

[James Zhang (Spreadtrum)]

We define sprd FM interface, so I change the bugzilla title.

Comment 18

[James Zhang (Spreadtrum)]

Attachment: GonkFMRadio_sprd.cpp

sprd FM implement

Comment 19

[James Zhang (Spreadtrum)]

I have uploaded the implement of sprd FM. But I don't know use which macro in configure.in, use vendor name "FM_QCOM_SUPPORT" or "FM_SPRD_SUPPORT" ?

Comment 20

[Marco Chen [:mchen]]

Hi James,

If your patch are ready for review, please assign the reviewer to Justin Lebar who is the peer of HAL module and the designer of FM feature. Thanks.

Comment 21

[James Zhang (Spreadtrum)]

Comment on attachment 782998 [details] [diff] [review]
GonkFMRadio_sprd.cpp

The implement of sprd FM, but I don't know use which macro in configure.in, is "FM_SPRD_SUPPORT" correct ?

Comment 22

[Marco Chen [:mchen]]

(In reply to James Zhang from comment #21)
> Comment on attachment 782998 [details] [diff] [review]
> GonkFMRadio_sprd.cpp

Hi James,

The format of your patch is wrong and please refer to the links as below for creating the correct patch. Or the reviewer is hard to help.

https://developer.mozilla.org/en-US/docs/Mercurial_Queues
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
 
> The implement of sprd FM, but I don't know use which macro in configure.in,
> is "FM_SPRD_SUPPORT" correct ?

Since different device can have different FM chip, 
 1. I would suggest to add a build flag on device folder (ex: https://github.com/mozilla-b2g/device-helix/blob/master/full_helix.mk#L35). 
 2. Then Android.mk in gonk-misk can be added codes for mapping build flag into env variable of make gecko. ex: https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L257. 
 3. In configure.in, we can check the env variable then add to AC_DEFINE. ex: https://mxr.mozilla.org/mozilla-central/source/configure.in#7699
 4. Finally we can choice the GonkFMXXX.cpp in the build time. ex: https://mxr.mozilla.org/mozilla-central/source/hal/moz.build#55.

Comment 23

[Xinhe Yan]

Attachment: use macro GONK_FM_VENDOR to choose FM vendor

Hi Justin Lebar,
Can you review the new patch for sprd fm?

As Marco suggest, I use macro GONK_FM_VENDOR to support different kinds of fm chip, is this acceptable to you?

If mozilla aggre to add this GONK_FM_VENDOR build flag, I will add mapping codes in gonk-misk.

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 782998 [details] [diff] [review]
GonkFMRadio_sprd.cpp

mwu may be able to get to this before me.

Comment 25

[Marco Chen [:mchen]]

Hi Michael,

As the request from Justin, could you help to review patch here? Or do you suggest any others if you have no bandwidth on this review? Thanks.

Comment 26

[Marco Chen [:mchen]]

Michael seems to be on vocation now until 08/17. 

Hi James, if you think this patch is urgent then please ask for Justin to review again.

Thanks.

Comment 27

[andy.liu]

(In reply to Marco Chen [:mchen] from comment #26)
> Michael seems to be on vocation now until 08/17. 

Hi James, if you think
> this patch is urgent then please ask for Justin to review again.

Thanks.

Hi Marco, We can wait until Michael comes back.

Comment 28

[andy.liu]

(In reply to Marco Chen [:mchen] from comment #26)
> Michael seems to be on vocation now until 08/17. 

Hi James, if you think
> this patch is urgent then please ask for Justin to review again.

Thanks.

Hi Marco ,could you please ask Michael to review again now?Thanks.

Comment 29

[Justin Lebar (not reading bugmail)]

Comment on attachment 782998 [details] [diff] [review]
GonkFMRadio_sprd.cpp

My last day at Mozilla is Friday, so I don't think I'm the right person to review this.

Comment 30

[Michael Wu [:mwu]]

Comment on attachment 782998 [details] [diff] [review]
GonkFMRadio_sprd.cpp

This looks like an older version of the latest patch, so marking it obsolete.

Comment 31

[Michael Wu [:mwu]]

In the end, this patch is just implementing another HAL by way of a named pipe in /data. This may be what we want, but I think we need to revisit bug 885370 . I have a few ideas.

Comment 32

[Michael Wu [:mwu]]

I'll take this.

Comment 33

[James Zhang (Spreadtrum)]

Hi mwu and mchen,

    Can you help to review and merge this patch into v1.2 branch? Tara and fugu also need this patch.

Comment 34

[Michael Wu [:mwu]]

Attachment: WIP: FM Radio driver for Fugu

This patch adds a FM radio driver for the fugu device that uses the standard V4L2 interface.

Comment 35

[Michael Wu [:mwu]]

Attachment: WIP: Add a standard V4L2 mode

This detects the presence/absence of the Tavarua driver and makes things behave accordingly.

Still a WIP because I need to configure spacing when starting a seek. The default bionic videodev2.h header doesn't have that field.

Comment 36

[Michael Wu [:mwu]]

The driver needs a few things done:

1. Needs to support more than one client opening, so another process can control the volume.
2. Might need adjustment to eliminate white noise when the device is turned on.
3. The default seek sensitivity probably needs to be increased - I keep missing a local station that is strong enough to listen to.

Comment 37

[Michael Wu [:mwu]]

Attachment: Add a standard V4L2 mode

This automatically selects the right field based on the Android version.

Need to find a reviewer for this code...

Comment 38

[Michael Wu [:mwu]]

Comment on attachment 830411 [details] [diff] [review]
Add a standard V4L2 mode

We don't have a lot of reviewers in this part of the code, so asking dhylands.

Comment 39

[Michael Wu [:mwu]]

There's a couple things we do in this patch:

1. Synchronously do certain operations that the tavarua driver does asynchronously. It seems that V4L2 expects most drivers to do things like seeking and tuning synchronously. Tavarua invented new events in order to do things asynchronously, but it seems that we do things off main thread anyway so it doesn't matter.
2. Skip the Tavarua specific initialization code.
3. Set more V4L2 configuration options, like stereo audio mode and channel spacing.

Comment 40

[Dave Hylands [:dhylands]]

Comment on attachment 830411 [details] [diff] [review]
Add a standard V4L2 mode

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

I have concerns about the I/O (ioctl) calls being made on the main thread.

Perhaps we need to file a bug to clean this up and move all of the ioctl calls off onto another thread.

::: hal/gonk/GonkFMRadio.cpp
@@ +306,5 @@
> +  sRadioEnabled = true;
> +
> +  hal::FMRadioOperationInformation info;
> +  info.operation() = hal::FM_RADIO_OPERATION_ENABLE;
> +  info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS;

This sends SUCCESS even if rc < 0. Is this intentional?

@@ +327,2 @@
>  
> +    pthread_join(sRadioThread, nullptr);

It looks like DisableFMRadio is called from the main thread.
http://dxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#207
http://dxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#541

If for some reason, the sRadioThread thread was hung, this would in turn hang the main thread.

In fact, I see all kinds of ioctl calls (like GetFMRadioFrequency()) being called on the main thread. Shouldn't all of this be done on a worker thread of some type?

@@ +353,2 @@
>    int rc = ioctl(sRadioFD, VIDIOC_S_HW_FREQ_SEEK, &seek);
> +  if (sTavaruaMode && rc >= 0)

Why don't we send the FM_RADIO_OPERATION_STATUS_SUCCESS for sTavaruaMode?

Is this because the success/failure is sent from runTavaruaRadio?

Perhaps a comment is warranted?

@@ +398,5 @@
>    int rc = ioctl(sRadioFD, VIDIOC_S_FREQUENCY, &freq);
>    if (rc < 0)
>      HAL_LOG(("Could not set radio frequency"));
> +
> +  if (sTavaruaMode && rc >= 0)

Why don't we send the FM_RADIO_OPERATION_STATUS_SUCCESS for sTavaruaMode?

Perhaps a comment is warranted?

Comment 41

[Michael Wu [:mwu]]

Comment on attachment 830411 [details] [diff] [review]
Add a standard V4L2 mode

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

Running ioctl calls from a separate thread would be nice. It seems like we're getting away with blocking on the main thread due to off thread compositing at the moment. I'll file a follow up. The most important calls to move off thread are the seeking and frequency setting calls, but we might be able to simplify things and just run everything off a separate thread.

::: hal/gonk/GonkFMRadio.cpp
@@ +306,5 @@
> +  sRadioEnabled = true;
> +
> +  hal::FMRadioOperationInformation info;
> +  info.operation() = hal::FM_RADIO_OPERATION_ENABLE;
> +  info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS;

Hmm.. I think I wanted this to succeed even if the band configuration wasn't successful, but it's probably better to fail early and not risk unpredictable results.

@@ +327,2 @@
>  
> +    pthread_join(sRadioThread, nullptr);

We can get away with it in this specific case since we know exactly what driver we're dealing with, and that it doesn't stall. For general V4L2 support though, we shouldn't assume that.

@@ +353,2 @@
>    int rc = ioctl(sRadioFD, VIDIOC_S_HW_FREQ_SEEK, &seek);
> +  if (sTavaruaMode && rc >= 0)

Yeah, the tavarua driver does a number of things asynchronously. We can use a general comment about the tavarua driver at the top of the file.

@@ +398,5 @@
>    int rc = ioctl(sRadioFD, VIDIOC_S_FREQUENCY, &freq);
>    if (rc < 0)
>      HAL_LOG(("Could not set radio frequency"));
> +
> +  if (sTavaruaMode && rc >= 0)

Ditto about tavarua.

Comment 42

[Michael Wu [:mwu]]

Attachment: Add a standard V4L2 mode, v2

I added a comment about the asynchronous update.

As for running these things off the main thread, I'll file a new bug for that.

I'll also file a new bug for doing better error reporting - in a lot of cases right now, we don't report when something fails, so that'll require more reworking. I actually did try adding an error message for the new ioctl call, but it has some weird behavior that I'll need more time to work out.

Comment 43

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/296ddf1e7dd9

Marking as leave open since the driver still needs work.

Comment 44

[Michael Wu [:mwu]]

For reference, I've filed bug 938806 and bug 938809 .

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/296ddf1e7dd9

Comment 47

[Michael Wu [:mwu]]

Attachment: WIP: FM Radio driver for Fugu, v2

This supports all the necessary features I think, but there's some issues that seem to cause the first open to occasionally not work right.

Comment 48

[Michael Wu [:mwu]]

Attachment: FM Radio driver for Fugu, v3

This one basically seems to work as expected. Had to fix some bugs in channel spacing configuration.

Comment 49

[Michael Wu [:mwu]]

Attachment: Patch for frameworks/base to use v4l2 volume controls

Comment 50

[Michael Wu [:mwu]]

Attachment: FM Radio driver for Fugu, v4

This just sets the default volume to zero.

Comment 51

[Michael Wu [:mwu]]

Attachment: FM Radio driver for Fugu, v5

This one mutes while seeking.

Comment 52

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Since bug 933916 is a dup of this bug and it affects Leo 1.1; noming for 1.3 or possibly push back to 1.4

Comment 53

[Michael Wu [:mwu]]

The commit that fixes bug 933916 landed already, so I think this will get fixed for 1.3 regardless. This bug is being held open until the new FM radio driver lands.

Comment 54

[Xinhe Yan]

v4l2 fm drive already merge to our branch, so this bug can be close. Thanks Michael.

Comment 55

[Marco Chen [:mchen]]

Based on comment 53 and 54, both patches for gecko (other bug) and kernel driver (this bug) are landed so close this bug.

According to there is no patch landed into our repos, I closed as worksforme due to FMRadio should be workable on newest partner image.