-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
slot bios handling improvements #9774
base: master
Are you sure you want to change the base?
Conversation
src/frontend/mame/ui/miscmenu.cpp
Outdated
if (device->owner()) | ||
{ | ||
const char *slot_option_name = device->owner()->tag() + 1; | ||
std::string bios = machine().options().slot_option(slot_option_name).bios(); | ||
if (bios.empty()) val = nullptr; else val = bios.c_str(); | ||
} | ||
else | ||
{ | ||
val = machine().options().value("bios"); | ||
} | ||
if (val) | ||
bios_val = atoi(val) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is broken in multiple ways:
- You use
.c_str()
on a local inside theif
statement body, and then try to use it after said local has fallen out of scope. - The preferred way of specifying BIOS options is by name – e.g. using
-bios japan
to select the Japanese Neo Geo BIOS. You’re not going to get the right results usingatoi
on that. - Conditional assignment is a lot clearer using the ternary conditional operator than crushing an
if
/else
construct onto a single line.
src/frontend/mame/ui/miscmenu.cpp
Outdated
@@ -51,6 +51,26 @@ menu_bios_selection::menu_bios_selection(mame_ui_manager &mui, render_container | |||
{ | |||
} | |||
|
|||
int menu_bios_selection::current_bios(device_t *device) | |||
{ | |||
int bios_val = device->default_bios(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t it be better to start with the system_bios()
here?
src/emu/machine.cpp
Outdated
std::string tag(device.tag()); | ||
std::string tag; | ||
for (device_t *dev = &device; dev->owner() != nullptr; dev = dev->owner()) | ||
{ | ||
std::ostringstream dev_tag; | ||
dev_tag << ":" << dev->basetag(); | ||
device_slot_interface *intf; | ||
if (dev->owner() && dev->owner()->interface(intf)) | ||
{ | ||
if (dev->system_bios() != 0 && dev->default_bios() != dev->system_bios()) | ||
util::stream_format(dev_tag, "_%d", dev->system_bios() - 1); | ||
} | ||
tag = dev_tag.str() + tag; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think using BIOS numbers in anything externally visible is a good idea – they a lot less stable than the mnemonic names.
Also, does the check actually work properly? What happens when the default BIOS isn’t the first one (e.g. osborne1
)? It looks like both the default BIOS and the first BIOS will have no suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this part of code is affecting only bios on slot device level, and for those on driver, logic is not changed, that is in lines 1099-1100.
For slot devices I have changed code according to comment.
@cuavas Would be thankful for comment on this after change. |
src/emu/machine.cpp
Outdated
std::string tag(device.tag()); | ||
std::string tag; | ||
for (device_t *dev = &device; dev->owner() != nullptr; dev = dev->owner()) | ||
{ | ||
std::ostringstream dev_tag; | ||
dev_tag << ":" << dev->basetag(); | ||
device_slot_interface *intf; | ||
if (dev->owner() && dev->owner()->interface(intf)) | ||
{ | ||
for (romload::system_bios const &bios : romload::entries(dev->rom_region()).get_system_bioses()) | ||
{ | ||
if (dev->system_bios() == bios.get_value()) | ||
{ | ||
util::stream_format(dev_tag, "_%s", bios.get_name()); | ||
break; | ||
} | ||
} | ||
} | ||
tag = dev_tag.str() + tag; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still not entirely sure this always makes sense to do. For example, should all the NVRAM files for every subdevice get their paths mangled when the system BIOS is changed? I’d like to solicit a bit more discussion from more people on this aspect of the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases NVRAM will work (like when it contains RTC data) independent of BIOS version. Only case where it will never work is when nvram data contains flash memory data. Adding that as a flag in nvram interface, and making other flash memory interface which all flash memory based chips will extend will make that possible. Or even separating handling of those two in general. Can work on solution when agreed for best approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking more about this and implemented next:
- We will add suffix/postfix only for flash devices ( those have parent rom region name that is equal to device name)
- Since we already have handling if this is going on on driver level, we are not altering those names (since they are anyway in separate directory)
src/frontend/mame/ui/miscmenu.cpp
Outdated
} | ||
if (val) | ||
{ | ||
for (romload::system_bios const &bios : romload::entries(device->rom_region()).get_system_bioses()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’ve got a local bios
here that shadows another local called bios
declared earlier in the function and still in scope. That’s a bad practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, shadowing is fixed
src/frontend/mame/ui/miscmenu.cpp
Outdated
@@ -51,6 +51,35 @@ menu_bios_selection::menu_bios_selection(mame_ui_manager &mui, render_container | |||
{ | |||
} | |||
|
|||
int menu_bios_selection::current_bios(device_t *device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the parameter device
a pointer rather than a reference? The function won't work if it’s null, and it can’t be indexed like an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this as well
…name) on those that are not on top level
Using
dev->set_system_bios(bios_val)
had side effect that causednvram_save
to save content for running system under new name, which is wrong. By using values from options we are able to keep menus working and them applied when reset is done.Since any of slot cards can have bios selection that can affect nvram content, added bios name id (if not default) after slot card name, same as we add for bios under driver level.