Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#8521 - Permanent setting for the pitch

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Sunday, 27 January 2008, 10:00 GMT+2
Last edited by Peter D'Hoye (petur) - Monday, 28 January 2008, 10:53 GMT+2
Task Type Patches
Category Settings
Status Unconfirmed
Assigned To No-one
Player type All players
Severity Low
Priority Normal
Reported Version current build
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

This patch makes the pitch setting permanent. You can set the pitch value that should be used after the DAP is switched on. This can also be changed in the pitch screen but the value set there is NOT saved. So if you set the pitch to 150% in the pitch screen but have it set to 100% in the sound settings, 100% will be used after the next power on.

Also, the initial mode for changing pitch (small steps/semitones) can be set.

Manual is also updated.

I changed the symbol HAVE_PITCHSCREEN to HAVE_ADJUSTABLE_PITCH since it better tells what we're at IMHO.
   pitch_setting.patch (34.2 KiB)
This task depends upon

Comment by Thomas Martitz (kugel.) - Sunday, 27 January 2008, 17:43 GMT+2
Not nessecary. You can define a default pitch level in fixed.cfg (or was it fixed_config.cfg?).
Comment by Alexander Levin (fml2) - Sunday, 27 January 2008, 19:43 GMT+2
I think, there is no setting for the pitch yet. Hence you can't define it via the config file (no matter which one). This patch makes it possible. Another question is whether we should allow the user to set the two settings via the GUI. I chose to do so. Otherwise they will be forgotten and will never be used.

Another note: it would be good if some indicator would display in the status bar if the pitch is not exactly 100%. Maybe different symbols for <100% and >100%. But this would be the subject for another patch once this is committed.
Comment by Thomas Martitz (kugel.) - Sunday, 27 January 2008, 19:58 GMT+2
Ok, a little test revealed that there's no pitch/speed setting in the config.

However, it would be easier and cleaner to just introduce a persistent. Especially after reading the title of this, I think your patch does too much at once, and it doesn't do it the right way.

But then again, I'm no dev. I'm just stating my opinion.
Comment by Alexander Levin (fml2) - Sunday, 27 January 2008, 20:50 GMT+2
> I think your patch does too much at once, and it doesn't do it the right way.

Care to elaborate on what would be not too much and what would be the right way to do it?
Comment by Thomas Martitz (kugel.) - Sunday, 27 January 2008, 20:56 GMT+2
Your change HAVE_PITCHSCREEN to HAVE_ADJUSTABLE_PITCH is definitely not need at all. HAVE_PITCHSCREEN does the job very well, since the pitchscreen is only activated for those target for which HAVE_PITCHSCREEN is defined.

Also, you are introducing several new settings. Are you sure they are needed, and worth the bin size grow and the menu cluttering? I don't think so.
Comment by Alexander Levin (fml2) - Sunday, 27 January 2008, 22:00 GMT+2
I introduced the settings in the menu because, as I understand, there are no settings without GUI access in Rockbox. The symbol name might indeed be changed in a separate patch but I think the new name better describes the feature, i.e. the capability of the player to adjust the pitch. And if the player is capable of that, Rockbox will of course allow that. So HAVE_PITCHSCREEN is an 'implication' of HAVE_ADJUSTABLE_PITCH.

Anyway, (positive ;-) comments from other people are welcome.
Comment by alex wallis (alexwallis646) - Sunday, 09 March 2008, 14:58 GMT+2
Hi. Could this patch please be updated to work with the latest svn code?

Loading...