Bootcore feature

https://github.com/MiSTer-devel/Main_MiSTer/wiki

Moderators: Mug UK, Zorro 2, Greenious, spiny, Sorgelig, Moderator Team

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Bootcore feature

Postby spark2k06 » Sun Mar 03, 2019 5:53 am

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.


Image

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?:

Image
Last edited by spark2k06 on Wed Mar 20, 2019 8:07 am, edited 5 times in total.

Sorgelig
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Sun Mar 03, 2019 9:26 am

spark2k06 wrote: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.

spark2k06 wrote: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?

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Sun Mar 03, 2019 1:53 pm

Sorgelig wrote: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. :wink:

Sorgelig wrote: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.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Feature coreboot Beta (MiSTer_20190302c)

Postby spark2k06 » Mon Mar 04, 2019 9:10 am

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
You do not have the required permissions to view the files attached to this post.

Sorgelig
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Mon Mar 04, 2019 12:17 pm

i suggest following options:
coreboot=last
coreboot=filename_of_core

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

Sorgelig
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Mon Mar 04, 2019 12:25 pm

and may be "bootcore" would sound better.

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

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 12:30 pm

Sorgelig wrote: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.
Last edited by spark2k06 on Mon Mar 04, 2019 12:40 pm, edited 1 time in total.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 12:32 pm

Sorgelig wrote:and may be "bootcore" would sound better.

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


Ok. :-)

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 12:38 pm

spark2k06 wrote:
Sorgelig wrote: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
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Mon Mar 04, 2019 12:50 pm

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

Or

bootcore=fixed,filename_of_core

for what?
1 = last
2 = filename

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 1:01 pm

Sorgelig wrote: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.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 1:42 pm

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
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Mon Mar 04, 2019 2:20 pm

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.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 3:00 pm

Sorgelig wrote: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.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 3:37 pm

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
Fuji Shaped Bastard
Fuji Shaped Bastard
Posts: 4760
Joined: Mon Dec 14, 2015 10:51 am
Location: Russia/Taiwan

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby Sorgelig » Mon Mar 04, 2019 4:59 pm

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

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Mon Mar 04, 2019 5:09 pm

Sorgelig wrote: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.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190304b)

Postby spark2k06 » Thu Mar 07, 2019 8:32 am

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)
You do not have the required permissions to view the files attached to this post.

BBond007
Captain Atari
Captain Atari
Posts: 365
Joined: Wed Feb 28, 2018 3:23 am

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby BBond007 » Thu Mar 07, 2019 10:59 am

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.
Last edited by BBond007 on Thu Mar 07, 2019 12:17 pm, edited 1 time in total.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Thu Mar 07, 2019 11:55 am

BBond007 wrote: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.
You do not have the required permissions to view the files attached to this post.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Thu Mar 07, 2019 12:11 pm

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
Captain Atari
Captain Atari
Posts: 365
Joined: Wed Feb 28, 2018 3:23 am

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby BBond007 » Thu Mar 07, 2019 8:25 pm

spark2k06 wrote: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)
Last edited by BBond007 on Thu Mar 07, 2019 8:44 pm, edited 1 time in total.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Thu Mar 07, 2019 8:38 pm

I take note to review it too. Thank you!

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Feature lastcoreboot Beta (MiSTer_20190302b)

Postby spark2k06 » Sat Mar 09, 2019 6:00 am

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.
You do not have the required permissions to view the files attached to this post.

User avatar
spark2k06
Atariator
Atariator
Posts: 27
Joined: Sat Feb 23, 2019 6:46 am
Location: Barakaldo, Spain

Re: Bootcore feature

Postby spark2k06 » Tue Mar 12, 2019 5:38 am

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


Return to “MiSTer”

Who is online

Users browsing this forum: No registered users and 3 guests