Bootcore feature

[spark2k06]

Feature available from MiSTer_20190318:

Code: Select all

;
[MiSTer]
; lastcore - Autoboot the last loaded core (corename autosaved in CONFIG/lastcore.dat) first found on the SD/USB
; lastexactcore - Autoboot the last loaded exact core (corename_yyyymmdd.rbf autosaved in CONFIG/lastcore.dat) first found on the SD/USB
; corename - Autoboot first corename_*.rbf found on the SD/USB
; corename_yyyymmdd.rbf - Autoboot first corename_yyyymmdd.rbf found on the SD/USB
;bootcore=lastcore    ; uncomment to autoboot a core, as the last loaded core.
bootcore_timeout=10  ; 10-30 timeout before autoboot, comment for autoboot without timeout.

After the research and development of this new feature, I leave in this thread the results obtained so far. Before sending a pull request to @Sorgelig, I would like to ask some questions.

Changes on github (attached binary on this post):

https://github.com/spark2k06/Main_MiSTer

New entry on MiSTer.ini is necessary:

Code: Select all

lastcoreboot=1         ; set to 1 for autoboot last core loaded.

Demo of this feature:

https://www.youtube.com/watch?v=GWp-JB4359Q

The questions are the following:

1. From Main_Mister program, is it possible to detect that the reset button of the IO addon has been pressed? It would be necessary, so that pressing it will always load the menu. I have not found any simple way to detect it.

2. I do not know why, but the "GnG" kernel does not work ... but not even building with the original sources of the Master branch. As we can see, the size of the result of my compilation differs from the original Sorgelig, any idea?:

[Sorgelig]

1. From Main_Mister program, is it possible to detect that the reset button of the IO addon has been pressed? It would be necessary, so that pressing it will always load the menu. I have not found any simple way to detect it.

Reset button is hardwired to cold reset. So, reset button on I/O board effectively the same as cold reset button on the DE10-nano.
HPS has no chance to sense the reset button as it gets hardware reset signal.
In theory it's possible to change the button behaviour to let the HPS to sense the button before reset. It will need to be included in every core.

2. I do not know why, but the "GnG" kernel does not work ... but not even building with the original sources of the Master branch. As we can see, the size of the result of my compilation differs from the original Sorgelig, any idea?:

different versions of gcc?

[spark2k06]

Reset button is hardwired to cold reset. So, reset button on I/O board effectively the same as cold reset button on the DE10-nano.
HPS has no chance to sense the reset button as it gets hardware reset signal.
In theory it's possible to change the button behaviour to let the HPS to sense the button before reset. It will need to be included in every core.

I already assumed the implication that it would have. If in the future we have that information for HPS, I implement it without problem. Although the reset does not behave as desired, it is a feature that can be disabled without problem. I understand that updating all cores is a laborious task. Thank you anyway.

different versions of gcc?

Maybe, yes:

"arm-linux-gnueabihf-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE."

That (I think) does not explain why GnG does not works. However, as the builds that you prepare there is no problem for GnG core, I will do now the pull request.

Soon I would like the variable 'lastcoreboot' to be configurable from the same menu ... but I'll leave that for later.

[spark2k06]

The pull request has been rejected for good reasons. After following the tips of Sorgelig and before doing pull request again, we return to beta mode with added functionality, which is the fixed load of a core. For this reason, the variable 'lastcoreboot' is now simply called 'coreboot' and has three possible values:

Code: Select all

; 0 - Always menu core
; 1 - Last core loaded (autosaved in CONFIG/coreboot.txt)
; 2 - Always core defined in CONFIG/coreboot.txt
coreboot=0             ; Autoboot


If coreboot > 0, to see the main menu go to the OSD of the running core (F12), and then select cold reset. With lastcore = 1 once this is done, this will be the last core if MiSTer is shutdowned.

I attach a new binary in this post to test and also I update my github with the modifications regarding the Master branch. If we detect any other problem it would be great, I would not like to requests again for failed pull requests to Sorgelig

https://github.com/spark2k06/Main_MiSTer/commit/2e9ef4635a4dc4d69f9985df87cb76d1338fca71

[Sorgelig]

i suggest following options:
coreboot=last
coreboot=filename_of_core

if absent then original behaviour. And you don't need additional text file.

[Sorgelig]

and may be "bootcore" would sound better.

By the way, check your feature against USB storage. Make sure it works as well.

[spark2k06]

i suggest following options:
coreboot=last
coreboot=filename_of_core

if absent then original behaviour. And you don't need additional text file.

I think it's a good idea to indicate the core in the MiSTer.ini configuration file itself. But if 'last' is indicated, I need another additional variable, so as not to depend on another auxiliary file.

[spark2k06]

and may be "bootcore" would sound better.

By the way, check your feature against USB storage. Make sure it works as well.

Ok.

[spark2k06]

i suggest following options:
coreboot=last
coreboot=filename_of_core

if absent then original behaviour. And you don't need additional text file.

I think it's a good idea to indicate the core in the MiSTer.ini configuration file itself. But if 'last' is indicated, I need another additional variable, so as not to depend on another auxiliary file.

Maybe:

bootcore=last
(That after will be) -> bootcore=last,filename_of_core

Or

bootcore=fixed,filename_of_core

[Sorgelig]

bootcore=last
(That after will be) -> bootcore=last,filename_of_core

Or

bootcore=fixed,filename_of_core

for what?
1 = last
2 = filename

[spark2k06]

for what?
1 = last
2 = filename

Then you propose the use of two variables in MiSTer.ini? True?

Because I need two data, if is the last (for write it or not when the core os selected), and the filename fullpath.

[spark2k06]

Well. I already know how I will do to not need the variable or additional file and with your original idea of ​​bootcore = last or bootcore = filename.

Soon new beta...

[Sorgelig]

Oh, i've overlooked.
For "last" option it definitely need some config file where last used core will be stored.
I mean for "filename_of_core" you don't need that config as core will be directly pointed in this option.

[spark2k06]

Oh, i've overlooked.
For "last" option it definitely need some config file where last used core will be stored.
I mean for "filename_of_core" you don't need that config as core will be directly pointed in this option.

I had thought about the following. If the option is 'bootcore=last', the program starts with main menu. When selecting the core, the bootcore entry is overwritten (if it is not considered a bad method) as 'bootcore=last,filename_core' where filename_core is the last selected core. In this way, updating the MiSTer.ini file would be transparent to the user.

If the option is 'bootcore=filename_core' or 'bootcore=last,filename_core', the program starts with filename_core core. Only if bootcore entry starts with 'last', when selecting the core, the entry is overwritten.

By last, if cold reset from core menu (F12) is selected, the entry bootcore is overwritten as 'bootcore=last'... so if the MiSTer is turned off at this point, return to your starting point with the main menu.

[spark2k06]

Anyway, I'm seeing that updating MiSTer.ini is not a simple task. Contrary to its reading thanks to the function ini_parse (), we have not yet implemented anything for the writing of the fields of the cfg structure, which could be good in the future ... but as I say, if the update of this file is not considered a bad method.


Possibly, to simplify then the solution that I do is the following:

bootcore entry not exists or is commented -> normal behavior, without auxiliary file
bootcore=last (last core is loaded, with auxiliary file for updating)
bootcore=fullpath_core (fixed core is loaded, without auxiliary file)

[Sorgelig]

Auto updating ini anyway is not a good idea. I want to keep it read-only.

[spark2k06]

Auto updating ini anyway is not a good idea. I want to keep it read-only.

I assumed it, totally understandable. I continue with the other idea and the auxiliary file only for bootcore = last.

[spark2k06]

New beta, new options:

Code: Select all

; lastcore - Autoboot the last loaded core (corename autosaved in CONFIG/lastcore.dat) first found on the SD/USB
; lastexactcore - Autoboot the last loaded exact core (corename_yyyymmdd.rbf autosaved in CONFIG/lastcore.dat) first found on the SD/USB
; corename - Autoboot first corename*.rbf found on the SD/USB
; corename_yyyymmdd.rbf - Autoboot first corename_yyyymmdd.rbf found on the SD/USB
bootcore=lastcore             ; uncomment to autoboot a core, as the last loaded core.


Git changes:

https://github.com/spark2k06/Main_MiSTer/commit/e6a3c977c5c2de9e464d695f3a74ab1a47e8a3e3

As you can see, I transferred all the functionality and auxiliary functions to a new source file (bootcore.cpp, with its respective bootcore.h)

I would have to check that in USB it works correctly, in SD I have tried it and I have not observed any problem. If someone can advance this review task, great ... I attach the binary based on the last master of Sorgelig (20190304)

[BBond007]

One thing I notice is that you seem to have unintended memory leaks.

for example:

in function 'FileLoadLastCore' on line 106 you potentially return NULL loosing reference of the memory you malloc on line 97.

You should probably free(bootcore) before returning NULL;

in function 'bootcore_init' on line 175:

bootcore = replaceStr(bootcore, auxstr2, "");

This looses reference to the memory potentially malloced by function 'FileLoadLastCore' without freeing it.

possible fix:

char * tmp = replaceStr(bootcore, auxstr2, "");
if(bootcore != cfg.bootcore)
free(bootcore);
bootcore = tmp;

function 'replaceStr' also does another malloc which is never freed.

possible fix:

At the end function 'bootcore_init' ':

if (bootcore != NULL && bootcore != cfg.bootcore)
free(bootcore);

Or maybe just refactor to use a fixed sized stack variable rather than heap.

[spark2k06]

One thing I notice is that you seem to have unnecessary memory leaks.

for example:

in function 'FileLoadLastCore' on line 106 you potentially return NULL loosing reference of the memory you malloc on line 97.

You should probably free(bootcore) before returning NULL;

in function 'bootcore_init' on line 175:

bootcore = replaceStr(bootcore, auxstr2, "");

This looses reference to the memory potentially malloced by function 'FileLoadLastCore' without freeing it.

function 'replaceStr' also does another malloc which is never freed.

Maybe put something like this in functions 'bootcore_init' and 'replaceStr':

if (bootcore != NULL && bootcore != cfg.bootcore)
free(bootcore);

Or maybe just refactor to use a fixed sized stack variable rather than heap.

Thanks for the warning. At the risk of adding new bugs, I have made several corrections to try to avoid memory leaks. I will not be able to check if it continues to work until tomorrow:

https://github.com/spark2k06/Main_MiSTer/commit/1068300396b42a2e1838d62ec914e5531e73e1f7

New beta attached.

[spark2k06]

The solution is not right. I have not allocated space to the destination variables used by strcpy ... I will have to refactor as you say and optimize. Another day, this has to be thought very well. Thanks again for notifying.


I will use this great application to detect possible problems with memory:

http://valgrind.org/
https://www.ibm.com/developerworks/community/blogs/6e6f6d1b-95c3-46df-8a26-b7efd8ee4b57/entry/detect_memory_leaks_with_memcheck_tool_provided_by_valgrind_part_i8?lang=en

[BBond007]

The solution is not right. I have not allocated space to the destination variables used by strcpy ... I will have to refactor as you say and optimize. Another day, this has to be thought very well. Thanks again for notifying.


I will use this great application to detect possible problems with memory:

http://valgrind.org/
https://www.ibm.com/developerworks/community/blogs/6e6f6d1b-95c3-46df-8a26-b7efd8ee4b57/entry/detect_memory_leaks_with_memcheck_tool_provided_by_valgrind_part_i8?lang=en

One thing I see now is this:

char bootcore[256];

...
...

if (bootcore != NULL) <-- this will ALWAYS be true (regardless of the contents of bootcore) because you are doing a pointer comparison.

you probably are intending on this:

if(bootcore[0] != '\0')

or

if(strlen(bootcore) > 0)

[spark2k06]

I take note to review it too. Thank you!

[spark2k06]

New beta refactorized, free of memory leaks, I think:

https://github.com/spark2k06/Main_MiSTer/commit/e8279f5b5f9225dc4f64daf01c058da2906824b2 commit 1: Bootcore feature
https://github.com/spark2k06/Main_MiSTer/commit/fb1cb7953462e5f4ee65193f60a26ec3abf6b748 commit 2: Optimizations and normalization of function names.
https://github.com/spark2k06/Main_MiSTer/commit/ecbe6ea4e287620f16ba3df2a615e3eb101d0db6commit 3: Fix checking if cfg.bootconfig has been defined.

I attach binary.

[spark2k06]

Feature available from MiSTer_20190318, more info in the first post.