All Projects

When I try to run a simulator compiled with default settings in amd64 Xubuntu 11.10, it hangs with:
* longjmp causes uninitialized stack frame *: /home/bgjenero/Rockbox/rockbox/build1sim/rockboxui terminated

Ubuntu sets _FORTIFY_SOURCE to 2. The gcc man page claims it is “activated when -O is set to 2 or higher”, but this means it’s also activated by just -O, which is equivalent to -O1. The problem goes away if I recompile firmware/threads.o with -U_FORTIFY_SOURCE. It remains if I use -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1.

Fortify doesn’t like the longjmp() near the end of make_context() in firmware/asm/thread-unix.c. It’s a jump to the setjmp() in trampoline(). trampoline() was called earlier as signal handler, in response to a signal sent from make_context(). The stack frame was definitely initialized, because make_context() waits for sig_handler_called to be set by trampoline().

As a simple workaround, you use SDL threads by giving the –sdl-threads argument to configure.

Here is a partial gdb backtrace:
#12 0x00007ffff71fb7f7 in fortify_fail ()
from /lib/x86_64-linux-gnu/libc.so.6
#13 0x00007ffff71fb789 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#14 0x00007ffff71fb6f3 in longjmp_chk () from /lib/x86_64-linux-gnu/libc.so.6
#15 0×0000000000462844 in make_context (ctx=0x898b08,

  f=0x4668d0 <scroll_thread>, 
  sp=0x8ae9c0 <<snipped out junk here>> 
  stack_size=43008)
  at /home/bgjenero/Rockbox/rockbox/firmware/asm/thread-unix.c:186

#16 0×0000000000463943 in setup_thread (context=0x4882c58)

  at /home/bgjenero/Rockbox/rockbox/firmware/asm/thread-unix.c:277

#17 load_context (addr=0x4882c58)

  at /home/bgjenero/Rockbox/rockbox/firmware/asm/thread-unix.c:303

#18 switch_thread () at /home/bgjenero/Rockbox/rockbox/firmware/thread.c:1256
#19 0×0000000000460576 in sleep (ticks=25)

  at /home/bgjenero/Rockbox/rockbox/firmware/kernel.c:247

#20 0x0000000000406e8c in init_tagcache ()

  at /home/bgjenero/Rockbox/rockbox/apps/main.c:329

#21 init () at /home/bgjenero/Rockbox/rockbox/apps/main.c:390
#22 main (argc=<optimized out>, argv=<optimized out>)

  at /home/bgjenero/Rockbox/rockbox/apps/main.c:163

I recently found that displayed battery level can change very rapidly after the hard drive spins down. This is very easy to see on my 5G iPod with the original 30GB hard drive, running 007f61f. It happens because battery_read_info() calls _battery_voltage(), which reads battery voltage from the hardware. I changed it to call battery_voltage(), which returns smoothed voltage, and that restored the familiar old behaviour. I’m attaching the patch.

I think some smoothing is needed. Maybe the old behaviour was doing too much smoothing, but total lack of smoothing is worse.

This only affects targets which measure battery voltage. The changed code also builds on other targets, because they provide a battery_voltage() function which just returns -1.

When Rockbox is directly accessing the disk, paths starting with /./ are not valid. The FAT32 root directory does not have the . and .. entries found in other directories. It is possible to use . and .. in other directories, even for getting to the root directory from a 1st level directory.

This causes HOME_DIR, introduced in r31430, to break some things. It seems dircache can deal with it. However, file creation always uses opendir_uncached(), which fails. Here are two examples:

When recording to the default recording directory, the path is “/./filename.ext”. With dircache disabled, the recording screen can’t be entered, because “/./” can’t be accessed. With dircache enabled, recording starts, but saving fails as if the disk is full.

When saving a playlist to the playlist catalog, the path is “/./Playlists/name.m3u8”. With dircache disabled, I can’t even enter a filename because the path is inaccessible. With dircache enabled, I can edit the filename and remove the /. at the start to make it work. If I don’t remove the /. then there is no error message but the playlist fails to save. This lack of error message is another bug.

I tested this on my 5G 30GB iPod running r31467.

With a real 5G iPod or sim running r31467, directory playback stops working after I save a playlist. To reproduce: play a directory, save the playlist, stop playback, and try to play a directory again. The error is “Playlist Buffer Full”.

This happens because the end of playlist_save() sets playlist→buffer = NULL and playlist→buffer_size = 0. The zero buffer_size causes playlist_add() to fail near the start of the function and display that error. When the current playlist file is being overwritten, playlist_save() correctly saves and restores the values, but otherwise, the values aren’t saved and they are “restored” from variable initializations at the start.

My patch here solves the problem by unconditionally saving old_buffer and old_buffer_size. (Compared to conditionally restoring the values, it’s simpler and it results in smaller binsize.) I removed buffer_handle saving because it’s not altered in playlist_save(). The patch also initializes buffer_handle in playlist_init().

I’m now able to build a working copy of Rockbox r31177 for my Archos Recorder V2 using binutils 2.21.1 and gcc 4.6.2, with -Os -flto. The main advantages are a binary size and memory use decrease of 7kb and automatic discarding of unused code, and the main disadvantage is much slower linking. I don’t know if this is worth it.

The new binutils is needed because a linker plugin is needed to enable link time optimization of object files stored in library archives, like libfirmware.a. Linker plugin support is automatically detected by gcc, so there’s no need for -fuse-linker-plugin.

The attached gcc patch is based on the current gcc-4.0.3-rockbox-1.diff by Jens Arnold (amiconn). I still need to investigate whether the workaround in gcc/config/sh/sh.h is actually needed. Including it shouldn’t cause any problems. You can find info about it in IRC logs around this date: http://www.rockbox.org/irc/rockbox-20060427.txt

The attached Rockbox patch changes rockboxdev.sh to build this toolchain, configure to add -flto for gcc 4.6.0 and above, and various things so Rockbox builds properly. The gcc patch can’t be automatically downloaded by rockboxdev.sh, so put it the download directory, which is by default, /tmp/rbdev-dl. Note that configure will only use -Os if it finds “rockbox” in the sh-elf-gcc version string, so if you want to try an unpatched gcc, you need to edit configure or the generated Makefile.

Most of the code changes simply add attribute1) to stuff that gcc -flto would otherwise throw away. When C code is only referenced by assembler code, gcc will throw it away. This even happens for references from inline assembler in the same C file. Functions in apps/plugins/lib/gcc-support.c were also getting discarded, resulting in “defined in discarded section” errors.

Link time optimization shuffles around code, and then divides into several large assembler files. (Note how in rockbox.map, instead of the normal .o files, you see a bunch of .ltrans.o files.) Code from the same file may end up in different assembler files. This is why the “bsr _UIE” couldn’t reach UIE(), and why .global is needed for _start_thread and _UIE4.

Various little notes: I see no improvement with GLOBAL_LDFLAGS=-fwhole-program, so gcc must be detecting that properly. Adding -ffunction-sections -Wl,–gc-sections is also not helpful. The patch doesn’t fix some warnings added by using gcc 4.6.2, but there are only a few, and they should be easy to deal with. It also doesn’t make changes needed for -flto for other targets. Without -flto, gcc 4.6.2 generates a binary that’s 3 kb bigger than the gcc 4.0.3 binary.

My Archos Recorder V2 uses the v3 bootloader for Recorder V2 with ROM. (When I dump flash, internal_rom_2000000-203FFFF.bin matches firmware_v2.bin in http://download.rockbox.org/bootloader/archos/flash-recorderv2-v3.zip up to 0×7000, except for one byte at 0xFC. The stuff after 0×7000 is the main Rockbox image flashed via rockbox_flash, and the one byte is a “hardware mask value” which is kept by firmware_flash.c. This means my bootloader and BootBox are both the same as in that file.)

I wonder what code was used to compile the bootloader. I see svn tags for various bootloaders, but no tag that might be the v3 Archos bootloader. The second image seems to be r18960. I don’t see a revision string in the decompressed BootBox image. The BootBox code I examined seems to match r18960.

The rescue boot function (hold F1, press on, loading /ajbrec.ajz) works perfectly as long as external power is not connected.

If F1 is held down while plugging in the a charger, The result is “ATA error: -11”. When I release F1, I get:
*PANIC*
storage: -11
and the red LED blinks. This behaviour is due to the charging_screen() being skipped and storage_init() failing in bootbox/main.c.

I suspect the charging screen exits because charger_inserted() is used, and it depends on probing from power_thread(). In r20634, apps/main.c was changed to use (power_input_status() & POWER_INPUT_MAIN_CHARGER), and I think this change also needs to be made in BootBox. The use of charger_inserted() in charging_screen() should not be dangerous because the button_get_w_tmo(HZ/2) should allow power_thread() to run and properly set power_thread_inputs. Note that r17955 fixes the usb_detect() line in BootBox. That fix would have to be backported if compiling the bootloader from some earlier versions. I expect the same issue exists on all BootBox targets with charging which are compiled from the same code. (Only Ondios don’t have charging.)

In flash/bootloader/bootloader.c, targets with hard drives turn off IDE power if the charger is inserted. For example, here is PLATFORM_FM:
if (ReadADC(0) < 0x1FF) /* charger plugged? */
{ /* switch off the HD, else a flat battery may not start */
Before r25593, ide_powered() always returned true, so ata_init() did not execute ide_power_enable(true). This problem should exist on V2 and FM Recorders, and Players which are able to control HD power. The V1 Recorder defines HAVE_ATA_POWER_OFF. On it, ata_init() should succeed, and the rescue boot should continue normally. Ondios should be unaffected.

On the V2 and FM Recorders and the player, charging is hardware controlled, so it should continue while the error is displayed. The V1 Recorder has Rockbox controlled charging, but it shouldn’t panic. The only problem is that turning on the disk when battery level is very low. This is not a big problem, because susceptible targets have removable AA batteries.

If F1 is held down while plugging in USB, the disk spins up, and I see
Rockbox
Rescue boot
This hangs until I remove the USB cable. At that point, the rescue boot continues, and it is successful.

This behaviour is consistent with staying in the while (usb_detect() == USB_INSERTED) loop in BootBox main(), but not getting button_get_w_tmo(HZ/10) == SYS_USB_CONNECTED. The loop condition seems fine because usb_detect() reads directly from the hardware. The problem seems to be that F1, which has to be pressed for running BootBox, is also the key Rockbox normally uses to ignore USB connection. So, usb_tick() posts to usb_queue. When usb_thread() gets that, it finds (button_status() & ~USBPOWER_BTN_IGNORE) == USBPOWER_BUTTON) and it doesn’t do the queue_broadcast(SYS_USB_CONNECTED, 0) that main() is expecting. This problem should only exist on the FM and V2 Recorders, because they HAVE_USB_POWER and assign F1 to both functions. If BootBox was built with r29889 or later, the problem would also appear on the Ondios, because only USBPOWER_BTN_IGNORE is used in the check.

It is possible to get into BootBox USB mode. When you insert the USB cable, you have to hold F1 down long enough for the bootloader to see it, but you must release it before usb_thread() in BootBox sees it. It’s not too difficult, but I recommend always ensuring that you have either a known-good boot file on disk or a known-good second image in flash. This means change one at a time, and test it before changing the other.

I am intentionally leaving severity and priority at the default values, because these errors shouldn’t matter during normal use, and recovery is possible even with these bugs.

A short explanation of contents of the bootloader flash image:

The first part is the bootloader itself. The main code is located in flash/bootloader/. It just does a bit of hardware initialization, lets you choose to run the first or second image decompresses the image if necessary, and then jumps to it. It also contains MiniMon, for recovery over a serial connection. There is no need to change this part of the flash.

The second part is BootBox. It is an extremely stripped-down version of Rockbox used for recovery in case of problems. It offers a charging screen, USB mode, and ability to load a boot file such as /ajbrec.ajz. The problems I described are in BootBox.

The third part is the second image, which is loaded by default. It can be a compressed Rockbox image or RomBox. During a normal Rockbox install via rockbox_flash, only the second image is changed. The bootloader and BootBox remain unaltered, enabling recovery if something goes wrong.

Update:

The bootloader in the v3 flash images corresponds to 66f496c (r18970 from trunk). I was able to build identical FM and player bootloaders using recent version c5f772c.

BootBox and the full version of Rockbox in the flash images might be from the v3.0 branch, because errors prevent a successful build of r18960 from the trunk. I’m not able to confirm the exact versions.

BootBox can be built by choosing (B)ootloader when running configure. The resulting binary has gotten bigger. Here are sizes:
v3.0.1-final: bin=36488 ucl=25260
c5f772c: bin=39964 ucl=27812
The c5f772c BootBox plus the bootloader cannot fit in the area before 0×7000.

The build already uses -ffunction-sections and -fdata-sections via EXTRA_DEFINES in Makefile, from configure, and -Wl,–gc-sections via BOOTBOXLDOPTS from tools/root.make. It may be possible to save a bit more space via -flto ( FS#12431 ), but I expect some code would have to be removed manually.

A BootBox build also wouldn’t work now because RoLo uses core alloc, and there’s no call to core_allocator_init(). Excluding buflib would save space. There may be other bugs. I guess BootBox has not been tested in a long time.

On my Archos Recorder V2, when running recent versions, including 3.10RC and r31107, entry into USB mode can be slow and problematic. Unfortunately, I'm not always able to reproduce this. When it happens, it happens repeatedly, but sometimes it repeatedly doesn't happen. Based on the following 3 pieces of evidence, it seems like Rockbox is trying to do something with the disk after the USB to IDE bridge chip is allowed to start using it.

When it happens, there is a long wait before the USB screen is displayed, during which time the red LED is on. I hear disk access as if there is disk access from the computer, and there are many disk errors in dmesg. The LED goes off right before the USB screen appears.

I edited r31107 firmware/usb.c to write strings at various points of usb_slave_mode(). When the problem happens, the string from after cpu_idle_mode(true) is on the screen and the red LED is on.

In the SMART error log on the original DK23EA-20 drive (accessible via smartctl), I saw ICRC errors, which I think means interface CRC, meaning Ultra DMA CRC. This would be consistent with Rockbox doing stuff which corrupts DMA data. (The ISD-300 configuration EPROM has been modified to enable Ultra DMA, using this http://www.rockbox.org/wiki/UDMAonUSB ) The drive passed the long SMART self-test, and reading of the entire drive surface completed without any errors reported or logged.

Edit: I have traced it to an opendir_uncached("/.rockbox/fonts") that starts after the end of usb_slave_mode(on). So, I guess the font code needs some changes. It would also be nice to have something at the storage or ata level preventing further storage access after storage_enable(false).

Edit 2: I have now traced it to an opening of the font file in glyph_cache_save(). Fonts are closed in gui_usb_screen_run(), but usb_acknowledge(SYS_USB_CONNECTED_ACK) happens before it. So, USB mode can be activated before font_unload() calls. In this patch, I move the usb_acknowledge(SYS_USB_CONNECTED_ACK) to just before the while (1) main loop of the USB screen. This fixes the problem on my Archos Recorder V2, and USB mode entry becomes fast and reliable. It ought to be safe to put the acknowledge earlier, but I see no problem with putting it just before the loop.

I have an electrical concern relating to this bug. It's not good to have the CPU fight the bridge chip. If one pin drives an IDE bus line high and another pin drives it low, that could overload the pins. I'd like to have more protection here, such as pin setup via ata_enable(false) which ensures that the CPU can't drive the IDE bus and/or blocking of disk activity at the ata or storage level. However this this patch ought to be good enough for now.

I wanted to add STORAGE_INIT_ATTR to some functions, but the prototypes were repeated in all the ata-target.h functions. After a bit of discussion on IRC, I decided to move them into one file. I originally intended to put them in firmware/export/ata.h, but it seems better to put prototypes for users of ata.c into ata.h and prototypes for target-specific functions used by ata.c into a separate file: ata-driver.h.

In ata-driver.h, I include both ata-defines.h and ata-target.h. That is because order matters. Conditionals in ata-driver.h depend on ata-target.h. Also, ata-defines.h contains some generic defines which can be overridden in ata-target.h, and so it must only be included after ata-target.h. It’s simplest to just have ata-driver.h deal with this, and have other files only include ata-driver.h. It would be easy to merge ata-driver.h and ata-defines.h.

There’s only one function in both ata.h and ata-driver.h: ata_enable(). Maybe some change is needed, because letting other code disable ata without telling ata.c may allow bad things to happen. However, that would be for another patch. This patch also doesn’t add any STORAGE_INIT_ATTR, because that can easily be done afterwards.

Bins compile cleanly for all stable targets and creativezvm (which has a “nasty hack”), and the patch doesn’t change the resulting rockbox.bin files. I don’t like how it changes so many source files. There are a few changes which could be removed from the patch or applied separately if necessary:
- In some ata-target.h, addition of #ifndef to prevent multiple inclusion
- Removal of unneeded inclusion of ata hearder files
- Including config.h in ata-target.h files which clearly depend on it
- Renaming ATA_SET_DEVICE_FEATURES to ATA_SET_PIO_TIMING
- Moving prototypes from ata.c into ata-driver.h

I’m also attaching remove_unused_ata_includes.patch, which removes unused ata includes from files which wouldn’t otherwise be touched. I created it as a side-effect of creating the main patch, because I had to review those files to make sure I wasn’t breaking anything when I changed those header files.

Now that core_alloc is used, firmware/buffer.c is almost entirely obsolete. It’s only used by core_allocator_init() to tell buflib_init() what memory to use for core_ctx. The contents of the associated header file, firmware/include/buffer.h, are also not needed. It’s included by several files, but that’s easy to remove. I’m attaching a patch for r31079.

There are currently no other allocations via buffer.c, and allocations could overlap with core_alloc allocations. That’s because core_allocator_init() does the following:
void *start = buffer_get_buffer(&size);
buflib_init(&core_ctx, start, size);
buffer_release_buffer(size);
It tells buflib to use the memory and then releases the same memory so allocations via buffer.c can also use it. Yes, one could just remove buffer_release_buffer(size), but what’s the point of an allocator which will just call panicf()?

Basically, the benefits are: removing unused code, removing an opportunity for errors, and a simpler future implementation of  FS#12403  (when putting .init at the start of the buffer, as kugel suggests).

Note that when files are deleted, you need to rerun “make dep”, or else you’ll get a “make: *** No rule to make target” error.

There seem to be several problems in r31061 buflib_compact() in firmware/buflib.c. I just created one tracker entry because this all involves just one function and a solution may address multiple issues.

The first problem I observed means a block ending at ctx→alloc_end may be lost:

When a block ends at ctx→alloc_end and only the hole filling strategy is used. Free blocks add to shift, and that is the proper amount for changing ctx→alloc_end. The change is made at the end of the function, via “ctx→alloc_end += shift”. However, after a block is moved successfully to fill a hole, this code is executed:
if (ctx→alloc_end == next_block)

  ctx->alloc_end = block;

As a result, at the end of the function, ctx→alloc_end points to the start of the last block.

It seems like other things can go wrong in the interaction between hole filling and moving by shift.

A failed attempt to move the allocation by shift will reset shift to 0, but the possibility remains that other blocks after the unmovable block can be moved into free space at first_free. Those moves won’t increase shift. As a result, ctx→alloc_end may end up wrong, and blocks moved by shift won’t be moved as much as possible. Perhaps the two lines I pasted above try to fix ctx→alloc_end in this situation, but they’re not always the right solution.

A successful move by shift doesn’t update first_free. That shouldn’t lead to a crash and it just means buflib_compact() won’t compact the buffer as much as possible. There are two possibilities:

If the block was shifted to start at first_free, then first_free→val is now positive and the hole filling code won’t run anymore.

If the block was shifted into another hole, then free space may remain at first_free, and blocks may be moved into it.

In either case, any free space after a shifted block is only usable for shifting the next block, and unmovable blocks result in holes which won’t be filled by moving other blocks into them.

I saw that INIT_ATTR isn’t implemented HWCODEC targets so I decided to implement it. This patch consists of several parts, which could be split if desired.
- Rewriting of SH crt0.S to copy and fill sections using a list of values in memory instead of repeated code. This adds an insignificant bit of overhead between operations, but the actual loops themselves are as fast as before. (Using copy and paste to create .init copying code felt wrong, and SH assembler was intriguing.)
- Enabling INIT_ATTR by editing config.h, adding the section to linker scripts, and copying .init in crt0.S.
- Changes to allow use of the plugin buffer for INIT_ATTR. Normally, the codec buffer would be used, but HWCODEC targets don’t have a codec buffer. In skin_data_load, buffer space is obtained from plugin_get_buffer. This buffer would otherwise overwrite INIT_ATTR code. The code only uses a small fraction of the buffer, and plenty remains for other uses.
- Adding INIT_ATTR mp3_init and functions only called from it.

This is currently a bit detrimental for RomBox. Initialization code in flash does not use RAM, and code added to implement this uses a bit of additional flash space. I didn’t even test RomBox because I don’t know how to fit current versions of Rockbox in ROM on my Recorder v2. However, I have in idea on how this could help RomBox in the future: .data and .init could be UCL packed to save flash space.

BTW While reviewing Archos target specific code for INIT_ATTR addition, I saw that the following functions could probably use INIT_ATTR:
serial_setup();
button_init_device(), but it’s declared in many header files
usb_init_device(), but it’s declared in many header files
system_init() for CONFIG_CPU != SH7034 (due to rolo)
i2c_init() for CONFIG_CPU != SH7034 (due to rolo)
ata_device_init() STORAGE_INIT_ATTR
ata_is_coldstart() STORAGE_INIT_ATTR

The iPod linker script contains a trick which causes .data to always be word aligned on disk. The section is not moved by startup code, so it remains word aligned in memory. If the section alignment was more strict, and as a result, more padding had to be added for alignment, .data would be at the wrong address in memory. This leads to a crash at startup.

The linker sets the section alignment to the strictest variable alignment in the section. Currently, nothing is increasing .data section alignment. The problem can only occur if a variable with more strict alignment (eg. CACHEALIGN_ATTR) is added to .data. Here’s one example of how to trigger the problem: http://www.rockbox.org/tracker/task/12391?getfile=24344

This issue seems to be present in multiple targets. The solution should be simple: remove the linker script trick from all targets which don’t have code to copy .data elsewhere. I’m attaching a patch which removes the trick for iPods. I’ll track down the other targets later.

I don’t think the “. = ALIGN(0×4);” lines have any purpose when . is not assigned to any value and the following section’s alignment is respected, but .text has the line, so I’m not removing it from .rodata. I’m not very familiar with linker scripts.

If I add “if (!write) cpucache_invalidate();” to ata_transfer_sectors in firmware/drivers/ata.c, a database commit almost always crashes. I suspect this is related to instability seen in PP502x IDE DMA ( FS#9708 ) (which was disabled in r29476 due to instability). Note that here, I’m just adding the cache_invalidate calls and not turning on IDE DMA.

The crash does not always happen during the same stage (number) of the database commit, but it always happens at the same spot: near the start of tempbuf_sort, currently starting at line 2088 in apps/tagcache.c:

      idlist = &lookup[i]->idlist;
      while (idlist->next != NULL)
          idlist = idlist->next;

The compiler inlines tempbuf_sort into build_index. One time I printed the first pointer pointing outside of RAM and it was 0xc0edbabe.

I’m attaching a patch I used to add the code to ata_transfer_sectors. If you want to reproduce this bug, you should probably keep the LCD awake manually or disable LCD sleep so that you can see the crash message. Note that /.rockbox/database_tmp.tcd will remain, and Rockbox will again attempt to commit the database on next startup. If /.rockbox/rockbox.ipod contains the patch, you may have to restart into disk mode to fix this, so you might want to put the patched rockbox.ipod elsewhere and load it from within Rockbox. If others can’t reproduce this, I can attach my database_tmp.tcd file.

I’ve confirmed this with r30989 and r31001 on my 5G 30GB iPod. Defining FORCE_SINGLE_CORE and increasing the tagcache stack did not help. I suspect this is a problem with PP502x cpucache_invalidate(), and not with the database.

I recently tried to compile Rockbox with binutils-2.21.1 and gcc-4.6.2 with link-time optimization (LTO). I didn’t produce copy that ran correctly, but the resulting rockbox.map was useful for finding unused code and data. LTO can also remove symbols for other reasons, so this list was created by finding which of the removed symbols only occur on one line in one C file. I only compared 5G iPod and Archos v2 Recorder builds.

Here are the small memory savings with r30944, using the normal toolchain:
For the 5G iPod:
< Binary size: 687368
< Actual size: 687360
< RAM usage: 1199984
—

Binary size: 686824
> Actual size: 686816
> RAM usage: 1199440

For the Archos v2 Recorder:
< Binary size: 301984
< Actual size: 301960
< RAM usage: 640700
—

Binary size: 301328
> Actual size: 301304
> RAM usage: 640044

What follows is the output from grepping through the r30944 code for these symbols. For more information, see the attached patch.

apps/playback.c:bool audio_buffer_state_trashed(void)
apps/playback.h:bool audio_buffer_state_trashed(void);

apps/radio/radio.h:struct gui_wps *fms_get(enum screen_type screen);
apps/radio/radio_skin.c:struct gui_wps *fms_get(enum screen_type screen)

apps/gui/list.c:bool gui_synclist_item_is_onscreen(struct gui_synclist *lists,
apps/gui/list.h:extern bool gui_synclist_item_is_onscreen(struct gui_synclist *lists,

firmware/export/mascodec.h:int mas_default_read(unsigned short *buf);
firmware/target/sh/archos/mascodec-archos.c:int mas_default_read(unsigned short *buf)

firmware/export/mascodec.h:int mas_direct_config_read(unsigned char reg);
firmware/target/sh/archos/mascodec-archos.c:int mas_direct_config_read(unsigned char reg)

firmware/enc_base.c:const unsigned long mp3_enc_sampr[MP3_ENC_NUM_SAMPR] =
firmware/export/enc_base.h:extern const unsigned long mp3_enc_sampr[MP3_ENC_NUM_SAMPR];

firmware/export/mp3_playback.h:long mp3_get_playtime(void);
firmware/target/sh/archos/audio-archos.c:long mp3_get_playtime(void)

apps/playlist_viewer.c:MENUITEM_FUNCTION(save_playlist_item, 0, ID2P(LANG_SAVE_DYNAMIC_PLAYLIST),

apps/gui/skin_engine/skin_display.c:void skin_statusbar_changed(struct gui_wps *skin)
apps/gui/skin_engine/skin_engine.h:void skin_statusbar_changed(struct gui_wps*);

apps/tagcache.c:long tagcache_get_commitid(void)

apps/tagcache.c:long tagcache_get_serial(void)
apps/tagcache.h:long tagcache_get_serial(void);

apps/tagcache.c:bool tagcache_is_busy(void)
apps/tagcache.h:bool tagcache_is_busy(void);

apps/tagcache.c:bool tagcache_is_ramcache(void)
apps/tagcache.h:bool tagcache_is_ramcache(void);

apps/tree.c:void tree_drawlists(void)
apps/tree.h:void tree_drawlists(void);

firmware/export/usb.h:void usb_storage_try_release_storage(void);
firmware/usbstack/usb_storage.c:void usb_storage_try_release_storage(void)

With one particular FLAC/CUE combination, if I try to seek to track 2 in the CUE, I end up in the next file in the playlist. This happens because flac_sync fails to sync in 10 tries at one point. Then, flac_seek fails, flac_decode_frame fails, and the codec returns CODEC_ERROR.

I am attaching “frame_sync problem.flac”, which consists of two frames cut from the FLAC file. The file seek gets to what corresponds to offset 0x5F9, a place inside the first frame. The code ought to find the second frame, but it gives up after 10 tries in the “for(unparseable_count” loop. Increasing the limit to 20 (”unparseable_count < 20”) causes the seek to succeed, but that is probably a workaround, and not an ideal fix.

The same file also triggered  FS#10476 . However, I think that bug is not related.

I noticed the problem on my 5G iPod running r22076, and I performed debugging on the r22076 5G iPod sim.

While playing a 561,500,563 byte FLAC file in r22076 on a 5G iPod, I get some pauses when the buffer is being refilled.

Bitrate calculation overflows when the filesize is multiplied by 8. The calculated bitrate is incorrect and very low. This may be seen in “Show track info”. The bitrate is used to calculate the watermark, resulting in an unreasonably low watermark.

I’m attaching a patch which fixes the calculation both in the metadata code and the FLAC codec. The metadata code is responsible for the above observations.

When changing the currently used backlight timeout setting, backlight_update_state() is called from outside backlight_thread(). That function call may lead to target specific backlight on/off and LCD enable/sleep/awake functions. If these functions allow other threads to run (eg. via sleep() or yield()), another concurrent call may happen from backlight_thread().

To see the problem on the 5G iPod, turn the backlight off by scrolling to “Off” on the currently used backlight timeout setting menu, wait for the backlight to fade out, and then turn on the backlight by scrolling down. Over half the time, brightness will be incorrect. For example if brightness is set to 5, the backlight may turn on at brightness 1. This is because of the sleep in _backlight_hw_enable() in backlight-nano_video.c. Other targets have sleep calls in code that runs for lcd_enable(true), and executing multiple copies of that concurrently can’t be good.

I discovered this while working on  FS#9890  (5G iPod LCD sleep). There I used a mutex to avoid lockups due to this issue. I see two solutions:
1) Use mutexes or other means to prevent problems in target-specific code
2) Ensure all calls of target-specific code happen from backlight_thread()

I prefer 2) because it is less prone to bugs.

(Behaviour confirmed with r20696)

Currently, the LCD sleep timer starts at the beginning of backlight PWM fade-out. This means that if LCD “Sleep (After Backlight Off)” is set to “Always” and “Backlight Fade Out” isn’t off, the LCD will go white at the beginning of the fade. This is ugly and unpleasant. A simple workaround is to set “Sleep (After Backlight Off)” to “5” (seconds) instead of “Always”. This attached patch fixes the problem.

This is a known issue with  FS#9890  iPod 5G LCD sleep. The patch is “lcd_sleep_after_pwm_fadeout.patch” from that task. However, it is actually a separate issue in platform independent code, so I guess it needs its own FS entry.

Note that a PWM fade can end in 3 different ways:
1) The LCD fades and the timer ISR handles the end.
2) Other code calls timer_register(), so the fade is interrupted, and backlight_release_timer() is called.
3) timer_register() fails.
Of these 1) is the only case which normally happens, but the patch handles all three.

(Behaviour confirmed and patch tested with r20696)

When playing 24 bit FLAC files I can hear some pops. For example, a 27 second 44100 Hz 24 bit FLAC file encoded by flac-1.2.1 has about 5 pops of varying intensity. They are always at the same positions. There are no pops when Winamp plays the same FLAC file or when Rockbox plays a 44100 Hz 24 bit WAV version of the same sound.

This is on my 5G 30GB iPod running r20636. The problem was first reported on 24/96 FLAC file on an iRiver iHP -120 by Kitlope at: http://forums.rockbox.org/index.php?topic=21255

I have a 5G 30GB iPod. Sometimes when I try to turn it on, nothing happens or there is just a brief flash of the low battery icon. After this, further attempts to power on normally do nothing, and the only way to turn on the iPod is a menu + select reset.

This is a known problem. For example, it is discussed in this thread: http://www.rockbox.org/mail/archive/rockbox-dev-archive-2009-02/0122.shtml Most people here believe this is a bug in Apple firmware in flash, and not a bug in Rockbox.

I created a small patch which shuts down Rockbox using the original firmware in flash. It writes a string to a specific location in IRAM and then resets the iPod. This causes the original firmware to act as if an attempt to boot failed. It assumes that the battery is low, and when a charger isn’t connected it shuts down. The patch seems to fix the problem. With it, I’ve never had to reset my iPod to turn it on.

I am attaching the patch as evidence that something can be done about the problem. I think the proper solution would would be to see what Rockbox needs to do and do that, instead of relying on the original firmware.

In practical terms, the patch only has has few minor disadvantages:
- there is a brief flash of the low battery icon after Rockbox shuts down
- attempts to turn on are ignored for a few seconds after Rockbox shuts down. (Just wait a few seconds after turning your iPod off before turning it on again. If you try to turn it on too soon, nothing will happen and you’ll have to do it again later, but you won’t have to do a menu+select reset.)
- If a charger is plugged immediately after the iPod is turned off, the ‘very low battery” screen is shown.

The patch could be extended to other PP502x iPods. I know the same problem happens on some other iPods, but I don’t know which ones, and I only have a 5G.

The main questions on my mind now: What state can be preserved when the iPod is turned off? What chips have power besides the LTC4066 and PCF50607?

Currently, the only sample rate supported on the 5G iPod is 44100 Hz. This patch adds support for all other rates which can be supported both by Rockbox and the codec. Supported rates are:
48000, 44100, 32000, 24000, 22050, 16000, 12000, 11025, 8000

The patch is based on the WM8983 datasheet, because the WM8758 datasheet is unavailable. The WM8983 supports sample rates between 8 and 48 kHz. It would be easy to compute settings for 96000, 88200 and 64000, but I think it’s best to not overclock the codec.

This is patch was inspired by problems with Doom sounds: http://forums.rockbox.org/index.php?topic=21111.0 . With this patch, a compile-time check makes Doom use 11025 instead of 44100 and then sounds work properly. However, there may be another underlying issue which needs to be looked at: sound from plugins may underrun too easily. In test_sampr, merely scrolling through a list causes underruns.

This does absolutely nothing for standard audio playback. There doesn’t seem to be any sample rate changing support there, and so software resamples to 44100 if necessary.

The same process which sets playback sample rates also sets recording sample rates. I have enabled use of all of the new sample rates for recording. I haven’t been able to test this however, because I don’t have a line in adapter. Playback sample rate changing may be tested via the test_sampr plugin. To build it, add test_sampr.c it to apps/plugins/SOURCES.

USB high speed mode never worked for me. When I add this initialization, it always works. A USB controller reset undoes this initialization, so I put it at the end of usb_drv_reset() in usb-drv-arc.c.

WARNINGS:
* I recommend making sure you have backup copies of files on your iPod before using Rockbox USB mass storage mode. (I have not encountered any problems; I just think it’s the responsible thing to do.)
* USB charging on iPods may not work or may be limited to 100 mA. See  FS#8802 . Your battery may discharge while the disk is being used. This may empty it, cause a shutdown in the middle of disk activity and result in filesystem problems. Note that the status line displays battery status even in USB mode.

IMPORTANT: Run “make clean” after patching. If old files are present, changes to the config header won’t automatically change the list of features, so lang.* won’t be updated and the build will fail.

Here is code which enables LCD sleep on the 5G iPod.
lcd_enable(false) puts the LCD to sleep and powers down the BCM.
lcd_enable(true) bootstraps the BCM and wakes up the LCD
These are used via platform-independent code relating to HAVE_LCD_ENABLE, HAVE_LCD_SLEEP and define HAVE_LCD_SLEEP_SETTING. As a result, in “Settings → General Settings → Display → LCD Settings” there is a new option, “Sleep (After Backlight Off)”, which allows you to set when the LCD goes to sleep. The default is 10 seconds.

I have also defined HAVE_LCD_SHUTDOWN and used lcd_enable(false) as lcd_shutdown(). I think putting the LCD to sleep before cutting power might be better than just clearing it.

I think concurrency-related problems won’t happen because LCD update calls never yield, lcd_state.display_on blocks them when lcd_enable yields, and lcd_enable is only called from backlight_thread. I’m tempted to wrap the LCD in a mutex but I think that may be unnecessary because other targets don’t do that. I guess LCD calls aren’t supposed to come from the COP.

Wakeup is not instantaneous because of the BCM initialization procedure. I have shortened some sleeps to speed it up. (It would be faster if the BCM kept running and only the LCD was put to sleep. Any LCD update command then automatically wakes the LCD. That would save less power.) (BTW. It seems the LCD framebuffer is not in BCM memory. The BCM merely uploads data to the LCD, and the LCD can independently keep displaying that image even if the BCM is turned off.)

I have changed the LCD update BCM command from 5 (0xFFFA0005) to 0 (0xFFFF0000). The former only updates a specified rectangle, and the latter updates the whole screen. The rectangle was always set to the whole screen, and this allows bcm_setup_rect to be removed. (For reference, these are words at 0xE0000 for command 5: 0×34, x, y, x + width - 1, y + height - 1, width * height * 2, 0 , 0. I wonder if that 0×34 means the command can also do other things.)

I feel more work is needed in firmware/backlight.c, some of which isn’t platform-dependent:
- If sleep is set ot “Always”, the LCD sleeps when the fade starts, which is ugly. I tried to fix this but something weird is going on, like the fade-in never finished, maybe because of timer_set_period(0) when bl_dim_current reaches BL_PWM_COUNT.
- Backlight settings changes led to backlight_update_state() calls outside of backlight_thread(). This led to multiple concurrent lcd_enable operations when lcd_enable yields. I replaced those with backlight_on().
- It would be nice if the user could wake the LCD if the backlight is always off and LCD sleep isn’t “Always” - It would be nice if the the backlight on hold settings were extended to “LCD on hold”.

It might be possible to power off the LCD instead of just putting it to sleep. It also might be possible to shut off the PCF5060X_D3REGC1 supply. This requires additional research.

When moving files or directories by cutting and pasting, if the destination exists I am asked if I want to overwrite. If I say yes, the operation fails. This is because the rename fails because the destination exists. Copying and pasting works properly.

The attached patch fixes the problem by deleting the target if it exists just before the rename. (At this point the user has already agreed to overwrite the target.)

This was tested with r19907 on my 30GB 5G iPod. It should not depend on the player type.

I have a 5th generation 30GB iPod with a stock MK3008GAL hard drive. If the disk is off when plugging in the USB cable, I hear the click-whine of the emergency head retract when rebooting. This should probably be avoided because it may wear out the disk more than normal head retracts.

In drivers/ata.c in ata_thread() when responding to SYS_USB_CONNECTED, the disk is turned on if it was off. However, no variables are changed to let ata.c know that the disk has been turned on, and so ata_sleepnow() won’t put it to sleep. Another consequence would be that settings don’t get saved. The disk is also not woken up if it was powered but sleeping, and this may be another bug. I am attaching a patch for these issues.

I am also wondering what’s the point of spinning up the disk. I think it only needs to be spun up if settings need to be saved.

I have a 30GB 5th generation iPod with a stock MK3008GAL hard drive. When starting Rockbox, I hear two clicks while the logo is displayed. It seems like the heads are unloaded and then loaded again. This delays startup a bit.

The unload is triggered when DEV_IDE0 is reset in system_init() in firmware/target/arm/system-pp502x.c. The load happens later when needed. From an experiment I found that resetting DEV_IDE0 unfreezes the lock, and the ATA standard says that can only happen if the device is powered down or hard reset. Because of this I think resetting DEV_IDE0 hard resets the ATA device.

DEV_IDE0 resets also seem to reset some host settings because if done after ata_device_init() (which initializes the host, not the device) they break DMA ( FS#9708 ). The following implementation of ata_reset() in firmware/target/arm/ata-pp5020.c works:
DEV_RS = DEV_IDE0;
DEV_RS = 0;
ata_device_init();
However, since everything currently works with an ata_reset() which does nothing, I am not submitting a patch for that.

I am submitting two patches to disable DEV_IDE0 resetting. One disables it only on the Video iPod and one disables it on all HD based pp502x targets. I have only tested this on my Video iPod, so it’s up to others to see if this works and speeds up startup on other targets.

This was started at: http://forums.rockbox.org/index.php?topic=19788.0

With r19629-081231, the disk isn’t spinning down during playback. It does sometimes spin down properly at other times.

This happens on both my 30 gig 5th generation video iPod and V2 Recorder.

This patch allows battery current to be viewed at the debug screen. It adds another channel in firmware/target/arm/ipod/adc-ipod-pcf.c. In “View I/O ports”, the raw value is displayed as 4066_ISTAT. In the “Power status” screen of “View battery” the value scaled to milliamps and power in milliwatts is displayed. The current is flowing into the battery when charging and out of the battery when discharging. When external power is supplied and the battery is charged, current is zero.

Note that current is dependent on battery voltage, because at lower voltages the DC-DC converters need more input current for the same output power. That’s why I chose to also display power (based on Vbat * Ibat). That may still be slightly wrong if Vbat changed between readings, and DC-DC converter efficiency may change as voltage changes. It’s probably best to compare current on the same device at the same Vbat.

These values are also displayed in the iPod diagnostic, in Power → A2DTests → ChargingADC. I did not verify the accuracy of the scale factor; I just use the same one. The name 4066_ISTAT implies that this reading comes from the ISTAT pin of the LTC4066 chip.

In ata_write_sectors/_write_sectors in firmware/drivers/ata.c, the only check at the end is wait_for_end_of_transfer(). That function only checks that BSY, RDY (DRDY) and DRQ. This only makes sure that the command finished, not that it succeeded.

The ATA standard defines normal outputs like these for WRITE SECTOR(S) and other data transfer commands:

Device register -

DEV shall indicate the selected device.

Status register -

BSY shall be cleared to zero indicating command completion.
DRDY shall be set to one.
DF (Device Fault) shall be cleared to zero.
DRQ shall be cleared to zero.
ERR shall be cleared to zero.

I don’t see why the device register should be checked. The status register should be checked however:
* BSY because if set, the command isn’t done and the other status bits are invalid. Check is probably redundant, but it’s free.
* DF because it is set for some errors which don’t correspond to error register bits.
* DRQ because if set, more data is left to transfer
* ERR obviously

I don’t see how DRDY cleared to zero would imply the current command failed, but the standard says it should be set and the current code checks it, so why not check it.

I’m attaching a simple patch. Note that wait_for_end_of_transfer() is also used at the end of ata_read_sectors/_read_sectors. Errors should be posted at the beginning of READ MULTIPLE blocks and the function checks for them properly. However, I see no harm in checking at the end also.

The read function does retries and the write function doesn’t. Some write failures can lead to panicf in firmware/drivers/fat.c. Some users may feel the panic makes things worse. If this becomes a problem I have two ideas:
* Instead of panic, stop doing writes. (Some operating systems remount partitions read-only after some errors.)
* Do retries for writes. I think it would be best to do this by merging read and write functions.

I got Ultra DMA working on my 5th generation 30 gig iPod. Mode 2 (33.3 MB/s) seems stable, and it doesn’t need CPU frequency boost. Mode 4 (66.7 MB/s) is the highest supported, but modes above 2 require CPU boosting.

The main benefit I can see from DMA is somewhat faster buffer filling. For example, with one large unfragmented FLAC file and the FS#9621 FAT read-ahead patch, the initial load is about twice as fast with DMA. Test_disk says 1 meg reads are 14102 KB/s and writes aren’t sped up. I suspect the reported read speed is from the drive’s cache, not the media. (BTW. Why are PIO reads slower than PIO writes in test_disk?)

The main cost of DMA is having to flush and invalidate the cache. I note that pp5020.h defines a CACHE_FLUSH_MASK, which makes me think it may be possible to only affect part of the cache. However, I don’t know how to use that yet.

While implementing this I significantly altered firmware/drivers/ata.c. I merged the read and write functions into a transfer function which can do both reads and writes with both PIO and DMA. This removes some code duplication and IMHO enhances writes by using “write multiple” and doing retries if needed. I plan to make other changes to ata.c and add interrupt support. However, I feel the right thing to do is to redo the patch with minimum modifications to ata.c. The ata.c changes can become another, separate patch.

Due to interest shown on the developer mailing list, I am submitting this preliminary patch so I can get it out ASAP. I know it needs to be cleaned up, and it won’t compile correctly for other targets, and things like detection of DMA modes need to be added. I’m sorry if this bothers you. I will release a cleaned up patch.

Note that messing with disk-related code can lead to filesystem corruption, losing all of your files, and having to restore your iPod. If you’re concerned about this, you may want to at first disable writes at “//if (write)” in ata.c. I didn’t have any problems, the test_disk write verify test passed, single sector transfers (like the FAT) are still done via PIO, and Ultra DMA CRC-checks DMA bursts.

It seems that in the past there was an attempt to load up to two cue sheets into memory, using curr_cue for the current track, and temp_cue for an upcoming track. Currently temp_cue is allocated and never loaded with data, wasting 70k. In gwps-common.c, the current filename is compared with uninitialized memory in temp_cue, which probably doesn’t cause problems but seems like a bad idea.

In this patch I’ve removed code relating to temp_cue.

On my V2 Recorder, the backlight sometimes turns off instantly and sometimes brightens instantly and then slowly fades. This patch makes it always turn off instantly. See comments in patch for explanation.

I notice that the first version Recorder has the same backlight code, but I don’t know if it has the same hardware issue, so I did not alter that code.

Some FLAC+CUE albums on my 5G 30 gig Video iPod hung on “Loading…” every time I tried to play them from a stopped state. The workaround was to play them while another FLAC or MP3 file was playing. (I didn’t try with other types.)

Disabling CUE support or moving the CUE file elsewhere also made them play properly. I tried to experiment with CUE file content and I got confusing and inconclusive results. However, replacing the CUE file with an identical copy fixed the problem.

I never got read errors on original CUE file and the file could be viewed. I just scanned the hard drive for file system and read errors with “chkdsk /r” on Windows Vista and no errors were found. SMART data in iPod diagnostics shows 0 pending sectors and 0 reallocs.

I am currently running r19090. The problem was also present in r18760.

Several parts of Rockbox have large arrays which are by default not used. They include at least:
- 64k for codepage_table in firmware/common/unicode.c, which is not used for the default ISO_8859_1 codepage or UTF_8.
- 21000 byte language_buffer in apps/language.c, which is not used if the compiled in language (normally English) is used.
- 10000 bytes for the font, in firmware/font.c, which is not used if just the compiled-in font is used.

Such things could be allocated in the buffer when they’re first needed. This would leave more buffer free for those who don’t use these features. This may only be worth it on Archos platforms with 2 megs of RAM, and even there I’m not sure because even the 8 MB mod supposedly only increases runtime by 15-30%.

The only problem I can see is that buffer_alloc conflicts with MPEG data in the buffer. Cuesheet and last.fm scrobbler code uses buffer_alloc and there a reboot is required. It wouldn’t be too unreasonable to require a reboot before loading another language or codepage, though it might be unreasonable before loading another font. I don’t think it’s necessary though; playing just needs to stop.

(Before I got this idea I experimented with disabling font and codepage loading at compile time and I have 1.733 MB free for the buffer.)

Based on current measurements charging appears to work properly even when Rockbox is locked up. The following issues are just with the charging screen, so I would consider this low severity. (I can’t set severity, I see “CriticalHighMediumLowVery Low” by the listbox instead of in it.)

If I plug in a charger while my V2 Recorder is off I get the charging screen, a brief flash of the Rockbox splash screen, and then the charging screen again. The voltage is <0.1 V but otherwise the screen appears fine, with the battery filling animation at the top and the wave animation in the large battery. After the battery animation at the top fills 5 times, all animation stops and the only thing I can do is hold the OFF button to shut down. At this point I can either disconnect the charger to turn off the device and leave it off, or hold OFF to turn off, after which the process repeats.

If I press ON while the charging animations are still moving I get a brief flash of the splash screen, a brief flash of “ATA error: -11 press ON to debug” and then the I/O ports debug screen. The battery voltage is again <0.1 V. If I attempt to get out of this screen I get a “*PANIC* ata: -11”. If I stay at the screen, it locks up. If instead of pressing ON I hold ON, the “ATA error” screen stays and I need to press ON again to get to the I/O ports screen.

The BootBox charging screen (accessed via holding F1 when plugging in the charger) also reports an incorrect battery voltage. Initially it is 0.02 V or so and it soon becomes 0.00 V. However, the screen doesn’t seem locked up and if I press ON that leads to a normal rescue boot.

Charging works properly if the charger or USB cable are plugged in after Rockbox is running. Battery voltage measurement also works. Charger detection on the “power status” debug screen (”view battery”, press UP) works though the external voltage measurement is questionable because it starts at >6V, higher than the open circuit voltage on the charger, and goes down to 4.3 or so.

This was tested using build r14396, with rockbox.ucl flashed. BootBox was from http://www.rockbox.org/twiki/bin/viewfile/Main/BootBox?rev=1.1;filename=bootbox_v2.zip and the charger was an Archos 102168 car charger.

I just realized the battery was pretty full now, on the constant voltage topping charge. I will try again when the battery is empty and the battery voltage while charging is lower, in case the lockup happens when Rockbox thinks the battery is full. It shouldn’t matter though if Rockbox can’t measure the voltage.