Rockbox

Tasklist

FS#10728 - Cowon D2: Add support for D2 in rbutil

Attached to Project: Rockbox
Opened by Tomer Shalev (tomers) - Monday, 26 October 2009, 22:00 GMT
Last edited by Tomer Shalev (tomers) - Thursday, 29 October 2009, 21:32 GMT
Task Type Patches
Category Rbutil
Status Closed
Assigned To Daniel Stenberg (bagder)
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch adds D2 support in Rockbox Utility.

I've compiled successfully on Linux. Needs further testing on Windows and Mac OS.
I used the modified command line tool mktccboot, and diffed its binary output with the output created by the unmodified script, and the files were the same.

There is some code similarities for ams and tcc makefiles, and rbutil classes, which can be unified in the future.

Please review the code and approve, so I can commit it soon.
This task depends upon

Closed by  Tomer Shalev (tomers)
Thursday, 29 October 2009, 21:32 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed in r23410.
Comment by Dominik Riebeling (bluebrother) - Monday, 26 October 2009, 23:08 GMT
Are you sure changes like this (in mkamsboot) are correct?

- put_uint32le(&buf[0x04], sum);
- put_uint32le(&buf[0x204], sum);
+ put_uint32le(sum, &buf[0x04]);
+ put_uint32le(sum, &buf[0x204]);

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.
Comment by Michael Chicoine (mc2739) - Tuesday, 27 October 2009, 11:31 GMT
>- 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).
Comment by Rob Purchase (shotofadds) - Tuesday, 27 October 2009, 13:47 GMT
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.
Comment by Tomer Shalev (tomers) - Tuesday, 27 October 2009, 17:33 GMT
- 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!
Comment by Rob Purchase (shotofadds) - Tuesday, 27 October 2009, 17:52 GMT
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!
Comment by Dominik Riebeling (bluebrother) - Tuesday, 27 October 2009, 18:19 GMT
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 (c) 2009. Do we have a policy for such situations?
Comment by Dave Chapman (linuxstb) - Tuesday, 27 October 2009, 22:02 GMT
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.
Comment by Tomer Shalev (tomers) - Wednesday, 28 October 2009, 20:28 GMT
- 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
Comment by Dave Chapman (linuxstb) - Wednesday, 28 October 2009, 22:53 GMT
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.
Comment by Tomer Shalev (tomers) - Thursday, 29 October 2009, 05:23 GMT
Updated patch, synched to r23382

Loading...