Rockbox

Are you sure changes like this (in mkamsboot) are correct?

- put_uint32le(&buf[0×04], sum);
- put_uint32le(&buf[0×204], sum);
+ put_uint32le(sum, &buf[0×04]);
+ put_uint32le(sum, &buf[0×204]);

Besides, separating the mkamsboot Makefile changes (which are not required for adding D2 support to rbutil at all) into its own patch would make it noticably easier to review the diff. Same goes for consolidating functions in mkboot_common and probably some more. Please keep separate changes separate.

- Fix AMS makefile and make it more readable.

I would prefer that mkamsboot not be changed just for readability. Incorrect changes to mkamsboot would not just be a minor inconvienience, it could BRICK devices. Since the majority of the AMS targets do not have a recovery mode, changes to mkamsboot should be made only by developers with intimate knowledge of the utility (i.e. linuxstb, funman).

Thanks for having a go at this - I was too lazy myself

I agree with the above comments that the AMS changes should be done first in a separate patch if they’re necessary, that way it becomes easier to review and make sure everything is correct.

Regarding the D2 specific things:

- This will only detect one of the many D2/D2+ USB Ids (see the DeviceDetection wiki page for all the others)
- How does it handle the fact that the firmware .bin needs to be copied to the root of the internal memory, but .rockbox should be installed to an SD card if one is present (as per the wiki instructions)?
- Several places in the code say things like “A tool to inject a bootloader into a Cowon D2 (tcc) firmware file”. mktccboot is rather generic, so it should say “A tool to inject a bootloader into a Telechips 77X/78X firmware file.” - The D2 is not an iAudio branded player, so the bootloader path shouldn’t be /iaudio/cowon_d2.bin, it should rather be something like /cowon/d2/bootloader.bin

EDIT: Also note that you can’t brick a D2, since USB-boot mode (tcctool) is always available. The AMS targets need a little more care in that respect….

EDIT2: It might also be worth detecting and showing an error if a D2 is connected in USB-boot mode (again the USB Id is on the DeviceDetection wiki), it does happen to me by mistake sometimes.

- Remove dependency of ipodpatcher with mkboot_common
- Correct remarks according to Rob Purchase’ comment
- Changed location of d2.bin in download server to /cowon/d2.bin (waiting for actual file relocation in server)
- Added all known Cowon D2/D2+ USB ID according to DeviceDetection wiki page
- Revert all changes of mkamsboot due to the sensitivity involved with bricking AMS devices, and focusing only on adding D2 to rbutil.

This will be done later, with the help of AMS maintainers.

- There is no known method of detecting and choosing the SD card if one is present. I think that we can start with having a pop-up advising the user to use SD card to install Rockbox on, but this is not in the scope of the current patch.

I would like to thank all those who reviewed my code!

   cowon_d2_rbutil_v2.diff (36.5 KiB)

There’s still some ipodpatcher changes in there, is that intentional?

Also, about the download path, I think sticking to an existing convention is best: either my earlier suggestion (consistent with the Gigabeat S: toshiba/gigabeat-s/bootloader.bin) or similar to the iRivers/iPods (eg. cowon/bootloader-d2.bin). Not that it really makes much difference

Note that I haven’t built or tested it yet, I just looked at the code. I’ll try it out later!

I still think that the mkboot_common changes should go into separate patch than adding D2 support to rbutil. Furthermore, IMO the name “mkboot_common” is slightly misleading – it’s holding some common helper function (like retrieving the filesize) and used by tools like ipodpatcher which by itself does not create a bootloader (image). Putting it in the already existing rbutil/tools might be more sensible (and perhaps call it “helpers.c” or similar?)

Something completely different: I’m not sure how assigning copyright has to be handled when moving code, but that mkboot_common stuff is definitely not © 2009. Do we have a policy for such situations?

I agree with the other comments - this patch is combining far too many unrelated things.

Some things also seem unnecessary, such as creating mkboot_common - the code is so trivial, that IMO it’s not worthwhile creating external dependencies (i.e. outside the tool’s own source directory) just to share that code. I don’t want ipodpatcher to be dependent on files outside of its own source directory.

There are also some strange changes to (C) messages - e.g. the text in the header in mktccboot.c has changed from:

“Copyright (C) 2007 by Dave Chapman. Based on mkboot, Copyright (C) 2005 by Linus Nielsen Feltzing”

to:

“Copyright (C) 2007 by Dave Chapman. Based on mkamsboot, Copyright (C) 2008 by Dave Chapman”

It’s impossible to see what’s changed with mktccboot from your patch - IMO it would be better to move mktccboot.c in SVN (but without changing it), and then post a patch with your changes.

- Remove mkboot_common
- Fix file copyright title (wrong year)
- Make rbutil/mktccboot/mktccboot.c more similar to tools/mktccboot.c (I don’t think we should do this in two steps - moving and then modifying)
- Remove future enhancment to patch process (check OF consistency) which was commented out

   cowon_d2_rbutil_v3.diff (31 KiB)

Following a discussion in IRC earlier today, I’ve moved mktccboot (without changing mktccboot.c itself) into a rbutil/mktccboot/ as r23382. If you can post an updated patch against current SVN, I think it will be easier for people to review your changes.

Thanks.

Updated patch, synched to r23382

   cowon_d2_rbutil_v4.diff (22.6 KiB)