• Status Closed
  • Percent Complete
  • Task Type Patches
  • Category FM Tuner
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by sdoyon - 2006-11-15
Last edited by sdoyon - 2007-11-07

FS#6331 - Voice and features for FM radio

This patch adds voice to the FM radio, re-does some menus and adds a few
new functions.

First of all, someone made it possible for the voice to talk while the
radio is playing. I’m pretty sure that wasn’t working two months ago. Big
thanks to whoever did that.

Requires P#6159 and P#6236.

First I had all the splashes and dialogs talk (just use ID2P, see
P#6159). Just that helps a lot. First thing that happens when going into
the radio screen for the first time is the auto-scan dialog, which was

I have it say the tune mode (scan vs preset) when it changes.

If the voice file preference is to say numbers, I have it say the index
of the preset, otherwise if the setting is for spelling and the preset
name is something else than the default auto-scan name, I have it spell
that. Obviously it’s convenient only for very short names or call
letters. If the preference is for spelling but there’s no name, or if
we’re in scan mode, then I have it also say the current frequency. I’m
assuming it’s OK to speak only one decimal place instead of the two that
are displayed.

But then I came to the radio_menu and the presets menu.

The radio_menu has three unusual elements: the menu item contains both a
field name and a value, and clicking it changes the value in place. They
are the entries for force mono mode, region, and scan mode. I added
ordinary full-blown submenus for those options. Until I can get some
feedback, I have it use the submenus if the talk_menu setting is enabled,
and the original method otherwise. At least for force mono and region,
I think I would prefer having a submenu, for uniformity and the ability
to cancel a change. And those two are not changed often. I’m not sure
what to make of the button preprocessor function, and it does say
#if 0 /* this screen needs fixing! */

The presets menu is even more of a problem: menu entries are preset
names. I need to have those spelled. The menu structure seems a bit
convoluted to me. There’s this preset_menu_items array that needs to be
constantly rebuilt with copies of the preset names from presets]. There’s
some elaborate manipulations to keep both lists in sync. Oh and there’s
that button preprocessor function, the purpose of which I don’t really
get. I chose to reimplement the presets menu using a gui_synclist. An
alternative might have been to come up with some menuing infrastructure
to allow arbitrary speech for each menu entry, perhaps through some sort
of voice callback. The gui_synclist made it easier to add a few new
functions however. And the callback avoids the duplicated preset lists.

cvs log tells me this menuing code stuff isn’t old, so I may be taking
apart fairly recent code here. Comments appreciated, or if anyone wants
to educate me wrt RB menus, or has a good idea how to handle complex
voice output for menu entries… Some other menu entries in RB that can’t
be spoken are: recording directory, simple equalizer.

I added two new functions to the preset context menu:
-Ability to move a preset,
-Ability to insert a new preset at a given spot in the list.

I was certainly influenced by the playlist_viewer which I just worked
with. I even put in the icons to indicate the currently playing
preset and the preset being moved. Oh and I indicate both the preset name
and frequency. If a sighted person could tell me whether this all works
right, it would be appreciated.

And in the radio_menu, I added a new function to sort the presets by frequency.

Oh and I fixed a little glitch when deleting the last preset: access
beyond array bounds, uses uninitialized data, tunes to frequency to 0.0
which seems to confuse my hardware…

Closed by  sdoyon
2007-11-07 02:15
Reason for closing:  Accepted
jteh commented on 2007-06-22 01:54

The last version of the original patch no longer applies at all, as there have been some major changes to the radio code, one of which is that the presets menu does now use a synclist. I chose to reimplement speech for the radio screen. It is based on the ideas of the original patch but does things a little differently. Differences in functionality include:
* It only adds speech and doesn’t include the additional features in the original; imo, these should probably be implemented separately now that the code doesn’t have to be modified so much to add speech.
* It does not make an exception to spelling the preset name if it is the default name assigned during autoscan, as I feel that there should not be an exception here. I understand the reason behind it, but it is then somewhat different to what is reflected by the rest of the UI. Perhaps autoscan should just assign blank preset names and only show the frequency for blank preset names? Hmm, I don’t quite like that idea, but I think the UI as a whole needs to handle this differently, not just the voice. (I know the original patch did change the UI a bit in this regard.)
* Currently, only the preset name is spoken in the presets menu and not the frequency. The original patch modified both the UI and voice to include the frequency. This is easy enough to do, but I’m trying to leave the UI out of this patch for now.

Thanks very much for the original idea and patch, Stephane. I’ve been wanting to do this for a long time but hadn’t gotten around to it. The new radio code is certainly much easier to work with! :)

Btw, I don’t entirely disagree with the ideas you have removed. I’m just trying to separate this out a bit for now so we can get it included and then work on some of the functional changes.

jteh commented on 2007-06-22 02:25

Err, I meant “don’t entirely disagree with the ideas *I* have removed”. Sorry.

I’m VERY glad to see some interest for this. I had quite lost hope that
anyone would ever look at this.

Yes I do agree that I threw in too many things together in that original
patch, and it was pretty intrusive too. Sorry. Most of my other patches
had attracted little or no interest, so at that point I was really doing
it for myself, so I did not bother to split things neatly.

So thanks for looking at this! I’ll try to find some time to update my

jteh commented on 2007-08-07 11:08

Synced with latest svn (r14229). One difference in this version of the patch is that “Scan mode” and “Preset mode” are now spoken as two strings: “Scan” or “Preset” and then “Mode”. This does introduce a pause and some strange inflection, but I don’t really see the point of adding two new strings for this when there is a suitable alternative. (LANG_PRESET is now used in a more unified manner, which is the reason that this has changed.)

One of the problems with splitting sentences into composite parts and joining them together is that different languages order words differently to say the same thing compared to english.

The French translation for example would probably have the words the other way round (”mode preset”, and “mode scan”).

jteh commented on 2007-08-07 11:52

This did occur to me. There are two solutions: just speak “Scan” and “Preset” and don’t bother to speak “Mode” or implement two new lang strings for the two modes. I am happy to do either. The first is arguably not as intuitive for newer users, yet less verbose for those of us who are familiar with it.

jteh commented on 2007-08-07 22:25

Reinstated the “Scan mode” and “Preset mode” voice strings.

jteh commented on 2007-08-09 20:24

Synced to r14260.

jteh commented on 2007-08-19 02:59

Synced to r14391.

Here’s a slightly modified version:

1) No need for talk_preset() to return a boolean, AFAICT, so we can simplify
the logic slightly.

2) Also you have bool talk = false; on entering the radio_screen(), whereas
I would generally have the current setting spoken right away, so I’d
initialize it to true. Also that gives a nice feedback on completion
of the first autoscan (otherwise the user is left kind of dangling).

I understand your point about not changing the UI, but I must say
that spelling out the frequencies in the preset names and the MHz letters
really doesn’t sound too good, and it’s very verbose, it becomes
somewhat annoying.

Perhaps I should write up the small required UI change in a separate patch.

Or perhaps the radio screen should only speak in scan mode, which is
presumably when you’re looking to match a frequency. You often don’t
need to be told the preset name, and I find it can get annoying when
you know perfectly well what it is and yet the voice drowns out the
station for a couple of seconds while it tells you again. Or you don’t
care what it is because you’re just looking for some good music on any
station. I’m normally one for total information access, but the trick
is in this case, one can just enter the preset menu to find out what
the current preset is. So it would take an extra step, but the info
would still be there. Thoughts?

The spelling of the default preset name is really too annoying:
“nine seven dot seven zero ehm aytch zee”.

radio-preset-default-empty.diff changes the default preset name to the
empty string, and modifies the preset menu list’s get_item callback to
substitute LANG_FM_DEFAULT_PRESET_NAME when the empty string is
encountered. So nothing changes in the on-screen UI, except when you go
to edit a preset name: if it was the default, you start with an empty
string, which is good because you don’t have to erase it.

Here’s a new radio_voice.patch. It recognizes when a preset name string
is empty, and in that case it speaks the frequency instead, as a number
(rather than spelling it).

This version of radio_voice.patch was also made to use my voice callback
for synclists: P7774. So it now depends on that.

It is a bit annoying when you have added a preset and you scrol over it. The voice tells you and you already know what it is… I am not complaining but do you think we could remove the preset talking code?
I think it is from the function:
void talk_preset(int preset, bool fallback, bool enqueue)

Just my opinion…

I would do it. But you don’t want me to update your patches because my formatting isn’t great yet. And second I am using r15239 and don’t want to resync everything. I am going to learn linux and stuff so I probably won’t update my rockbox for a while. Or even write patches…

O yeah re the talking presets you would have to remove the call to that function as well…

Ok modified a big patch sdoyon sent me so can’t upload my changes. Might later if I think they’re useful.
Just removed a call to the talking preset function.
Commented it out actually. If I was going to upload it I would need to do it properly. And remove the actuall function and remove the calls to it. Not comment stuff out… I have still got the function here just commented out the calls.

I will see how it goes and if I like it might upload a new version.

OK so voicing the frequency and/or preset name in the radio screen on
every station change is annoying:
-because when you’ve identified one station, you often know what the
neighboring stations are,
-because often you’re just looking for good music on any station.

OTOH on some occasions it’s really nice to be able to identify what
station you are listening to or to be able to find a particular station.

So an idea: speak the frequency and/or preset name when switching stations
but only when the radio is paused.

Patch attached.


Available keyboard shortcuts


Task Details

Task Editing