Details

I'd like to commit my initial work in converting rockbox to run as an application relatively soon because its fairly difficult to keep in sync.

In order to facilitate review and cleanup, I've split the patch into two portions. The first is just ifdef nonsense needed to allow "application" builds to use different settings then sim builds. This is likely to be non-controversial, though I wouldn't mind any suggestions what the define for "rockbox is being compiled as an application on another OS" should be. Right now I use "SDL" but thats not quite right.

The second patch is the actual code needed to make this work: changes to the configure script, some code in the target tree, keymaps, etc. I've left most of the target tree stuff out of this patch since its not much good without a working port to some other device. Otherwise its just a copy of the uisimulator/sdl code in the target tree, and that seems kind of a mess. Instead I just compile the uisimulator folder and use it for SDL functions.

For now I've created a "generic" entry in the target tree. I'm not sure if this is the right name, but its basically just code that is specific to the application project, but does not assume any specific CPU. However, if we ever got an SDL port to an ARM target, this wouldn't quite work either since there'd need to be an arm/sdl folder in addition to any generic code in generic/sdl.

For the actual target specific stuff, I've used the gigabeat F keymap, screen size, etc so that WPSes still work. It seems as sensible as any other target but I'm open to suggestions.

Finally for this to work, you will need to duplicate one of the bmps in the uisimulator/sdl folder and name it "UI-sdl.bmp". I'm not really sure what the app bitmap should look like, or if one is even needed, so I haven't done anything about this yet.

I've only looked at the sdl_preprocess.patch so far, and my thoughts on the "SDL" define is that this should be replaced by a number of different #defines, depending on the feature that is being included/excluded. A lot of the use of SIMULATOR is because a certain feature isn't working properly in the sim - something that shouldn't be applicable for rockbox-as-app (if a feature doesn't work, it can simply be disabled in the config-target.h file instead) - so you shouldn't follow the SIMULATOR define religously.

e.g. looking through sdl_preprocess.patch, the first use of SDL is in apps/codecs.c. This is used to decide whether to use the code that enables the Rockbox target method of loading codecs (i.e. a binary blob loaded to a fixed memory location), or shared libraries. So maybe this could be a HAVE_SHARED_LIBS define (or simply something like ROCKBOX_APP).

The next changes related to entering disk mode and USB detection - so maybe something like HAVE_DISK_MODE ? (we have some fire-wire only targets, so shouldn't introduce more USB defines, when they also apply to firewire)

Next is apps/recorder/radio.c - as far as I know, recording isn't implemented in the sim, but should be on any rockbox-as-app target. So for now, you can simply remove the recording (and radio) code by not defining those #defines in your config-sdl.h file. Also, that recording code is only for hwcodec, and I would be very surprised to see a hwcodec version of rockbox-as-app.

Next is plugin.lds - I wouldn't expect this file to be used in sim builds, so am not sure you need to use an SDL define there.

Next is the overlay code for plugins - this could perhaps use the HAVE_SHARED_LIBS define.

Next is apps/plugins/SOURCES and SUBDIRS - if you want to (temporarily) disable plugins for a particular target, you can do this via the configure script - setting plugins="no"

Next is apps/gui/statusbar.c - the first shouldn't be needed, as you won't define CHARGING_MONITOR in your config-target.h file. The second shouldn't be needed, as (if your target implemented recording), you would have the rec_freq_sampr[] array defined. In fact, I'm not sure why this isn't defined for the sim...

A few more random comments from later files:

apps/filetree.c - this should probably be something like "HAVE_ROLO"

apps/playback.c - the first hunk shouldn't need an SDL define (either implement recording for your target app, or don't declare HAVE_RECORDING). Later ones are related to the "HAVE_SHARED_LIBS" or "ROCKBOX_APP" define.

That's all I have time for at the moment, but I hope you get the idea... The code in firmware/ is probably going to be harder though.

Trying to answer your questions - yes, I think all existing targets will need HAVE_DISK_MODE. Although some new ports (e.g. the Telechips ports) don't have a disk mode implemented yet, so those could not have the define set.

What don't you understand about apps/codecs.c? But yes, that code is loading the codecs, so should use the HAVE_SHARED_LIBS define, if that's what you're going to use.

I'm sorry I haven't looked closer on this in a long time, but is this v3 reasonably matching what you have at this point?

I would like the player to be called something less generic than "sdl" in the configure for example. Possibly SDL-320 for the width of the screen (to allow versions with different screen sizes). Or perhaps "SDL-320-linux" and truly allow the app to use linux-specific functions for stuff such as RTC (using /dev/rtc or hwclock etc) that I would assume rockbox-as-an-app on a linux-based device wants.

I don't like how this sdl target defines SDL and the simulator doesn't. It makes it look internally as if SDL is the name for the target and not the ui/sound/io layer that in fact is used by both this new target and the simulator.

Nits: target_id 42 is already taken in configure. You're using >80 column lines in configure. I also think you should rather remove unused defines from config-sdl.h than leaving lots of them commented. Line 66 in the v3 patch breaks the check for simulator in configure due to a stray ']' letter.

I dislike the question for the RAM size on archos as this makes the "flow" different to other players -- IMO this should get moved to the advanced configuration as it is a mod like f.e. the rtc mod for h100 is. The same way the width and height values for the application target could assume some sensible defaults and changed be done via the advanced menu. This would make configuration identical for all targets.

Sorry for the long turn around, I'm already back in school and rather busy at the moment.

>'m sorry I haven't looked closer on this in a long time, but is this v3 reasonably matching what you have at this point?

Yes.

>I would like the player to be called something less generic than "sdl" in the configure for example. Possibly SDL-320 for the width of the screen (to allow versions with different screen sizes). Or perhaps "SDL-320-linux" and truly allow the app to use linux-specific functions for stuff such as RTC (using /dev/rtc or hwclock etc) that I would assume rockbox-as-an-app on a linux-based device wants.

Ok changed.

>I don't like how this sdl target defines SDL and the simulator doesn't. It makes it look internally as if SDL is the name for the target and not the ui/sound/io layer that in fact is used by both this new target and the simulator.

I agree. I've been slowly removing them and replacing them with other defines in the configure file, but we should pick one define to determine if its an application or an operating system build. Maybe APP_BUILD?

>Nits: target_id 42 is already taken in configure.

Fixed.

>You're using >80 column lines in configure.

Fixed.

>I also think you should rather remove unused defines from config-sdl.h than leaving lots of them commented. Line 66 in the v3 patch breaks the check for simulator in configure due to a stray ']' letter.

Both fixed.

>Wouldn't it make sense to have LCD_WIDTH and LCD_WIDTH being questions in configure for this kind of configuration, much like RAM size on Archos/Ipods?

The code depends on the size in a few places (like the bmp for the Window background for instance), so I'm not really sure how much sense it makes to move this into the configure script instead of just leaving it in config-whatever.h. You'd probably still have to edit a few things in the code to change the resolution anyway.

This latest version still seems to include code which is unrelated to the changes required for Rockbox as App. I can see changes to lines in configure that are around the Creative Zen port, and also some changes to buildzip.pl around the 7zip and tar targets for example - and this is on only a very cursory first glance through. It makes me wonder how much other less obviously unrelated changes might also be present ?