864327 - (FM-RDS) Support of FM RDS in WebAPI

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::FMRadio, defect)

Description

[Paolo]

Attachment: webfm_rds_support_proposal_20130422.txt

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.64 Safari/537.31

Steps to reproduce:

The WebAPI for FM radio don't support FM RDS at the moment. Here is a proposal, discussed in mozilla.dev.webapi, to include also FM RDS support. A few RDS parameters would allow to automatically recognize the FM radio station, so enabling interesting use cases (see detail in the attachment and discussion on mozilla.dev.webapi  https://groups.google.com/forum/#!topic/mozilla.dev.webapi/viKr7qCiKaM )

Comment 1

[Paolo]

Changed Component and Platform (thanks Andy).
Added CC as in original https://bugzilla.mozilla.org/show_bug.cgi?id=779500

Comment 2

[Boris Zbarsky [:bzbarsky]]

>   attribute Function onpicodechange; // PI Code becomes available or changes (REQUIRED)

Function, or EventHandler?  They have different behavior; I assume you want the latter.

>   readonly attribute int piCode; // PI Code: the Program Identification (REQUIRED)

There is no "int" in WebIDL.  I assume this is meant to be a "long"?

>   readonly attribute string psCode; // PS name: Program Service name

And a DOMString here?

Comment 3

[Paolo]

Attachment: Proposal for FM RDS support (r02)

Comment 4

[Paolo]

Good points Boris, thank you.
I integrate them in the text. I confirm onpicodchange and the other Functions are meant to handle events, I just added them to a basic existing structure (e.g. https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/PraULCQntqA). If you have suggestions to improve it according to webIDL syntax and Mozilla best pratices, feel free to modify the proposal.

Comment 6

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #4)
> Good points Boris, thank you.
> I integrate them in the text. I confirm onpicodchange and the other
> Functions are meant to handle events, I just added them to a basic existing
> structure (e.g.
> https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/
> PraULCQntqA). If you have suggestions to improve it according to webIDL
> syntax and Mozilla best pratices, feel free to modify the proposal.

Hi Paolo, thanks for your proposal, I'm going to work on this feature, is there any update for the API proposal? Since we need someone in WebAPI team to review this, and if you think it's ready to be reviewed, I would ask the WebAPI guys to help us.

Comment 7

[Paolo]

Hi Pin Zhang,
this is great news for us! We're taking some time because I think the draft should also be checked by our EBU colleagues. Second, the API should be future proof and allow access to digital radio too (e.g. DAB+/DMB). 

We'll be back in a few days.
Kind regards
Paolo


(In reply to Pin Zhang [:pzhang] from comment #6)
> (In reply to Paolo from comment #4)
> > Good points Boris, thank you.
> > I integrate them in the text. I confirm onpicodchange and the other
> > Functions are meant to handle events, I just added them to a basic existing
> > structure (e.g.
> > https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/
> > PraULCQntqA). If you have suggestions to improve it according to webIDL
> > syntax and Mozilla best pratices, feel free to modify the proposal.
> 
> Hi Paolo, thanks for your proposal, I'm going to work on this feature, is
> there any update for the API proposal? Since we need someone in WebAPI team
> to review this, and if you think it's ready to be reviewed, I would ask the
> WebAPI guys to help us.

Comment 8

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #7)
> Hi Pin Zhang,
> this is great news for us! We're taking some time because I think the draft
> should also be checked by our EBU colleagues. Second, the API should be
> future proof and allow access to digital radio too (e.g. DAB+/DMB). 
> 
> We'll be back in a few days.
> Kind regards
> Paolo
> 
> 
> (In reply to Pin Zhang [:pzhang] from comment #6)
> > (In reply to Paolo from comment #4)
> > > Good points Boris, thank you.
> > > I integrate them in the text. I confirm onpicodchange and the other
> > > Functions are meant to handle events, I just added them to a basic existing
> > > structure (e.g.
> > > https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/
> > > PraULCQntqA). If you have suggestions to improve it according to webIDL
> > > syntax and Mozilla best pratices, feel free to modify the proposal.
> > 
> > Hi Paolo, thanks for your proposal, I'm going to work on this feature, is
> > there any update for the API proposal? Since we need someone in WebAPI team
> > to review this, and if you think it's ready to be reviewed, I would ask the
> > WebAPI guys to help us.

Hi Paolo, any updates?

Comment 9

[Paolo]

Attachment: Proposal for FM RDS and DAB Digital Radio support (r03)

This proposal aims to extend support to FM RDS and DAB/DAB+ Digital Radio (the most adopted standard for digital radio).

Comment 10

[Paolo]

(In reply to Paolo from comment #9)
> Created attachment 822281 [details]
> Proposal for FM RDS and DAB Digital Radio support (r03)
> 
> This proposal aims to extend support to FM RDS and DAB/DAB+ Digital Radio
> (the most adopted standard for digital radio).

(In reply to Pin Zhang [:pzhang] from comment #8)
> (In reply to Paolo from comment #7)
> > Hi Pin Zhang,
> > this is great news for us! We're taking some time because I think the draft
> > should also be checked by our EBU colleagues. Second, the API should be
> > future proof and allow access to digital radio too (e.g. DAB+/DMB). 
> > 
> > We'll be back in a few days.
> > Kind regards
> > Paolo
> > 
> > 
> > (In reply to Pin Zhang [:pzhang] from comment #6)
> > > (In reply to Paolo from comment #4)
> > > > Good points Boris, thank you.
> > > > I integrate them in the text. I confirm onpicodchange and the other
> > > > Functions are meant to handle events, I just added them to a basic existing
> > > > structure (e.g.
> > > > https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/
> > > > PraULCQntqA). If you have suggestions to improve it according to webIDL
> > > > syntax and Mozilla best pratices, feel free to modify the proposal.
> > > 
> > > Hi Paolo, thanks for your proposal, I'm going to work on this feature, is
> > > there any update for the API proposal? Since we need someone in WebAPI team
> > > to review this, and if you think it's ready to be reviewed, I would ask the
> > > WebAPI guys to help us.
> 
> Hi Paolo, any updates?

Hi Pin Zhang,
I've just uploaded the new proposal for the API, including both FM/RDS and DAB/DAB+ digital radio support. This proposal should enhance radio support and also enable all use cases related to Hybrid Radio (RadioDNS Project www.radiodns.org)

Best regards
Paolo

Comment 11

[Pin Zhang [:pzhang] (inactive)]

Hi Paolo, thanks for your updates. I have questions about the DAB/DAB+ radio, is there mobile phone(s) that support the DAB/DAB+ radio? Is there any related chipset for mobile device? 

It's always good to cover all the usecases, but currently we still need to consider the development effort, if there isn't an appropriate mobile device that both supports DAB/DAB+ and also has B2G ported, then I think this will spend too much effort.

Comment 12

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #11)
> Hi Paolo, thanks for your updates. I have questions about the DAB/DAB+
> radio, is there mobile phone(s) that support the DAB/DAB+ radio? Is there
> any related chipset for mobile device? 
> 
> It's always good to cover all the usecases, but currently we still need to
> consider the development effort, if there isn't an appropriate mobile device
> that both supports DAB/DAB+ and also has B2G ported, then I think this will
> spend too much effort.

Hi Pin, thank you for the quick feedback.
we're very happy about the decision to develop a standard API for FM/RDS Radio access, and of course the inclusion of the digital radio part depends on your evaluations.

Some facts could clarify the context. There are phones supporting DAB/DAB+ radio indeed. In South Korea specially (they use DAB/DAB+/DMB too, and they have some DAB enabled smartphones, e.g. Samsung Galaxy Note II DMB http://www.youtube.com/watch?v=nRUGpekzf_w ). DAB will become popular in the next months, now it's a niche on the phones. However, look at http://www.theidag.org/ and specially at http://www.theidag.org/device.html for devices available also in Europe. Also note, some countries are officially considering FM switch off in favor of DAB (e.g. Norway 2017, UK, Sweden, Denmark, ref. http://www.worlddab.org/news/denmark  http://www.garfors.com/2013/06/sweden-joins-fm-switch-off-club.html http://www.digitalradioltd.com/fm-switch-off-confirmed/ ).

I think the important thing for now is to define and agree on the interface, for DAB too, allowing early device manufacturers to use a standard API. The real implementation of the DABRadio Interface can wait of course. Do you think it's possible to just define the interface and let it be available for the next months?

Best regards
Paolo

Comment 13

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #12)> 
> I think the important thing for now is to define and agree on the interface,
> for DAB too, allowing early device manufacturers to use a standard API. The
> real implementation of the DABRadio Interface can wait of course. Do you
> think it's possible to just define the interface and let it be available for
> the next months?
> 

How about open a separate API for DAB/DAB+ supporting?

Comment 14

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #12)> 
> I think the important thing for now is to define and agree on the interface,
> for DAB too, allowing early device manufacturers to use a standard API. The
> real implementation of the DABRadio Interface can wait of course. Do you
> think it's possible to just define the interface and let it be available for
> the next months?
> 

How about open a separate bug for DAB/DAB+ supporting?

Comment 15

[Pin Zhang [:pzhang] (inactive)]

> //  NEW BEGIN
> readonly attribute long signalStrength; // the signal strength, a measure of signal quality

Hi Paolo, could you provide more detailed information about the attribute |signalStrength|, like min and max value?

We have discussed this before, I think you can check bug 779500 comment 128.

Comment 16

[Paolo]

Hi Pin,
we're discussing a common proposal. I hope I can send you the final draft in a few days.

Comment 17

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #16)
> Hi Pin,
> we're discussing a common proposal. I hope I can send you the final draft in
> a few days.

Is this discussion public? We need someone in WebAPI team to review your proposal, so I think it will be helpful if the discussion thread could be publicly accessed.

Comment 18

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #17)
> (In reply to Paolo from comment #16)
> > Hi Pin,
> > we're discussing a common proposal. I hope I can send you the final draft in
> > a few days.
> 
> Is this discussion public? We need someone in WebAPI team to review your
> proposal, so I think it will be helpful if the discussion thread could be
> publicly accessed.

Hi Pin,
the public discussions can be accessed at the RadioDNS-developers forum: https://groups.google.com/forum/#!topic/radiodns-developers/lgBkFzVTdxw

Comment 19

[Julian Parker]

It would be great to have RDS-support for the FM-radio, so that the name of the radio-station and additional info can be displayed (the station-names also in the station-favorites).

Comment 20

[Paolo]

Attachment: webfm_fm_rds_support_proposal_20131118.txt

The attachment obsoletes previous drafts.
The proposal now focus on FM/RDS only (leaving out Digital Radio at the moment), as requested.

Comment 21

[Paolo]

Attachment: A simple state diagram, illustrating the basic operation at a glance

A simple state diagram, illustrating the basic operation at a glance.
It refers to version r04 of the proposal.
"searching" state is triggered by setFrequency(), seekUp() and seekDown()
"antennaAvailable" is a pre-requisite, not included in the diagram to simplify it

Comment 22

[Paolo]

Dear Pin,
here is the amended proposal for the FM/RDS API. 
As requested, we left out the Digital Radio part.
The proposal indicates that "dBm" is the preferred unit for signalStrength. However:
- it was agreed that the first purpose of signalStrength is to give the app an indication of signal quality, not to precisely measure the signal power
- different chips can give different values of this parameter, not easily comparable
My undertanding is that if you think the dBm value could lead to problems for the app developers, it's possible to map signalStrength on a [0,1] closed interval, allowing a relative measure. In this case it should be a "double". Of course the actual max and min output values of the FM chip (to be mapped on the interval) depend on each manufacturer. 

Best regards
Paolo

Comment 23

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #22)
> Dear Pin,
> here is the amended proposal for the FM/RDS API. 
> As requested, we left out the Digital Radio part.
> The proposal indicates that "dBm" is the preferred unit for signalStrength.
> However:
> - it was agreed that the first purpose of signalStrength is to give the app
> an indication of signal quality, not to precisely measure the signal power

Agree.

> - different chips can give different values of this parameter, not easily
> comparable
> My undertanding is that if you think the dBm value could lead to problems
> for the app developers, it's possible to map signalStrength on a [0,1]
> closed interval, allowing a relative measure. In this case it should be a
> "double". Of course the actual max and min output values of the FM chip (to
> be mapped on the interval) depend on each manufacturer. 

I don't think it's a good idea to have a different min/max value of signal strength depend on each manufacturer (bug 779500 comment 131), or I think app developers won't know how to deal with it if we don't provide two more extra attributes maxsignaltrength and minsignalstrenght.

Comment 24

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8333754 [details]
webfm_fm_rds_support_proposal_20131118.txt

>Proposed FM/RDS low level Radio API, r04
>
>interface FMRadio : EventTarget { 
>   readonly attribute boolean disabled; // NEW: when the interface is created, is in the "disabled" state
>   readonly attribute boolean enabled; 

Why do we need |disabled|, isn't it disabled when enabled=false?

>   readonly attribute boolean searching;   // NEW: "searching" state

I prefer |seeking| which is more consistent with |seekUp| and |seekDown|.

>   readonly attribute boolean paused;      // NEW: "paused" state
>   readonly attribute boolean starting;    // NEW: "starting" state

How about |enabling|? I think it's intuitively more connected with |enable|.

>   attribute Function onpause;   // NEW: signalStrength changes
>   attribute Function onresume;  // NEW: signalStrength changes
>   attribute Function onstartingcomplete;  // NEW: signalStrength changes

I think it's the same as |onenabled|, right?

>   attribute Function onsearching;  // NEW: signalStrength changes
>   attribute Function onsearchingcomplete;  // NEW: signalStrength changes
>

Same here, how about onseeking and onseekingcomplete?

Comment 25

[David Bruant]

Hi Paolo,

Why is there a need for the new paused/starting/disabled/searching states? Can't that be inferred from the rest (mostly events I imagine?). From your diagram, I understand, this is a state machine and only one of these value can be true at once, so why not just a "state" attribute that would take one of the said values?
IIRC, the radio API was moved to privileged, so API breaking changes can be allowed. That would enable removing the "enabled" attribute. Or it could stay alongside "state", no big deal.

What the semantics of pausing? How is it different from disabling?

Otherwise, the changes look good to me :-)

Comment 26

[Paolo]

Hi Pin, hi David
thank you for the quick feedback! Very good points.
- signalStrength: Pin, of course different min and max could be confusing. Your proposal of a [0,1] relative signal strength interval could be a good solution (each chip will try to map the min to 0 and max to 1). On the other hand, I'm not a specialist, a manufacturer's feedback could help a lot, I'll ask again.
- enabling instead of starting: Pin, I agree, it's more consistent with other names
- enabled/disabled: of course it could be enabled with true/false values, however, in my opinion, it would be cleaner to have distinct states, and it would be more coherent with the other states (paused, enabling, seeking...).
- onseeking, onseekingcomplete: ok, the same as starting
- onpause, onresume, onseekingcomplete: the same as |onenable|? Could you write down your suggestion?
- state: David, my opinion is that tracking the states, while not strictly necessary, would help the developer a lot and perhaps it could facilitate app implementation. You suggest to create just one state attribute with different values (perhaps named "state" or "currentState"?), it could be a choice. Which one fits better to WebAPI best practices?
- pause: it was added after thinking about possible use cases. Something similar was included in Ericsson API (2010) too. |pause| it's different from disabling because the receiver can immediately pause and resume. How this can be achieved is left to the implementation, however I can think that pausing the radio is similar to "muting" it, so it can immediately recover. After disabling it, the tuner sould be re-enabled, the channel tuned, data structures re-allocated and so on. It could be very fast too, but it generally depends on the hardware and middleware. A similar example of the same concept with Android: playing an internet radio stream can take from about 2s to 15s (!!!) depending on the phone model (and firmware), and it's not immediate at all. Moreover, there can be also external reasons forcing the receiver in a |pause| state (a phone call arriving).

I'm waiting for your feedback before uploading a new version of the proposal.

Comment 27

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #26)
> - onpause, onresume, onseekingcomplete: the same as |onenable|? Could you
> write down your suggestion?

What I commented for is:
 >   attribute Function onstartingcomplete;  // NEW: signalStrength changes

and what i meant is, when the enabling process is done, |onenabled| event would be fired, so |onstartingcomplete| in your proposal is kinda a duplicated event.

Comment 28

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #27)

> What I commented for is:
>  >   attribute Function onstartingcomplete;  // NEW: signalStrength changes

|onstartingcomplete| will be removed as a duplicate.

Comment 29

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #27)

> What I commented for is:
>  >   attribute Function onstartingcomplete;  // NEW: signalStrength changes

Pin, maybe we should reconsider |onstartingcomplete|, as it could be useful as well. E.g, an action could be done when the tuner is |enabled| (after starting, seeking or resuming...), and other actions specifically when the tuner exited the |starting| state or the |pause| state.

What do you think about that?

Comment 30

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #29)
> (In reply to Pin Zhang [:pzhang] from comment #27)
> 
> > What I commented for is:
> >  >   attribute Function onstartingcomplete;  // NEW: signalStrength changes
> 
> Pin, maybe we should reconsider |onstartingcomplete|, as it could be useful
> as well. E.g, an action could be done when the tuner is |enabled| (after
> starting, seeking or resuming...), and other actions specifically when the
> tuner exited the |starting| state or the |pause| state.
> 
> What do you think about that?

Sorry, i still didn't quite understand, could you give me a more detailed explanation?

Comment 31

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #30)

> Sorry, i still didn't quite understand, could you give me a more detailed
> explanation?

Hi Pin, first of all, maybe the names I used are confusing, because the "enabled" state of the old API had a different meaning.
Look at the state machine diagram (png file):
- change "enabled" with "playing" (a more meaningful name)
- now, we can call "enabled" the group of states after the transition from "disabled" ("starting", "playing", "seeking", "paused"...)

Is it better? If yes, I'm going to update the state diagram so it's more detailed, and we can go on with the callback details.

Comment 32

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #31)
> (In reply to Pin Zhang [:pzhang] from comment #30)
> 
> > Sorry, i still didn't quite understand, could you give me a more detailed
> > explanation?
> 
> Hi Pin, first of all, maybe the names I used are confusing, because the
> "enabled" state of the old API had a different meaning.
> Look at the state machine diagram (png file):
> - change "enabled" with "playing" (a more meaningful name)
> - now, we can call "enabled" the group of states after the transition from
> "disabled" ("starting", "playing", "seeking", "paused"...)
> 
> Is it better? If yes, I'm going to update the state diagram so it's more
> detailed, and we can go on with the callback details.

OK, I see, thanks!

Hi Marco, is it possible to mute the FM radio without turning it off?

Comment 33

[David Bruant]

(In reply to Paolo from comment #26)
> - state: David, my opinion is that tracking the states, while not strictly
> necessary, would help the developer a lot and perhaps it could facilitate
> app implementation. You suggest to create just one state attribute with
> different values (perhaps named "state" or "currentState"?), it could be a
> choice. Which one fits better to WebAPI best practices?
The common practice would be one attribute with different string values.

> - pause: it was added after thinking about possible use cases. Something
> similar was included in Ericsson API (2010) too. |pause| it's different from
> disabling because the receiver can immediately pause and resume. How this
> can be achieved is left to the implementation
Not entirely. The semantics of "paused" and how it differs from "disabled" need to be explained in the spec. It's generally good for app developers if the "paused" semantics has common characteristics over different implementation. You're mentioning radio resume latency, another thing authors may be concerned with is battery (see below)

> however I can think that
> pausing the radio is similar to "muting" it, so it can immediately recover.
> After disabling it, the tuner sould be re-enabled, the channel tuned, data
> structures re-allocated and so on. It could be very fast too, but it
> generally depends on the hardware and middleware. A similar example of the
> same concept with Android: playing an internet radio stream can take from
> about 2s to 15s (!!!) depending on the phone model (and firmware), and it's
> not immediate at all. Moreover, there can be also external reasons forcing
> the receiver in a |pause| state (a phone call arriving).
For what you describe, it looks like "pause" is equivalent to setting the volume to 0. Among other things, it means that the phone keeps receiving/parsing/analyzing the radio data (which I imagine isn't the case for disabled), thus probably consuming more battery for instance. Is it the intention? It's fine if it is, I'm just making sure nothing is overlooked.

Comment 34

[Marco Chen [:mchen]]

(In reply to Pin Zhang [:pzhang] from comment #32)
> 
> Hi Marco, is it possible to mute the FM radio without turning it off?

Currently, AudioChannel will use the control of audio routing path from AudioManager to mute/unmute FMRadio.
This is a system wide control on audio codec configuration.

On the other way, FM chip itself should support mute/unmute functionality.

Comment 35

[Pin Zhang [:pzhang] (inactive)]

(In reply to Marco Chen [:mchen] from comment #34)
> (In reply to Pin Zhang [:pzhang] from comment #32)
> > 
> > Hi Marco, is it possible to mute the FM radio without turning it off?
> 
> Currently, AudioChannel will use the control of audio routing path from
> AudioManager to mute/unmute FMRadio.
> This is a system wide control on audio codec configuration.
> 
> On the other way, FM chip itself should support mute/unmute functionality.

It's cool if FM chip support this.

Comment 36

[Paolo]

Attachment: webfm_fm_rds_support_proposal_20131121_r05.txt

Comment 37

[Paolo]

Attachment: A simple state diagram, illustrating the basic operation (r05)

Comment 38

[Paolo]

Dear Pin, David and Marco,
I've uploaded a revision of the API, including corrections, suggestions and improvements. There's also an improved smd (pdf). Here are some comments:

- in the state machine diagram, you can see the new |enabled| state, including |enabling|, |playing|, |tuning|, |seeking|, |paused|. Pin: do you think we should change something?
- Pin: the onenabled handler should fire when entering the enabled group of states (before any operation). What was the original purpose of this handler? 
- a new |tuning| state has been added: the setFrequency is not an immediate operation
- the old |starting| is now |enabling|
- there's still one boolean attribute for each state; David, in case you think there a better approach could you detail your proposal?
- each transient state has a callback: onseekingcomplete, ontuningcomplete, onenablingcomplete
- the onfrequencychange handler can be called after a seek(up,down) or a setFrequency
- two event handlers have been associated to |pause|: onpause and onresume, so that if there's an external event forcing pause (e.g. phone call) the app will be properly notified
- signalStrength has been changed to double, [0,1] range. Maybe there'll be additional feedback from manufacturers

David: |paused| state: you're right, the semantics differs from the |enabled|. I confirm the important requirement is to quickly switch from |playing| to |paused| and viceversa. It would be very useful if the fm chip supported mute/unmute. In other cases probably it should be implemented muting the audio.

Pin Zhang and David, is there a preferred way to make a prototype or mockup to just check the consistency of the API?

As always, feedbacks/corrections/modifications are welcome.

Best regards
Paolo

Comment 39

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #38)
> Dear Pin, David and Marco,
> I've uploaded a revision of the API, including corrections, suggestions and
> improvements. There's also an improved smd (pdf). Here are some comments:
> 
> - in the state machine diagram, you can see the new |enabled| state,
> including |enabling|, |playing|, |tuning|, |seeking|, |paused|. Pin: do you
> think we should change something?

|enabled| equals to |!disabled|, right? So I think we can just keep |disabled| state. 

> - Pin: the onenabled handler should fire when entering the enabled group of
> states (before any operation). What was the original purpose of this
> handler? 

The original |onenabled| event indicates the FM radio HW is turned on.

> - each transient state has a callback: onseekingcomplete, ontuningcomplete,
> onenablingcomplete

I think the new |onenablingcomplete| event is the same as the original |onenabled|.

> - signalStrength has been changed to double, [0,1] range. Maybe there'll be
> additional feedback from manufacturers

I found there are two attributes for signal strength in WifiNetworkAPI[1], one is |signalStrength| in dBm, another is |relSignalStrength| in the range [0,100], how about add the same two attributes for WebFM?

[1] https://developer.mozilla.org/en-US/docs/Web/API/WifiManager.getNetworks#Network

Comment 40

[Marco Chen [:mchen]]

Hi all,

The another concern here is for HW Resource Competing.

Overview:
  The current implementation in FxOS allowed multiple clients to use FM Hardware in the same time.
  Do we all agree this?

Use Case & Concern:
  Once a user listen FMRadio from App1 and the later App2 tries to enable TMC on another frequency.

Possible Behavior:
  1. We allowed multiple clients to access one FM HW, so App2 can set frequency and App1 loss the channel.

  2. We reject access from App2 and notify it that there is a client occupied the FM.

May I know everyone's opinion? Thanks.

Comment 41

[Pin Zhang [:pzhang] (inactive)]

(In reply to Marco Chen [:mchen] from comment #40)
> Hi all,
> 
> The another concern here is for HW Resource Competing.
> 
> Overview:
>   The current implementation in FxOS allowed multiple clients to use FM
> Hardware in the same time.
>   Do we all agree this?
> 
> Use Case & Concern:
>   Once a user listen FMRadio from App1 and the later App2 tries to enable
> TMC on another frequency.
> 
> Possible Behavior:
>   1. We allowed multiple clients to access one FM HW, so App2 can set
> frequency and App1 loss the channel.
> 
>   2. We reject access from App2 and notify it that there is a client
> occupied the FM.
> 

How do you define 'occupied by one app'? If I opened FM app and turned it on, can we power it off in the system app? Currently we disable the fm radio in system app when airplane mode is enabled.

Comment 42

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #39)
> (In reply to Paolo from comment #38)

> |enabled| equals to |!disabled|, right? So I think we can just keep
> |disabled| state. 

OK. Just to be sure I correctly understand: so when your tuner object is created, the |disabled| is true. And when |disabled| is false, we simply know that the tuner has left this state (we don't really know if the tuner is turning on or audio is playing). We will check other state attributes to get the actual state.

> The original |onenabled| event indicates the FM radio HW is turned on.

OK. So, the original |onenabled| fired when the music was effectively playing? Or immediately at the exit from |disabled|? Let's be careful as the operations are not instantaneous.

> I think the new |onenablingcomplete| event is the same as the original
> |onenabled|.

OK. We have here two different states: the |enabled| = !|disabled|, that is a group of states, and  |enabling| that is a transient state before the |playing| state. Do you think we can remove |onenabled| as well as the |enabled| state?

> I found there are two attributes for signal strength in WifiNetworkAPI[1],
> one is |signalStrength| in dBm, another is |relSignalStrength| in the range
> [0,100], how about add the same two attributes for WebFM?
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/API/WifiManager.
> getNetworks#Network

Perfect, I see no reason to do otherwise. Can you update the API proposal according to your suggestions?

Comment 43

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #42)
> (In reply to Pin Zhang [:pzhang] from comment #39)
> > (In reply to Paolo from comment #38)
> 
> > |enabled| equals to |!disabled|, right? So I think we can just keep
> > |disabled| state. 
> 
> OK. Just to be sure I correctly understand: so when your tuner object is
> created, the |disabled| is true. And when |disabled| is false, we simply
> know that the tuner has left this state (we don't really know if the tuner
> is turning on or audio is playing). We will check other state attributes to
> get the actual state.

Yes, and in your proposal, when |enabled| is true, we still need to check other states attributes to get the actual state, right? So what David's suggestion about just keep one attribute |state|| in comment 25 could prevent this, what do you think, Paolo?

> 
> > The original |onenabled| event indicates the FM radio HW is turned on.
> 
> OK. So, the original |onenabled| fired when the music was effectively
> playing? Or immediately at the exit from |disabled|? Let's be careful as the
> operations are not instantaneous.
> 
Yes, that's why the |enable| function take a frequency value as the initial parameter[1], so the FM radio wouldn't be the turnedon-but-not-playing state..

[1] https://developer.mozilla.org/en-US/docs/Web/API/FMRadio.enable

> > I think the new |onenablingcomplete| event is the same as the original
> > |onenabled|.
> 
> OK. We have here two different states: the |enabled| = !|disabled|, that is
> a group of states, and  |enabling| that is a transient state before the
> |playing| state. Do you think we can remove |onenabled| as well as the
> |enabled| state?

Yes, I think so.

Comment 44

[Paolo]

(In reply to Marco Chen [:mchen] from comment #40)

> The another concern here is for HW Resource Competing.
> 
> Overview:
>   The current implementation in FxOS allowed multiple clients to use FM
> Hardware in the same time.
>   Do we all agree this?

Marco, as it's an important feature impacting on the architecture and usage, do you know if there was a discussion about use cases? Does concurrent access mode enable relevant use cases or it only complicates the usage of the API? 
I've sent your question to the RadioDNS group, to have a feedback from them too. 

> Possible Behavior:
>   1. We allowed multiple clients to access one FM HW, so App2 can set
> frequency and App1 loss the channel.
> 
>   2. We reject access from App2 and notify it that there is a client
> occupied the FM.
> 
> May I know everyone's opinion? Thanks.

A. Maybe it's useful to notify other apps about current states and attributes (frequency, if the system is in |playing| mode, ps or, if implemented, notification on RadioText change ...). However, my opinion is that it could add too much complexity. Are there relevant use cases?

B. I vote for (1). Is there a difference between an app the user is effectively using and a background app? If yes, maybe the foreground app should control the FM, so your point (1) could be the best behaviour. An example from the media player: when the user is playing a podcast with an app, and the she selects another app and another podcast, she wants to play the new podcast, not being notified "You cannot play music because there's another app still playing".

Comment 45

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #43)

> Yes, and in your proposal, when |enabled| is true, we still need to check
> other states attributes to get the actual state, right? So what David's
> suggestion about just keep one attribute |state|| in comment 25 could
> prevent this, what do you think, Paolo?

You're right. And David's proposal significantly adds consistency and reduces complexity. Feel free to update the current API doc.

> Yes, that's why the |enable| function take a frequency value as the initial
> parameter[1], so the FM radio wouldn't be the turnedon-but-not-playing
> state..
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/API/FMRadio.enable

OK.

Comment 46

[Marco Chen [:mchen]]

(In reply to Paolo from comment #44)
> A. Maybe it's useful to notify other apps about current states and
> attributes (frequency, if the system is in |playing| mode, ps or, if
> implemented, notification on RadioText change ...). However, my opinion is
> that it could add too much complexity. Are there relevant use cases?
> 

Hi Paolo and Pin,

Use Case:
  1. An Navigator app is launched and receives TMC from FM RDS (Frequency X).
  2. Then user enables another FM app to listen broadcast (Frequency Y).
  3. User tries to put Navigator back to foreground but found TMC is no function now. (NG)
  4. User tries to enable TMC again.
  5. User finds that broadcast/audio from FM app is changed too.

Then user may fall into a loop between 3 and 5 because he doesn't know that TMC and FM app compete to a single FM HW.

Comment 47

[Paolo]

(In reply to Marco Chen [:mchen] from comment #46)

> Use Case:
>   1. An Navigator app is launched and receives TMC from FM RDS (Frequency X).
>   2. Then user enables another FM app to listen broadcast (Frequency Y).
>   3. User tries to put Navigator back to foreground but found TMC is no
> function now. (NG)
>   4. User tries to enable TMC again.
>   5. User finds that broadcast/audio from FM app is changed too.
> 
> Then user may fall into a loop between 3 and 5 because he doesn't know that
> TMC and FM app compete to a single FM HW.

Hi Marco,
first of all, good to know FirefoxOS also supports TMC for traffic messages! 
This seems an important use case. My concern is that specifically targeting this use case would create problems to other more frequent scenarios (e.g. users wishing to simply change station with a different app). As a user, I would be very annoyed at not being able to change channel.

Do you think a notification could solve the problem? E.g.
  1. A Navigator app is launched and receives TMC from FM RDS (Frequency X).
  1.1. User is warned that TMC only works with specific FM channels.
  2. User enables another FM app to listen broadcast (Frequency Y).
  [ optionally 2.1 An overlay message temporarily appears, listing the applications affected by the change - maybe a patent already covers this behavior]
  3. Putting the Navigator back in foreground, there's no TMC, with a message explaining the reason.

Comment 48

[Pin Zhang [:pzhang] (inactive)]

Hey guys, we already have made a consensus on the changes for RDS supporting, how about we open another bug for the improvement proposal of other parts, including: singal strength, more states and one-client-a-time etc? Then we can go to next step, RDS supporting things could move forward.

Comment 49

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #48)
> Hey guys, we already have made a consensus on the changes for RDS
> supporting, how about we open another bug for the improvement proposal of
> other parts, including: singal strength, more states and one-client-a-time
> etc? Then we can go to next step, RDS supporting things could move forward.

For me it's OK if you think it will help.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hey everyone, if you need API help with design or reviews here, Ehsan from the WebAPI team as offered to help.

Comment 51

[(no longer active)]

Yes, I meant to take a look at this today but I'm not sure if I'm going to have enough time.  Needinfo-ing myself to get back to you guys on Monday.

Comment 52

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL

This is the webidl patch for adding RDS support, it's slight different from Paolo's proposal, to be consistent with pi/ecc/ps, I changed |radioText| to abbreviation |rt|.

Comment 53

[Pin Zhang [:pzhang] (inactive)]

Hi guys, I filed bug 942727 for further discussion about other improvements than RDS, including: signal strength, state related attributes and event handlers.

Comment 54

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #52)

> This is the webidl patch for adding RDS support, it's slight different from
> Paolo's proposal, to be consistent with pi/ecc/ps, I changed |radioText| to
> abbreviation |rt|.

Great!

Comment 55

[(no longer active)]

Comment on attachment 8337584 [details] [diff] [review]
Part I: WebIDL

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

Sorry for the delay here.  I read a bit about FM RDS but my understanding may still be quite lacking, so if I'm missing something please feel free to correct me!  :-)

::: dom/webidl/FMRadio.webidl
@@ +34,5 @@
> +  /* Programme Identification */
> +  readonly attribute long pi;
> +
> +  /* Extended Country Code */
> +  readonly attribute long ecc;

If my understanding of <http://www.2wcom.com/fileadmin/redaktion/dokumente/Company/RDS_Basics.pdf> is correct, it seems like the ECC is only sent in the RDS group 1A.  I'm not 100% sure if I understand correctly what that means, but does that mean that in some cases we may not have an ECC to return here?  If yes, then this should probably be nullable...

What is the expected range of this value?

Another question, it seems to me that the PI contains both the country code and the program name, and uses 4 bits for the country code, which causes us to not be able to represent every country in that field, hence the need for the ECC field.  Now, what is the use case of this API?  If our only use case is to retrieve the country, perhaps we should have two attributes such as "programName" and "countryCode", the first one representing the name of the current program and the second one representing the country code information retrieved from both the PI and the ECC?

And last question, how does one map country codes to names here?

@@ +37,5 @@
> +  /* Extended Country Code */
> +  readonly attribute long ecc;
> +
> +  /* Programme Service */
> +  readonly attribute DOMString ps;

Please document that this value will have a maximum of 8 characters.

Also, with respect to my question below, isn't "programName" a better name here?  I understand that this is the FM RDS terminology, but it seems to me like "programName" can give a better impression of what this attribute returns.

@@ +40,5 @@
> +  /* Programme Service */
> +  readonly attribute DOMString ps;
> +
> +  /* Radio Text */
> +  readonly attribute DOMString rt;

I don't think we should try to abbreviate the names like this.  Names that describe what the attribute/method does, so I suggest naming these programIdentification, extendedCountryCode, programService, and radioText respectively.

@@ +67,5 @@
> +  /* Fired when the Programme Service is changed */
> +  attribute EventHandler onpschange;
> +
> +  /* Fired when the Radio Text is changed */
> +  attribute EventHandler onrtchange;

These may require renaming based on whatever names we come up with above.

Comment 56

[Paolo]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #55)

I can comment on some points.

> If my understanding of
> <http://www.2wcom.com/fileadmin/redaktion/dokumente/Company/RDS_Basics.pdf>
> is correct, it seems like the ECC is only sent in the RDS group 1A.  I'm not
> 100% sure if I understand correctly what that means, but does that mean that
> in some cases we may not have an ECC to return here?  If yes, then this
> should probably be nullable...

I confirm that. Extended Country Code use is not widespread, and it's sometimes not broadcast.

> What is the expected range of this value?

ECC is broadcast using 8bits. So the range is [0,FF].

> Another question, it seems to me that the PI contains both the country code
> and the program name, and uses 4 bits for the country code, which causes us
> to not be able to represent every country in that field, hence the need for
> the ECC field.  Now, what is the use case of this API?  If our only use case
> is to retrieve the country, perhaps we should have two attributes such as
> "programName" and "countryCode", the first one representing the name of the
> current program and the second one representing the country code information
> retrieved from both the PI and the ECC?

It's right, globally unique country information can only be inferred from the Country Code in the PI (4 most significant bits) + the Extended Country Code (ECC, 8bits). My opinion is that using programName + countryCode could be more intelligible but also more tricky, as we cannot guarantee program names are unique (instead, the PI code is unique in a country, theoretically).  We first proposed to have both PI and ECC as a quick way to build this information. 

Maybe a "global country code" (gcc) can be defined instead of ECC, like RadioDNS has done (gcc = 4msb of pi + ecc). This could be useful. However, PI code cannot be removed as it's a significant parameter to uniquely identify the program. 

Do we agree to define a gcc (or similar) instead of using ECC?

> And last question, how does one map country codes to names here?

There's a table in IEC 62106:1999, Annex D, table D.1 (you can buy the specs but sometimes you can find them on the internet). E.g. Italy ISO code:IT, ECC:E0, Country code:5 ...

> Please document that this value will have a maximum of 8 characters.

> Also, with respect to my question below, isn't "programName" a better name
> here?  I understand that this is the FM RDS terminology, but it seems to me
> like "programName" can give a better impression of what this attribute
> returns.

Personally I agree with you. However, historically, it's the "Programme Service name", or ps. Which one is better? It depends on the target. For FM broadcast people, ps or Programme Service name is quite familiar, for other people programName can be definitely better. However, people reading also the specs will find better refer to ps. I would leave ps as it is if there's no strong reason to do otherwise.

Comment 57

[andy]

> Do we agree to define a gcc (or similar) instead of using ECC?

I may have misunderstood our current intentions with this API, but I thought we wanted to enhance the Radio API to provide us with the minimum required information to enhance the radio experience through standards such as RadioDNS in applications that utilise the API.

Would it not be better for the API to exactly report what is coming OTA and no more, rather than to begin adding logic to merge values etc.? Surely the process of converting first nibble of PI + ECC in to an ISO country code would be better in the application that utilises the API?

Comment 58

[David Bruant]

(In reply to Paolo from comment #56)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #55)
> > Also, with respect to my question below, isn't "programName" a better name
> > here?  I understand that this is the FM RDS terminology, but it seems to me
> > like "programName" can give a better impression of what this attribute
> > returns.
> 
> Personally I agree with you. However, historically, it's the "Programme
> Service name", or ps. Which one is better? It depends on the target. For FM
> broadcast people, ps or Programme Service name is quite familiar, for other
> people programName can be definitely better. However, people reading also
> the specs will find better refer to ps. I would leave ps as it is if there's
> no strong reason to do otherwise.
I agree with this line of reasoning. The field exposed in JavaScript are expected to be perfect matches with values defined in some other spec (RDS). RDS-aware people will understand the JS API out of the box. If property names were different than RDS field names, it would create unnecessary confusion for these people (who are the ones who are going to use this API).
For those who are not familiar with RDS, the documentation will need to refer to the RDS spec. They need to understand the exact semantics of the fields anyway.

Comment 59

[Paolo]

(In reply to andy from comment #57)

> Would it not be better for the API to exactly report what is coming OTA and
> no more, rather than to begin adding logic to merge values etc.? Surely the
> process of converting first nibble of PI + ECC in to an ISO country code
> would be better in the application that utilises the API?

Good point Andy, maybe defining a new parameter could be confusing for people having some experience in broadcast or having read other documents. It's OK for me.

Comment 60

[(no longer active)]

(In reply to comment #56)
> > If my understanding of
> > <http://www.2wcom.com/fileadmin/redaktion/dokumente/Company/RDS_Basics.pdf>
> > is correct, it seems like the ECC is only sent in the RDS group 1A.  I'm not
> > 100% sure if I understand correctly what that means, but does that mean that
> > in some cases we may not have an ECC to return here?  If yes, then this
> > should probably be nullable...
> 
> I confirm that. Extended Country Code use is not widespread, and it's sometimes
> not broadcast.

Thanks.  So this field should be nullable to indicate that possibility.  The only problem with doing that would be the case where ECC is zero, since then a check such as:

if (radio.extraCountryCode == 0) { /*...*/ }

will succeed, since null is coerced to 0.  Is 0 a sensible value for this field?

> > What is the expected range of this value?
> 
> ECC is broadcast using 8bits. So the range is [0,FF].

Then we should probably change the type of this attribute to octet.

> > Another question, it seems to me that the PI contains both the country code
> > and the program name, and uses 4 bits for the country code, which causes us
> > to not be able to represent every country in that field, hence the need for
> > the ECC field.  Now, what is the use case of this API?  If our only use case
> > is to retrieve the country, perhaps we should have two attributes such as
> > "programName" and "countryCode", the first one representing the name of the
> > current program and the second one representing the country code information
> > retrieved from both the PI and the ECC?
> 
> It's right, globally unique country information can only be inferred from the
> Country Code in the PI (4 most significant bits) + the Extended Country Code
> (ECC, 8bits). My opinion is that using programName + countryCode could be more
> intelligible but also more tricky, as we cannot guarantee program names are
> unique (instead, the PI code is unique in a country, theoretically).  We first
> proposed to have both PI and ECC as a quick way to build this information. 
> 
> Maybe a "global country code" (gcc) can be defined instead of ECC, like
> RadioDNS has done (gcc = 4msb of pi + ecc). This could be useful. However, PI
> code cannot be removed as it's a significant parameter to uniquely identify the
> program. 
> 
> Do we agree to define a gcc (or similar) instead of using ECC?

So Andy disagrees here.  I am not an expert in this area, so I trust your judgement.  :-)

> > And last question, how does one map country codes to names here?
> 
> There's a table in IEC 62106:1999, Annex D, table D.1 (you can buy the
> specs but sometimes you can find them on the internet). E.g. Italy ISO code:IT,
> ECC:E0, Country code:5 ...

Does this mean that application authors are expected to do this on their own then?  I don't think that's good...  Should we add a way of mapping these codes to something more legible, perhaps the ISO 3166-1 alpha-2 country code?

> > Please document that this value will have a maximum of 8 characters.
> 
> > Also, with respect to my question below, isn't "programName" a better name
> > here?  I understand that this is the FM RDS terminology, but it seems to me
> > like "programName" can give a better impression of what this attribute
> > returns.
> 
> Personally I agree with you. However, historically, it's the "Programme Service
> name", or ps. Which one is better? It depends on the target. For FM broadcast
> people, ps or Programme Service name is quite familiar, for other people
> programName can be definitely better. However, people reading also the specs
> will find better refer to ps. I would leave ps as it is if there's no strong
> reason to do otherwise.

Unless you seriously object, I don't like the idea of adding the abbreviated names, so if we have to use "programServiceName", then so be it!  On the same note, do you think we should use the British spelling?  We usually don't do that for web APIs.

Another thing which I forgot to ask in my previous comments: do we have all of these values in memory ready to be accessed when these attributes are accessed?  I'm asking because if we don't, then we cannot expose these as synchronous attributes, so we should make sure that is not the case here.

Comment 61

[David Bruant]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > However, historically, it's the "Programme Service
> > name", or ps. Which one is better? It depends on the target. For FM broadcast
> > people, ps or Programme Service name is quite familiar, for other people
> > programName can be definitely better. However, people reading also the specs
> > will find better refer to ps. I would leave ps as it is if there's no strong
> > reason to do otherwise.
> 
> Unless you seriously object, I don't like the idea of adding the abbreviated
> names, so if we have to use "programServiceName", then so be it!
The Bluetooth API is a precedent of an API using abbreviated name.
https://developer.mozilla.org/en-US/docs/Web/API/BluetoothAdapter
BluetoothAdapter.ona2dpstatuschanged
    A handler for the a2dpstatuschanged event; it is triggered when an A2DP connection status changes.
BluetoothAdapter.onhfpstatuschanged
    A handler for the hfpstatuschanged event; it is triggered when an HFP connection status changes.
BluetoothAdapter.onscostatuschanged
    A handler for the scostatuschanged event; it is triggered when a SCO connection status changes.

I know nothing about Bluetooth, so A2DP, HFP and SCO sound like gibberish to me. But I imagine that they make a lot of sense for those familiar with the Bluetooth technology/spec. The day I'll need to use Bluetooth, I'll learn about the technology and these name will make sense.

I don't know a lot about WebRTC, but it has APIs using the ICE accronym (ICECandidate, onicecandidate), I just learned now, that ICE stands for "Interactive Connectivity Establishment".

I don't know WebGL, but I'm sure they do the same thing too (just looked up, there are suspicious OES and EXT prefixes in some places)

The situation with RDS is the same.
Re-inventing name feels like an unnecessary level of indirection. It only confuses people who already know the technology and the benefit to those who don't is unclear.
Some APIs are for specialists, I think the best we can do is expose an API that makes sense for them and document well enough for the newcomers.

Comment 62

[(no longer active)]

That is a fair point.  I fall into the non-specialist bucket here myself, so I'd defer this to Paolo and others who are more familiar with FM RDS.

Comment 63

[Pin Zhang [:pzhang] (inactive)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > I confirm that. Extended Country Code use is not widespread, and it's sometimes
> > not broadcast.
> 
> Thanks.  So this field should be nullable to indicate that possibility.  The
> only problem with doing that would be the case where ECC is zero, since then
> a check such as:
> 
> if (radio.extraCountryCode == 0) { /*...*/ }
> 
> will succeed, since null is coerced to 0.  Is 0 a sensible value for this
> field?

Paolo, can you comment more about what it means when ecc is 0?

Comment 64

[Pin Zhang [:pzhang] (inactive)]

Paolo, what's the range of PI?

Comment 65

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V2

Both ps and ecc are described as nullable attribute, regardless they are broadcast or not, any value are meaningless when the FM radio HW is disabled, just like the attribute |frequency|.

Comment 66

[Paolo]

(In reply to David Bruant from comment #61)

> The situation with RDS is the same.
> Re-inventing name feels like an unnecessary level of indirection. It only
> confuses people who already know the technology and the benefit to those who
> don't is unclear.
> Some APIs are for specialists, I think the best we can do is expose an API
> that makes sense for them and document well enough for the newcomers.

Hi David, Ehsan,
yes, people with a broadcast background, or having already developed radio apps with existing (limited) API are familiar with those acronyms. Of course also the full name is also clear for them (programServiceName or radioText). So "ps" or "programmeServiceName" are self-explanatory. Personally, I'd opt for the acronyms, but as Mozilla has a set of best practices and conventions I'd without doubt follow them. 

There's another point consider: this can be a first level of API, giving access to the necessary HW functions. On the top of them another thin layer can always be built, so if radio apps development will (hopefully ;-) become very popular, maybe other conventions will be used. 

And also consider this: other standards are gaining momentum (e.g. DAB+/DAB first of all, but also DRM or HDRadio in the USA). A unified API layer, abstracting the tuner features, would be very important. So, if we reduce the "indirection" or extra-semantics of the first thin API level (this one), using it for a more general API will be more straightforward.

Comment 67

[Paolo]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)

> will succeed, since null is coerced to 0.  Is 0 a sensible value for this
> field?

Well, there's no point in the specs saying ECC shouldn't be zero. However its values are enumerated in the specs (Annex D, Annex N) and 0 is never used, so 0 is not a permitted value. By the way, the revision of the specs is also available on -> http://www.rds.org.uk/2010/RDS-Specification.htm (the password will be sent on request for free). This is a VERY USEFUL resource.
Perhaps the key point is that ECC is available asynchronously, so I imagine that I can only use ECC (PI, and so on) if the corresponding event handler has been called. Does it make sense?

> Then we should probably change the type of this attribute to octet.

OK.

> So Andy disagrees here.  I am not an expert in this area, so I trust your
> judgement.  :-)

:-)

> Does this mean that application authors are expected to do this on their own
> then?  I don't think that's good...  Should we add a way of mapping these
> codes to something more legible, perhaps the ISO 3166-1 alpha-2 country code?

It should be clear that this step is not striclty needed for a minimum set of API. Having said that, an helper function creating the ISO code would be very useful, I completely agree. Hard-core developers will use PI4msb + ECC, others will use the ISO value directly.

> Unless you seriously object, I don't like the idea of adding the abbreviated
> names, so if we have to use "programServiceName", then so be it!  On the
> same note, do you think we should use the British spelling?  We usually
> don't do that for web APIs.

See the answer to you and David on this topic. I confirm it's not a typo, on the specs there's "Programme Service Name".

> Another thing which I forgot to ask in my previous comments: do we have all
> of these values in memory ready to be accessed when these attributes are
> accessed?  I'm asking because if we don't, then we cannot expose these as
> synchronous attributes, so we should make sure that is not the case here.

They are asynchronous. Our first idea was to set them after the associated event handler was called, maybe there's a better way to access them. Here is an indicative timing (from one of the first proposals):

An example of the timings in the receiver is the following (time in seconds):
0.00s   the user starts the radio app 
3.00s   the FM turns on 
3.10s   the music starts playng  
3.30s   PI and ECC available (and onpichange is called); the radio app begins enhancing the radio UX with content from the internet 
7:30s   station name appears (PS available) 

Some documentation about RDS/RDBS:
RDS: http://webstore.iec.ch/webstore/webstore.nsf/mysearchajax?Openform&key=62106 (it is not free - maybe you can find something on the web searching rds1999.pdf)
Another useful primer is http://en.wikipedia.org/wiki/Radio_Data_System
RDBS: http://www.nrscstandards.org/SG/nrsc-4-B.pdf

Comment 68

[David Bruant]

(In reply to Paolo from comment #67)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > Another thing which I forgot to ask in my previous comments: do we have all
> > of these values in memory ready to be accessed when these attributes are
> > accessed?  I'm asking because if we don't, then we cannot expose these as
> > synchronous attributes, so we should make sure that is not the case here.
> 
> They are asynchronous. Our first idea was to set them after the associated
> event handler was called, maybe there's a better way to access them.
I agree with the idea of exposing attributes asynchronously. I however have to point out that it's been discussed in the past [1]. The agreement was to expose synchronous properties and the tradeoff between performance and ease of programming favoured ease of programming.
Is the tradeoff any different in this instance?


[1] https://groups.google.com/d/msg/mozilla.dev.webapi/H-bKMtn_XUI/tnKBvNMyUjUJ

Comment 69

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #64)
> Paolo, what's the range of PI?

Hi Pin, PI is a 16 bits value (ref. specs, D.2, D.5, D.7). Its value will be available quickly (included in every block, however it's asynchronous, i.e. it's available after a (short) time.

For ECC, see the answer to Ehsan. ECC values listed in the specs don't include 0, so ECC cannot be zero, however, it's asynchronous. Moreover: isn't it tricky to choose one value (0) to say it's "invalid"?

Other details for RDS PI:
b15 b14 b13 b12: Country code. A fixed value between 0x01 and 0x0F. i.e. ANY VALUE EXCEPT 0 (ref. D.2)
b11 b10 b09 b08: 0x01 program(me) coverage area
b07 b06 b05 b04 b03 b02 b01 b00: program(me) reference number

Hence, RDS PI code of audio programmes cannot be zero.

RDBS is almost exactly the same while semantics for PI is different (the United States National Radio Systems Committee made it publicly and freely available, it includes only the *differences* from the RDS http://www.nrscstandards.org/SG/nrsc-4-B.pdf).

Comment 70

[Paolo]

(In reply to David Bruant from comment #68)
> (In reply to Paolo from comment #67)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > > Another thing which I forgot to ask in my previous comments: do we have all
> > > of these values in memory ready to be accessed when these attributes are
> > > accessed?  I'm asking because if we don't, then we cannot expose these as
> > > synchronous attributes, so we should make sure that is not the case here.
> > 
> > They are asynchronous. Our first idea was to set them after the associated
> > event handler was called, maybe there's a better way to access them.
> I agree with the idea of exposing attributes asynchronously. I however have
> to point out that it's been discussed in the past [1]. The agreement was to
> expose synchronous properties and the tradeoff between performance and ease
> of programming favoured ease of programming.
> Is the tradeoff any different in this instance?

David, I really cannot comment about the design decision here, it's more related to Mozilla's best practices. 
Just to be sure, could the states discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=942727 be helpful?

> [1]
> https://groups.google.com/d/msg/mozilla.dev.webapi/H-bKMtn_XUI/tnKBvNMyUjUJ

Comment 71

[David Bruant]

(In reply to Paolo from comment #70)
> (In reply to David Bruant from comment #68)
> > (In reply to Paolo from comment #67)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > > > Another thing which I forgot to ask in my previous comments: do we have all
> > > > of these values in memory ready to be accessed when these attributes are
> > > > accessed?  I'm asking because if we don't, then we cannot expose these as
> > > > synchronous attributes, so we should make sure that is not the case here.
> > > 
> > > They are asynchronous. Our first idea was to set them after the associated
> > > event handler was called, maybe there's a better way to access them.
> > I agree with the idea of exposing attributes asynchronously. I however have
> > to point out that it's been discussed in the past [1]. The agreement was to
> > expose synchronous properties and the tradeoff between performance and ease
> > of programming favoured ease of programming.
> > Is the tradeoff any different in this instance?
> 
> David, I really cannot comment about the design decision here, it's more
> related to Mozilla's best practices.
oh sorry, my comment was more addressed to Ehsan since he's Mozilla representative. I didn't mean to put this decision on your shoulders, sorry for the confusion.

> Just to be sure, could the states discussed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=942727 be helpful?
I need to look that in details. I'll comment there if I feel something is off.

Comment 72

[(no longer active)]

(In reply to Paolo from comment #66)
> (In reply to David Bruant from comment #61)
> 
> > The situation with RDS is the same.
> > Re-inventing name feels like an unnecessary level of indirection. It only
> > confuses people who already know the technology and the benefit to those who
> > don't is unclear.
> > Some APIs are for specialists, I think the best we can do is expose an API
> > that makes sense for them and document well enough for the newcomers.
> 
> Hi David, Ehsan,
> yes, people with a broadcast background, or having already developed radio
> apps with existing (limited) API are familiar with those acronyms. Of course
> also the full name is also clear for them (programServiceName or radioText).
> So "ps" or "programmeServiceName" are self-explanatory. Personally, I'd opt
> for the acronyms, but as Mozilla has a set of best practices and conventions
> I'd without doubt follow them. 

OK, I'm convinced.  Let's use the abbreviated names here!

> There's another point consider: this can be a first level of API, giving
> access to the necessary HW functions. On the top of them another thin layer
> can always be built, so if radio apps development will (hopefully ;-) become
> very popular, maybe other conventions will be used. 

What other APIs are you conceiving on top of this one?  It would be nice to at least have future extensions to the API in mind when thinking about what we want this API to look like.

Comment 73

[(no longer active)]

(In reply to Paolo from comment #67)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> 
> > will succeed, since null is coerced to 0.  Is 0 a sensible value for this
> > field?
> 
> Well, there's no point in the specs saying ECC shouldn't be zero. However
> its values are enumerated in the specs (Annex D, Annex N) and 0 is never
> used, so 0 is not a permitted value.

That's good to know.  What about pi?  We're going to have the exact same issue there as well.

> By the way, the revision of the specs
> is also available on -> http://www.rds.org.uk/2010/RDS-Specification.htm
> (the password will be sent on request for free). This is a VERY USEFUL
> resource.

I'd appreciate if you could please forward the password to me using my bugmail email address.  Thanks!

> Perhaps the key point is that ECC is available asynchronously, so I imagine
> that I can only use ECC (PI, and so on) if the corresponding event handler
> has been called. Does it make sense?

Yes, it makes a lot of sense!  So given this, I don't think that there is any reason for us to convert these values into promises.  Simply keeping them as nullable should enable us to return null if we have not read them yet, and once they are available, we can make them available in the content process for fast synchronous access.

> > Does this mean that application authors are expected to do this on their own
> > then?  I don't think that's good...  Should we add a way of mapping these
> > codes to something more legible, perhaps the ISO 3166-1 alpha-2 country code?
> 
> It should be clear that this step is not striclty needed for a minimum set
> of API. Having said that, an helper function creating the ISO code would be
> very useful, I completely agree. Hard-core developers will use PI4msb + ECC,
> others will use the ISO value directly.

Actually thinking about this more, I think that the conversion routine from PI/ECC to the ISO country code can be implemented entirely as a JS library, so there's probably no need for us to worry about it at this point.

> > Unless you seriously object, I don't like the idea of adding the abbreviated
> > names, so if we have to use "programServiceName", then so be it!  On the
> > same note, do you think we should use the British spelling?  We usually
> > don't do that for web APIs.
> 
> See the answer to you and David on this topic. I confirm it's not a typo, on
> the specs there's "Programme Service Name".

OK, the name "Programme" sounds good.

> > Another thing which I forgot to ask in my previous comments: do we have all
> > of these values in memory ready to be accessed when these attributes are
> > accessed?  I'm asking because if we don't, then we cannot expose these as
> > synchronous attributes, so we should make sure that is not the case here.
> 
> They are asynchronous. Our first idea was to set them after the associated
> event handler was called, maybe there's a better way to access them. Here is
> an indicative timing (from one of the first proposals):
> 
> An example of the timings in the receiver is the following (time in seconds):
> 0.00s   the user starts the radio app 
> 3.00s   the FM turns on 
> 3.10s   the music starts playng  
> 3.30s   PI and ECC available (and onpichange is called); the radio app
> begins enhancing the radio UX with content from the internet 
> 7:30s   station name appears (PS available) 

Thanks for the explanation, makes sense.

Comment 74

[(no longer active)]

(In reply to David Bruant from comment #68)
> (In reply to Paolo from comment #67)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > > Another thing which I forgot to ask in my previous comments: do we have all
> > > of these values in memory ready to be accessed when these attributes are
> > > accessed?  I'm asking because if we don't, then we cannot expose these as
> > > synchronous attributes, so we should make sure that is not the case here.
> > 
> > They are asynchronous. Our first idea was to set them after the associated
> > event handler was called, maybe there's a better way to access them.
> I agree with the idea of exposing attributes asynchronously. I however have
> to point out that it's been discussed in the past [1]. The agreement was to
> expose synchronous properties and the tradeoff between performance and ease
> of programming favoured ease of programming.
> Is the tradeoff any different in this instance?

I'm not sure how much of that discussion is relevant here.  My question was whether we need to read something synchronously (from the device driver/network for example) to retrieve these values.  If the only problem here is that these values are only available in the chrome process, we should be able to cache them in the content process if needed; no need to make the attributes return a promise just because of that reason.

Comment 75

[(no longer active)]

Comment on attachment 8339775 [details] [diff] [review]
Part I: WebIDL V2

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

r=me with the below fixed.

::: dom/webidl/FMRadio.webidl
@@ +31,5 @@
>     */
>    readonly attribute double channelWidth;
>  
> +  /* Abbr. for Programme Identification. */
> +  readonly attribute long? pi;

This should be unsigned short.

@@ +43,5 @@
> +  /**
> +   * Abbr. for Programme Service Name, it contains max 8 alphanumeric
> +   * characters.
> +   */
> +  readonly attribute DOMString ps;

Please make this nullable as well.

@@ +46,5 @@
> +   */
> +  readonly attribute DOMString ps;
> +
> +  /* Abbr. for Radio Text. */
> +  readonly attribute DOMString rt;

This too.

Comment 76

[Pin Zhang [:pzhang] (inactive)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #73)
> (In reply to Paolo from comment #67)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > 
> > > will succeed, since null is coerced to 0.  Is 0 a sensible value for this
> > > field?
> > 
> > Well, there's no point in the specs saying ECC shouldn't be zero. However
> > its values are enumerated in the specs (Annex D, Annex N) and 0 is never
> > used, so 0 is not a permitted value.
> 
> That's good to know.  What about pi?  We're going to have the exact same
> issue there as well.
> 

This is what Paolo mentioned in comment 69:
  "Hence, RDS PI code of audio programmes cannot be zero."

Comment 77

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V3

Addressed the nits listed in comment 75.

Comment 78

[Paolo]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #72)

> What other APIs are you conceiving on top of this one?  It would be nice to
> at least have future extensions to the API in mind when thinking about what
> we want this API to look like.

There's a "Universal Smartphone Radio Project", involving broadcasters and broadcaster associations, from Europe to USA, at its early stage. E.g. check http://www.worlddab.org/public_document/file/411/GA-final.pdf?1383049678 Wednesday, 15:20 "...Delegates will hear also about the Universal Smartphone Radio Project led by the BBC and Global Radio in the UK and others around the world including Clearchannel and EBU". As soon as we have proper information we'll report to this group.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #73)
> That's good to know.  What about pi?  We're going to have the exact same
> issue there as well.

From the specs, PI cannot be zero. 
RDS spec -> Figure D.1 - PI structure -> Nibble 1 values: 0x01 to 0x0F, code 0x00 shall NOT be used for country identification. So PI cannot be zero.
RDBS spec -> different semantics for PI in the USA, but -> after all the calculations and special values substitutions it seems 0 value is not possible for PI code (you can check at ftp://ftp.rds.org.uk/pub/acrobat/rbds1998.pdf .

> Yes, it makes a lot of sense!  So given this, I don't think that there is
> any reason for us to convert these values into promises.  Simply keeping
> them as nullable should enable us to return null if we have not read them
> yet, and once they are available, we can make them available in the content
> process for fast synchronous access.

Great!

> Actually thinking about this more, I think that the conversion routine from
> PI/ECC to the ISO country code can be implemented entirely as a JS library,
> so there's probably no need for us to worry about it at this point.

OK.

Comment 79

[Paolo]

(In reply to Pin Zhang [:pzhang] from comment #76)

> This is what Paolo mentioned in comment 69:
>   "Hence, RDS PI code of audio programmes cannot be zero."

OK... you already answered. See reply to Ehsan, this applies to RDBS PI code (USA...). A double check could be useful ftp://ftp.rds.org.uk/pub/acrobat/rbds1998.pdf

Comment 80

[(no longer active)]

(In reply to comment #76)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #73)
> > (In reply to Paolo from comment #67)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> > > 
> > > > will succeed, since null is coerced to 0.  Is 0 a sensible value for this
> > > > field?
> > > 
> > > Well, there's no point in the specs saying ECC shouldn't be zero. However
> > > its values are enumerated in the specs (Annex D, Annex N) and 0 is never
> > > used, so 0 is not a permitted value.
> > 
> > That's good to know.  What about pi?  We're going to have the exact same
> > issue there as well.
> > 
> 
> This is what Paolo mentioned in comment 69:
>   "Hence, RDS PI code of audio programmes cannot be zero."

Oh right, sorry I missed that!

Comment 81

[(no longer active)]

(In reply to Paolo from comment #78)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #72)
> 
> > What other APIs are you conceiving on top of this one?  It would be nice to
> > at least have future extensions to the API in mind when thinking about what
> > we want this API to look like.
> 
> There's a "Universal Smartphone Radio Project", involving broadcasters and
> broadcaster associations, from Europe to USA, at its early stage. E.g. check
> http://www.worlddab.org/public_document/file/411/GA-final.pdf?1383049678
> Wednesday, 15:20 "...Delegates will hear also about the Universal Smartphone
> Radio Project led by the BBC and Global Radio in the UK and others around
> the world including Clearchannel and EBU". As soon as we have proper
> information we'll report to this group.

Interesting.  FWIW I'm much less keen on exposing things that are this new in this area to web applications yet.  I think this would be an interesting area to watch, and once new standards like this stabilize and become popular in production environments, we can consider adopting them for web applications, but in general being an early adopter in this area seems rather scary to me.  :-)

Last but not least, thanks a lot for your help here so far, it's much appreciated!

Comment 82

[Attila Ladanyi]

By the way: if we want to install an FM radio, we also have to think of countries in where FM is sent with and without RDS in parallel. A complete different search function is needed. 

I think FM-RDS support is fairly meaningless without good ODA implementation. RadioDNS itself will have ODA support. However we should make sure that radio will working even without an internet connection.

RDS is just in development towards smart phones. (see RDS-Forum -> Future)
The gRT (graphical Radio Text) is based on HTML5/CSS databases and the transmission rates will be increased tenfold with xRDS. The data structures remain unchanged.

When Firefox OS wants to fight against iOS or Android then we should integrate the latest technologies to inspire the users instead of getting bored. This also means that we should help to expand the possibilities of the Broadcasters.(Also known as chicken - egg principle) These still waiting for a mobile platform which can carry more show in affordable way to the listener. Their satisfaction would be Firefox best advertisement. The most important is a middleware which sets a good virtual interface to a variety of chipsets and drivers. Everyone should be able to write Apps with little effort for ODA-s. Radio only to listen? That was 30 years ago.

Regards

Comment 83

[Paolo]

(In reply to Attila Ladanyi from comment #82)
> By the way: if we want to install an FM radio, we also have to think of
> countries in where FM is sent with and without RDS in parallel. A complete
> different search function is needed. 

Attila, here Pin restricted the scope of this bug to RDS. If RDS is not transmitted, it doesn't apply. Moreover, almost all FM stations use RDS as far as I know. Simply, if a station doesn't use RDS, it won't have access to the info. 

> I think FM-RDS support is fairly meaningless without good ODA
> implementation. RadioDNS itself will have ODA support. However we should
> make sure that radio will working even without an internet connection.

I don't agree at all. Simple PI and ECC codes enable most use cases, with minimum extra effort.
Moreover, RadioDNS doesn't use ODA. Check yourself the docs http://radiodns.org/documentation/
 
> RDS is just in development towards smart phones. (see RDS-Forum -> Future)
> The gRT (graphical Radio Text) is based on HTML5/CSS databases and the
> transmission rates will be increased tenfold with xRDS. The data structures
> remain unchanged.

Are you sure technology is going in that direction? Use cases are not clear. Most broadcasters don't use ODA or gRT. Receivers don't support that. Moreover, FM/RDS is very important *now*. The trend for the future is to use digital technologies (Europe http://tech.ebu.ch/docs/r/r138.pdf , Australia, and other countries too). Why should we complicate the APIs?

Comment 84

[Attila Ladanyi]

Hi Paolo, thank you for your comment.

>Attila, here Pin restricted the scope of this bug to RDS.
For pure FM reception we have to build a second Interface? 
e.g. in Beijing, I have 2 or 3 RDS stations and up to 30 without RDS on air.

> If RDS is not transmitted, it doesn't apply. Moreover, almost all FM stations use RDS as far as I know. Simply, if a station doesn't use RDS, it won't have access to the info. 

If you enable RDS for _searching_ for a radio station, you will not see other sources. RDS are not (yet) used in Korea and Japan. Mixed broadcasting is used for example in China and Russia. 
We speak about Europe only?

> I don't agree at all. Simple PI and ECC codes enable most use cases, with minimum extra effort.

ECC is not so simple and not widely used and enables no use cases, except the use case is limited to listening to music and news, or for build RadioDNS string..

> Moreover, RadioDNS doesn't use ODA. Check yourself the docs http://radiodns.org/documentation/

Correct, not yet, is only at the beginning. I have not quite figured all the benefits.
 
> Are you sure technology is going in that direction? 

_Absolutely!_ For everything else, there are mp3 player.

> Use cases are not clear. Most broadcasters don't use ODA or gRT.

Nearly correct. So far, the devices were missing, now we have them, and the appropriate operating system just now. How did looks phones from 5 or 10 years ago?
ODA is widely used. RT+ is ODA. If you want radio text in Asia or Arabia you have to use eRT, another ODA application. TMC is ODA and is worldwide used.
eRT uses UTF-8 and UCS-2, Firefox OS is based on HTML5 capable to use the same, why not take advantage compared with stupid ASCII text radio?

> Receivers don't support that. 

Correct. Other receivers does not. That is the reason why we are only now start and with smartphones.
They swap smartphones every 2 years, so this is the perfect medium to introduce newer technologies.
We must not only running behind others, we can also show what is possible with the standard.

>Moreover, FM/RDS is very important *now*. The trend for the future is to use digital technologies (Europe http://tech.ebu.ch/docs/r/r138.pdf , Australia, and other countries too).
 
After 15 years agony of DAB and additional 3 years sickness of DAB+ is the hope for this almost religion. EBU = little Part of Europe, the government broadcasters. DAB is not the future, because it is too expensive and complicated and not a world standard. HD-Radio is limited for the USA, maybe a little bit for Canada and North Mexico (200 miles zone). The number of digital listener are modest worldwide. 
Furthermore, Internet is not a broadcast medium. Latter is often overlooked and provides frustration for the listeners. We have to deliver a solution that works both in Africa and in Alaska, or not?

> Why should we complicate the APIs?

We should to make easier the Interface for App-Writer. App writers are not broadcast engineers and can with their normal tools hardly work with bit fields. RDS (& DAB & co) are a bitfield cementery.
Radio is on smart devices, such as the car-radio, gps, tablet or smartphone not only music but also data supplier. Radio has compared with the Internet, the gigantic advantage to work in real time (no login needed), completely anonymous, and be independent of the number of recipients. Besides consuming an FM chip in orders of magnitude less amount of electricity as a network any kind or digital radio.

When it would be my opinion:
Why should I send if no receiver there?
Why should I build receivers if no station there?
Why should I build interface when the receiver can't?
Why should I build the hardware if there is no interface available?
Would be result:
How I'm supposed to do something when the hardware does not support, and the interfaces are too skinny, and the receiver can not because there is nothing on air?

I think, if we want to have to compete against the biggest,then only with what they do not have:
IP and UMTS, IP-Radio & similar, the others are good enough. There is no more space.
In FM they have never clever delivered (Apple has completely failed, has only the ODA for tagging registered), that would be our chance and we run against open doors. 
e.g. http://www.radiorocksmyphone.com/
Of course this is just an opinion.

Regards.

(Little question: Is here someone else from the broadcasting site?)
.

Comment 85

[Attila Ladanyi]

(In reply to Paolo from comment #47)
> (In reply to Marco Chen [:mchen] from comment #46)

Hi Paolo,Marco,Pin.

> 
> > Use Case:
> >   1. An Navigator app is launched and receives TMC from FM RDS (Frequency X).
> >   2. Then user enables another FM app to listen broadcast (Frequency Y).
> >   3. User tries to put Navigator back to foreground but found TMC is no
> > function now. (NG)
> >   4. User tries to enable TMC again.
> >   5. User finds that broadcast/audio from FM app is changed too.
> > 
> > Then user may fall into a loop between 3 and 5 because he doesn't know that
> > TMC and FM app compete to a single FM HW.
> 
> Hi Marco,
> first of all, good to know FirefoxOS also supports TMC for traffic messages! 
> This seems an important use case. My concern is that specifically targeting
> this use case would create problems to other more frequent scenarios (e.g.
> users wishing to simply change station with a different app). As a user, I
> would be very annoyed at not being able to change channel.
> 
> Do you think a notification could solve the problem? E.g.
>   1. A Navigator app is launched and receives TMC from FM RDS (Frequency X).
>   1.1. User is warned that TMC only works with specific FM channels.
>   2. User enables another FM app to listen broadcast (Frequency Y).
>   [ optionally 2.1 An overlay message temporarily appears, listing the
> applications affected by the change - maybe a patent already covers this
> behavior]
>   3. Putting the Navigator back in foreground, there's no TMC, with a
> message explaining the reason.

TMC will be sended in a lot of countrys on a lot of Frequencies and from a lot of Broadcasters.
e.g. in Germany from all ARD Stations, the service provider is maybe changed, but the Location Table
Number is the same and the content is nearly the same. You can continue with the TMC processing even you drive from Stuttgart (SWR) to Munich (BR) and you lost SWR between Ulm and Augsburg.
The same effect if you have enough from Pop-Music SWR-3 and switching to SWR-2 Culture. Different PI but absolutely the same TMC stream.
The TMC Module should first look on the new 3A Groups. If no 3A Groups, there is no TMC. Different 
provider could send the same info if have the same Location-Table. 

Regards

Comment 86

[Attila Ladanyi]

And an additional comment to TMC & co.

TMC is an ODA Group, NOT fixed on group 8A !!!!!!!!!!!!
RT+ is also ODA Group and not fixed on Group 11A
eRT is ODA Group not fixed to any group number.

ECC will be carried on Group 1A variant 0 in Block 3.
1A will be transmitted at least once per MINUTE. (60s)

Comment 87

[andy]

Just want to chime in and point out we've veering off topic here. We're specifically discussing implementing support for RDS in the WebAPI. We started with an understanding that we would only request a very minimum, most widely supported slice of RDS (PI, PS, RT, ECC [if available])

If you wish to request support for ODA or RDS+, these feel like they should be new tickets with a new set of requirements.

Please could we refocus our effort around the originally discussed requirements?

Comment 88

[Attila Ladanyi]

OK, back to the roots. The absolutely minimum:

The biggest Problem is in Smartphones, that the different FM chips needs different commands and the functionality is also not the same.
Therefore we need a - simple - middelware. On the "downside" command-primitives that is different for each hardware and on the "top-side" standardised Data formats and calbacks.
The downside is bitfield oriented, the upper side must be java proof. 

RDS has built-in and free parts. 
Absolutely essential ist the 0A(sometimes with 0B) Group. In this group is PI, PS, AF, PTY and TP bit concentrated.
All other Groups are voluntary. This says, we should make the first shot with this group.
AF is in Europe important, we have big broadcast chains across the countries.

Data:
PI: is normally in hexadecimal. Nobody knows in other form. 
We should store it in 4 Byte String. In PI, further information can be hidden, as about regionalization. The same Fequenci can have different PI-s, so in case of shared Programming. 

PS: is normally an 8 Byte String. Often with 2 alternative text e.g ""HITRADIO"+"OE3"" and also as a "scrolling PS", a kind of Radio Text for 8 character displays. It is forbidden, but nobody cares.
I Propose a flag for this 3 states (Fix,Alternate,scrollng) and 2 Strings with 8 Bytes. 

AF: Two methods of transmitting AFs are possible. AF method A is used for lists up to 25 in number and AF method B is used for larger lists. AF method B is also used where it is required to indicate frequencies of generically related services. We can store the Frequencies as a Channel Number.
We have 205 possible channels in the FM Band, so is an Array of Bytes with max 64 items allways enough.

PTY. A List of Programme types. Two different lists exists, USA and RoW. 5 Bit Value, One Byte Integer is enough.

TP: Traffic programm. Bool, 1 Bit. Yes or No. 

M/S = Music Speech Flag(bool) and DI Decoder-identification control (4 bit) is also in the 0A Group but not so extremly important. We can save it in Variables for future use. (e.g. M/S flag for DSP programming)

RT:
We can also decode the 2A Group = RadioText. 
For this we need 2 Strings with each 64 Character. Downside as Character, Topside converted in UTF-8 String. The Byte-Code is not ASCII, therefore the conversion to UTF-8 must be eveloped in project.
We need 2 strings, because the transmitter sends alternating the two versions.
If a string (A or B) is completed, the application should be triggered. 

For the first step is this enough. RT+ is more complicated und is an ODA application. 
For ODA, I have a second proposal.

Commands:
As commands we need the "tune to" "next" "prev" "search" "check AF".
"Tune to" translate the command to the chip with Frequency or channel number.
"next" and "prev" uses the value "LIMIT". LIMIT is the computed quality for a channel with Field strength + Multipath + RDS BitErrorRate. If applicable. Primitive chips have only fiel strength.
The LIMIT value is unique for each receiver, depending on their antenna and sensitivity. The "next" and "prev" should go to the frequency with quality > LIMIT, or back to the starting frequency.
"Search" checks all channels for quality and delivers a list of channels better than LIMIT.
"Check AF" tunes to the frequencies in the AF list (if avaiable) and delivers the best quality for changing the Frequency. 
If the hardware can not provide one of this functions, the interface should be simulate it.

If we found a non-RDS channel over the LIMIT, we should set the PI to "0000" (illegal) and the PS-Name to "FM XXX.X" (frequency), now we can use the radio for all possibilities on all continents.

Important: the Data field should be saved under the PI-Code (if not 0). This can speed up the tune or startup time.

For decoding the groups 0A and 2A, I have the sources in "c" or "c++" what you want.
The code for send proprietary commands to the Chipset, we must check the documentation from hardvare provider. I know more than 5 different chipsets and this is only the smaller part of the reality.

that's all.

Comment 89

[Attila Ladanyi]

see also attachments:

https://bug942727.bugzilla.mozilla.org/attachment.cgi?id=8342310

https://bug942727.bugzilla.mozilla.org/attachment.cgi?id=8342319

Comment 90

[Paolo]

(In reply to Attila Ladanyi from comment #88)

Thank you for the useful information. I'll comment only to points relevant for this specific bug. For other issues, please open a new ticket.

> PI: is normally in hexadecimal. Nobody knows in other form. 
> We should store it in 4 Byte String. In PI, further information can be
> hidden, as about regionalization. The same Fequenci can have different PI-s,
> so in case of shared Programming. 

You can always convert from short to string. The proposal is to use a short (see the WebIDL attachment).
 
> PS: is normally an 8 Byte String. Often with 2 alternative text e.g
> ""HITRADIO"+"OE3"" and also as a "scrolling PS", a kind of Radio Text for 8
> character displays. It is forbidden, but nobody cares.
> I Propose a flag for this 3 states (Fix,Alternate,scrollng) and 2 Strings
> with 8 Bytes. 

There's an event triggered when PS changes. We can manage each (and more) of these (strongly deprecated) scenarios with the callback.

> RT:
> We can also decode the 2A Group = RadioText. 
> For this we need 2 Strings with each 64 Character. Downside as Character,
> Topside converted in UTF-8 String. The Byte-Code is not ASCII, therefore the
> conversion to UTF-8 must be eveloped in project.
> We need 2 strings, because the transmitter sends alternating the two
> versions.

I agree the API should give UTF-8 strings. We know there are different character repertoires defined by IEC 62106, however, our implicit assumption was that nobody would like to be bothered with the technicalities of these repertoires (or even the default one, used by almost all broadcasters). We simplify the interface returning a UTF-8 string(64 *chars* maximum), as any of these character repertoires can be translated to UTF-8.

> If a string (A or B) is completed, the application should be triggered. 
> a channel with Field strength + Multipath + RDS BitErrorRate. If applicable.
> Primitive chips have only fiel strength.

Please read the comment on bug 942727, this is very relevant there. Here it's off-topic.

> If we found a non-RDS channel over the LIMIT, we should set the PI to "0000"
> (illegal) and the PS-Name to "FM XXX.X" (frequency), now we can use the
> radio for all possibilities on all continents.

Good for bug 942727.
 
> Important: the Data field should be saved under the PI-Code (if not 0). This
> can speed up the tune or startup time.

Can you give us details? And please give us a reference to the specs, e.g. the publicly available (draft for ed3.0) http://www.rds.org.uk/2010/RDS-Specification.htm  or to IEC 62106 ed2.0/2009

Comment 91

[Attila Ladanyi]

>Can you give us details? And please give us a reference to the specs, e.g. the publicly available (draft for ed3.0) http://www.rds.org.uk/2010/RDS-Specification.htm  or to IEC 62106 ed2.0/2009

Of course, I am RDS-Forum and NRSC member, and since 20 Years in the branche. I can answer a lot of questions, but I can not free distribute complete works that are not public, such as this book or the "Guidelines". Sorry. (please contact me in mail)

Comment 92

[Paolo]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #81)

Hi Ehsan, Pin, Marco and all,
the difinition of these specific RDS parameters seems to be complete, great! Do you think we can go forward?

Comment 93

[Pin Zhang [:pzhang] (inactive)]

(In reply to Paolo from comment #92)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #81)
> 
> Hi Ehsan, Pin, Marco and all,
> the difinition of these specific RDS parameters seems to be complete, great!
> Do you think we can go forward?

Hi Paolo, thanks for your great help, Marco and I will work on Gonk and Gecko part respectively, if there's anything we want from you, will let you know!

Comment 94

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: WIP Gecko Part

This is the work-in-progess Gecko part to implement the RDS API, still need integrate with the gonk support.

Comment 95

[Andrea Marchesini [:baku]]

Comment on attachment 8374633 [details] [diff] [review]
Part II: WIP Gecko Part

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

::: dom/fmradio/FMRadio.cpp
@@ +283,5 @@
> +    return Nullable<uint16_t>();
> +
> +  int16_t pi = IFMRadioService::Singleton()->GetPi();
> +  if (pi == 0u)
> +    return Nullable<uint16_t>();

if (pi == 0u) {
  ...
}

here and everywhere else.

Comment 96

[Michael Wu [:mwu]]

Attachment: Raw RDS support for Flame kernel

Comment 97

[Michael Wu [:mwu]]

Attachment: WIP: Gecko RDS support

This adds a simple webapi for RDS along with all the necessary low level parts to hook it up to the driver. The API is currently kept simple to focus on testing the low level parts.

Comment 98

[Michael Wu [:mwu]]

Attachment: FMRadio app hack to show RDS data

This is a crude hack to the FM radio app to show RDS info provided by the previous patch.

Comment 99

[:gerard-majax]

Attachment: FM RDS : Chérie FM part 1

Comment 100

[:gerard-majax]

Attachment: FM RDS : Chérie FM part 2

Comment 101

[:gerard-majax]

Attachment: FM RDS : Chérie FM part 3

Comment 102

[Michael Wu [:mwu]]

Attachment: WIP: Gecko RDS support, v2

Crash on radio stop is now fixed. I also added support for raw RDS processing so any other RDS data can be decoded in js. Untested though, mostly because there's almost no FM stations here that have RDS.

Comment 103

[Michael Wu [:mwu]]

Attachment: Raw RDS support for Flame kernel, v2

This makes the driver report itself as RDS capable.

Comment 104

[Michael Wu [:mwu]]

Attachment: Raw RDS support for Flame kernel, v3

This one actually builds.

Comment 105

[Pin Zhang [:pzhang] (inactive)]

Hi Michael, i found there are differences between the API changes in your patch and the consensus we made here, for example, a new api |setRDSGroupMask| is added in your patch.

So may i ask is there any discussion that i missed? thanks.

Comment 106

[Michael Wu [:mwu]]

Nope you didn't miss anything. Mine is an independent implementation. I didn't rely on the current proposal. At the moment, I'm focusing on the low level implementation rather than the webapi. There will likely be things that end up different as a result of understanding how the low level parts function.

Comment 107

[Pin Zhang [:pzhang] (inactive)]

(In reply to Michael Wu [:mwu] from comment #106)
> Nope you didn't miss anything. Mine is an independent implementation. I
> didn't rely on the current proposal. At the moment, I'm focusing on the low
> level implementation rather than the webapi. There will likely be things
> that end up different as a result of understanding how the low level parts
> function.

So, will you update the patches to fit into the webapi?

Comment 108

[Michael Wu [:mwu]]

I don't know yet.

Comment 109

[Pin Zhang [:pzhang] (inactive)]

(In reply to Michael Wu [:mwu] from comment #108)
> I don't know yet.

Hi Michael, do you think it's the time for me to integrate the Gecko part (attachment 8374633 [details] [diff] [review]) with your Gonk patch? or should i wait for your next update?

Comment 110

[Michael Wu [:mwu]]

I'm still working on things. Hold on.

Comment 111

[Michael Wu [:mwu]]

Comment on attachment 8446466 [details] [diff] [review]
Raw RDS support for Flame kernel, v3

New patch in Bug 1041085.

Comment 112

[Michael Wu [:mwu]]

Attachment: WIP: Gecko RDS support, v3

This seems to behave ok. The API feels closer too, though there's still rough edges. All the hal parts have been put into another bug so this just leaves the dom parts.

Comment 113

[Michael Wu [:mwu]]

Attachment: FMRadio app hack to show RDS data, v2

Adapted to work the new enable/disable rds api.

Comment 114

[Michael Wu [:mwu]]

So some thoughts on the RDS API.

I think the original proposed api and gecko patch made some incorrect assumptions about where different parts of the RDS machinery would be. In particular, it looks like it was assumed that the low level driver API would give us things such as pi, ps, radiotext, and ecc all ready for use.

This is not the case.

The V4L2 api gives us RDS *blocks*. Some drivers offer parsing, but those APIs are not standardized.

Because of this, we can offer a much more forward looking API.

Certain commonly used and frequently transmitted things like PI, PS, PTY, and Radiotext are offered by this API. In addition, this API offers access to any RDS group types the app wants to parse itself. A 32 bit bitmask can be passed while calling enableRDS to indicate what group types should be passed to the app.

Parsing text strings provided by RDS is a bit tricky though - we have a RDS codepoint to UCS16 codepoint conversion table to do it correctly. It may be a bit much to ask apps to duplicate that, so it's probably worth directly supporting PTYN too so apps never need to figure out how to convert a RDS string. eRT is UTF8/UCS16, which may be more reasonable to have apps support.

It may be worth exposing more data provided by group 0A/0B since it is transmitted so frequently. Right now we only expose PS. More can be added in later.

Group 3A might also be interesting to add direct support for, though I don't have strong feelings about it either way.

Comment 115

[andy]

As a developer who is interested in developing applications with RDS data, I hope it's OK if I provide some ideas for what would be interesting to me to see accessible via the API?

I wonder if there's value in providing access in the API to get any raw group the app developer is interested in, callback that receives the full payload to allow the dev to do what they choose. Then abstracting the "valuable" groups (PS, PI, RT, ECC) etc. in to higher level more meaningful API calls (onText etc.)

To give you an idea of the potentially wide scope of useful groups:

Group 0, 1 and 2 are all fairly crucial - this is the very basic RDS support most would expect and is, indeed, covered by the discussed ECC, PI, PS, RT endpoints already.

A "generic" API to access all the ODA channels (3A, 3B, 4B, optional 5-13), allowing access to the raw data contained within, would then allow devs to build apps that use RDS-based applications that exist within these frames. These can vary from region to region, so this API should be suitably abstract, providing the payload and allowing the app developer to extrapolate that at a higher level.

4A could prove useful as "yet another" source of data/time for the phone, though the accuracy of these values isn't that useful, particularly when you have cell/NTP etc. elsewhere in the phone stack.

14 provides programme type information (EON) which could be handy, though realistically few broadcasters implement this or don't provide particularly useful values in this field to make it a really worthwhile addition.

15B is useful again for tuning information, so I'd encourage this.

Comment 116

[Michael Wu [:mwu]]

(In reply to andy from comment #115)
> As a developer who is interested in developing applications with RDS data, I
> hope it's OK if I provide some ideas for what would be interesting to me to
> see accessible via the API?
> 
> I wonder if there's value in providing access in the API to get any raw
> group the app developer is interested in, callback that receives the full
> payload to allow the dev to do what they choose. Then abstracting the
> "valuable" groups (PS, PI, RT, ECC) etc. in to higher level more meaningful
> API calls (onText etc.)
> 

This is exactly what's provided in the API in the patch I posted. Commonly used pieces of data are provided, and raw RDS group data is passed to the app. Any group type can be received, even if it's being handled by gecko.

> To give you an idea of the potentially wide scope of useful groups:
> 
> Group 0, 1 and 2 are all fairly crucial - this is the very basic RDS support
> most would expect and is, indeed, covered by the discussed ECC, PI, PS, RT
> endpoints already.
> 

Only groups 0 and 2 have direct support. I've only found group 1 on one station around here.

> 4A could prove useful as "yet another" source of data/time for the phone,
> though the accuracy of these values isn't that useful, particularly when you
> have cell/NTP etc. elsewhere in the phone stack.
> 

Yeah. Not all devices have cell radios though. But most devices still have GPS, which is still better.

> 15B is useful again for tuning information, so I'd encourage this.

Looks easy enough to add.

Comment 117

[andy]

Apologies, I'm struggling to follow all of the work that is going on here.

I'm just keen to show some support for this work! Thank you again.

Comment 118

[Michael Wu [:mwu]]

Attachment: New FMRadio RDS API

I've changed the attribute names to match the previous proposal. ECC was removed, but a new attribute and event to read raw RDS groups was added. A pair of calls was also added to enable/disable RDS, as RDS can unnecessarily wake up the CPU if nobody is actually using it. There's also an attribute to specify what RDS groups the code wants to see.

Comment 119

[Michael Wu [:mwu]]

Attachment: FMRadio RDS implementation in DOM

This gets pretty close to what I want, but the RDS group mask thing isn't properly implemented yet. I haven't yet figured out how to send that info back to the parent process.

Comment 120

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8471937 [details] [diff] [review]
FMRadio RDS implementation in DOM

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

::: dom/fmradio/FMRadioService.h
@@ +104,5 @@
> +  virtual Nullable<unsigned short> GetPI() const = 0;
> +  virtual Nullable<uint8_t> GetPTY() const = 0;
> +  virtual void GetPS(nsString& aPsname) = 0;
> +  virtual void GetRadiotext(nsString& aRadiotext) = 0;
> +  virtual uint64_t GetRDSGroup() = 0;

I prefer to keep the function names consistent with what we have in FMRadio.h

::: dom/fmradio/ipc/FMRadioChild.cpp
@@ +191,5 @@
> +  }
> +  if (!mRadiotext.IsEmpty()) {
> +    mRadiotext.Truncate();
> +    NotifyFMRadioEvent(RadiotextChanged);
> +  }

We don't fire frequency changed event when FM hw is disabled, so mPI will not be null when FM hw is disabled, right?

The attribute |frequency| has the similar definition as |pi|, both of them are null when FM radio is disabled. To make sure the return value of |frequency| is null when the FM radio is off, we simply check if |Enabled()|[1], i think we could do the same check in |FMRadio::GetPi|.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadio.cpp?from=FMRadio.cpp&case=true#245

::: dom/fmradio/ipc/PFMRadio.ipdl
@@ +79,5 @@
>    NotifyEnabledChanged(bool enabled, double frequency);
> +  /**
> +   * Sent when we have a new PI code.
> +   */
> +  NotifyPIChanged(bool valid, uint16_t code);

In FMRadioParent.cpp, |valid| only indicates if PI is null, and i found |nullable| is supported in ipdl, here is an example:  
  http://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PPluginModule.ipdl#128

so how about:
  NotifyPIChanged(nullable uint16_t code);
?

Comment 121

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8471937 [details] [diff] [review]
FMRadio RDS implementation in DOM

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

I don't feel comfortable to go through the details of FMRadioService::Notify because of my poor knowledge in this area, sorry, :(

::: dom/fmradio/FMRadioService.cpp
@@ +299,5 @@
> +  NotifyRunnable(FMRadioEventType aType) : mType(aType) { }
> +
> +  NS_IMETHOD Run()
> +  {
> +    FMRadioService::Singleton()->NotifyFMRadioEvent(mType);

We notify frequency changed event by calling |NotifyFMRadioEvent| directly, why do we need to notify PI/PYT changed event in a runnable?

@@ +761,5 @@
> +  if (IsFMRadioOn()) {
> +    hal::EnableRDS(aGroupMask | DEFAULT_RDS_MASK);
> +  }
> +
> +  aReplyRunnable->SetReply(SuccessResponse());

Should we fail the request when FM radio hw is off?

@@ +776,5 @@
> +  if (IsFMRadioOn()) {
> +    hal::DisableRDS();
> +  }
> +
> +  aReplyRunnable->SetReply(SuccessResponse());

Same here, if the FM hw is off, should we fail the request?

@@ +884,5 @@
>        // should send the `FrequencyChanged` event manually.
>        NotifyFMRadioEvent(FrequencyChanged);
> +
> +      if (mRDSEnabled) {
> +        hal::EnableRDS(mRDSGroupMask | DEFAULT_RDS_MASK);

So we don't have to re-enable the RDS in the app if user turn on and off, right? Then the behaviors of FMRadio.enable are slightly different based on when it's invoked, it's kinda weird from API perspective.

BTW, how can we get the disabled/enabled status of RDS in the app?

@@ +912,4 @@
>    }
>  }
>  
> +static const uint16_t sRDSToUnicodeMap[256] = {

Is this map a standard? If not, maybe it's better to do the conversion in gonk.

::: dom/fmradio/FMRadioService.h
@@ +236,5 @@
> +  uint16_t mLastPI;
> +  uint16_t mLastPTY;
> +  Atomic<uint32_t> mPI;
> +  Atomic<uint32_t> mPTY;
> +  Atomic<bool> mPISet;

As my understanding, mPISet is a kinda flag that indicates if |mPI| comes from current radio station (we set mPISet to false when frequency changed), so is it possible to define |mPI| as a nullable attribute?

Comment 122

[(no longer active)]

Comment on attachment 8471933 [details] [diff] [review]
New FMRadio RDS API

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

::: dom/webidl/FMRadio.webidl
@@ +161,5 @@
> +   * Disable RDS reception.
> +   *
> +   * If the radio is off, RDS will not be enabled when the radio is turned on.
> +   */
> +  DOMRequest disableRDS();

Wouldn't it also make sense to add a way to see if RDS is enabled or not?  Perhaps a simple readonly boolean attribute would suffice?

Comment 123

[Michael Wu [:mwu]]

I don't disagree with anything in comment 120.

(In reply to Pin Zhang [:pzhang] (PTO until 18th Aug ) from comment #121)
> ::: dom/fmradio/FMRadioService.cpp
> @@ +299,5 @@
> > +  NotifyRunnable(FMRadioEventType aType) : mType(aType) { }
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    FMRadioService::Singleton()->NotifyFMRadioEvent(mType);
> 
> We notify frequency changed event by calling |NotifyFMRadioEvent| directly,
> why do we need to notify PI/PYT changed event in a runnable?
> 

In that case, the HAL side notifies the radio service of the frequency change on the main thread. But with the RDS API, data is delivered to the FMRadioService on a separate dedicated thread, so we need the Runnable to run things on the main thread.

> @@ +761,5 @@
> > +  if (IsFMRadioOn()) {
> > +    hal::EnableRDS(aGroupMask | DEFAULT_RDS_MASK);
> > +  }
> > +
> > +  aReplyRunnable->SetReply(SuccessResponse());
> 
> Should we fail the request when FM radio hw is off?
> 
> @@ +776,5 @@
> > +  if (IsFMRadioOn()) {
> > +    hal::DisableRDS();
> > +  }
> > +
> > +  aReplyRunnable->SetReply(SuccessResponse());
> 
> Same here, if the FM hw is off, should we fail the request?
> 

As currently specified in the webidl patch, turning RDS on before just means that RDS will be turned on when the radio is turned on. It's definitely a different approach than what the current FM radio API attempts - I think that's worth its own discussion.

> @@ +884,5 @@
> >        // should send the `FrequencyChanged` event manually.
> >        NotifyFMRadioEvent(FrequencyChanged);
> > +
> > +      if (mRDSEnabled) {
> > +        hal::EnableRDS(mRDSGroupMask | DEFAULT_RDS_MASK);
> 
> So we don't have to re-enable the RDS in the app if user turn on and off,
> right? Then the behaviors of FMRadio.enable are slightly different based on
> when it's invoked, it's kinda weird from API perspective.
> 

Agreed - same comment as above.

> BTW, how can we get the disabled/enabled status of RDS in the app?
> 

None. There's also no way to determine if RDS failed to enable right now. I'll adjust the webidl API a bit.

> @@ +912,4 @@
> >    }
> >  }
> >  
> > +static const uint16_t sRDSToUnicodeMap[256] = {
> 
> Is this map a standard? If not, maybe it's better to do the conversion in
> gonk.
> 

Yeah, it's a standard. If you find a copy of the RDS/RBDS spec, you can find this table in there.

> ::: dom/fmradio/FMRadioService.h
> @@ +236,5 @@
> > +  uint16_t mLastPI;
> > +  uint16_t mLastPTY;
> > +  Atomic<uint32_t> mPI;
> > +  Atomic<uint32_t> mPTY;
> > +  Atomic<bool> mPISet;
> 
> As my understanding, mPISet is a kinda flag that indicates if |mPI| comes
> from current radio station (we set mPISet to false when frequency changed),
> so is it possible to define |mPI| as a nullable attribute?

We could, but right now I'm keeping the two parts separate in order to use Atomic<> and update this data without a lock. Possibly a little too clever given the RDS transmission rate, but mPI and mPTY rarely change.

Comment 124

[Michael Wu [:mwu]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #122)
> Wouldn't it also make sense to add a way to see if RDS is enabled or not? 
> Perhaps a simple readonly boolean attribute would suffice?

Yeah, I think we need that at minimum to match the general pattern in the FM API.

The RDS API also acts differently by maintaining state after the radio is turned off, whereas the FM API doesn't let you do much while the radio is off.

What I really want is to make the API based on setting the attribute rather than calling dedicated functions to change settings. For example, instead of:

radio.enable(96.3);

We'd do something like:

radio.frequency = 96.3;
radio.enabled = true;

Similarly for RDS, instead of radio.enableRDS(), we'd do something like:

radio.RDSEnabled = true;

No idea how feasible that is though. If not feasible, I'll should probably change the RDS API to only allow enabling when the radio is on.

Comment 125

[(no longer active)]

I like the idea of having a boolean attribute.  I think the setter is quite feasible, it effectively branches on the argument to choose whether to call the internal EnableRDS or DisableRDS function.  I'm not sure if the underlying API here allows you to query the status of the RDS or not.

Thinking a bit about the statefulness of he underlying RDS API though, perhaps we should change directions and do something like:

dictionary RDSEnableOptions {
  attribute float frequency;
  // other state here
};

  void enableRDS(RDSEnableOptions options);
  void disableRDS();

Therefore, effectively forcing the authors to pass in the options, and therefore not rely on any possible state preserved through the API.  This will be closer to the current semantics of the FM API I think.

What's your take on this, Michael?

Comment 126

[Michael Wu [:mwu]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #125)
> I like the idea of having a boolean attribute.  I think the setter is quite
> feasible, it effectively branches on the argument to choose whether to call
> the internal EnableRDS or DisableRDS function.  I'm not sure if the
> underlying API here allows you to query the status of the RDS or not.
> 

We could allow querying at the HAL level, but it's probably not necessary when we can keep track of the state locally. We also have to start a thread to read and process RDS data in the parent process, so it makes more sense to check the state of that.

> Thinking a bit about the statefulness of he underlying RDS API though,
> perhaps we should change directions and do something like:
> 
> dictionary RDSEnableOptions {
>   attribute float frequency;
>   // other state here
> };
> 
>   void enableRDS(RDSEnableOptions options);
>   void disableRDS();
> 
> Therefore, effectively forcing the authors to pass in the options, and
> therefore not rely on any possible state preserved through the API.  This
> will be closer to the current semantics of the FM API I think.
> 
> What's your take on this, Michael?

The underlying FM API (V4L2) is mostly stateful, though we don't treat it as such. However, I think making the FM radio API more stateful should simplify the FM radio app. Right now, the app needs to keep track of what the frequency should be shown in the UI, even when the radio is off. If the frequency attribute is always available, the app can query the attribute directly instead of maintaining its own frequency variable.

Comment 127

[(no longer active)]

Making the API more stateful in the future sounds fine, but I suspect you're not trying to do that in this bug, right?

Comment 128

[Michael Wu [:mwu]]

Nope - I'm just going to add a rdsEnabled attribute (or whatever name you'd like) for now. Making the API more stateful deserves its own bug.

Comment 129

[(no longer active)]

rdsEnabled it is then!

Comment 130

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8471937 [details] [diff] [review]
FMRadio RDS implementation in DOM

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

This looks reasonable to me, although I think Pin's review is worth more than mine here.

Comment 131

[Michael Wu [:mwu]]

Attachment: New FMRadio RDS API, v2

rdsEnabled added.

Comment 132

[Michael Wu [:mwu]]

Attachment: FMRadio RDS implementation in DOM, v2

I think this needs to be polished a little bit further, but all the important parts seem to be in place and appear to work.

Comment 133

[Michael Wu [:mwu]]

(In reply to Pin Zhang [:pzhang] from comment #120)
> ::: dom/fmradio/ipc/PFMRadio.ipdl
> @@ +79,5 @@
> >    NotifyEnabledChanged(bool enabled, double frequency);
> > +  /**
> > +   * Sent when we have a new PI code.
> > +   */
> > +  NotifyPIChanged(bool valid, uint16_t code);
> 
> In FMRadioParent.cpp, |valid| only indicates if PI is null, and i found
> |nullable| is supported in ipdl, here is an example:  
>  
> http://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PPluginModule.
> ipdl#128
> 
> so how about:
>   NotifyPIChanged(nullable uint16_t code);
> ?

/home/mikew/moz/B2G/gecko/dom/fmradio/ipc/PFMRadio.ipdl:86: error: `nullable' qualifier for type `uint16_t' makes no sense
Specification is not well typed.

Looks like nullable only works for certain types.

Comment 134

[Michael Wu [:mwu]]

Attachment: New FMRadio RDS API, v3

This adds rdsEnabled as proposed before, but also adds onrdsenabled and onrdsdisabled for listening to changes in that attribute.

Comment 135

[Michael Wu [:mwu]]

Attachment: FMRadio RDS implementation in DOM, v3

There's one issue in this which I haven't addressed - RDS group setting only works for one user. I have ideas on how to make that work for more than one user, but I'd like to fix that in a followup.

Another thing to fix in a followup is properly reporting if RDS was actually enabled. Right now enabling rds always succeeds. We'll need to adjust the HAL layer a little to make this possible.

I made the code reset PI/PTY/PS/RT when the radio is disabled, rather than having the code check if it is disabled when returning values.

The RDS parsing code requires some knowledge of the RDS standard to understand - let me know if you want to convert some numbers into constants to make things easier to read.

Comment 136

[Pin Zhang [:pzhang] (inactive)]

Attachment: Screenshot - mojibake radiotext

Hi Michael, i tested on Flame, it works very well except the mojibake radiotext, do you have any idea? CJK characters?

Comment 137

[Michael Wu [:mwu]]

Hmm, maybe radiotext in China uses different encodings? You could try bypassing the RDS -> Unicode lookup table and interpret it directly as as UCS16 or UTF8. Radiotext uses the same conversion table as PS Name.

Comment 138

[Attila Ladanyi]

Attachment: GB-T 15770-1995.pdf Old chinese RDS Standard

For chinese Radiotext
See Groups 2,  Page 11 - 12

Comment 139

[Attila Ladanyi]

(In reply to Pin Zhang [:pzhang] from comment #136)
> Created attachment 8484803 [details]
> Screenshot - mojibake radiotext
> 
> Hi Michael, i tested on Flame, it works very well except the mojibake
> radiotext, do you have any idea? CJK characters?

(In reply to Michael Wu [:mwu] from comment #137)
> Hmm, maybe radiotext in China uses different encodings? You could try
> bypassing the RDS -> Unicode lookup table and interpret it directly as as
> UCS16 or UTF8. Radiotext uses the same conversion table as PS Name.

Yes Sir!
The FM90.0 is the TMC source in Beijing (for AUDI & others), I have helped to develop it.
The Chinese colleagues have used an old Chinese RDS standard for this radio text.
See Document GB-T 15770-1995.pdf Page 11-12

In the Future RDS will be change to UTF-8 generally. Only the old 2A/2B remains on the EBU Charset.

Comment 140

[andy]

It's important to note that in Western Europe implementations are based on the EBU Charset and are extremely unlikely to change.

Comment 141

[Attila Ladanyi]

(In reply to andy from comment #140)
> It's important to note that in Western Europe implementations are based on
> the EBU Charset and are extremely unlikely to change.

Correct for the traditional RT.
In the Standard is the ODA eRT responsible for UCS-2 and UTF-8 Radiotext.

The RDS standard is however continue to developed, the EBU Charset is in the medium term definitely dead. Already using 99% all radio stations only the 7-bit ASCII.
The RDS Forum has agreed to change the character set in the future on the Internet most widely used UTF-8. Why the permanent conversion in radio if the smart displays anyway usually operate with Java and UTF-8? Who today uses only ASCII, would not notice the change. (Excepted 2A/2B Radio Text.)
Now it is recommended to use eRT (ODA), also alternately with RT possible to support old radios.
Think not only on the US or West-European users. Russian, Arabic an Chinese are also a big market, much more bigger meanwhile.

.

Comment 142

[andy]

Thanks for the clarification.

The point I was trying to make is that it's extremely unlikely any changes will be made to RDS services that are currently on air today, at least in my experience of the UK market. Costs and access associated to changing the hardware are prohibitive and future innovation is most likely to happen on hybrid and digital transmission platforms.

Comment 143

[Attila Ladanyi]

(In reply to andy from comment #142)
> Thanks for the clarification.
> 
> The point I was trying to make is that it's extremely unlikely any changes
> will be made to RDS services that are currently on air today, at least in my
> experience of the UK market. Costs and access associated to changing the
> hardware are prohibitive and future innovation is most likely to happen on
> hybrid and digital transmission platforms.


I am pleased when you are sure.
Otherwise I would have invited you to join us in the RDS-Forum and NRSC-RBDS-Committee.
Well, we make no propaganda battle as the DAB colleagues, but we also not sleeping.
We are going to make the new standards (including hybridization), we can always use help.
:-)

Comment 144

[Pin Zhang [:pzhang] (inactive)]

(In reply to Attila Ladanyi from comment #139)
> (In reply to Michael Wu [:mwu] from comment #137)
> > Hmm, maybe radiotext in China uses different encodings? You could try
> > bypassing the RDS -> Unicode lookup table and interpret it directly as as
> > UCS16 or UTF8. Radiotext uses the same conversion table as PS Name.
> 
> Yes Sir!
> The FM90.0 is the TMC source in Beijing (for AUDI & others), I have helped
> to develop it.
> The Chinese colleagues have used an old Chinese RDS standard for this radio
> text.
> See Document GB-T 15770-1995.pdf Page 11-12
> 
> In the Future RDS will be change to UTF-8 generally. Only the old 2A/2B
> remains on the EBU Charset.

Hi Attila, thanks for your help, let's file a separate bug for the encoding issue of RDS in China.

Comment 145

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8484569 [details] [diff] [review]
FMRadio RDS implementation in DOM, v3

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

::: dom/fmradio/ipc/FMRadioChild.cpp
@@ +104,5 @@
> +
> +uint64_t
> +FMRadioChild::GetRdsgroup()
> +{
> +  return mRDSGroup;

Same for rdsGroup, it could be null due to the webidl definition:
  readonly attribute Uint16Array? rdsgroup;

but it can't be null in the implementation.

@@ +199,5 @@
> +    mPTY.SetNull();
> +    NotifyFMRadioEvent(PTYChanged);
> +  }
> +  if (!mPSName.IsEmpty()) {
> +    mPSName.Truncate();

The definition of ps and rt are nullable string:
   readonly attribute DOMString? ps;
   readonly attribute DOMString? rt;

but we only truncate |ps| and |rt| when the FM radio HW is disabled, so there is no chance for them to be null value, right? Though there is no differences between a null-value string and an empty string in most cases, we need to keep consistent between the API spec and implementation.

::: dom/fmradio/ipc/FMRadioParent.cpp
@@ +117,5 @@
> +    }
> +    case PTYChanged: {
> +      Nullable<uint8_t> pty = IFMRadioService::Singleton()->GetPty();
> +      unused << SendNotifyPTYChanged(!pty.IsNull(),
> +                                    pty.IsNull() ? 0 : pty.Value());

Nits, indention.

Comment 146

[Michael Wu [:mwu]]

Attachment: FMRadio RDS implementation in DOM, v4

Review comments addressed.

Comment 147

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8486779 [details] [diff] [review]
FMRadio RDS implementation in DOM, v4

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

r=me on most codes except the rdsgroup parsing part, i.e. FMRadioService::Notify, i don't think i'm qualified, sorry, :( 

Let's ask :khuey for additional review.

Comment 148

[Michael Wu [:mwu]]

Pin, I'm not sure if there's anyone at Mozilla besides myself who knows the RDS standard. It might be worth trying to read and understand the standard - we'll be able to get this through faster that way. It's not too hard to understand, and I think even the old chinese RDS spec will tell you everything you need to know to review this.

Comment 149

[Pin Zhang [:pzhang] (inactive)]

(In reply to Michael Wu [:mwu] from comment #148)
> Pin, I'm not sure if there's anyone at Mozilla besides myself who knows the
> RDS standard. It might be worth trying to read and understand the standard -
> we'll be able to get this through faster that way. It's not too hard to
> understand, and I think even the old chinese RDS spec will tell you
> everything you need to know to review this.

OK, then let me have a try, but it will probably take couple of days, :)

Comment 150

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8486779 [details] [diff] [review]
FMRadio RDS implementation in DOM, v4

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

::: dom/fmradio/FMRadioService.cpp
@@ +772,5 @@
> +FMRadioService::SetRDSGroupMask(uint32_t aRDSGroupMask)
> +{
> +  mRDSGroupMask = aRDSGroupMask;
> +  if (IsFMRadioOn()) {
> +    hal::EnableRDS(mRDSGroupMask | DEFAULT_RDS_MASK);

As my understanding, we need to receive 0A/0B/2A/2B/15B type groups to get PI/PTY/PS/RT even if the group mask is set to 0 which means app doesn't want to receive any group info, right?

It is kinda misleading to have the name DEFAULT_*, i think we need a better name for it, say SUPPLEMENT_RDS_MASK, what do you think? And would you mind to add some comments?

@@ +1025,5 @@
> +  /* Bit 11 in block B determines whether this is a type B group. */
> +  uint16_t lastPI = blocks[1] & (1 << 11) ? blocks[2] : mLastPI;
> +
> +  /* Update PI if it's not set or if we get two PI with the new value. */
> +  if ((mPI != blocks[0] && lastPI == blocks[0]) || !mPISet) {

Is it possible that we get two different PI from block 0 and block 2 of type B group?

@@ +1052,5 @@
> +  switch (grouptype) {
> +    case 0: // 0a
> +    case 1: // 0b
> +    {
> +      uint16_t offset = (blocks[1] & 0x3) << 1;

Why shift left here?

@@ +1060,5 @@
> +        mPSNameState |= 1 << (blocks[1] & 0x3);
> +      }
> +
> +      mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] >> 8];
> +      mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] & 0xFF];

OK, i think i understand why we shift left on |offset| now, i prefer keeping |offset| as what the specification defines for DI:
  uint16_t di = blocks[1] & 0x3;

then line1060 could be changed to:
  mPSNameState |= 1 << di;

and shift it here:
  uint16_t offset = di << 1;
  mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] >> 8];
  mTempPSName[offset] = sRDSToUnicodeMap[blocks[3] & 0xFF];

what do you think?

@@ +1075,5 @@
> +      break;
> +    }
> +    case 4: // 2a Radiotext
> +    {
> +      uint16_t offset = (blocks[1] & 0xF) << 2;

Same here, i prefer keeping offset as the specification defines for text segment address code:
  uint16_t textSegAddr = blocks[1] & 0xF;

@@ +1089,5 @@
> +
> +      if (!offset) {
> +        mRadiotextState = 1;
> +      } else {
> +        mRadiotextState |= 1 << (blocks[1] & 0xF);

then this line could be changed to:
   mRadiotextState |= 1 << textSegAddr;

@@ +1098,5 @@
> +      segment[1] = blocks[2] & 0xFF;
> +      segment[2] = blocks[3] >> 8;
> +      segment[3] = blocks[3] & 0xFF;
> +
> +      bool done = false;

and offset is defined here as:
  uint16_t offset = textSegAddr << 2;

@@ +1100,5 @@
> +      segment[3] = blocks[3] & 0xFF;
> +
> +      bool done = false;
> +      for (int i = 0; i < 4; i++) {
> +        if (segment[i] == 0x0D) {

I prefer converting the ascii value as a constant if you don't mind, thanks.

@@ +1102,5 @@
> +      bool done = false;
> +      for (int i = 0; i < 4; i++) {
> +        if (segment[i] == 0x0D) {
> +          mTempRadiotext[offset++] = 0;
> +          done = true;

Why don't break the loop here?

@@ +1112,5 @@
> +        done = true;
> +      }
> +
> +      if (!done ||
> +          (mRadiotextState + 1) != (1 << ((blocks[1] & 0xF) + 1)) ||

What does this check for, possible out-of-order? I think more comment is needed here ...

@@ +1125,5 @@
> +      break;
> +    }
> +    case 5: // 2b Radiotext
> +    {
> +      uint16_t offset = (blocks[1] & 0xF) << 1;

Same here.

@@ +1146,5 @@
> +      segment[0] = blocks[3] >> 8;
> +      segment[1] = blocks[3] & 0xFF;
> +
> +      bool done = false;
> +      for (int i = 0; i < 4; i++) {

s/4/2/, because the size of |segment| is 2.

@@ +1149,5 @@
> +      bool done = false;
> +      for (int i = 0; i < 4; i++) {
> +        if (segment[i] == 0x0D) {
> +          mTempRadiotext[offset++] = 0;
> +          done = true;

Same here, why don't break the loop?

@@ +1181,5 @@
> +      break;
> +    }
> +  }
> +
> +  if (!(mRDSGroupMask & (1 << grouptype)))

We receive groups more than what the app is interested in, so we need to check if current group should be delivered, right? I prefer adding some comments if you don't mind.

Comment 151

[Michael Wu [:mwu]]

(In reply to Pin Zhang [:pzhang] from comment #150)
> Comment on attachment 8486779 [details] [diff] [review]
> FMRadio RDS implementation in DOM, v4
> 
> Review of attachment 8486779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fmradio/FMRadioService.cpp
> @@ +772,5 @@
> > +FMRadioService::SetRDSGroupMask(uint32_t aRDSGroupMask)
> > +{
> > +  mRDSGroupMask = aRDSGroupMask;
> > +  if (IsFMRadioOn()) {
> > +    hal::EnableRDS(mRDSGroupMask | DEFAULT_RDS_MASK);
> 
> As my understanding, we need to receive 0A/0B/2A/2B/15B type groups to get
> PI/PTY/PS/RT even if the group mask is set to 0 which means app doesn't want
> to receive any group info, right?
> 

Yeah.

> It is kinda misleading to have the name DEFAULT_*, i think we need a better
> name for it, say SUPPLEMENT_RDS_MASK, what do you think? And would you mind
> to add some comments?
> 

Sure, I'll rename.

> @@ +1025,5 @@
> > +  /* Bit 11 in block B determines whether this is a type B group. */
> > +  uint16_t lastPI = blocks[1] & (1 << 11) ? blocks[2] : mLastPI;
> > +
> > +  /* Update PI if it's not set or if we get two PI with the new value. */
> > +  if ((mPI != blocks[0] && lastPI == blocks[0]) || !mPISet) {
> 
> Is it possible that we get two different PI from block 0 and block 2 of type
> B group?
> 

Possible. I've found RDS reception to be very noisy despite the error detection/correction built into it. Blocks are often corrupted too much for the checkword to handle, depending on how bad the reception is.

> @@ +1052,5 @@
> > +  switch (grouptype) {
> > +    case 0: // 0a
> > +    case 1: // 0b
> > +    {
> > +      uint16_t offset = (blocks[1] & 0x3) << 1;
> 
> Why shift left here?
> 
> @@ +1060,5 @@
> > +        mPSNameState |= 1 << (blocks[1] & 0x3);
> > +      }
> > +
> > +      mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] >> 8];
> > +      mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] & 0xFF];
> 
> OK, i think i understand why we shift left on |offset| now, i prefer keeping
> |offset| as what the specification defines for DI:
>   uint16_t di = blocks[1] & 0x3;
> 
> then line1060 could be changed to:
>   mPSNameState |= 1 << di;
> 
> and shift it here:
>   uint16_t offset = di << 1;
>   mTempPSName[offset++] = sRDSToUnicodeMap[blocks[3] >> 8];
>   mTempPSName[offset] = sRDSToUnicodeMap[blocks[3] & 0xFF];
> 
> what do you think?
> 

Sure.

> @@ +1075,5 @@
> > +      break;
> > +    }
> > +    case 4: // 2a Radiotext
> > +    {
> > +      uint16_t offset = (blocks[1] & 0xF) << 2;
> 
> Same here, i prefer keeping offset as the specification defines for text
> segment address code:
>   uint16_t textSegAddr = blocks[1] & 0xF;
> 

Ok.

> @@ +1089,5 @@
> > +
> > +      if (!offset) {
> > +        mRadiotextState = 1;
> > +      } else {
> > +        mRadiotextState |= 1 << (blocks[1] & 0xF);
> 
> then this line could be changed to:
>    mRadiotextState |= 1 << textSegAddr;
> 

Ok.

> @@ +1098,5 @@
> > +      segment[1] = blocks[2] & 0xFF;
> > +      segment[2] = blocks[3] >> 8;
> > +      segment[3] = blocks[3] & 0xFF;
> > +
> > +      bool done = false;
> 
> and offset is defined here as:
>   uint16_t offset = textSegAddr << 2;
> 
> @@ +1100,5 @@
> > +      segment[3] = blocks[3] & 0xFF;
> > +
> > +      bool done = false;
> > +      for (int i = 0; i < 4; i++) {
> > +        if (segment[i] == 0x0D) {
> 
> I prefer converting the ascii value as a constant if you don't mind, thanks.
> 

I'll use '\r' which is a bit more obvious.

> @@ +1102,5 @@
> > +      bool done = false;
> > +      for (int i = 0; i < 4; i++) {
> > +        if (segment[i] == 0x0D) {
> > +          mTempRadiotext[offset++] = 0;
> > +          done = true;
> 
> Why don't break the loop here?
> 

Can be done. Don't think it makes a big difference though - the loop is for just 4 characters.

> @@ +1112,5 @@
> > +        done = true;
> > +      }
> > +
> > +      if (!done ||
> > +          (mRadiotextState + 1) != (1 << ((blocks[1] & 0xF) + 1)) ||
> 
> What does this check for, possible out-of-order? I think more comment is
> needed here ...
> 

Yeah this is more paranoia since RDS reception can be unreliable. This ensures we receive data at all address up to 0x0D before reporting the radiotext.

> @@ +1125,5 @@
> > +      break;
> > +    }
> > +    case 5: // 2b Radiotext
> > +    {
> > +      uint16_t offset = (blocks[1] & 0xF) << 1;
> 
> Same here.
> 
> @@ +1146,5 @@
> > +      segment[0] = blocks[3] >> 8;
> > +      segment[1] = blocks[3] & 0xFF;
> > +
> > +      bool done = false;
> > +      for (int i = 0; i < 4; i++) {
> 
> s/4/2/, because the size of |segment| is 2.
> 

Good catch!

> @@ +1149,5 @@
> > +      bool done = false;
> > +      for (int i = 0; i < 4; i++) {
> > +        if (segment[i] == 0x0D) {
> > +          mTempRadiotext[offset++] = 0;
> > +          done = true;
> 
> Same here, why don't break the loop?
> 

It's such a short loop.. :) (also hoping the compiler unrolls it)

> @@ +1181,5 @@
> > +      break;
> > +    }
> > +  }
> > +
> > +  if (!(mRDSGroupMask & (1 << grouptype)))
> 
> We receive groups more than what the app is interested in, so we need to
> check if current group should be delivered, right? I prefer adding some
> comments if you don't mind.

Will do.

Comment 152

[Michael Wu [:mwu]]

Attachment: FMRadio RDS implementation in DOM, v5

Most comments addressed, though I used segAddr instead of di or textSegAddr. Also used DOM_PARSED_RDS_GROUPS to replace DEFAULT_RDS_MASK.

Comment 153

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8494761 [details] [diff] [review]
FMRadio RDS implementation in DOM, v5

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

r=me, ask @khuey for additional review.

Hi @khuey, this patch is not a small one, it will be appreciated if you would take a look, thanks!

Comment 154

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8494761 [details] [diff] [review]
FMRadio RDS implementation in DOM, v5

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

I looked at this briefly, but I'm most trusting Pin's review.

Comment 155

[Michael Wu [:mwu]]

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

Comment 156

[Michael Wu [:mwu]]

Backed out. Hal part needs more work to build on ICS.

Comment 157

[Michael Wu [:mwu]]

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

Comment 158

[Michael Wu [:mwu]]

Second try:

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

Comment 159

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba6ad0e48fe5

Comment 160

[Michael Wu [:mwu]]

The FM Radio app in Gaia is being updated in bug 1037460 which may add support for some RDS features exposed in this API.

Comment 161

[Attila Ladanyi]

Now a new RDS version comes to the end of the year (2015). Some new features.
Does it interest anyone?