Rockbox

so for the moment the only goal is to have it compiling so that I could test code… by now the problem is: https://gist.github.com/1389121 that can be turned around once by adding:
#ifndef MKIMXDUALBOOT_H
#define MKIMXDUALBOOT_H

#ifdef cplusplus
extern “C” {
#endif #ifdef cplusplus
};
#endif
#endif

to the file rbutil/mkimxboot/bootloader.h
but:
1) rbutil/mkamsboot/bootloader.h file doesn’t have any of this so I must be missing something… 2) it will compile only once and will end with: https://gist.github.com/1388994 on any other build attempt

   imx-rbutil.diff (4.6 KiB)

ok I made a cleaner diff
the 2 new files are probably wrong but should compile

   imx-rbutil.diff (4.6 KiB)

this is the new file

   imx-rbutil2.diff (9.9 KiB)

The problem is that the rbutil build system puts all object files for libraries in the same directory, so the dualboot.o fron mkamsboot overwrites the one from mkimxboot.

I’ve updated your patch to fix this. I’m not entirely happy with the fix ($LIBS gets a bit too long for my tastes), but everything compiles

   imx-rbutil3.diff (11.4 KiB)

Frank: the problem is not the rbutil build system, the problem is that all externally built libs put their objects into a subfolder of the build folder called “build”. You can simply change that in the mkimxboot Makefile (lines 38 / 40).

Jean-Louis: I’ll try to give this patch a closer look tonight. Please try to avoid adding trailing spaces :)

Dominik: yes, but I’ll claim that most of those internal tools sit under rbutil/ ;)

But yes, that is indeed the underlying problem. I think they all need to be updated to put their objects somewhere more specific. Just changing mkimxboot will fix this particular issue, but we should fix this once and for all.

anyway I had to edit this rbutil/rbutilqt/rbutilqt.pro file in order to add the library link so I guess that if people what to add new tools they will just do as I did: doing the same as others already did on other methods. From now with the correct rbutilqt.pro file and with an updated wiki (that i’m going to do) it should then be ok, isn’t it?

So now I can compile, thanks very much to both of you! :)
For what I understood from the code: installation methods are supposed to follow a quite thin path: take the bootloader download path and OF local path that rbutil give then throught Bootloaderinstallbase class and then do whatever it takes to patch and install. As the bootloader isn’t yet on the server (pamaury would like to wait a little to be sure it doesn’t need any change) I was supposed to implement something asking the user for the 2 files (bootloader + OF). The methods doesn’t seems to work like that. Should I implement something on a upper level, to have bootloaderbase’s class able to handle it? Or should I just I just wait for the bootlloader to be on the server?

I don’t think it’s a good idea to add additional code just because the file isn’t uploaded to the server. Override the download and read the file from local disk, adding more GUI is simply wasted work because it won’t be present anymore once the file is in place.

I’ve started working on restructuring the lib Makefiles to have a proper way of avoiding the filename clash.

I saw you did a lot of work on the tree is it ready then? Can I go on? Or are you planning further modification?

So i’m starting again after the whole remake. I had to adapt a few things. Still I’ve got some weird error with the dualboot part of mkimxboot. I just don’t get everything seems consistent with mkamsboot which also has such a part. No way to have it compiling:
rbutil/rbutilqt/build//libmkimxboot.a(mkimxboot.o):(.rodata+0×68): undefined reference to `dualboot_fuzeplus’ collect2: ld returned 1 exit status
make: *** [RockboxUtility] Erreur 1

   rbutil-imx-v4.diff (8.5 KiB)

I’ve committed the first part of changes to the Makefiles that should fix the issue with duplicate source file names, which is likely to be the dualboot_fuzeplus issue (r31261).

well with the v4 patch I still get the very same dualboot_fuzeplus issue (r31266).

/home/jean-louis/Bureau/rockbox-devtree/rbutil-mkinit/rockbox/rbutil/rbutilqt/build//libmkimxboot.a(mkimxboot.o):(.rodata+0×68): undefined reference to `dualboot_fuzeplus’ collect2: ld returned 1 exit status
make: *** [RockboxUtility] Erreur 1

I don’t get such an issue – have you cleaned your build tree? Especially make sure to remove the build folder (in case you’re building in-tree), since a make clean won’t remove the objects and libraries build during the process.

right! I reinstalled the tree and now it works… Funny because I did erase the content of build… Anyway thanks very much!
I noticed a warning during compilation:
base/system.cpp: In static member function ‘static QMap<unsigned int, QString> System::listUsbDevices()’:
base/system.cpp:249:9: warning: variable ‘res’ set but not used [-Wunused-but-set-variable]
base/system.cpp:250:13: warning: variable ‘count’ set but not used [-Wunused-but-set-variable]

So here is the patch. For testing purpose the first section of the modurl patch is changing the url of the serveur to my public dropbox. The treereleease is free of this section:
- I tested the file upgrading my device with and it works.
- anyway I wasn’t able to have the same md5sum as with the standalone mkimxboot. The std output while running rbutil from console give also strange stuf at the end. I suppose the this is related to the debug fonction but I’m not quite sure about it… It would be wise that someone check that…

   rbutil-imx-v5-treerelease.diff (10 KiB)
   rbutil-imx-v5-modurl.diff (10.5 KiB)

so I made some improvements:
- add more debug info and display more info while installing
- remove an unused include
- correct url in the hint that was wrong
- I noticed that I often get random error on patching or even moving the files! Sometime it works flawlessly, sometime not. Very strange… But this seems to works everytime on the first attemps. I suspect some temporary files are messing up

   rbutil-imx-v6-modurl.diff (10.5 KiB)
   rbutil-imx-v6-treerelease.diff (9.9 KiB)

I’ve made some improvement and simplification and will post them soon

so here a new patch:
- the problem with random error is solved: it was related to the name of the temporary file writed to disk before being copied to the device. No more errors now.
- Another bug was related to the fact that and existing bootloader file on the device was rename to .orig. But if such a file also already exist it was leading to a copy faillure… Anyway this security check is not needed because such a file on the device is not the bootloader itself but just a file to update the bootloader. We can safely remove it unstead of rename it: in the murphy user senario the user will just need to download a new OF.
- implemented hint: reminder to have enough battery (before installing because once the firmware is on the device any deconnection would start updating)
- implemented posthint: splited a general hint to have a specific hint on how to boot in OF mode from an already rockboxed device. this might seems a little messed up but this is the only way to reuse some part of the already written hints while having all hints showing up in the right order for the fuze+

   rbutil-imx-v7-modurl.diff (12.2 KiB)
   rbutil-imx-v7-release.diff (11.6 KiB)

I’ve checked the v5 patch yesterday. Since you already updated the patch some of my comments might be outdated, I’ve only skimmed the v7 patch as of now.

1. For getting the basename you can use QFileInfo::baseName() instead of splitting a QString. Splitting a QString might fail if it’s using other path separators than the one specified (could be an issue on Windows).
2. You retrieve a char* to the filename. However, I’m not sure how long such a pointer is still valid. You should pass that to the mkimxboot() call directly, as done with m_offile. Using a hadcoded filename as done in v7 is in general a bad idea.
3. around the middle of installStage2() you use a rather weird conversion of m_blfile from QString via a 8bit encoding to a QString again. m_blfile is already a QString.
4. In installed() we can look for the .orig file and use that to determine if a Rockbox bootloader is installed. Also, is there any magic in the patched firmware file we can check the firmware file for our bootloader?
5. There is no need to flush() the patched bootloader file before close()ing it.
6. I don’t think we should put instructions on how to dualboot into the bootloader installation. We don’t do this for other players. Alternatively we could add it for all players (though I’d prefer to keep that information in the manual – a pointer to the manual might be a good idea though)
7. We should avoid abbreviations like “OF” in user messages – that’s Rockbox lingo and most users are unlikely to be familiar to that. For the same reason I’d like to avoid using “rbutil” in our support channels :)

Oh, another point: for easier reviewing I’d suggest not posting two almost identical patches but a main patch and a second one that builds on the main one and does the download path adjustments.

1. ok this is what was causing my problem. Anyway it was useless from the beginning to extract the basename and then move the file anyway… So since v7 there is just temporary name, and this is way more simple! I didn’t know that pointers have some lifetime.
2. Ok I wasn’t sure what was the cleaner and more readable way to do it. I’ve taken away all those char* then
3. right another useless convertion
4. The file copied to the device get erased after update. So we cannot rely on it for detecting rbbootloader/version. I also ask Pamaury if it would be possible to have some header in RB booloader with some version. He didn’t seems very enthousiastic, and it will be a lot of work to versionning all the different patched bootloader version… A solution could be to retrieve the date the bootloader’s file was added to the server. We could write it into some versioning file on install.
Then before patching we could check this file (no file == unknow or new install) and compare it to the file from rb server to see if a more recent was not already installed… 5. ok
6. I thing that it should be added for all device: people are not going to look at the manual 2 seconds before being able to try RB. This basicly means that they are going to ignore the link and show up directly on the forum and irc saying: “your stuff doesn’t work!” :D The problem is that there is nothing general here, booting into OF can be the same way on a group of target, specific to one player or a combination of both… Idealy I suppose we should have a class in bootloaderinstallbase to send message from the different install method. We could have standardized message and set the one we want in some argument + the possibility to send specific message… Having player specific code in base is not really in the spirit, is it?
For now the pointer to manual seems the simplier solution anyway… except we don’t have manual yet for Fuze+! :D
Actually this is the very same problem with the recommandation I added to ofHint about battery to be over 70%. This is quite important to remind it to the user but this has nothing to do with off
7. right

Thank you very much for your comment and help on this! :)

   rbutil-imx-v8.diff (10.6 KiB)
   rbutil-imx-testurl.diff (0.8 KiB)

4. I’m not necessarily thinking about versioning here – having a way to identify the Rockbox bootloader would be good enough. For example, the mi4 format has some magic bytes for the Rockbox bootloader. However, if the bootloader file is removed during firmware upgrade then that’s of not much use anyway, we can then simply rely on the presence of the .orig file. As for versions, Rockbox Utility doesn’t care about versions for the bootloader (we can’t retrieve the bootloader version in most cases) and instead uses the file timestamp on the server.
6. When pointing to the manual for all devices we need to handle the case that some players don’t have dualboot (the iaudio ones AFAIK). That means that either the manual needs to have a specific section about it that is also present for those (like the current “Starting the original firmware” section) or we need to distinguish in Rockbox Utility. Putting it explicitly in the manual is better IMO because it makes it clear that there is no way to boot the original firmware on those players.

4. So we could write the timestamp to a file and compare it to the one of the downloaded bootloader… What is this .orig file you are talking about?
5. yes the fact to have some “Starting the original firmware” on every target seems the better solution. this means I’m going to go on with fuze+ manual first (I would mike to implement 4. first)

4. I’m talking about the original firmware file – looks like this isn’t present anymore. Older versions of the patch did create such a file, I guess this is a leftover from the location the code has been copied from :) The timestamp is handled in BootloaderInstallBase::logInstall() and uses the timestamp on the file on the server, so there is no need to store the timestamp in some other location. And since the timestamp is considered the version string it is stored in the installation log. No extra work :)
5. I agree. I’m not sure what’s the best way to link that, since there is no guarantee that deep links to the manual won’t change – the simplest way should be to simply indicate the user to read the manual if he wants to start the original firmware.

4. see this comment: http://www.rockbox.org/tracker/task/12402#comment41957 I remove this part myself because this could lead to some bug (pehraps just because of the link and variable that I was using. Anyway as I wrote this was rather useless as the file get deleted on update. So If the user left a bootloader on the device, this means he wan’t to update his device. And if he is using rbutil to install the bootloader it means he wan’t to install rockbox. So I assumed we could delete an already present file safely.
5. That’s right. It is simpler. We’ll see further if it’s really needed then.

I’ll post the last modification (just the rtm stuff) soon and then go on for the manual.

so this is just adding : “(Please refer to Rockbox’s manual on how to do so)” after the line: msg += tr(”<li>Reboot your player into the original firmware.</li>” this should be ready for the svn now… (at least as soon as we’ll get a bootloader on line)

   rbutil-imx-v9.diff (10.8 KiB)