slot bios handling improvements #9774
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.
I think this is broken in multiple ways:
- You use
.c_str()on a local inside theifstatement 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 japanto select the Japanese Neo Geo BIOS. You’re not going to get the right results usingatoion that. - Conditional assignment is a lot clearer using the ternary conditional operator than crushing an
if/elseconstruct 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
…name) on those that are not on top level
Using
dev->set_system_bios(bios_val)had side effect that causednvram_saveto 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.