Comment 1 Pin Zhang [:pzhang] (inactive) 2012-04-27 22:46:11 PDT

I just found Samsung_Galaxy_S_II has two chips related to FM: FM Chip Si4709 and BCM4330 (not the BCM4329 i mentioned before). I think implementation based on Si4709 will be more straightforward.

Comment 2 Andy Buckingham 2012-06-01 06:26:36 PDT

I note the introduction of support for FM radio in to B2G with great interest and would like to draw your interest to a couple of interesting projects.

Firstly, a group of broadcasters from across the world are forming a discussion group surrounding the harmonisation of FM/HD/DAB radio access in mobile devices, to ensure stable and reliable 3rd party software implementations utilising the radio hardware in devices. It would be great to open discussions with you regarding this work to and you would be best contacting the chair person of this project, message me I can provide you with introductions.

Secondly, with regards to the FM application on B2G I'd like to inform you about a project called RadioDNS (http://radiodns.org) which provides a free, open, non-proprietary technology platform to build hybrid radio applications upon which can link received FM/DAB/HD radio to IP-based services. For example, RadioVIS is an application built on top of RadioDNS which allows the rich colour screen of a mobile device to be populated by relevant visuals and short text messages from the broadcaster, in synchronisation with the broadcast audio allowing for a more app-like experience than a large "87.8 FM" display. You can find out loads more information on the website above, or feel free to reach out to me for any additional information.

Comment 3 StevenLee[:slee] 2012-06-17 02:24:58 PDT

Created attachment 633873 [details] [diff] [review]
AudioManager supports FM

Comment 4 StevenLee[:slee] 2012-06-17 02:38:26 PDT

Created attachment 633876 [details] [diff] [review]
hal layer supports FM operations

Files in hal/gonk/radio except RadioManager.cpp are borrow from CodeAurora. RadioManager is the interface that connects the FM chips operations and hal layer.

Comment 5 Michael Wu [:mwu] 2012-06-17 05:48:58 PDT

(In reply to StevenLee from comment #4)
> Created attachment 633876 [details] [diff] [review]
> hal layer supports FM operations
> 
> Files in hal/gonk/radio except RadioManager.cpp are borrow from CodeAurora.
> RadioManager is the interface that connects the FM chips operations and hal
> layer.

Can you be specific about what repos you borrowed what files from?

Comment 6 StevenLee[:slee] 2012-06-17 19:20:19 PDT

The files are from quic. But I cannot find this repo now. Here is the link that is still available on codeaurora.
https://www.codeaurora.org/patches/quic/la/PATCH_M76XXUSNEKNLYA1090_8529_FM_Open_to_fail_10102011.tgz

Comment 7 Mounir Lamouri (:mounir) 2012-06-18 03:34:58 PDT

Comment on attachment 633876 [details] [diff] [review]
hal layer supports FM operations

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

The code you borrowed from "Code Aurora" might need to go in other-licenses/, please check with the appropriate persons.

Comment 8 Michael Wu [:mwu] 2012-06-18 12:19:00 PDT

It's apache 2.0 which is compatible with mpl 2 and should not go into other-licenses/.

Comment 9 Mounir Lamouri (:mounir) 2012-06-19 05:46:45 PDT

(In reply to Michael Wu [:mwu] from comment #8)
> It's apache 2.0 which is compatible with mpl 2 and should not go into
> other-licenses/.

Ok.

Also, Steven, make sure you ask a HAL peer [1] to review the changes under hal/.

[1] a HAL module is going to be created, for the moment, you can the assume the peers are :cjones, :jlebar and :mounir

Comment 10 Michael Wu [:mwu] 2012-06-25 02:21:32 PDT

Comment on attachment 633876 [details] [diff] [review]
hal layer supports FM operations

jlebar has agreed to take a look at the generic hal parts of this.

Comment 11 Michael Wu [:mwu] 2012-06-25 02:24:32 PDT

Comment on attachment 633873 [details] [diff] [review]
AudioManager supports FM

Eric, do you have any opinions on this? Does the bluetooth stuff want to use setDeviceConnectionState too? If so, we might want a more generic interface.

Comment 12 Justin Lebar (not reading bugmail) 2012-06-25 02:57:17 PDT

I'd like to understand this imported code better, Steven.  Are the copyright notices original, or did you add them yourself?  It seems strange to me that the copyright would be owned by "Code Aurora Forum".

Also, it's not clear to me what the license is.  Mwu says the license is Apache 2, but the files in the patch attached to this bug have BSD headers.  Which is it?

The file linked in comment 6 is a kernel driver patch.  How does that patch apply to this bug?

If we're taking external code here, I'd like to understand exactly where it came from and what the license is.  In particular, I'd like us to find a link to the repository containing the code, if that repository still exists.

Comment 13 Justin Lebar (not reading bugmail) 2012-06-25 02:59:02 PDT

It looks like Radio_WCN2243.cpp was not originally Mozilla code, but it has an MPL2 license header.  What's going on here?

Comment 14 Eric Chou [:ericchou] [:echou] 2012-06-25 03:13:44 PDT

(In reply to Michael Wu [:mwu] from comment #11)
> Comment on attachment 633873 [details] [diff] [review]
> AudioManager supports FM
> 
> Eric, do you have any opinions on this? Does the bluetooth stuff want to use
> setDeviceConnectionState too? If so, we might want a more generic interface.

Yes, it does. We will use setDeviceConnectionState

Currently on my branch, I hard-coded to route audio to Bluetooth SCO by calling
AudioManager::SetAudioRoute(3). So I think we need a more generic interface, but 
I haven't had any good idea yet.

Comment 15 Justin Lebar (not reading bugmail) 2012-06-25 05:15:56 PDT

Comment on attachment 633876 [details] [diff] [review]
hal layer supports FM operations

This is a high-level review.  I have enough questions about the interface that I wanted to send this back without going over all the code.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
> using mozilla::hal::FlashMode;
> using mozilla::hal::LightType;
> using mozilla::hal::LightMode;
> using mozilla::hal::SensorType;
> using mozilla::hal::SensorAccuracyType;
> using mozilla::hal::WakeLockControl;
> using mozilla::hal::SwitchState;
> using mozilla::hal::SwitchDevice;
>+using mozilla::hal::RadioChannelSpaceType;
>+using mozilla::hal::RadioOperation;
>+using mozilla::hal::RadioOperationStatus;
>+using mozilla::hal::RadioSeekDirection;

Where we say "radio", we mean "FM radio", right?  If so, every name in this
patch, including the filenames, has to change.  Sorry.  :-/

>@@ -66,29 +70,42 @@ struct WakeLockInformation {
>+struct RadioOperationInforamtion {
>+  RadioOperation operation;
>+  RadioOperationStatus status;
>+  uint32_t frequency;
>+};
>+
>+struct RadioSettings {
>+  uint32_t upperLimit;
>+  uint32_t lowerLimit;
>+  RadioChannelSpaceType spaceType;
>+};
> }

Why is it the caller's responsibility to choose these parameters?  Wouldn't it
make more sense for the caller to say "I'm in Japan", and have this code set
these parameters appropriately?  It looks like that's what
fmradio_qcom_fm_hal.c::enable() is kind of trying to do. (BTW, that code
/really/ needs to be in a namespace.)

I'm also confused as to how to map from upperLimit to a frequency -- it looks
like I have to multiply by some internal parameter to the FM code we imported
here?

We should have a clear and consistent unit for frequencies, unrelated to the
implementation.  Hertz or kilohertz sounds sensible to me.

>+    EnableRadio(RadioSettings aSettings);
>+    DisableRadio();
>+    RadioSeek(RadioSeekDirection aDirection);
>+    sync GetRadioSettings()
>+      returns (RadioSettings settings);
>+    SetRadioFrequency(int32 frequency);
>+    sync GetRadioFrequency()
>+      returns (int32 frequency);

Please use a consistent type for frequency (it's a uint32 in RadioSettings).  Either uint32 or int32 is fine.

>+++ b/hal/HalTypes.h
>@@ -48,17 +48,59 @@ enum SwitchDevice {
> 
> enum SwitchState {
>   SWITCH_STATE_UNKNOWN = -1,
>   SWITCH_STATE_ON,
>   SWITCH_STATE_OFF,
>   NUM_SWITCH_STATE
> };
> 
>+class RadioOperationInforamtion;
>+
>+enum RadioOperation {
>+  RADIO_OPERATION_UNKNOWN = -1,
>+  RADIO_OPERATION_ENABLE,
>+  RADIO_OPERATION_DISABLE,
>+  RADIO_OPERATION_SEEK,
>+  NUM_RADIO_OPERATION
>+};
>+
>+enum RadioOperationStatus {
>+  RADIO_OPERATION_STATUS_UNKNOWN = -1,
>+  RADIO_OPERATION_STATUS_SUCCESS,
>+  RADIO_OPERATION_STATUS_FAIL,
>+  NUM_RADIO_OPERATION_STATUS
>+};

It looks like SUCCESS is the only value ever used here.  If so, can we just
remove this enum altogether?

>+
>+enum RadioSeekDirection {
>+  RADIO_SEEK_DIRECTION_UNKNOWN = -1,
>+  RADIO_SEEK_DIRECTION_UP,
>+  RADIO_SEEK_DIRECTION_DOWN,
>+  NUM_RADIO_SEEK_DIRECTION
>+};
>+
>+enum RadioBandType {
>+  RADIO_BAND_TYPE_UNKNOWN = -1,
>+  RADIO_BAND_TYPE_US_EU,
>+  RADIO_BAND_TYPE_JAPAN_STANDARD,
>+  RADIO_BAND_TYPE_JAPAN_WIDE,
>+  RADIO_BAND_TYPE_USER_DEFINED,
>+  NUM_RADIO_BAND_TYPE
>+};

It looks like you never use this type or these values.  But per my comment above, it certainly seems like you should.

>+enum RadioChannelSpaceType {
>+  RADIO_CHANNEL_SPACE_TYPE_UNKNOWN = -1,
>+  RADIO_CHANNEL_SPACE_TYPE_200KHZ,
>+  RADIO_CHANNEL_SPACE_TYPE_100KHZ,
>+  RADIO_CHANNEL_SPACE_TYPE_50KHZ,
>+  NUM_RADIO_CHANNEL_SPACE_TYPE
>+}; 

It's not clear to me why this is an enum instead of an int.  Even if some code only supports the values "200", "100", and "50", we could assert, right?

Comment 16 StevenLee[:slee] 2012-07-01 20:32:55 PDT

Hi Justin and Michael,
Sorry for late reply. My daughter was born last week.

(In reply to Justin Lebar [:jlebar] from comment #12)
> I'd like to understand this imported code better, Steven.  Are the copyright
> notices original, or did you add them yourself?  It seems strange to me that
> the copyright would be owned by "Code Aurora Forum".
> 
> Also, it's not clear to me what the license is.  Mwu says the license is
> Apache 2, but the files in the patch attached to this bug have BSD headers. 
> Which is it?
Radio_WCN2243.cpp, fm_hal.h, fm_hal_routines.h, fmradio.h, and fmradio_qcom_fm_hal.c are from the patch.
But I renamed the file, fmhal.c, to Radio_WCN2243.cpp. I also modified some code to eliminate compile warning.
tavarua.h is not in the patch but I found it in CodeAurora.

> 
> The file linked in comment 6 is a kernel driver patch.  How does that patch
> apply to this bug?
I pasted the wrong link, it should be https://www.codeaurora.org/patches/quic/la/PATCH_M8960AAAAANAZN1021_6870_and_9584_FM_HAL_Layer_12122011.tar.gz

> 
> If we're taking external code here, I'd like to understand exactly where it
> came from and what the license is.  In particular, I'd like us to find a
> link to the repository containing the code, if that repository still exists.
I cannot find the FM related code in the repository. So that I searched the patch list and got these code.

Comment 17 Justin Lebar (not reading bugmail) 2012-07-02 02:52:57 PDT

Thanks for clarifying the situation, Steven.

I think all this external code should live in a separate directory, so it's clear what is and isn't Mozilla-original code.  Additionally, we want to be extremely clear about what modifications we've made to the external code -- for that purpose, it would be sufficient to post a patch with the unmodified external code, then post a second patch with your modifications.

> tavarua.h is not in the patch but I found it in CodeAurora.

We need a link to this as well, please.

Comment 18 StevenLee[:slee] 2012-07-02 19:19:22 PDT

Created attachment 638572 [details] [diff] [review]
The patch from CodeAurora

This is the patch from https://www.codeaurora.org/patches/quic/la/PATCH_M8960AAAAANAZN1021_6870_and_9584_FM_HAL_Layer_12122011.tar.gz, but I delete the files that I do not use in this bug. Please check it.

Comment 19 Justin Lebar (not reading bugmail) 2012-07-02 21:02:37 PDT

> for that purpose, it would be sufficient to post a patch with the unmodified external code, then 
> post a second patch with your modifications.

What I think we should do is check this code in as two (or more) commits.  The first commit would contain the unmodified CodeAurora code (just the files you intend to keep), not hooked up to the build system.  The second commit would apply your modifications and hook it up to the build.

That way, Firefox will build correctly at both commits, but we'll have a clear record of the changes we made to the CodeAurora code.

Comment 20 StevenLee[:slee] 2012-07-02 22:16:28 PDT

Hi Justin,
Could I put the code in gonk, /root/frameworks/base/fm_radio/qcom/codeAurora and /root/frameworks/base/fm_radio/qcom/? /qcom/codeAurora is the unmodified code and /qcom/ is the code the will be called by Gecko. Do you think it is OK?
Thanks~

Comment 21 Justin Lebar (not reading bugmail) 2012-07-03 06:28:35 PDT

> Could I put the code in gonk,

If this code needs to be called by Gecko, how would you invoke it if it lived in Gonk?  Do we have some cross-library calling set up that I'm not aware of?

Comment 22 Justin Lebar (not reading bugmail) 2012-07-03 21:48:43 PDT

FYI, we've discussed what happens when two separate apps try to use the FM radio on the b2g mailing list.

Please see Jonas's and my posts in [1], and let us know if you think this is the right approach!

[1] https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.b2g/5f1sNxvYqR0

Comment 23 StevenLee[:slee] 2012-07-03 23:35:53 PDT

(In reply to Justin Lebar [:jlebar] from comment #21)
> If this code needs to be called by Gecko, how would you invoke it if it
> lived in Gonk?  Do we have some cross-library calling set up that I'm not
> aware of?
I meant that we can make fm_radio as a library and Gecko can call the functions through this library.

Comment 24 Justin Lebar (not reading bugmail) 2012-07-04 07:23:23 PDT

> I meant that we can make fm_radio as a library and Gecko can call the functions through this library.

Oh, I see.  I guess you could do that.  I'd defer to whatever mwu would prefer.

Comment 25 Eric Chou [:ericchou] [:echou] 2012-07-08 19:17:44 PDT

Comment on attachment 633873 [details] [diff] [review]
AudioManager supports FM

Sorry for the late reply. As I mentioned before, Bluetooth also needs an interface to control audio route. No idea you're going to provide a generic interface in this patch or will open another bug. Just let Kyle and I know the final decision.

Comment 26 Kyle Machulis [:kmachulis] [:qdot] 2012-07-08 23:05:44 PDT

I still say we agree on a setting name and use the settings API for routing. Means we get e10s for free, and since we only care about routing on the B2G platform, we don't really need something that has to be cross-platform.

Comment 27 StevenLee[:slee] 2012-07-11 22:34:49 PDT

(In reply to Justin Lebar [:jlebar] from comment #24)
> > I meant that we can make fm_radio as a library and Gecko can call the functions through this library.
> 
> Oh, I see.  I guess you could do that.  I'd defer to whatever mwu would
> prefer.

Hi mwu,
What do you think if I move the fm implementations to gonk as a library?

Comment 28 Michael Wu [:mwu] 2012-07-12 15:04:52 PDT

(In reply to StevenLee from comment #27)
> (In reply to Justin Lebar [:jlebar] from comment #24)
> > > I meant that we can make fm_radio as a library and Gecko can call the functions through this library.
> > 
> > Oh, I see.  I guess you could do that.  I'd defer to whatever mwu would
> > prefer.
> 
> Hi mwu,
> What do you think if I move the fm implementations to gonk as a library?

Not sure what that achieves. Can you elaborate?

Comment 29 StevenLee[:slee] 2012-07-12 20:10:02 PDT

(In reply to Michael Wu [:mwu] from comment #28)
> > Hi mwu,
> > What do you think if I move the fm implementations to gonk as a library?
> 
> Not sure what that achieves. Can you elaborate?

How about hardware/qcom/fm/? 
For "other-company", it could be hardware/other-company/fm.

Comment 30 Michael Wu [:mwu] 2012-07-12 20:15:38 PDT

(In reply to StevenLee from comment #29)
> (In reply to Michael Wu [:mwu] from comment #28)
> > > Hi mwu,
> > > What do you think if I move the fm implementations to gonk as a library?
> > 
> > Not sure what that achieves. Can you elaborate?
> 
> How about hardware/qcom/fm/? 
> For "other-company", it could be hardware/other-company/fm.

I'm asking what you're trying to do. Are you trying to make a hal layer for fm radio?

Comment 31 StevenLee[:slee] 2012-07-12 20:46:20 PDT

From comment #17, Justin replied that external code should live in a separate folder. So that I think fm control code should be put in gonk. 
In gecko, we still have an interface for fm in hal, say RadioManager. Apps could use RadioManager to control fm operations. 
In gonk, the fm controls should be able to fit RadioManager. I think this structure is clear. For our partners, they just need to modify the code in gonk.

Comment 32 Justin Lebar (not reading bugmail) 2012-07-12 23:08:24 PDT

Mike can correct me if I'm wrong, but AIUI we've been modifying Gecko a lot more than we've been modifying Gonk.  Perhaps that alone is a good reason to put this code in Gecko.  That I wanted it in a different folder shouldn't imply that it needs to go in a completely separate project.

But I don't really care where the code lives, either way.

Comment 33 Michael Wu [:mwu] 2012-07-13 20:09:21 PDT

Ok makes sense.

Steven, I think splitting this out to gonk and creating a sort of HAL which is easy for partners to interface to can be done in a follow up. For now, let's just focus on making the code correct within gecko.

Comment 34 Michael Wu [:mwu] 2012-08-15 10:18:56 PDT

Comment on attachment 633873 [details] [diff] [review]
AudioManager supports FM

Clearing review for now since the bigger, more important piece of this bug needs a new patch.

Comment 35 StevenLee[:slee] 2012-08-20 00:52:50 PDT

Created attachment 653299 [details] [diff] [review]
Part 1 - FM chip control

Comment 36 StevenLee[:slee] 2012-08-20 00:53:43 PDT

Created attachment 653300 [details] [diff] [review]
Part 2 - FM functions in hal

Comment 37 StevenLee[:slee] 2012-08-20 00:55:21 PDT

Created attachment 653302 [details] [diff] [review]
Part 3 - AudioRouting and volume settings in AudioManager

Comment 38 Justin Lebar (not reading bugmail) 2012-08-20 16:57:45 PDT

Comment on attachment 653299 [details] [diff] [review]
Part 1 - FM chip control

I'm confused about what code is for inclusion in Firefox here.  Is is just parts 1, 2, and 3, not including the patch from code aurora?

More high-level questions:

>diff --git a/hal/gonk/fmradio/FmRadio_WCN2243.cpp b/hal/gonk/fmradio/FmRadio_WCN2243.cpp
>+++ b/hal/gonk/fmradio/FmRadio_WCN2243.cpp
>@@ -0,0 +1,963 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */

To be clear: You wrote all the code in this file completely from scratch?  You did not copy/paste any code here from anywhere else?

>diff --git a/hal/gonk/fmradio/fm_hal.h b/hal/gonk/fmradio/fm_hal.h
>+++ b/hal/gonk/fmradio/fm_hal.h
>+/*
>+Copyright (c) 2011, Code Aurora Forum. All rights reserved.

This file, on the other hand, comes from Code Aurora's fm_hal.h, plus some of
your own modifications, right?

We need to talk to Gerv about the technicalities of complying with the
three-clause BSD license here.  Do we put an MPL license on our changes?  And
where do we add Code Aurora so that we reproduce the copyright notice in our
binary application (i.e., in b2g)?

The patches here don't match what I was asking for in comment 19:

> What I think we should do is check this code in as two (or more) commits.
> The first commit would contain the unmodified CodeAurora code (just the files
> you intend to keep), not hooked up to the build system.  The second commit
> would apply your modifications and hook it up to the build. (*)
>
> That way, Firefox will build correctly at both commits, but we'll have a
> clear record of the changes we made to the CodeAurora code.

In other words, the "second patch" above would be this patch, except that
instead of fm_hal.h being an entirely new file as it is here, this patch would
contain your modifications to fm_hal.h from Code Aurora.

Also, the Code Aurora patch would be a patch which applies to mozilla-central
(placing the relevant files in their correct places) and would not contain any
files which you don't intend to keep.

Does that make sense?  I would like to do this so that there is a clear record
of what code we adopted, and exactly how we modified it.  This is particularly
important if we ever want to take changes to these files in the future from
Code Aurora: We need a record of our modifications so that we know how we've
diverged from upstream, so we know how upstream's changes apply to us.

>diff --git a/hal/gonk/fmradio/fmradio.h b/hal/gonk/fmradio/fmradio.h
>new file mode 100644
>--- /dev/null
>+++ b/hal/gonk/fmradio/fmradio.h
>@@ -0,0 +1,173 @@
>+/*
>+ * Copyright (C) ST-Ericsson SA 2010
>+ * Copyright (C) 2010 The Android Open Source Project
>+ *
>+ * Licensed under the Apache License, Version 2.0 (the "License");

We also need Gerv's input on how to comply with Apache 2 here.

>diff --git a/hal/gonk/fmradio/tavarua.h b/hal/gonk/fmradio/tavarua.h

Where did this file come from?

The versions I found online (e.g. this one:

https://www.codeaurora.org/git/projects/froyo-gb-dsds-7227/repository/revisions/39141d7a9dbdd2e9acf006430a7e7557ffd1efce/entry/kernel/include/media/tavarua.h

) have copyright notices.

Comment 39 Justin Lebar (not reading bugmail) 2012-08-20 17:01:58 PDT

gerv, see comment 38 for licensing questions.

Comment 40 Justin Lebar (not reading bugmail) 2012-08-20 17:04:53 PDT

Comment on attachment 653299 [details] [diff] [review]
Part 1 - FM chip control

Also, as a high-level comment: All the Code Aurora code we import should be in its own C++ namespace.

Comment 41 Gervase Markham [:gerv] 2012-08-21 03:06:19 PDT

(In reply to Justin Lebar [:jlebar] from comment #38)
> >diff --git a/hal/gonk/fmradio/fm_hal.h b/hal/gonk/fmradio/fm_hal.h
> >+++ b/hal/gonk/fmradio/fm_hal.h
> >+/*
> >+Copyright (c) 2011, Code Aurora Forum. All rights reserved.
> 
> This file, on the other hand, comes from Code Aurora's fm_hal.h, plus some of
> your own modifications, right?
> 
> We need to talk to Gerv about the technicalities of complying with the
> three-clause BSD license here.  Do we put an MPL license on our changes?  And
> where do we add Code Aurora so that we reproduce the copyright notice in our
> binary application (i.e., in b2g)?

If the file is exclusively or mostly code from a 3-clause BSD source, the best thing is simply to leave it as 3-clause BSD.

If it's mostly code we've written, then we can add an MPL header, but the BSD header requires its own preservation, so we should keep a copy of that header at an appropriate point in the file, with a note about which code it used to belong to. Let me know if this is the case, and we can work on the specific situation.

> >diff --git a/hal/gonk/fmradio/fmradio.h b/hal/gonk/fmradio/fmradio.h
> >new file mode 100644
> >--- /dev/null
> >+++ b/hal/gonk/fmradio/fmradio.h
> >@@ -0,0 +1,173 @@
> >+/*
> >+ * Copyright (C) ST-Ericsson SA 2010
> >+ * Copyright (C) 2010 The Android Open Source Project
> >+ *
> >+ * Licensed under the Apache License, Version 2.0 (the "License");
> 
> We also need Gerv's input on how to comply with Apache 2 here.

You comply with Apache 2.0 by leaving the Apache 2.0 license header in the file. That's it :-) Apache 2.0 is fully permitted in the Mozilla source code base.
 
> The versions I found online (e.g. this one:
> 
> https://www.codeaurora.org/git/projects/froyo-gb-dsds-7227/repository/
> revisions/39141d7a9dbdd2e9acf006430a7e7557ffd1efce/entry/kernel/include/
> media/tavarua.h
> 
> ) have copyright notices.

Definitely do not strip copyright notices off any file. If there is code whose copyright licensing status is uncertain, then please ask the licensing team to help you try and make sure it's suitable for use.

Gerv

Comment 42 Justin Lebar (not reading bugmail) 2012-08-21 10:03:07 PDT

Thanks, Gerv.

Do we have to do anything to comply with the second clause of the BSD license?

Comment 43 StevenLee[:slee] 2012-08-21 20:01:29 PDT

(In reply to Justin Lebar [:jlebar] from comment #38)
> Comment on attachment 653299 [details] [diff] [review]
> Part 1 - FM chip control
> 
> I'm confused about what code is for inclusion in Firefox here.  Is is just
> parts 1, 2, and 3, not including the patch from code aurora?
> 

I will update part0 for code auroa. 

> More high-level questions:
> 
> >diff --git a/hal/gonk/fmradio/FmRadio_WCN2243.cpp b/hal/gonk/fmradio/FmRadio_WCN2243.cpp
> >+++ b/hal/gonk/fmradio/FmRadio_WCN2243.cpp
> >@@ -0,0 +1,963 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> To be clear: You wrote all the code in this file completely from scratch? 
> You did not copy/paste any code here from anywhere else?
I will revert this header to its original version.

> 
> >diff --git a/hal/gonk/fmradio/tavarua.h b/hal/gonk/fmradio/tavarua.h
> 
> Where did this file come from?
> 
> The versions I found online (e.g. this one:
> 
> https://www.codeaurora.org/git/projects/froyo-gb-dsds-7227/repository/
> revisions/39141d7a9dbdd2e9acf006430a7e7557ffd1efce/entry/kernel/include/
> media/tavarua.h
> 
> ) have copyright notices.
This file is generated when I built the repo of code aurora and I copied it.
I tried some "tavarau.h" and found that they cannot be used. Fm radio uses some
enumerators that are only defined in this version. So that I copied this one.
Should I add code aurora's license header?

Comment 44 Justin Lebar (not reading bugmail) 2012-08-21 20:16:09 PDT

> [tavarau.h] is generated when I built the repo of code aurora and I copied it.

I'm not sure how to handle this, although I expect that putting Code Aurora's copyright notice is a reasonable thing to do.  Perhaps Gerv has insight.

Comment 45 Justin Lebar (not reading bugmail) 2012-08-21 21:43:24 PDT

Comment on attachment 653300 [details] [diff] [review]
Part 2 - FM functions in hal

Two high-level comments:

1. We're inconsistent about the name of this thing in this patch: I see
"FMRadio", "FmRadio" (in filenames), "FM", and "Radio".  

I'd say: "FM" should always be capitalized, and we should never call this thing
the "radio", because the phone has many radios.  "FMRadio" and "FM" probably
both have a place, but we should never talk about "the FM".

2. The DOM API returns DOMRequest objects for most actions.  The request may
succeed or fail, and we need to fire the onsuccess method or onfailure method
on the right object.

But at the moment, it's not straightforward to do this, because the API here
merely notifies me that a seek completed, but doesn't tell me which one.

I'd been thinking that this was OK, because two seeks can't run at the same
time.  So the front-end could call DoSeek(), which would make a synchronous
call up to the parent to find out if we're seeking.  If so DoSeek() would
immediately return a failure code.

Then, if DoSeek() didn't return failure, the front-end could assume that the
seek will eventually succeed.  It could remember that seek's DOMRequest, and
then when it sees a "seek succeeded" message, it could fire onsuccess on that
particular DOMRequest.

But this won't work if a seek can fail for a reason that we can't detect when
the seek is made.  Because suppose a page does the following:

  fmradio.seekUp(); // fails for some weird reason
  fmradio.seekUp(); // fails because there's an outstanding seek

Now we'll fire a "seek failed" message for the second seek.  But which
DOMRequest should we fire "onfailure" on?  We don't know which seek failed!

Unfortunately I think there are lots of "weird reasons" a seek might fail, that
we can't synchronously detect at the time the seek is made.  The radio might be
disabled while the seek is pending.  The antenna might be unplugged.  Or, I
think a seek could fail because we can't find any channels above the current
frequency, right?

So it seems like we need to attach an identifier to these callbacks.

What do you think?

>diff --git a/hal/gonk/GonkRadio.cpp b/hal/gonk/GonkRadio.cpp

GonkFMRadio.cpp, please.

>+void EnableFMRadio(const hal::FMRadioSettings& aInfo) {
>+  if (!sFmRadioManager) {
>+    sFmRadioManager = new FmRadioManager();
>+  }
>+
>+  sFmRadioManager->Enable(aInfo);
>+}

It's not clear to me what we gain by having the FmRadioManager class.  What if
we pushed the FMRadioManager implementation down into this file?

>+void DisableFMRadio() {
>+  sFmRadioManager->Disable();
>+}
>+
>+void FMRadioSeek(const FMRadioSeekDirection& aDirection) {
>+  sFmRadioManager->Seek(aDirection);
>+}

How do you ensure that we don't call FMRadioSeek while sFmRadioManager is null?
And so on for the rest of the file.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>@@ -739,10 +739,90 @@ SetProcessPriority(int aPid, ProcessPrio
>+typedef mozilla::ObserverList<FMRadioOperationInformation> RadioObserverList;

FMRadioObserverList, please.

>+static RadioObserverList sRadioObserver;

sFMRadioObserver.

>+static bool sCancelSeek;

Is this here because when you cancel a seek, do you get a seek-successful
notification?

If so, this logic needs to live in the root process.  Otherwise we have the
following race condition:

 * Process 1 calls seek().
 * Process 2 calls cancelSeek(), canceling process 1's seek.
 * Process 1 gets a seek-completed notification.

It's probably better to have this logic in the gonk-specific code (which lives
in the root process) anyway, because it's kind of a hack around strange
behavior the particular FM stack we have in Gonk.

>+void
>+EnableFMRadio(const FMRadioSettings& aInfo) {
>+  AssertMainThread();
>+  RETURN_PROXY_IF_SANDBOXED(EnableFMRadio(aInfo));
>+}

This function returns void, so you should use PROXY_IF_SANDBOXED (see the macro
definitions).

>+void
>+DisableFMRadio() {
>+  AssertMainThread();
>+  RETURN_PROXY_IF_SANDBOXED(DisableFMRadio());
>+}

... similarly here and elsewhere.

>diff --git a/hal/fallback/FallbackRadio.cpp b/hal/fallback/FallbackRadio.cpp
>+++ b/hal/fallback/FallbackRadio.cpp
>@@ -0,0 +1,46 @@
>+void
>+FMRadioSeek(const hal::FMRadioSeekDirection& aDirection)
>+{}
>+
>+void
>+GetFMRadioSettings(hal::FMRadioSettings* aInfo)
>+{}

We should assign something into aInfo, indicating that the radio is disabled,
the antenna is detached, etc.

>+void
>+SetFMRadioFrequency(const uint32_t frequency)
>+{}
>+
>+void
>+GetFMRadioFrequency(uint32_t *frequency)
>+{}
>+
>+bool IsFMRadioOn()
>+{}

Don't you want to return something here?

>+void GetFMRadioSignalStrength(uint32_t* strength)
>+{}

I'd rather this actually returned uint32_t instead of taking an out param.  I
know the IPDL methods may need to take an out param, but there's no need to do
so here.  Not using an out param avoids the footgun where you do

  uint32_t strength;
  GetFMRadioSignalStrength(&strength);
  // now strength is undefined!

>diff --git a/hal/gonk/FmRadioManager.h b/hal/gonk/FmRadioManager.h

FMRadioManager, here and elsewhere, please.

>+#ifndef _mozilla_gonk_radio_
>+#define _mozilla_gonk_radio_

_mozilla_gonk_FMRadioManager_h, please.

>+class RadioRunnable : public nsRunnable
>+{
>+public:
>+  RadioRunnable(FMRadioOperationInformation& info) : mInfo(info) {
>+  }
>+  
>+  NS_IMETHOD Run() {
>+    NotifyFMRadioStatus(mInfo);
>+    return NS_OK;
>+  }
>+private:
>+  FMRadioOperationInformation mInfo;
>+};

I think you could move this to the cpp file.

>+class FmRadioManager
>+{
>+public:
>+   FmRadioManager();
>+  ~FmRadioManager();
>+
>+  void Enable(const FMRadioSettings& aInfo);
>+  void Disable();
>+  void Seek(const FMRadioSeekDirection& aDir);
>+  void GetRadioSettings(FMRadioSettings *aInfo);
>+  void SetRadioFrequency(const uint32_t aFrequency);
>+  void GetRadioFrequency(uint32_t* aFrequency);

Return the uint32_t, please.

>+  bool IsRadioOn();
>+  void GetRadioSignalStrength(uint32_t* strength);
>+  void CancelRadioSeek();
>+  static bool ConvertSettings(FMRadioSettings &aSettings)
>+  {

Could you please move the implementation of this to the cpp file?  (Unless this
needs to be very fast for some reason?)

>diff --git a/hal/gonk/fmradio/FmRadioManager.cpp b/hal/gonk/fmradio/FmRadioManager.cpp
>new file mode 100644
>--- /dev/null
>+++ b/hal/gonk/fmradio/FmRadioManager.cpp
>+#include "../FmRadioManager.h"

You should change the Makefile.in to include hal/gonk, so you don't have to do "../".

I guess this file is in the "hal/gonk/fmradio" directory because it interacts
with the Code Aurora code?  It's kind of confusing to have this code in a
different directory from its header file.  But I guess the idea is that we
could have different implementations of FMRadioManager.cpp for different
chipsets?  Anyway, it seems like all of this code should be shoved into
GonkFMRadio.cpp, so maybe it doesn't matter.  :)

I need to look at part 1 before I understand this file, so I didn't look very
closely here.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>--- a/hal/sandbox/PHal.ipdl
>+++ b/hal/sandbox/PHal.ipdl
>@@ -17,16 +17,20 @@ using mozilla::hal::FlashMode;
> using mozilla::hal::LightType;
> using mozilla::hal::LightMode;
> using mozilla::hal::SensorType;
> using mozilla::hal::SensorAccuracyType;
> using mozilla::hal::WakeLockControl;
> using mozilla::hal::SwitchState;
> using mozilla::hal::SwitchDevice;
> using mozilla::hal::ProcessPriority;
>+using mozilla::hal::FMRadioBandType;
>+using mozilla::hal::FMRadioOperation;
>+using mozilla::hal::FMRadioOperationStatus;
>+using mozilla::hal::FMRadioSeekDirection;
> using nsIntRect;
> using PRTime;
> 
> namespace mozilla {
> 
> namespace hal {
> struct BatteryInformation {
>   double level;
>@@ -68,29 +72,44 @@ struct WakeLockInformation {
> 
> struct ScreenConfiguration {
>   nsIntRect rect;
>   ScreenOrientation orientation;
>   uint32_t colorDepth;
>   uint32_t pixelDepth;
> };
> 
>+struct FMRadioOperationInformation {
>+  FMRadioOperation operation;
>+  FMRadioOperationStatus status;
>+  uint32_t frequency;
>+};

This is kind of a hack, but OK.  :)

Comment 46 Justin Lebar (not reading bugmail) 2012-08-21 21:44:19 PDT

Comment on attachment 653302 [details] [diff] [review]
Part 3 - AudioRouting and volume settings in AudioManager

We dropped letting the FM radio play over the phone's speaker for v1, so does that mean we don't need this patch?  Please re-request review if we do need the patch.

Comment 47 Justin Lebar (not reading bugmail) 2012-08-22 09:39:07 PDT

Comment on attachment 653299 [details] [diff] [review]
Part 1 - FM chip control

Cancelling the review until we split out the Code Aurora code from our changes.

Comment 48 StevenLee[:slee] 2012-08-22 20:01:01 PDT

(In reply to Justin Lebar [:jlebar] from comment #45)
> Comment on attachment 653300 [details] [diff] [review]
> Part 2 - FM functions in hal
> 
> Two high-level comments:
> 2. The DOM API returns DOMRequest objects for most actions.  The request may
> succeed or fail, and we need to fire the onsuccess method or onfailure method
> on the right object.
> Unfortunately I think there are lots of "weird reasons" a seek might fail,
> that
> we can't synchronously detect at the time the seek is made.  The radio might
> be
> disabled while the seek is pending.  The antenna might be unplugged.  Or, I
> think a seek could fail because we can't find any channels above the current
> frequency, right?
> 
> So it seems like we need to attach an identifier to these callbacks.
> 
> What do you think?
The original seeking function of code aurora is a synchronous function. There is a
busy loop keeps checking a flag. Do they think this function never fails? I
think adding an identifier is a good idea. Apps can get more information from the
callback and do the right decision. But I need to test if the FM control code would 
callback twice if I have 2 successive seeking without waiting its first callback. 

> >+void EnableFMRadio(const hal::FMRadioSettings& aInfo) {
> >+  if (!sFmRadioManager) {
> >+    sFmRadioManager = new FmRadioManager();
> >+  }
> >+
> >+  sFmRadioManager->Enable(aInfo);
> >+}
> 
> It's not clear to me what we gain by having the FmRadioManager class.  What
> if
> we pushed the FMRadioManager implementation down into this file?
I think FMRadioManager is the interface that gecko hal can communicate with android.
FMRadioManager implementation should move to android in the future. Ideally, if we 
need to support other FM chips, we can implement another FMRadioManager in android 
without modifying the gecko hal. 
So that I put |static bool ConvertSettings(FMRadioSettings &aSettings)| in the header
file since it may be used by others.

> >+static RadioObserverList sRadioObserver;
> 
> sFMRadioObserver.
> 
> >+static bool sCancelSeek;
> 
> Is this here because when you cancel a seek, do you get a seek-successful
> notification?
> 
> If so, this logic needs to live in the root process.  Otherwise we have the
> following race condition:
> 
>  * Process 1 calls seek().
>  * Process 2 calls cancelSeek(), canceling process 1's seek.
>  * Process 1 gets a seek-completed notification.
> 
> It's probably better to have this logic in the gonk-specific code (which
> lives
> in the root process) anyway, because it's kind of a hack around strange
> behavior the particular FM stack we have in Gonk.
You mean I should put this flag in mozilla::hal_impl, am I right? 

> 
> >diff --git a/hal/fallback/FallbackRadio.cpp b/hal/fallback/FallbackRadio.cpp
> >+++ b/hal/fallback/FallbackRadio.cpp
> >@@ -0,0 +1,46 @@
> >+void
> >+FMRadioSeek(const hal::FMRadioSeekDirection& aDirection)
> >+{}
> >+
> >+void
> >+GetFMRadioSettings(hal::FMRadioSettings* aInfo)
> >+{}
> 
> We should assign something into aInfo, indicating that the radio is disabled,
> the antenna is detached, etc.
OK~ I will add the FM radio enable/disable status in it. But antenna status is 
not available here. If we need to report antenna status, we should register an
observer to switch handler, should I do this?

> 
> >diff --git a/hal/gonk/fmradio/FmRadioManager.cpp b/hal/gonk/fmradio/FmRadioManager.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/hal/gonk/fmradio/FmRadioManager.cpp
> >+#include "../FmRadioManager.h"
> 
> You should change the Makefile.in to include hal/gonk, so you don't have to
> do "../".
> 
> I guess this file is in the "hal/gonk/fmradio" directory because it interacts
> with the Code Aurora code?  It's kind of confusing to have this code in a
> different directory from its header file.  But I guess the idea is that we
> could have different implementations of FMRadioManager.cpp for different
> chipsets?  Anyway, it seems like all of this code should be shoved into
> GonkFMRadio.cpp, so maybe it doesn't matter.  :)
>
I am confused. You mean we do not need FMRadioManager interface? We just put the implementations into GonkFMRadio.cpp. If we need to support other chipsets, how 
should we do?

Comment 49 Andy Buckingham 2012-08-22 22:04:49 PDT

Scanning the source code it doesn't appear that it is obtaining the RDS/RBDS PI code (or ECC when, rarely, available) to expose to the API and DOM. Please could I ask that this be added? Simply adding this additional identification parameter, already supported within V4L2, would open up significant possibilities for apps based on the FM radio functionality.

Comment 50 StevenLee[:slee] 2012-08-22 22:17:20 PDT

Hi Andy,

We will add these functions in the future. Currently, we will have the basic functions first.

Comment 51 StevenLee[:slee] 2012-08-22 22:38:05 PDT

(In reply to StevenLee from comment #48)
> (In reply to Justin Lebar [:jlebar] from comment #45)
> > Comment on attachment 653300 [details] [diff] [review]
> > Part 2 - FM functions in hal
> > 
> > Two high-level comments:
> > 2. The DOM API returns DOMRequest objects for most actions.  The request may
> > succeed or fail, and we need to fire the onsuccess method or onfailure method
> > on the right object.
> > Unfortunately I think there are lots of "weird reasons" a seek might fail,
> > that
> > we can't synchronously detect at the time the seek is made.  The radio might
> > be
> > disabled while the seek is pending.  The antenna might be unplugged.  Or, I
> > think a seek could fail because we can't find any channels above the current
> > frequency, right?
> > 
> > So it seems like we need to attach an identifier to these callbacks.
> > 
> > What do you think?
> The original seeking function of code aurora is a synchronous function.
> There is a
> busy loop keeps checking a flag. Do they think this function never fails? I
> think adding an identifier is a good idea. Apps can get more information
> from the
> callback and do the right decision. But I need to test if the FM control
> code would 
> callback twice if I have 2 successive seeking without waiting its first
> callback. 
> 
I found that if I call 2 seeks very quick, the result would be 
 1. no callback
 2. only one callback
I think the code will be changed as if we are waiting for the first seeking 
result and there is a new seeking request, it will return the error code, say
|FM_IS_BUSY|. It tells the apps that they should issue the seeking command
later.

Comment 52 StevenLee[:slee] 2012-08-23 00:20:41 PDT

Created attachment 654537 [details] [diff] [review]
Part 0 - Code Aurora patch

Comment 53 Justin Lebar (not reading bugmail) 2012-08-23 01:26:04 PDT

> The original seeking function of code aurora is a synchronous function. There is a
> busy loop keeps checking a flag.

Oh, wow.  You run this code off the main thread, right?  If not, we should!

> I think FMRadioManager is the interface that gecko hal can communicate with android.
> FMRadioManager implementation should move to android in the future. Ideally, if we 
> need to support other FM chips, we can implement another FMRadioManager in android 
> without modifying the gecko hal. 

> I am confused. You mean we do not need FMRadioManager interface? We just put
> the implementations into GonkFMRadio.cpp. If we need to support other
> chipsets, how should we do?

Okay, I understand better what you're trying to do now.

In general, I YAGNI is good guidance in situations like this.  Better to design for the situation at hand and change it when the situation changes later, since "you ain't gonna need it".

But I don't think it's at all unreasonable to consider what would happen if we wanted to support a new chipset.  If we eliminated FMRadioManager (moving its code into GonkFMRadio) and we wanted to support some other chipset, couldn't we rename GonkFMRadio.cpp to GonkFMRadioChipset1.cpp, and then have GonkFMRadioChipset2.cpp for the new chipset?  That's basically what we'd do with the FMRadioManager code, right?

If we later moved the FMRadioManager code into Android itself, we could have GonkBuiltinFMRadio.cpp, which called the builtin FMRadioManager functions.

So I still don't see what the extra layer of the FMRadioManager is buying us.  Am I missing something?

> So that I put |static bool ConvertSettings(FMRadioSettings &aSettings)| in the header
> file since it may be used by others.

I guess you're thinking that different FMRadioManager implementations would all share the same header file, so you wanted them to share the same ConvertSettings implementation?

Why don't we move the ConvertSettings implementation up out of the FMRadioManager and into a higher layer?  In fact, I don't really like this interface:

  struct FMRadioSettings {
    FMRadioBandType type;
    uint32_t upperLimit;
    uint32_t lowerLimit;
    uint32_t spaceType;
    uint32_t preEmphasis;
  };

It would make more sense to me if we removed the type (perhaps this should be "band"?) in the FMRadioSettings struct.  Then you could have your ConvertSettings function like:

  namespace mozilla::hal {
    FMRadioSettings GetFMBandSettings(FMRadioBand band);
  }

That way we're not mixing chipset-dependent code with chipset-independent code.  And by removing the band from the FMRadioSettings struct, we don't have this situation where the struct sometimes does not have complete information.

> You mean I should put this flag in mozilla::hal_impl, am I right? 

Yes.

> OK~ I will add the FM radio enable/disable status in it. But antenna status is 
> not available here. If we need to report antenna status, we should register an
> observer to switch handler, should I do this?

I need to re-read the patches to figure out how you do the antenna, but you don't necessarily need to report the antenna status in FMRadioSettings, so long as, in FallbackHal, we always report that the antenna is disconnected.

> I found that if I call 2 seeks very quick, the result would be 
>   1. no callback
>   2. only one callback

I'm not sure what you mean.  Is it that when you do two seeks in quick
succession, you get either zero or one callbacks, randomly?

Or do you mean that you always get one calback in total; that one of the seeks
gets one callback and the other seek gets zero callbacks?

> I think the code [should] be changed as: if we are waiting for the first seeking 
> result and there is a new seeking request, it will return [an] error code, say
> |FM_IS_BUSY|. [This] tells the apps that they should issue the seeking command
> later.

If I understand you correctly, this seems quite reasonable.  But we still need to handle other seek failures (due to the radio being disabled during a seek, the antenna being unplugged, us not being able to find any channels because we're inside a Faraday cage, etc.).

Comment 54 StevenLee[:slee] 2012-08-23 02:09:18 PDT

(In reply to Justin Lebar [:jlebar] from comment #53)
> > The original seeking function of code aurora is a synchronous function. There is a
> > busy loop keeps checking a flag.
> 
> Oh, wow.  You run this code off the main thread, right?  If not, we should!
Yes, I modified the seeking function and made it asynchronous.

> But I don't think it's at all unreasonable to consider what would happen if
> we wanted to support a new chipset.  If we eliminated FMRadioManager (moving
> its code into GonkFMRadio) and we wanted to support some other chipset,
> couldn't we rename GonkFMRadio.cpp to GonkFMRadioChipset1.cpp, and then have
> GonkFMRadioChipset2.cpp for the new chipset?  That's basically what we'd do
> with the FMRadioManager code, right?
> 
> If we later moved the FMRadioManager code into Android itself, we could have
> GonkBuiltinFMRadio.cpp, which called the builtin FMRadioManager functions.
> 
> So I still don't see what the extra layer of the FMRadioManager is buying
> us.  Am I missing something?
I understand. I will remove FMRadioManager in the next patch.

> Why don't we move the ConvertSettings implementation up out of the
> FMRadioManager and into a higher layer?  In fact, I don't really like this
> interface:
> 
>   struct FMRadioSettings {
>     FMRadioBandType type;
>     uint32_t upperLimit;
>     uint32_t lowerLimit;
>     uint32_t spaceType;
>     uint32_t preEmphasis;
>   };
> 
> It would make more sense to me if we removed the type (perhaps this should
> be "band"?) in the FMRadioSettings struct.  Then you could have your
> ConvertSettings function like:
> 
>   namespace mozilla::hal {
>     FMRadioSettings GetFMBandSettings(FMRadioBand band);
>   }
> 
> That way we're not mixing chipset-dependent code with chipset-independent
> code.  And by removing the band from the FMRadioSettings struct, we don't
> have this situation where the struct sometimes does not have complete
> information.
That's a good idea! Thanks for the suggestion.

> I need to re-read the patches to figure out how you do the antenna, but you
> don't necessarily need to report the antenna status in FMRadioSettings, so
> long as, in FallbackHal, we always report that the antenna is disconnected.
In this bug, I don't handle the antenna things. Apps will detect the antenna
status.

> 
> > I found that if I call 2 seeks very quick, the result would be 
> >   1. no callback
> >   2. only one callback
> 
> I'm not sure what you mean.  Is it that when you do two seeks in quick
> succession, you get either zero or one callbacks, randomly?
  Yes, I may get either zero or one callbacks.

> > I think the code [should] be changed as: if we are waiting for the first seeking 
> > result and there is a new seeking request, it will return [an] error code, say
> > |FM_IS_BUSY|. [This] tells the apps that they should issue the seeking command
> > later.
> 
> If I understand you correctly, this seems quite reasonable.  But we still
> need to handle other seek failures (due to the radio being disabled during a
> seek, the antenna being unplugged, us not being able to find any channels
> because we're inside a Faraday cage, etc.).
I can handle the radio being disabled situation. But for some other situations, 
I have no way to detect them. I call the seeking function and wait for callbacks.

Comment 55 Gervase Markham [:gerv] 2012-08-23 04:34:08 PDT

(In reply to Justin Lebar [:jlebar] from comment #42)
> Thanks, Gerv.
> 
> Do we have to do anything to comply with the second clause of the BSD
> license?

Yes; add a copy of the text to about:license, in what I hope should be an obvious way, cloning an existing entry.

(In reply to Justin Lebar [:jlebar] from comment #44)
> > [tavarau.h] is generated when I built the repo of code aurora and I copied it.
> 
> I'm not sure how to handle this, although I expect that putting Code
> Aurora's copyright notice is a reasonable thing to do.  Perhaps Gerv has
> insight.

I may need more info here, but if this file is generated by the build process, there is no special need to edit it manually to add copyright info.

Gerv

Comment 56 Justin Lebar (not reading bugmail) 2012-08-23 11:13:15 PDT

> Yes; add a copy of the text to about:license, in what I hope should be an obvious way, 
> cloning an existing entry.

Steven, this file is toolkit/content/license.html (which I found by pulling up about:license and grepping my source tree for some text in there).

> there is no special need to edit it manually to add copyright info.

But we also shouldn't put an MPL header on it, right?  So we just leave it bare, perhaps adding a comment that it was generated as part of X build process?

>> I'm not sure what you mean.  Is it that when you do two seeks in quick
>> succession, you get either zero or one callbacks, randomly?
>  Yes, I may get either zero or one callbacks.

Is that because either one or both of the seeks is failing?  The DOM needs to say "success" or "failure" for every seek() request, so if the back-end randomly drops callbacks, that's pretty bad!

Comment 57 StevenLee[:slee] 2012-08-23 21:01:27 PDT

(In reply to Justin Lebar [:jlebar] from comment #56)
> >> I'm not sure what you mean.  Is it that when you do two seeks in quick
> >> succession, you get either zero or one callbacks, randomly?
> >  Yes, I may get either zero or one callbacks.
> 
> Is that because either one or both of the seeks is failing?  The DOM needs
> to say "success" or "failure" for every seek() request, so if the back-end
> randomly drops callbacks, that's pretty bad!
I think maybe I can use the task queue of nsIThread to buffer the seek requests. 
Then the seek requests can be processed one by one.
But I have a question. When there is an incoming seek request. Before the 
request is done, app calls disableFM. Should I callback something like seeking 
fails or I do not need to callback anything?

Comment 58 Gervase Markham [:gerv] 2012-08-24 04:52:04 PDT

Yes, leaving it bare is fine.

Gerv

Comment 59 StevenLee[:slee] 2012-08-27 10:16:40 PDT

Created attachment 655630 [details] [diff] [review]
part 1 - FM chip control v2

Comment 60 StevenLee[:slee] 2012-08-27 10:17:25 PDT

Created attachment 655631 [details] [diff] [review]
Part 2 - FM functions in hal - V2

Comment 61 StevenLee[:slee] 2012-08-27 10:18:33 PDT

Created attachment 655632 [details] [diff] [review]
Part 3 - Volume settings in AudioManager

Comment 62 Mounir Lamouri (:mounir) 2012-08-27 10:45:33 PDT

Comment on attachment 655630 [details] [diff] [review]
part 1 - FM chip control v2

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

Where is hal/gonk/fmradio/tavarua.h coming from? It's clearly copy/pasted. You should give credits and put license boilerplate.

In general, this patch doesn't look like a patch ready to be landed. Seems more like WIP quality.

::: hal/gonk/fmradio/fm_hal.h
@@ +166,5 @@
>  {
>          unsigned char moststate : 1;
>  }audioIndicator;
>  
> +//audioIndicator xyz;

Remove the line.

@@ +567,5 @@
>    uint8	 rx_rds_pi_timer;
>    tsFtmFmRdsTxPsType tuFmPSParams;
>    tsFtmFmRdsTxRtType tuFmRTParams;
>  }fm_cfg_request;
> +*/

Why not removing it?

::: hal/gonk/fmradio/fmhal.c
@@ +43,2 @@
>  #include <cutils/properties.h>
> +//#endif

Remove the #ifdef/#endif or keep it but don't have them behind comments.

@@ +92,5 @@
>    frequency = fm_global_params.current_station_freq;
>  
>    LOGE("Going to call on_PS_RDS_data_found() callback function\n");
> +  //if(callbacks_t != NULL)
> +  //  callbacks_t->on_rds_data_found(&rds_bundle, frequency);

ditto, don't comment code

@@ +116,5 @@
>    memcpy(rds_bundle.rt,fm_global_params.radio_text,radiotext_size);
>  
>    LOGE("Going to call on_RT_RDS_data_found() callback function\n");
> +  //if(callbacks_t != NULL)
> +  //  callbacks_t->on_rds_data_found(&rds_bundle, frequency);

ditto

@@ +199,5 @@
>      case TAVARUA_EVT_ABOVE_TH:
>        LOGI("Received above TH\n");
>        fm_global_params.service_available = FM_SERVICE_AVAILABLE;
> +      //if(callbacks_t != NULL)
> +      //  callbacks_t->on_signal_strength_changed((int)getSignalStrength(&sig_strength));

ditto

@@ +209,5 @@
>      case TAVARUA_EVT_MONO:
>        LOGI("Received Mono Mode\n");
>        fm_global_params.stype = FM_RX_MONO;
> +      //if(callbacks_t != NULL)
> +      //  callbacks_t->on_playing_in_stereo_changed(FM_RX_MONO);

ditto

@@ +943,5 @@
>  
>    return FM_CMD_SUCCESS;
>  }
>  
> +/**/

?

Comment 63 Mounir Lamouri (:mounir) 2012-08-27 10:55:49 PDT

Comment on attachment 655631 [details] [diff] [review]
Part 2 - FM functions in hal - V2

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

You need this patch to be reviewed by a HAL peer [1].
Andreas can review it but you will need another review in order to have it landed.

[1] https://wiki.mozilla.org/Modules/Core#HAL

Comment 64 Justin Lebar (not reading bugmail) 2012-08-27 12:05:05 PDT

Why did you flag Andreas instead of me for review here?  I've reviewed the previous iterations of this bug; it seems wasteful to throw out the knowledge I've accrued, and I bet Andreas has better things to do with his time than review large patches.

Comment 65 StevenLee[:slee] 2012-08-27 17:36:42 PDT

Justin, 

Kevin said that you should be Brazil and he did not see you.
He suggested I can ask Andreas to review the code. Since you are
here now. Would you review my code? 
Thanks.

Comment 66 Justin Lebar (not reading bugmail) 2012-08-27 17:38:54 PDT

> Would you review my code?

I will at the earliest opportunity, yes.

Comment 67 StevenLee[:slee] 2012-08-27 17:52:59 PDT

Created attachment 655838 [details] [diff] [review]
Part 1 - FM chip control V3

Comment 68 StevenLee[:slee] 2012-08-27 20:46:49 PDT

Created attachment 655869 [details] [diff] [review]
Part 2 - FM functions in hal - V2

Comment 69 Kevin Hu [:khu] 2012-08-28 06:46:18 PDT

Steven, Justin is still the reviewer. The reviewer is NOT changed. Since Justin will review the codes, please ask Justin for review.

Comment 70 StevenLee[:slee] 2012-08-29 03:15:52 PDT

Created attachment 656384 [details] [diff] [review]
Part 0 - Code Aurora patch

Delete fmradio_qcom_fm_hal.c. It is not used.

Comment 71 StevenLee[:slee] 2012-08-29 07:19:17 PDT

Created attachment 656433 [details] [diff] [review]
Part 2 - FM functions in hal - V2

Comment 72 Justin Lebar (not reading bugmail) 2012-08-29 08:58:35 PDT

>diff --git a/hal/gonk/fmradio/fmhal.c b/hal/gonk/fmradio/fmhal.c

I'm concerned about the extensive changes we've made to this file.

In my experience, it's important that we make minimal changes to third-party
code which we might want to take updates to at a later point in time.

For example, we were for years unable to take updates to our JPEG decoding
library, because we'd hacked the JPEG library so hard.  (We eventually ended up replacing
the whole library with something new.)

I'd normally say that we should make as few changes as possible to the upstream code (e.g don't remove its logging code) and upstream the cleanups you've made (like removing unused variables).

But it's not clear whether an upstream repo exists (the only links in this bug
are to zips of patches, e.g. in comment 16).

mvines, do you have any insight to share about the status of this code and whether we can expect to pull updates?

Comment 73 Michael Vines [:m1] [:evilmachines] 2012-08-29 09:54:50 PDT

We have recommended, and continue to do so, that this patch be used as a base for an "fm hal": http://bit.ly/QTpx5M
This sample was explicitly prepared by the team for B2G.  It looks like the current baseline is something else that was unearthed on codeaurora?  

But really the "fm hal" is the V4L2 kernel interface.

Comment 74 Justin Lebar (not reading bugmail) 2012-08-29 14:10:03 PDT

Thanks, Michael.

mwu tells me he's working on a patch for the bottom layer, either incorporating the code from comment 73 or coming up with our own whitebox implementation.  That puts the rest of these patches in flux, so I've been looking at other things.  I'll try to get you some feedback, though.

Comment 75 Justin Lebar (not reading bugmail) 2012-08-30 06:11:31 PDT

It doesn't look like you implemented (2) from comment 45.

I have nits on this patch, but I'm mostly concerned about that change.  It's a
big change that we probably need to coordinate with the front-end code.

>+typedef mozilla::ObserverList<FMRadioOperationInformation> FMRadioObserverList;
>+static FMRadioObserverList sFMRadioObserver;

sFMRadioObservers, please.  (This list can contain more than one observer.)

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
>@@ -406,16 +406,84 @@ bool SetAlarm(int32_t aSeconds, int32_t 

>+/**
>+ * Notify the operation status of FM radio, EnableFMRadio, DisableFMRadio,
>+ * and FMRadioSeek.
>+ */

I'm not sure what you mean here.

>+void NotifyFMRadioStatus(const hal::FMRadioOperationInformation& aRadioState);
>+
>+/**
>+ * Enable FM radio. Apps should provide the information about, upper limit,
>+ * lower limit and the different between 2 frequency.
>+ */

s/Apps/Callers/.

But I think you should just say "Enable the FM radio and configure it according to the settings in aInfo."

>+void EnableFMRadio(const hal::FMRadioSettings& aInfo);
>+
>+/**
>+ * Disable FM radio.

s/FM/the FM/.

>+/**
>+ * Seek an available FM radio station.

s/Seek an/Seek to an/.

>+void FMRadioSeek(const hal::FMRadioSeekDirection& aDirection);
>+
>+/**
>+ * Get the FM radio settings.

s/the FM/the current FM/.

>+/**
>+ * Set FM radio frequency.

s/FM radio/the FM radio's/

>+void SetFMRadioFrequency(const uint32_t frequency);
>+
>+/**
>+ * Get FM radio frequency.

s/FM radio/the FM radio's/

>+uint32_t GetFMRadioFrequency();
>+
>+/**
>+ * Get FM radio power state

s/FM radio/the FM radio's/

>+bool IsFMRadioOn();
>+
>+/**
>+ * Get FM radio signal strength

s/FM radio/the FM radio's/

>+uint32_t GetFMRadioSignalStrength();

>+enum FMRadioBandType {

I don't think we're using "band type" correctly in "FMRadioBandType".
FMRadioBandType is actually FMRadioBand, or perhaps FMRadioCountry.

>diff --git a/hal/fallback/FallbackFMRadio.cpp b/hal/fallback/FallbackFMRadio.cpp
>+void
>+GetFMRadioSettings(hal::FMRadioSettings* aInfo)
>+{}

As indicated in the earlier review, we need to initialize FMRadioSettings here.

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "Hal.h"
>+#include "HalTypes.h"
>+#include "fmradio/fm_hal.h"
>+#include "fmradio/fm_hal_routines.h"
>+#include "fmradio/fmradio.h"
>+#include "nsThreadUtils.h"
>+
>+using namespace mozilla::hal;
>+
>+namespace mozilla {
>+namespace hal_impl {
>+
>+static fmradio_vendor_callbacks_t* sCallbacks;
>+static bool sFMTurnedOn = false;
>+static uint32_t sCancelSeekID = 0;
>+static uint32_t sSeekID = 0;
>+static FMRadioSettings sSettings;
>+static nsCOMPtr<nsIThread> sWorkerThread;
>+
>+class FMRadioRunnable : public nsRunnable
>+{
>+public:
>+  FMRadioRunnable(FMRadioOperationInformation& info) : mInfo(info) {
>+  }
>+
>+  NS_IMETHOD Run() {
>+    NotifyFMRadioStatus(mInfo);
>+    return NS_OK;
>+  }
>+private:
>+  FMRadioOperationInformation mInfo;
>+};
>+
>+static void
>+RadioPowerStateCallback(bool turn_on, bool aSuccess)
>+{
>+  FMRadioOperationInformation info;
>+  info.operation() = turn_on ? FM_RADIO_OPERATION_ENABLE : FM_RADIO_OPERATION_DISABLE;
>+  info.status() = aSuccess ? FM_RADIO_OPERATION_STATUS_SUCCESS : 
>+                   FM_RADIO_OPERATION_STATUS_FAIL;
>+  sFMTurnedOn = turn_on;
>+  NS_DispatchToMainThread(new FMRadioRunnable(info));
>+}
>+
>+static void
>+SeekCompleteCallback(uint32_t frequency, bool aSuccess)
>+{
>+  FMRadioOperationInformation info;
>+  info.operation() = FM_RADIO_OPERATION_SEEK;
>+  info.status() = aSuccess ? FM_RADIO_OPERATION_STATUS_SUCCESS : 
>+                    FM_RADIO_OPERATION_STATUS_FAIL;
>+  info.frequency() = frequency;
>+  NS_DispatchToMainThread(new FMRadioRunnable(info));
>+}
>+
>+class FMTask : public RefCounted<FMTask>
>+{
>+public:
>+  FMTask()
>+  {
>+  }
>+
>+  FMTask(const FMRadioSettings& settings) : mSettings(settings)
>+  {
>+  }
>+
>+  FMTask(const fmradio_seek_direction_t aDir, uint32_t aID) : mDirection(aDir),
>+    mSeekID(aID)
>+  {
>+  }
>+
>+  emphasis_type
>+  GetEmphasisType(uint32_t aEmphasis)
>+  {
>+    if (aEmphasis == 75) {
>+      return FM_RX_EMP75;
>+    } else {
>+      return FM_RX_EMP50;
>+    }
>+  }
>+
>+  channel_space_type
>+  GetSpaceType(uint32_t aType)
>+  {
>+    if (aType == 200) {
>+      return FM_RX_SPACE_200KHZ;
>+    } else if (aType == 100) {
>+      return FM_RX_SPACE_100KHZ;
>+    } else {
>+      return FM_RX_SPACE_50KHZ;
>+    }
>+  }
>+
>+
>+  void EnableFM()
>+  {
>+    if (sCallbacks) {
>+      return;
>+    }
>+
>+    sFMTurnedOn = false;
>+    sCallbacks = new fmradio_vendor_callbacks_t();
>+    memset(sCallbacks, 0, sizeof(fmradio_vendor_callbacks_t));
>+    sCallbacks->on_radio_power = RadioPowerStateCallback;
>+
>+    fm_config_data fmCfg;
>+    memset(&fmCfg, 0, sizeof(fmCfg));
>+    /* these fields should be modified for different country*/
>+
>+    fmCfg.band = FM_USER_DEFINED;
>+    fmCfg.emphasis = GetEmphasisType(mSettings.preEmphasis());
>+    fmCfg.rds_system = FM_RX_NO_RDS_SYSTEM;
>+    fmCfg.is_fm_tx_on = 0;
>+    fmCfg.spacing = GetSpaceType(mSettings.spaceType());
>+    fmCfg.bandlimits.lower_limit = mSettings.lowerLimit();
>+    fmCfg.bandlimits.upper_limit = mSettings.upperLimit();
>+    enableFM(&fmCfg, sCallbacks);
>+  }
>+
>+  void DisableFM()
>+  {
>+    disableFM();
>+    delete sCallbacks;
>+    sCallbacks = NULL;
>+  }
>+
>+  void SeekStation()
>+  {
>+    if (!sCallbacks || mSeekID < sCancelSeekID) {
>+      SeekCompleteCallback(0, false);
>+    }
>+
>+    int frequency;
>+    seekStation(&frequency, mDirection);
>+    SeekCompleteCallback(frequency, true);
>+  }
>+
>+  void CancelSeek()
>+  {
>+    cancelSearch();
>+  }
>+private:
>+  FMRadioSettings mSettings;
>+  fmradio_seek_direction_t mDirection;
>+  uint32_t mSeekID;
>+};
>+
>+void
>+EnableFMRadio(const hal::FMRadioSettings& aInfo) {
>+  if (sWorkerThread == nullptr) {
>+    NS_NewThread(getter_AddRefs(sWorkerThread));
>+  }
>+
>+  nsCOMPtr<nsIRunnable> event;
>+  event = NS_NewRunnableMethod(new FMTask(aInfo), &FMTask::EnableFM);
>+  sWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);
>+
>+  sSettings = aInfo;
>+}
>+
>+void
>+DisableFMRadio() {
>+  nsCOMPtr<nsIRunnable> event;
>+  event = NS_NewRunnableMethod(new FMTask(), &FMTask::DisableFM);
>+  sWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);
>+}
>+
>+void
>+FMRadioSeek(const FMRadioSeekDirection& aDir) {
>+  nsCOMPtr<nsIRunnable> event;
>+  event = NS_NewRunnableMethod(new FMTask(aDir == FM_RADIO_SEEK_DIRECTION_UP ? 
>+            FMRADIO_SEEK_UP : FMRADIO_SEEK_DOWN, sSeekID++), &FMTask::SeekStation);
>+  sWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);
>+}
>+
>+void
>+GetFMRadioSettings(FMRadioSettings* aInfo) {
>+  *aInfo = sSettings;
>+}
>+
>+void
>+SetFMRadioFrequency(const uint32_t aFrequency) {
>+  setStationFrequency((uint32)aFrequency);
>+}
>+
>+uint32_t
>+GetFMRadioFrequency() {
>+  if (!sFMTurnedOn) {
>+    return 0;
>+  }
>+  return getStationFrequency();
>+}
>+
>+bool
>+IsFMRadioOn() {
>+  return sFMTurnedOn;
>+}
>+
>+uint32_t
>+GetFMRadioSignalStrength() {
>+  if (!sFMTurnedOn) {
>+    return 0;
>+  }
>+
>+  unsigned char str;
>+  getSignalStrength(&str);
>+  return str;
>+}
>+
>+void
>+CancelFMRadioSeek() {
>+  sCancelSeekID = ++sSeekID;
>+  nsCOMPtr<nsIRunnable> event;
>+  event = NS_NewRunnableMethod(new FMTask(), &FMTask::CancelSeek);
>+  sWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);
>+}
>+
>+} // hal_impl
>+} // mozilla
>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+struct FMRadioOperationInformation {
>+  FMRadioOperation operation;
>+  FMRadioOperationStatus status;
>+  uint32_t frequency;
>+};
>+
>+struct FMRadioSettings {
>+  FMRadioBandType type;
>+  uint32_t upperLimit;
>+  uint32_t lowerLimit;
>+  uint32_t spaceType;
>+  uint32_t preEmphasis;
>+  bool isFMRadioOn;
>+};

I don't think isFMRadioOn belongs in FMRadioSettings.  When translating "USA
band" to an FMRadioSettings object, we don't know whether the radio is on.

Or, put another way: What happens if I call EnableFMRadio() but pass a settings
object where the radio is off?  It just doesn't make sense to have that option.

Comment 76 StevenLee[:slee] 2012-08-30 07:06:03 PDT

(In reply to Justin Lebar [:jlebar] from comment #75)
> It doesn't look like you implemented (2) from comment 45.
In this version, |seekStation| is a synchronous function and 
will be executed by sWorkerThread. When there are many seek
requests coming in, these jobs will be queued in sWorkerThread.
And the seek results will be callback sequentially.

In GonkFMRadio.cpp, I also added two variables, sSeekID and 
sCancelSeekID. For every seek task, it will be assigned a ID.
When sWorkerThread processes the FMTask and find that 
sCancelSeekID is larger the FMTask's mSeekID, it will callback 
seek fail.

Comment 77 Justin Lebar (not reading bugmail) 2012-08-30 07:18:03 PDT

But that only solves part of the problem, as I understand it.

Suppose two separate processes call SeekUp() simultaneously.  One of those calls will arrive at the root process first, but neither of the calling processes knows if it's the first.

Both processes will see one seekSucceeded and one seekFailed, but neither process knows whether /its own/ seek failed, so they don't know whether to fire onfailed on the seeks' domrequest objects.

Unrelatedly, are you sure the logic is correct?  As I read it, if we do (in one process)

  seek();
  // wait for seek to finish
  seek();

the second seek will fail, because mSeekID == 1 and sCanceledSeekId == 0.

Comment 78 StevenLee[:slee] 2012-08-30 07:24:12 PDT

(In reply to Justin Lebar [:jlebar] from comment #75)
> >diff --git a/hal/Hal.h b/hal/Hal.h
> >--- a/hal/Hal.h
> >+++ b/hal/Hal.h
> >@@ -406,16 +406,84 @@ bool SetAlarm(int32_t aSeconds, int32_t 
> 
> >+/**
> >+ * Notify the operation status of FM radio, EnableFMRadio, DisableFMRadio,
> >+ * and FMRadioSeek.
> >+ */
> 
> I'm not sure what you mean here.
> 
> >+void NotifyFMRadioStatus(const hal::FMRadioOperationInformation& aRadioState);
|Notify the results of these functions, EnableFMRadio, DisableFMRadio,
and FMRadioSeek. | Is it better?

> >diff --git a/hal/fallback/FallbackFMRadio.cpp b/hal/fallback/FallbackFMRadio.cpp
> >+void
> >+GetFMRadioSettings(hal::FMRadioSettings* aInfo)
> >+{}
> 
> As indicated in the earlier review, we need to initialize FMRadioSettings
> here.
> 
> >+struct FMRadioSettings {
> >+  FMRadioBandType type;
> >+  uint32_t upperLimit;
> >+  uint32_t lowerLimit;
> >+  uint32_t spaceType;
> >+  uint32_t preEmphasis;
> >+  bool isFMRadioOn;
> >+};
> 
> I don't think isFMRadioOn belongs in FMRadioSettings.  When translating "USA
> band" to an FMRadioSettings object, we don't know whether the radio is on.
> 
> Or, put another way: What happens if I call EnableFMRadio() but pass a
> settings
> object where the radio is off?  It just doesn't make sense to have that
> option.

Ah, I misunderstand what you mean in the previous comments. you said
that, "We should assign something into aInfo, indicating that the radio is disabled, the antenna is detached, etc." I thought you wanted
me to add a new variable to indicate the FM status. 
I will initialize the FMRadioSettings in next patch.

Comment 79 Andrew Overholt [:overholt] 2012-08-30 07:40:52 PDT

FM radio is a v1 requirement.

Comment 80 StevenLee[:slee] 2012-08-30 07:52:24 PDT

(In reply to Justin Lebar [:jlebar] from comment #77)
> But that only solves part of the problem, as I understand it.
> 
> Suppose two separate processes call SeekUp() simultaneously.  One of those
> calls will arrive at the root process first, but neither of the calling
> processes knows if it's the first.
> 
> Both processes will see one seekSucceeded and one seekFailed, but neither
> process knows whether /its own/ seek failed, so they don't know whether to
> fire onfailed on the seeks' domrequest objects.
I think I should prevent that there are more than one processes to 
call SeekUp(). I think I can add one variable in mozilla::hal to 
indicate that the FM radio is enabled or not. When I find FM radio
is not enabled and users call FMRadioSeek, I should callback seek 
failed directly.

> 
> Unrelatedly, are you sure the logic is correct?  As I read it, if we do (in
> one process)
> 
>   seek();
>   // wait for seek to finish
>   seek();
> 
> the second seek will fail, because mSeekID == 1 and sCanceledSeekId == 0.

But my code is to check whether mSeekID is smaller than sCancelSeekID.
If it is true, I will callback seek failed. In this case,
mSeekID is still larger than sCancelSeekID.

Comment 81 Justin Lebar (not reading bugmail) 2012-08-30 09:56:33 PDT

> I think I should prevent that there are more than one processes to 
> call SeekUp(). I think I can add one variable in mozilla::hal to 
> indicate that the FM radio is enabled or not. When I find FM radio
> is not enabled and users call FMRadioSeek, I should callback seek 
> failed directly.

I don't understand how this solves the race condition above.

How does the process know that specifically /its/ seek failed if the "seek failed" message does not contain a unique token?

That is:

  * Process A calls SeekUp();
  * Process B calls SeekUp();
  * Main process sends "seek failed" immediately after receiving process B's SeekUp() call.
  * Main process sends "seek succeeded" some time later, indicating that A's SeekUp() succeeded.

How does process A know that its seek succeeded?  In order to know this, wouldn't process A need to know that the main process received its SeekUp() before B's SeekUp()?  But how would A know this?

Comment 82 Justin Lebar (not reading bugmail) 2012-08-30 11:04:05 PDT

> But my code is to check whether mSeekID is smaller than sCancelSeekID.
> If it is true, I will callback seek failed. In this case,
> mSeekID is still larger than sCancelSeekID.

Ah, I understand now.  Thanks.  I /think/ it's correct!

Comment 83 Michael Wu [:mwu] 2012-08-30 12:16:37 PDT

Created attachment 656984 [details] [diff] [review]
Simplified gonk radio backend

This is a simplified FM radio backend. Tavarua specific things are named as such, and the rest of the code is generic radio code for V4L2 radio devices.

Some issues I'm aware of:
1. Cancel seek is not implemented. Not sure what the value of it is.
2. FM radio initialization is slow and probably should be placed on another thread.
3. We need to include tavarua.h. This is a header from the kernel. For reference, this is what Android puts on top of its linux kernel headers:

/****************************************************************************
 ****************************************************************************
 ***
 ***   This header was automatically generated from a Linux kernel header
 ***   of the same name, to make information necessary for userspace to
 ***   call into the kernel available to libc.  It contains only constants,
 ***   structures, and macros generated from the original header, and thus,
 ***   contains no copyrightable information.
 ***
 ****************************************************************************
 ****************************************************************************/

Comment 84 Michael Wu [:mwu] 2012-08-30 12:23:23 PDT

Comment on attachment 655632 [details] [diff] [review]
Part 3 - Volume settings in AudioManager

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

::: dom/system/gonk/AudioManager.cpp
@@ +113,5 @@
>      return NS_ERROR_FAILURE;
>    }
> +
> +  if (mFmEnabled && AudioSystem::setFmVolume(aMasterVolume)) {
> +    return NS_ERROR_FAILURE; 

Watch out for leading whitespace.

::: dom/system/gonk/AudioManager.h
@@ +45,5 @@
>    ~AudioManager();
>  
>    static void SetAudioRoute(int aRoutes);
> +  static NS_IMETHODIMP EnableFM();
> +  static NS_IMETHODIMP DisableFM();

NS_IMETHODIMP is probably inappropriate here. void or nsresult are better choices depending on whether you care about the results.

::: media/libsydneyaudio/src/gonk/AudioSystem.h
@@ +523,5 @@
>      static status_t getInputBufferSize(uint32_t sampleRate, int format, int channelCount,
>          size_t* buffSize);
>  
>      static status_t setVoiceVolume(float volume);
> +    static status_t setFmVolume(float volume);

This symbol doesn't always exist on all devices. You should declare it weak and use our usual runtime tricks to figure out if we can call this function.

Comment 85 Michael Wu [:mwu] 2012-08-30 12:48:35 PDT

For reference, https://github.com/michaelwu/mozilla-central/tree/radio has all the radio patches necessary to use the fm radio app. It's a combination of the current patches plus the simplified radio backend.

Comment 86 StevenLee[:slee] 2012-08-30 20:33:24 PDT

(In reply to Justin Lebar [:jlebar] from comment #81)
> I don't understand how this solves the race condition above.
> 
> How does the process know that specifically /its/ seek failed if the "seek
> failed" message does not contain a unique token?
> 
> That is:
> 
>   * Process A calls SeekUp();
>   * Process B calls SeekUp();
>   * Main process sends "seek failed" immediately after receiving process B's
> SeekUp() call.
>   * Main process sends "seek succeeded" some time later, indicating that A's
> SeekUp() succeeded.
> 
> How does process A know that its seek succeeded?  In order to know this,
> wouldn't process A need to know that the main process received its SeekUp()
> before B's SeekUp()?  But how would A know this?
My basic idea is that only one process can access the FM radio
operations. Do we need to support more than one processes to 
access FM radio at the same time?
If there is only one process can access the FM radio, we will not
have the above problem. When the FM radio is enabled by process A 
and process B wants to SeekUp(), it will callback failed to process
B. In the same time process A can call SeekUp() correctly, since 
that the variable indicates FM enalbed is stored in mozilla::hal.

Comment 87 Justin Lebar (not reading bugmail) 2012-08-31 06:02:06 PDT

> Do we need to support more than one processes to 
> access FM radio at the same time?

I thought about this last night, and I think not.

We do need to behave in a sane way when two processes try to access the FM radio at the same time.  But I think at this point the best thing to do is to figure that out in a separate bug.

Can you post an updated patch with the other changes from my review?  I'll have a look at mwu's code plus the front-end code soon.

Comment 88 StevenLee[:slee] 2012-08-31 10:26:31 PDT

Created attachment 657346 [details] [diff] [review]
Part 2 - FM functions in hal - V3

Comment 89 StevenLee[:slee] 2012-08-31 10:30:20 PDT

(In reply to Michael Wu [:mwu] from comment #83)
> Created attachment 656984 [details] [diff] [review]
> Simplified gonk radio backend
> 
> This is a simplified FM radio backend. Tavarua specific things are named as
> such, and the rest of the code is generic radio code for V4L2 radio devices.
> 
> Some issues I'm aware of:
> 1. Cancel seek is not implemented. Not sure what the value of it is.
FM_RX_CANCEL_SEARCH is defined in hal/gonk/fmradio/fm_hal.h:86
and its value is zero.

Comment 90 Justin Lebar (not reading bugmail) 2012-08-31 10:58:15 PDT

I'm OK punting on cancel search for now.  It's not clear how useful it is.

Comment 91 Justin Lebar (not reading bugmail) 2012-09-05 12:33:11 PDT

Comment on attachment 656984 [details] [diff] [review]
Simplified gonk radio backend

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
>new file mode 100644
>index 0000000..91b2a48
>--- /dev/null
>+++ b/hal/gonk/GonkFMRadio.cpp
>@@ -0,0 +1,334 @@
>+/* Copyright 2012 Mozilla Foundation and Mozilla contributors
>+ *
>+ * Licensed under the Apache License, Version 2.0 (the "License");
>+ * you may not use this file except in compliance with the License.
>+ * You may obtain a copy of the License at
>+ *
>+ *     http://www.apache.org/licenses/LICENSE-2.0
>+ *
>+ * Unless required by applicable law or agreed to in writing, software
>+ * distributed under the License is distributed on an "AS IS" BASIS,
>+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>+ * See the License for the specific language governing permissions and
>+ * limitations under the License.
>+ */

Why is this Apache 2 instead of MPL?

>+namespace mozilla {
>+namespace hal_impl {
>+
>+uint32_t GetFMRadioFrequency();
>+
>+static int sRadioFD;
>+static bool sRadioEnabled;
>+static pthread_t sMonitorThread;
>+
>+static int
>+setControl(uint32_t id, int32_t value)
>+{
>+  struct v4l2_control control;
>+  control.id = id;
>+  control.value = value;
>+  return ioctl(sRadioFD, VIDIOC_S_CTRL, &control);
>+}
>+
>+class RadioUpdate : public nsRunnable {
>+  hal::FMRadioOperation mOp;
>+public:
>+  RadioUpdate(hal::FMRadioOperation op)
>+    : mOp(op)
>+  {}
>+
>+  NS_IMETHOD Run() {
>+    hal::FMRadioOperationInformation info;
>+    info.operation() = mOp;
>+    info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS;
>+    info.frequency() = GetFMRadioFrequency();
>+    hal::NotifyFMRadioStatus(info);
>+    return NS_OK;
>+  }
>+};
>+
>+/*
>+ * Frequency conversions
>+ *
>+ * V4L2 uses units of 62.5kHz for frequencies.
>+ * The HAL uses units of 1k
>+ * Multiplying by (625 / 10000) converts from V4L2 units to HAL.
>+ */

This comment is out of place (in fact, I ask for precisely this comment later
on in the review.)

>+static void *
>+pollTavaruaRadio(void *)
>+{
>+  uint8_t buf[128];
>+  struct v4l2_buffer buffer = {0};
>+  buffer.index = 1;
>+  buffer.type = V4L2_BUF_TYPE_PRIVATE;
>+  buffer.length = sizeof(buf);
>+  buffer.m.userptr = (long unsigned int)buf;
>+  int rc = ioctl(sRadioFD, VIDIOC_DQBUF, &buffer);
>+  if (rc)
>+    return NULL;
>+
>+  for (unsigned int i = 0; i < buffer.bytesused; i++) {
>+    switch (buf[i]) {
>+    case TAVARUA_EVT_RADIO_READY:
>+      NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_ENABLE));
>+      break;
>+    case TAVARUA_EVT_SEEK_COMPLETE:
>+      NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_SEEK));
>+      break;
>+    case TAVARUA_EVT_RADIO_DISABLED:
>+      NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_DISABLE));
>+      break;
>+    default:
>+      HAL_LOG(("received message %d", buf[i]));
>+      break;
>+    }
>+  }
>+
>+  if (sRadioEnabled)
>+    pollTavaruaRadio(NULL);

We're relying on the compiler optimizing tail recursion here; I'd rather write
an explicit loop.

>+  return NULL;
>+}
>+
>+void
>+EnableFMRadio(const hal::FMRadioSettings& aInfo)
>+{
>+  if (sRadioEnabled) {
>+    HAL_LOG(("Radio already enabled!"));
>+    return;
>+  }
>+
>+  int fd = open("/dev/radio0", O_RDWR);
>+  if (fd < 0) {
>+    HAL_LOG(("Unable to open radio device"));
>+    return;
>+  }
>+
>+  struct v4l2_capability cap;
>+  int rc = ioctl(fd, VIDIOC_QUERYCAP, &cap);
>+  if (rc < 0) {
>+    HAL_LOG(("Unable to query radio device"));
>+    return;
>+  }

Leaking fd here (unless the failed ioctl closes the fd?).  You should probably
use mozilla::AutoFDClose.

>+  HAL_LOG(("Radio: %s (%s)\n", cap.driver, cap.card));
>+
>+  if (!(cap.capabilities & V4L2_CAP_RADIO)) {
>+    HAL_LOG(("/dev/radio0 isn't a radio"));
>+    close(fd);
>+    return;
>+  }
>+
>+  if (!(cap.capabilities & V4L2_CAP_TUNER)) {
>+    HAL_LOG(("/dev/radio0 doesn't support the tuner interface"));
>+    close(fd);
>+    return;
>+  }
>+  sRadioFD = fd;
>+
>+  // Tavarua specific start
>+  char command[64];
>+  snprintf(command, sizeof(command), "fm_qsoc_patches %d 0", cap.version);
>+  rc = system(command);

If you like, you could do

  system(nsPrintfCString("fm_qsoc_patches %d 0", cap.version).get());

But less trivially, I'm worried about attacks where someone places an
executable somewhere in our path and then causes us to run the wrong thing.
(We've had similar attacks on Windows with DLLs.)

Can we hardcode in the full path to fm_qsoc_patches?

And of course, if this is slow, we need to get it off the main thread.

>+  if (rc) {
>+    HAL_LOG(("Unable to initialize radio"));
>+    close(fd);

Don't we need to clear sRadioFD at this point, if only for sanity's sake?

>+    return;
>+  }
>+
>+  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_STATE, FM_RECV);
>+  if (rc < 0) {
>+    HAL_LOG(("Unable to turn on radio |%s|", strerror(errno)));
>+    close(fd);
>+    return;
>+  }
>+
>+  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_REGION, TAVARUA_REGION_US);
>+  if (rc < 0) {
>+    HAL_LOG(("Unable to configure region"));
>+    close(fd);
>+    return;
>+  }
>+
>+  int preEmphasis = aInfo.preEmphasis() <= 50;
>+  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_EMPHASIS, preEmphasis);
>+  if (rc) {
>+    HAL_LOG(("Unable to configure preemphasis"));
>+    close(fd);
>+    return;
>+  }
>+
>+  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_RDS_STD, 0);
>+  if (rc) {
>+    HAL_LOG(("Unable to configure RDS"));
>+    close(fd);
>+    return;
>+  }
>+
>+  int spacing;
>+  switch (aInfo.spaceType()) {
>+  case 50:
>+    spacing = 2;
>+    break;
>+  case 100:
>+    spacing = 1;
>+    break;
>+  case 200:
>+    spacing = 0;
>+    break;

Can these be constants?

>+  default:
>+    HAL_LOG(("Unsupported space value - %d", aInfo.spaceType()));
>+    close(fd);
>+    return;
>+  }
>+
>+  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_SPACING, spacing);
>+  if (rc) {
>+    HAL_LOG(("Unable to configure spacing"));
>+    close(fd);
>+    return;
>+  }
>+
>+  struct v4l2_tuner tuner = {0};
>+  tuner.rangelow = (aInfo.lowerLimit() * 10000) / 625;
>+  tuner.rangehigh = (aInfo.upperLimit() * 10000) / 625;

Please explain where these numbers come from.

>diff --git a/hal/gonk/tavarua.h b/hal/gonk/tavarua.h
>new file mode 100644
>index 0000000..52194f9
>--- /dev/null
>+++ b/hal/gonk/tavarua.h

Maybe we can put a disclaimer at the top of this file, if only so we remember
where it came from and why we think it's safe?

r=me even without moving the initialization onto a new thread; we can do that as a follow-up.  But we need to remember to do that!

Comment 92 Justin Lebar (not reading bugmail) 2012-09-05 12:55:06 PDT

Comment on attachment 655632 [details] [diff] [review]
Part 3 - Volume settings in AudioManager

One addition to mwu's comments from comment 84.

>diff --git a/dom/system/gonk/AudioManager.h b/dom/system/gonk/AudioManager.h
>--- a/dom/system/gonk/AudioManager.h
>+++ b/dom/system/gonk/AudioManager.h
>@@ -40,20 +40,25 @@ class AudioManager : public nsIAudioMana
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIAUDIOMANAGER
> 
>   AudioManager();
>   ~AudioManager();
> 
>   static void SetAudioRoute(int aRoutes);
>+  static NS_IMETHODIMP EnableFM();
>+  static NS_IMETHODIMP DisableFM();

These should be in nsIAudioManager.idl as

  attribute boolean fmRadioAudioEnabled;

because otherwise we have a confusing mix of static and non-static members on
this class.

Comment 93 Michael Wu [:mwu] 2012-09-05 13:14:11 PDT

(In reply to Justin Lebar [:jlebar] from comment #91)
> Comment on attachment 656984 [details] [diff] [review]
> Simplified gonk radio backend
> 
> >diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
> >new file mode 100644
> >index 0000000..91b2a48
> >--- /dev/null
> >+++ b/hal/gonk/GonkFMRadio.cpp
> >@@ -0,0 +1,334 @@
> >+/* Copyright 2012 Mozilla Foundation and Mozilla contributors
> >+ *
> >+ * Licensed under the Apache License, Version 2.0 (the "License");
> >+ * you may not use this file except in compliance with the License.
> >+ * You may obtain a copy of the License at
> >+ *
> >+ *     http://www.apache.org/licenses/LICENSE-2.0
> >+ *
> >+ * Unless required by applicable law or agreed to in writing, software
> >+ * distributed under the License is distributed on an "AS IS" BASIS,
> >+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> >+ * See the License for the specific language governing permissions and
> >+ * limitations under the License.
> >+ */
> 
> Why is this Apache 2 instead of MPL?
> 

It's for consistency with bug 774614 which changed hal/gonk/* to Apache 2.

> >+  HAL_LOG(("Radio: %s (%s)\n", cap.driver, cap.card));
> >+
> >+  if (!(cap.capabilities & V4L2_CAP_RADIO)) {
> >+    HAL_LOG(("/dev/radio0 isn't a radio"));
> >+    close(fd);
> >+    return;
> >+  }
> >+
> >+  if (!(cap.capabilities & V4L2_CAP_TUNER)) {
> >+    HAL_LOG(("/dev/radio0 doesn't support the tuner interface"));
> >+    close(fd);
> >+    return;
> >+  }
> >+  sRadioFD = fd;
> >+
> >+  // Tavarua specific start
> >+  char command[64];
> >+  snprintf(command, sizeof(command), "fm_qsoc_patches %d 0", cap.version);
> >+  rc = system(command);
> 
> If you like, you could do
> 
>   system(nsPrintfCString("fm_qsoc_patches %d 0", cap.version).get());
> 

Sounds like a good idea.

> But less trivially, I'm worried about attacks where someone places an
> executable somewhere in our path and then causes us to run the wrong thing.
> (We've had similar attacks on Windows with DLLs.)
> 
> Can we hardcode in the full path to fm_qsoc_patches?
> 

Sure, though I think if it gets to that point, we're already screwed.

> And of course, if this is slow, we need to get it off the main thread.
> 
> >+  if (rc) {
> >+    HAL_LOG(("Unable to initialize radio"));
> >+    close(fd);
> 
> Don't we need to clear sRadioFD at this point, if only for sanity's sake?
> 

sRadioFD is only valid when sRadioEnabled is true.

> >+  int spacing;
> >+  switch (aInfo.spaceType()) {
> >+  case 50:
> >+    spacing = 2;
> >+    break;
> >+  case 100:
> >+    spacing = 1;
> >+    break;
> >+  case 200:
> >+    spacing = 0;
> >+    break;
> 
> Can these be constants?
> 

I'll see.

> >diff --git a/hal/gonk/tavarua.h b/hal/gonk/tavarua.h
> >new file mode 100644
> >index 0000000..52194f9
> >--- /dev/null
> >+++ b/hal/gonk/tavarua.h
> 
> Maybe we can put a disclaimer at the top of this file, if only so we remember
> where it came from and why we think it's safe?
> 

Ok.

Comment 94 Justin Lebar (not reading bugmail) 2012-09-05 13:50:27 PDT

Comment on attachment 657346 [details] [diff] [review]
Part 2 - FM functions in hal - V3

r=me with nits addressed.  I'm really sorry this review took so long -- it's been hard for me to find time to keep up with reviews for the past two weeks, but now that I'm back home, I should be able to do a better job.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>+struct FMRadioSettings {
>+  FMRadioCountry type;

Call the field "country"?

>+  uint32_t upperLimit;
>+  uint32_t lowerLimit;
>+  uint32_t spaceType;
>+  uint32_t preEmphasis;
>+};

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
>@@ -406,16 +406,83 @@ bool SetAlarm(int32_t aSeconds, int32_t 
>  * Set the priority of the given process.
>  *
>  * Exactly what this does will vary between platforms.  On *nix we might give
>  * background processes higher nice values.  On other platforms, we might
>  * ignore this call entirely.
>  */
> void SetProcessPriority(int aPid, hal::ProcessPriority aPriority);
> 
>+/**
>+ * Register an observer for the FM radio.
>+ */
>+void RegisterFMRadioObserver(hal::FMRadioObserver *aRadioObserver);

Nit: Please be consistent with your placement of *'s.  In this file (and
increasingly throughout Mozilla), we tend to put the * on the typename, rather
than the variable.

>+/**
>+ * Unregister the observer for the FM radio.
>+ */
>+void UnregisterFMRadioObserver(hal::FMRadioObserver *aRadioObserver);

Watch out for the * here, too.

>+/**
>+ * Notify the operation status of FM radio, EnableFMRadio, DisableFMRadio,
>+ * and FMRadioSeek.
>+ */
>+void NotifyFMRadioStatus(const hal::FMRadioOperationInformation& aRadioState);

You suggested

  Notify the results of these functions, EnableFMRadio, DisableFMRadio,
  and FMRadioSeek.

If you s/functions,/functions:/, that would be an improvement.

Better even might be

  Notify observers that a call to EnableFMRadio, DisableFMRadio, or FMRadioSeek
  has completed, and indicate what the call returned.

>+/**
>+ * Get FM radio band settings by country.
>+ */
>+hal::FMRadioSettings GetFMBandSettings(hal::FMRadioCountry aBand);

We should be consistent with our names.  If it's FMRadioCountry, then
GetFMSettingsForCountry(aCountry) would probably be a better name.

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
>new file mode 100644
>--- /dev/null
>+++ b/hal/gonk/GonkFMRadio.cpp

I prefer mwu's version of this, because it doesn't rely on CodeAurora.

Comment 95 Justin Lebar (not reading bugmail) 2012-09-05 13:58:01 PDT

Comment on attachment 655838 [details] [diff] [review]
Part 1 - FM chip control V3

This is all obsoleted by mwu's clean-room implementation, right?

Comment 96 Justin Lebar (not reading bugmail) 2012-09-05 14:14:43 PDT

Comment on attachment 656384 [details] [diff] [review]
Part 0 - Code Aurora patch

We're going to use mwu's clean-room implementation of this instead.

Comment 97 StevenLee[:slee] 2012-09-07 03:04:31 PDT

Comment on attachment 656984 [details] [diff] [review]
Simplified gonk radio backend

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

Hi mwu, 
I tested this patch in Taiwan and found 2 problems.
1. The frequency will be shifted 500 kHz, ex, for the 
   station whose frequency is 100700 will be shifted to
   100200.
2. Cannot seek correctly. I can only seek one or two 
   stations between 87000 to 108000.

::: hal/gonk/GonkFMRadio.cpp
@@ +156,5 @@
> +    close(fd);
> +    return;
> +  }
> +
> +  rc = setControl(V4L2_CID_PRIVATE_TAVARUA_REGION, TAVARUA_REGION_US);

This parameter should be set after preEmphasis, space type, 
and tuner range. If we set it here, the frequency will be
shifted as 500 kHz. 
And if I use TAVARUA_REGION_US, I can only seek very few 
or even no stations in Taiwan. If I change it to 
TAVARUA_REGION_OTHER, the seek operation works fine. I am
not sure if it has the same problem in other countries.

Comment 98 StevenLee[:slee] 2012-09-07 03:21:41 PDT

Comment on attachment 656984 [details] [diff] [review]
Simplified gonk radio backend

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

::: hal/gonk/GonkFMRadio.cpp
@@ +261,5 @@
> +
> +void
> +GetFMRadioSettings(hal::FMRadioSettings* aInfo)
> +{
> +  aInfo->isFMRadioOn() = false;

isFMRadioOn is obsoleted. Sorry I did not mention in the last comment.

Comment 99 StevenLee[:slee] 2012-09-09 19:17:46 PDT

Created attachment 659616 [details] [diff] [review]
Part 2 - FM functions in hal - V4

Comment 100 StevenLee[:slee] 2012-09-09 20:51:32 PDT

Created attachment 659623 [details] [diff] [review]
Part 3 - Volume settings in AudioManager - V2

Comment 101 Pin Zhang [:pzhang] (inactive) 2012-09-10 00:52:23 PDT

Comment on attachment 659616 [details] [diff] [review]
Part 2 - FM functions in hal - V4

The patch can not be applied to the latest m-c.

Comment 102 Pin Zhang [:pzhang] (inactive) 2012-09-10 19:01:19 PDT

(In reply to Justin Lebar [:jlebar] from comment #84)
> >+/* readonly attribute long signalStrength; */
> >+NS_IMETHODIMP FMRadio::GetSignalStrength(float *aSignalStrength)
> >+{
> >+  uint32_t signalStrength;
> >+
> >+  // The range of signal strengh is [0-100], higher than 100 means signal is strong.
> 
> Nit: This is a comma splice.  Replace the comma with a period.
> 
> But I also don't understand this comment; the signal strength is [0-100], but
> it can be bigger than 100?  Perhaps you could elaborate a bit in the comment.
> 
Steven asked Qualcomm about this, and this is what Qualcomm answered.

Comment 103 Pin Zhang [:pzhang] (inactive) 2012-09-10 19:03:06 PDT

Sorry for the comment 102, it should be commented on bug779500 ....

Comment 104 StevenLee[:slee] 2012-09-10 19:30:54 PDT

Created attachment 659941 [details] [diff] [review]
Part 2 - FM functions in hal - V4.1

Comment 105 Justin Lebar (not reading bugmail) 2012-09-10 21:37:00 PDT

Comment on attachment 659623 [details] [diff] [review]
Part 3 - Volume settings in AudioManager - V2

>diff --git a/dom/system/gonk/nsIAudioManager.idl b/dom/system/gonk/nsIAudioManager.idl
>--- a/dom/system/gonk/nsIAudioManager.idl
>+++ b/dom/system/gonk/nsIAudioManager.idl
>@@ -18,16 +18,21 @@ interface nsIAudioManager : nsISupports
>   attribute float masterVolume;
> 
>   /**
>    * Master volume muted?
>    */
>   attribute boolean masterMuted;
> 
>   /**
>+   * The FM radio enabled?
>+   */
>+  attribute boolean fmRadioAudioEnabled;

That's not really what this attribute means, right?  The FM radio can be
enabled, but fmRadioAudioEnabled could be false, indicating that we're not
piping the FM radio to the headphones/speakers.

How about:

  "Are we playing audio from the FM radio?"

>diff --git a/dom/system/gonk/AudioManager.h b/dom/system/gonk/AudioManager.h
>--- a/dom/system/gonk/AudioManager.h
>+++ b/dom/system/gonk/AudioManager.h
>@@ -40,20 +40,22 @@ class AudioManager : public nsIAudioMana
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIAUDIOMANAGER
> 
>   AudioManager();
>   ~AudioManager();
> 
>   static void SetAudioRoute(int aRoutes);
>+
> protected:
>   int32_t mPhoneState;
> 
> private:
>   nsAutoPtr<mozilla::hal::SwitchObserver> mObserver;
>+  static bool mFmEnabled;

Now that I'm on the lookout for this (from your other patch :), the same
applies here: This should be called sFmEnabled, if it's static, but it also
probably shouldn't be static.  It looks like this might Just Work if you make
it non-static (and initialize it in the constructor, of course!).

Comment 106 Pin Zhang [:pzhang] (inactive) 2012-09-11 00:41:49 PDT

>+GetFMBandSettings(FMRadioCountry aCountry) {
>+  FMRadioSettings settings;
>+
>+  switch (aCountry) {
>+    case FM_RADIO_COUNTRY_US:
>+    case FM_RADIO_COUNTRY_EU:
In order to get the radio settings, i need to know which country it is, so how can i get the country value? By reading a setting from the Settings API? Then what is the setting? Or we need define such a setting?

Comment 107 Pin Zhang [:pzhang] (inactive) 2012-09-11 00:46:57 PDT

The patches can not be compiled, here is the error:
  hal/gonk/GonkFMRadio.cpp:265: error: 'class mozilla::hal::FMRadioSettings' has no member named 'isFMRadioOn'
  hal/gonk/GonkFMRadio.cpp:279: error: 'class mozilla::hal::FMRadioSettings' has no member named 'isFMRadioOn'

Comment 108 Pin Zhang [:pzhang] (inactive) 2012-09-11 00:50:33 PDT

(In reply to Ray Cheung [:pzhang] from comment #106)
> >+GetFMBandSettings(FMRadioCountry aCountry) {
> >+  FMRadioSettings settings;
> >+
> >+  switch (aCountry) {
> >+    case FM_RADIO_COUNTRY_US:
> >+    case FM_RADIO_COUNTRY_EU:
> In order to get the radio settings, i need to know which country it is, so
> how can i get the country value? By reading a setting from the Settings API?
> Then what is the setting? Or we need define such a setting?
And what if the country is not defined in the hal, like China, then what the settings should be?

Comment 109 StevenLee[:slee] 2012-09-11 01:47:56 PDT

(In reply to Ray Cheung [:pzhang] from comment #107)
> The patches can not be compiled, here is the error:
>   hal/gonk/GonkFMRadio.cpp:265: error: 'class mozilla::hal::FMRadioSettings'
> has no member named 'isFMRadioOn'
>   hal/gonk/GonkFMRadio.cpp:279: error: 'class mozilla::hal::FMRadioSettings'
> has no member named 'isFMRadioOn'
As in comment 98, it is an obsolete variable. I think you can mark it first.

(In reply to Ray Cheung [:pzhang] from comment #108)
> And what if the country is not defined in the hal, like China, then what the
> settings should be?
If the country is not on the list, the callers should input frequency range, 
space type and pre-emphasis by themselves.

Comment 110 StevenLee[:slee] 2012-09-11 23:38:28 PDT

Created attachment 660329 [details] [diff] [review]
PartPart 3 - Volume settings in AudioManager - V3

1. Change the description in nsIAudioManager.idl
2. Remove static member, mFmEnabled.
3. Use IsFmRadioAudioOn to check the FM audio status.

Comment 111 StevenLee[:slee] 2012-09-13 21:09:21 PDT

After asking the FM chip vendor again, they said that the strength signal range they provided
is not correct. It should be -120(0x88)db to -20(0xEC)db. Should we normalize the strength to
0 ~ 100?

Comment 112 Justin Lebar (not reading bugmail) 2012-09-14 06:53:19 PDT

Aha, they /are/ giving you decibels!

It's not at all clear to me what's the right way to transform this to a linear scale.  If you're getting db, I think actually you might as well expose the raw db and let the front-end figure out the scaling.  (Although I think we may just want to report the db in the DOM API and let the FM radio app figure out how to show that...)

Comment 113 Michael Wu [:mwu] 2012-09-14 16:10:05 PDT

Created attachment 661398 [details] [diff] [review]
Simplified gonk radio backend, v2

Review comments addressed, unbitrotted.

Comment 114 StevenLee[:slee] 2012-09-17 23:32:44 PDT

(In reply to Michael Wu [:mwu] from comment #113)
> Created attachment 661398 [details] [diff] [review]
> Simplified gonk radio backend, v2
> 
> Review comments addressed, unbitrotted.
Hi mwu,

pzhang said that there is no FM_RADIO_OPERATION_DISABLE callback. I test the
code and found that only TAVARUA_EVT_RADIO_READY called even when we disable
the FM radio. Maybe we should check sRadioEnabled and decide whether callback
FM_RADIO_OPERATION_ENABLE or FM_RADIO_OPERATION_DISABLE?

setControl(V4L2_CID_PRIVATE_TAVARUA_REGION, TAVARUA_REGION_OTHER) should move to
line 206. If we don't, we have the problem mentioned in Comment 97.

Comment 115 StevenLee[:slee] 2012-09-17 23:42:29 PDT

Created attachment 662048 [details] [diff] [review]
Part 2 - FM functions in hal - V4.2

rebase

Comment 116 Michael Wu [:mwu] 2012-09-19 09:07:52 PDT

Created attachment 662582 [details] [diff] [review]
Simplified gonk radio backend, v3

Patch updated to
1. Improve disable handling
2. Switch polling thread to explicit loop
3. Move the region configuration code

Comment 117 Michael Wu [:mwu] 2012-09-19 10:58:17 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0709603af6 (gonk backend plus hal changes)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa535ebebed4 (audiomanager changes)

Comment 118 Michael Wu [:mwu] 2012-09-19 11:27:45 PDT

FallbackFMRadio.cpp didn't compile.

https://hg.mozilla.org/integration/mozilla-inbound/rev/45a10fa9d60b

Comment 119 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-19 12:43:32 PDT

I backed out a range of patches including this one which caused a perma-leak in all of our test suites on debug builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60144694d56

Comment 120 Michael Wu [:mwu] 2012-09-19 12:54:58 PDT

(In reply to Ehsan Akhgari [:ehsan] from comment #119)
> I backed out a range of patches including this one which caused a perma-leak
> in all of our test suites on debug builds:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e60144694d56

Thanks!

Steven, these patches should have been tested on try first. Please fix the issues, test on try, and post a new patch.

Comment 121 Justin Lebar (not reading bugmail) 2012-09-19 13:11:12 PDT

> Steven, these patches should have been tested on try first.

TBF, nobody had claimed that this was checkin-needed.  :)

Comment 122 Pin Zhang [:pzhang] (inactive) 2012-09-19 18:31:35 PDT

The app page is not responding for about 1.8s when the FM radio is being enabled. Is the enable function changed back to sync?

Comment 123 StevenLee[:slee] 2012-09-20 23:06:10 PDT

Created attachment 663298 [details] [diff] [review]
Part 2 - FM functions in hal - V4.3

rebase and fixed a memory leak problem of nsTArray.
Here is the try log. https://tbpl.mozilla.org/?tree=Try&rev=a026fcc06b92

Comment 124 Justin Lebar (not reading bugmail) 2012-09-21 10:54:36 PDT

I didn't put checkin-needed because it's not clear whether slee's newest patch has mwu's FallbackFMRadio.cpp fix, and so on.  But I think this is basically ready for mwu to push again.

Comment 125 Michael Wu [:mwu] 2012-09-21 13:44:51 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a74d48fd5f (all hal changes)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd02a2c2f32 (audiomanager changes)

Comment 126 Ryan VanderMeulen [:RyanVM] 2012-09-21 19:53:14 PDT

https://hg.mozilla.org/mozilla-central/rev/e5a74d48fd5f
https://hg.mozilla.org/mozilla-central/rev/6bd02a2c2f32

Comment 127 Paul Theriault [:pauljt] 2013-12-08 18:50:29 PST

Reviewed this ages ago, and doesn't need as much review since FMradio was made certified only. We can revisit if/when this changes.

Comment 128 Jean-Yves Perrier [:teoli] 2014-12-28 08:24:08 PST

Documentation has been written long ago: https://developer.mozilla.org/en-US/docs/Web/API/WebFM_API