Rockbox

Tasklist

FS#10185 - rbutil : bootloader support for sansa AMS targets

Attached to Project: Rockbox
Opened by Dominik Wenger (Domonoky) - Sunday, 03 May 2009, 15:13 GMT
Last edited by Dominik Wenger (Domonoky) - Saturday, 15 August 2009, 13:05 GMT
Task Type Patches
Category Rbutil
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch is the beginning of bootloader support for AMS targets in rbutil.

The modifications needed, will go into different patches.
- General Bootloader class rework to better support the ams targets.
- Modifications to mkamsboot, so its better to use in rbutil
- BootloaderInstallAMS class.
- rbutil.ini modifications for the new targets.

First patch: general Bootloader class rework:
- renamed NeedsFlashing into NeedsOF and moved the Hint for this into the Bootloader classes, also setfunction for OF file is now in the base class.
- Enforce proper parents for all bootloaderinstall classes
- Make some functions, which must be implemented, abstract.
- move the postInstallHints to the child classes (and non-static)

Next patches will follow.
This task depends upon

Closed by  Dominik Wenger (Domonoky)
Saturday, 15 August 2009, 13:05 GMT
Reason for closing:  Accepted
Comment by Thomas Martitz (kugel.) - Sunday, 03 May 2009, 15:27 GMT
I'm wondering why we can't have rbutil downloading the OF. They're distributed on Bagder's site, aren't they?
Comment by MichaelGiacomelli (saratoga) - Sunday, 03 May 2009, 16:08 GMT Comment by Dominik Wenger (Domonoky) - Sunday, 03 May 2009, 16:40 GMT
We cant distribut the original Firmwares, as we dont have a licence to distribute them.
What Badger does on his own Site is his own problem. :-)

And we also request the User for a OF for the Hxx0 installs, so that shouldnt be a problem.
Comment by Thomas Martitz (kugel.) - Sunday, 03 May 2009, 17:23 GMT
We can still make rbutil download it, not?
Comment by Dominik Riebeling (bluebrother) - Sunday, 03 May 2009, 18:08 GMT
- I'm not sure if renaming NeedsFlashing is really needed -- we put a file on the player that gets flashed, so that term isn't wrong. OTOH, the X5 / M5 / M3 also flashes a file and it doesn't require an OF file, so installation handled by the rbutil file class, so NeedsOf might more exact.
- I disagree with moving the postInstallHints to the installation classes. The idea behind making it a static function was that the installation classes should *not* know (and not need to know) about the player model they are installing. Ideally the postInstallHints would get moved out of the bootloader classes completely, but having it in the base class as static function is a compromise for better reuse once we get a command line interface. We could, however, consider a general "hints" class that only returns such strings.
Comment by Dominik Wenger (Domonoky) - Sunday, 03 May 2009, 19:08 GMT
- I think the renaming is needed, to make clear what it really does. This flag does nothing Flashing specific, but states that it needs a OF File.
- this together with the ofHint in the class, makes it easier to add similar classes.
- about the postInstallHint, i agree that it should be made static again. (maybe a comment could have made the intend of this clearer :-) ).

New patch in this series: Modifications to mkamsboot.
- The code was reorganised so the main consitsts now of only a few steps.
- load, check and pack the of,
- load, check and pack the bootloader
- patch it
- write out.
- There may be a few more functions needed to really do the same in rbutil

Work left for this:
- it needs to be built to a lib, excluding the main()
- The builtin dualboot code should be in svn as binary (so you dont need the arm-gcc to build rbutil)


Comment by Dominik Wenger (Domonoky) - Saturday, 09 May 2009, 19:09 GMT
Updates:
- I commited the general bootloader class changes with the changes bluebrother suggested.

New patch for mkamsboot:
- It can now be built as lib, but there is still the problem of the needed arm binarys. (currently i commented the arm rules out, so you need prebuild dualboot.o files)
Maybe some Makefile guru can help us out to improve this ?
- Also more Makefile magic is needed for mac os X (ar has problems with multiplatform .o files (see rbspeex makefile) ). This also applies to tools/ucl/src/libucl.a.

New patch: InstallBootloaderAms Patch:
- It correctly makes a patched bootloader. But as there is no Bootloader on the download server it asks the user ( addition GUI depency, needs to be removed, when binarys are available).

New patch: Sample modifications for rbutil.ini to add e200v2 target for testing.
Comment by Dominik Riebeling (bluebrother) - Saturday, 09 May 2009, 19:50 GMT
Comments on the rbutil patch:
- why the ofHint() in the bootloader class? That should go into rbutilqt.cpp as done for the "hex" install type.
- the bootloader install class must NOT utilitze any GUI elements (you #include <QtGui> in base/bootloaderinstallams.h (!) and use a file chooser). The file provided by the user has to get handled by the caller, i.e. in rbutilqt.cpp
- it would be good if you'd try to limit lines to around 80 characters. While rbutil deviates from the hard 80 characters limit of main Rockbox it would be good to at least try to follow it if possible. I'm fine with ignoring it in places where it doesn't make sense (but in such cases it shouldn'd get exceeded too much).
Comment by Rafaël Carré (funman) - Thursday, 28 May 2009, 13:26 GMT
I have merged your work on mkamsboot with other diffs in  FS#10253 
Comment by Dominik Wenger (Domonoky) - Tuesday, 02 June 2009, 19:53 GMT

New patch, adapted to mkamsboot in svn.
Also included are the modifications to rbutil.ini to add the e200v2 target. Other AMS Targets only need similar entrys.

The code still includes the hack, to work around the non-existing bootloader on the server. But aside from that, it should be commitable.

mkamsboot and libucl probably still need some Makefile work to build as universal archive on Mac.
Comment by Rafaël Carré (funman) - Tuesday, 02 June 2009, 20:02 GMT
Thanks!

+ emit logItem("No room to insert bootloader",LOGERROR);
Perhaps should be
+ emit logItem("No room to insert bootloader, try another firmware version",LOGERROR);

and
+ emit logItem("Uninstallation not possible, only installation info removed", LOGINFO);
+ emit logItem("To uninstall, perform a firmware upgrade with an unmodified original firmware", LOGINFO);

Do you want a patch with these diffs (provided you agree) and .ini entries for Fuzev1 Clipv1 and m200v4 ?
Comment by Dominik Wenger (Domonoky) - Tuesday, 02 June 2009, 20:47 GMT

Yes a new patch with this modifications would be nice.
And i agree, that those texts are better.
Comment by Rafaël Carré (funman) - Tuesday, 02 June 2009, 20:54 GMT
will do.

Also I wonder what happens if the user selects an original firmware not supported by mkamsboot (released after rbutil was built for example).

Will the error message only go to stderr or will it be displayed in GUI ?

EDIT: uh oh when I merged your patch I removed r21073 : "mkamsboot: really error out if OF model is different from bootloader model" , so untested firmwares will get accepted anyway.
Comment by Hilton Shumway (HIllshum) - Tuesday, 02 June 2009, 20:59 GMT
It should send that error to the GUI
Comment by Dominik Riebeling (bluebrother) - Tuesday, 02 June 2009, 21:54 GMT
a few comments:
- the 'extern "C"' should be in the header, not the cpp file to avoid forgetting it when including from another file. #ifdef __cplusplus is your friend here.
- the patch contains unrelated changes (apps/plugins/SOURCES)

Hilton: "I wonder what happens" does NOT mean "how should it behave". Not notifying the user about an error is is out of question, but how does this implementation behave if this situation occurs? I can't see some general error-catching in the code but maybe I'm too tired right now.
Comment by Rafaël Carré (funman) - Tuesday, 02 June 2009, 22:43 GMT
Apply mentioned changes.

I didn't add m200v4 USB ids.
Also I suppose "usberror" entry is for detecting if the device is in MTP mode ?

What about "usbincompat" ? e200v1 is "incompatible" with the Sansa Fuze..
Comment by Dominik Wenger (Domonoky) - Wednesday, 03 June 2009, 16:25 GMT
Yes, the extern "C" could go into the mkamsboot header. But i dont see that as a big problem.
And about the error handling: did anyone here read the code ? It Outputs the same as mkamsboot in svn. Instead of stderr it uses the gui progresslogger.

About the usb-ids: Yes usberror is for a device in a wrong mode, and usbincompat for inkompatible Devices, for example a Ipod 6gen.

Thanks for the patch.
Comment by Rafaël Carré (funman) - Thursday, 04 June 2009, 16:40 GMT
As suggested by Llorean on http://www.rockbox.org/mail/archive/rockbox-dev-archive-2009-06/0012.shtml , we could download a list of tested OF with their md5sums, rather than having the list built in mkamsboot.

That way rbutil could support OF tested after its compilation (new OF releases for example).

I am not sure which format we could use for this purpose: I suppose a simple text file would be easier to parse, something like
e200v2 3.01.11 1 e622ca8cb6df423f54b8b39628a1f0a3\n
fuze 1.01.11 1 cac8ffa03c599330ac02c4d41de66166\n

(model name, OF version, firmware file format, md5sum, all spaces separated and terminated by new line)
Comment by Dominik Wenger (Domonoky) - Thursday, 04 June 2009, 17:33 GMT
Yes, downloading the list of md5sums should be no problem.

About the content of the file:
we should probably provide the same info as mkamsboot already uses in the md5sum struct, then its easier to modify mkamsboot for it.
(mkamsboot standalone uses builtin list, and rbutil provides its own downloaded list)
So we should use the enum number instead of the modelname. (or maybe additionally)

About the format:
Your proposed format surely would work, and would be easy to parse. But even easier would be a ini format, because Qt has builtin support for ini (no parsing needed).
So something like:

[1]
modelname = e200v2
modelenum = 3
version = 3.0.1.11
fwversion = 1
md5sum = e622ca8cb6df423f54b8b39628a1f0a3
[2]
modelname = e200v2
modelenum = 3
version = 3.0.1.14
fwversion = 1
md5sum = 2c1d0383fc3584b2cc83ba8cc2243af6
[3]
....

Loading...