Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11899 - logo.c keymap defaults instead of error on undefined targets

Attached to Project: Rockbox
Opened by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 13:07 GMT+2
Last edited by Dominik Riebeling (bluebrother) - Sunday, 27 February 2011, 17:42 GMT+2
Task Type Bugs
Category Plugins
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.7.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Starting at line 218 logo.c defines keymaps to any target that did not explicitly get defined beforehand. Here is patch that adds defines for the the missing targets and errors out when it gets compiled for an unsupported target just like all the other plugins.
   logo.c_KEYMAP_NO_DEFAULT.patch (0.6 KiB) 
 ./rockbox/apps/plugins/logo.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

This task depends upon

Closed by  Dominik Riebeling (bluebrother)
Sunday, 27 February 2011, 17:42 GMT+2
Reason for closing:  Not a Bug
Additional comments about closing:  I read the last comment as request for closure, so closing it. If I'm mistaken on this please request a reopen.
Comment by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 14:02 GMT+2
I realize that changing such generic catch all code can be fatal I suspect it was left over from some time ago when there were just a few targets. I am half way done testing on all the sdl sim builds, where I can, and have not had any trouble compiling.
Comment by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 15:41 GMT+2
Here are my test results for this patch. If this is acceptable to any devs, I would like to see this patch committed So I can move ahead dissecting FS#11898 into smaller manageable patches and finally get the plugins compiling nicely for android.
   logo.c_patch_test.txt (5.5 KiB)
Comment by Thomas Martitz (kugel.) - Saturday, 22 January 2011, 17:00 GMT+2
Your file mentions that logo.rock doesn't run on the iHP-100/110/115 series. Does it work without your patch?
Comment by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 17:32 GMT+2
the results show only the simulator build failed but every single other plugin error catches targets without keymaps. Logo wants to assign them arbitrary ones which can cause problems. I will check again just for you.
Comment by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 19:27 GMT+2
I retested and my mistake it seems to build the simulator quiet nicely for theiHP-100/110/115 and logo is running perfectly well.
Comment by Benjamin Brown (BenjaminBrown) - Saturday, 22 January 2011, 20:05 GMT+2
Looking over everything this change is not mandatory for FS#11898 at all and can be considered solely for the benefit of better code. So sorry for the confusion and extra time I spent on it, ha I'm a dork.
Comment by Thomas Martitz (kugel.) - Saturday, 22 January 2011, 20:11 GMT+2
Will commit it anyway :)
Comment by Dominik Riebeling (bluebrother) - Sunday, 23 January 2011, 12:18 GMT+2
My h100 does have logo.rock and it runs (r29079) without any modifications of logo.c. Is this patch only relevant for simulator builds?
Comment by Benjamin Brown (BenjaminBrown) - Monday, 24 January 2011, 15:59 GMT+2
It's relevant to all builds the old code caught every target without a defined keymap, But really considering this patch further I concede that leaving as is will keep it a safe bet come compile time. Close this patch its unneeded.

Loading...