DRAFT: Removing emu_options reference from machine_config

[npwoods]

This change is intended to be a (as the circumstances allow) minimal change required to decouple emu_options from machine_config, and doesn't address the broader Sisyphean issues of how software list handling is coupled with device loading. For this reason, there is some grossness done in the name of "doing no harm", but this does succeed in decoupling machine_config and emu_options

• Because software_list_device no longer has access to an emu_options, it can no longer parse software lists on demand, and the various call sites that can exercise this code path now have to ensure that software lists are parsed.
• software_get_default_slot() now needs the image name from options to be passed. This was a somewhat intrusive name because some additional grossness needed to be added to get_default_card_software_hook

A proximate benefit is now that when -listxml is invoked, we don't have to feed machine_config an emu_options, and the resulting configuration is created without any slots specified. This removes some of the extraneous -listxml items associated with the default configuration.

This change has a known issue that should block merging. A number of communications devices (e.g. - m3comm_device) were grabbing emu_options in their constructors to build hostnames. While it would be straight forward to change these to be lazy loaded, we probably should identify a better way for this pattern to be implemented.

[ajrhacker]

Some first impressions here:

Passing image_name through get_default_card_software_hook seems basically OK, even though it's responsible for most of the source files being changed here. The parse_if_necessary loops, on the other hand, look very inefficient, and it's also asking for trouble if the software lists somehow don't get parsed before they get accessed. Attaching slot devices to machine_config could probably also be done outside the main constructor.

I think a less bad solution for the hash_path problem is inserting it into machine_config, since, besides software_list_device, hashfile_extrainfo also needs to get at it whether or not a running_machine is present. At least the value of the hash_path option isn't going to be changing around very much, unlike the image- and slot-related options.

The comms devices (whose rewrites by @SailorSat are stuck in PR limbo) probably shouldn't need to build the socket filenames until device_start time.

[npwoods]

The parse_if_necessary loops, on the other hand, look very inefficient, and it's also asking for trouble if the software lists somehow don't get parsed before they get accessed.

Yeah I don't like that either. I think the bigger problem is that software_list_device shouldn't really be in the business of being a receptacle for loaded software lists at all; it is a gross abuse of the device system. I think it would be better if we extracted all of that functionality into something else (software_list_dispenser?) and reduced software_list_device to some sort of minimal thing that was only used to identify the name of the software list.

Attaching slot devices to machine_config could probably also be done outside the main constructor.

This occurred to me as well. This felt like the sort of thing that could easily be done once there was alignment on how to address the "bigger rocks"

I think a less bad solution for the hash_path problem is inserting it into machine_config, since, besides software_list_device, hashfile_extrainfo also needs to get at it whether or not a running_machine is present. At least the value of the hash_path option isn't going to be changing around very much, unlike the image- and slot-related options.

Yeah that's "less bad" than the status quo, but I suppose I was hoping (perhaps naively) we could do better.

The comms devices (whose rewrites by @SailorSat are stuck in PR limbo) probably shouldn't need to build the socket filenames until device_start time.

I have not seen these? Do they make the current problem (trying to access emu_options in device constructors) go away?

Then again, it is probably very easy to move the current code from the constructor to device_start(); it would allow this PR to actually run -listxml without crashing.

[ajrhacker]

I have not seen these?

#13421 covers mb89374, m1comm, m2comm and s32comm, but not m3comm.

[cuavas]

I don't think this is well-thought-out.

Changing behaviour so that "creating a machine configuration with no options is not the same as with default options" violates the principle of least surprise. This is bad engineering. Even if you want to move the options from the machine configuration to the machine, doing something different with no options versus all defaults is just making it harder to understand. We don't need more pitfalls.

We already have a whole lot of "you can't do X before Y" situations that trip people up, e.g. people regularly run into "you can't read inputs during the start phase" when it comes to configuration switches. Gratuitously adding another one (can't access options before machine is set) just makes development more difficult. It's also a part of session setup that most people aren't even thinking about – most developers are at least aware of the basic config/start/reset lifecycle phases due to the device member function calls.

It means anything that needs to be done differently depending on an option has to be deferred until after building the device tree entirely. I can see this being a problem for CPUs that want to do things differently if recompiling is disabled, for example.

Software lists needing another step to be told to parse themselves is just more complication for the front-end/UI code.

[cuavas]

On top of that, I think your goal of emptying all slots for -listxml is going to create more problems than it solves.

First of all, it’s going to be a problem for “dumb” front-ends that don’t walk slot options. If slots are empty, then systems that get their display, audio output and storage (floppy drives, etc.) from cards, like the original IBM PC for example, will appear to have no video, audio or media options in the -listxml output. Giving users of “dumb” front-ends an indication of the features the system has in its default configuration is far more useful behaviour.

Secondly, the device_ref elements are used to power, “What uses this device?” type queries, so you can get a list of things that might be good testing candidates when working on a device. With systems populated with default cards, you can do a single index lookup to get all the systems that use a given device in their default configuration.

If slots were all empty, a simple lookup wouldn’t give much at all for something like a floppy drive device that’s always instantiated as a “slot card” on a “floppy connector” slot. You’d need to recursively check whether each device is a default slot card option anywhere, look up devices/systems that reference it, etc. while guarding against getting into an infinite loop. It would greatly complicate the logic, as well as requiring far more queries, and requiring O(n) storage.

(And all that aside, if skipping things in slots for -listxml was a good idea, you could just stop walking the device tree on encountering a card in a non-fixed slot. Changing the behaviour of -listxml doesn’t require an intrusive change to the framework.)

[npwoods]

Changing behaviour so that "creating a machine configuration with no options is not the same as with default options" violates the principle of least surprise. This is bad engineering

I'll buy this, and this touches on something that @ajrhacker said ("Attaching slot devices to machine_config could probably also be done outside the main constructor") that I agree with

I tried rectifying this by getting rid of the machine_config constructor that takes emu_options and added a new machine_config::add_slot_options() method. The weakness of this approach is that if add_slot_options() is not called, it is now necessary to call a complete() method so that the config completed handlers get called.

I'm not sure what we should really be doing here - maybe we should make the constructor private and instead have static methods? I'm open for ideas on better ways to solve this (@ajrhacker)?

It means anything that needs to be done differently depending on an option has to be deferred until after building the device tree entirely. I can see this being a problem for CPUs that want to do things differently if recompiling is disabled, for example.

machine_config needs to be usable with -listxml, which means that they need to operate in scenarios where there is no meaningful emu_options object at all, so this is a problem that we have already. Arguably making it so they don't have access to emu_options forces developer to more explicitly reckon with this reality, therefore it is an improvement.

Software lists needing another step to be told to parse themselves is just more complication for the front-end/UI code.

Agreed. I'm going to see what needs to be happen to extract functionality related to live software list management out of the current device classes. Perhaps this should be called software_list_dispenser or software_list_pool?

On top of that, I think your goal of emptying all slots for -listxml is going to create more problems than it solves.
[snip]

I can be persuaded by this, but at the very least I think that it is clear the lack of a simple way to identify <device_ref> and <slot> instances that originate from the slot configuration is bad. At the very least, surely we can agree that such instances should be decorated with something like from_slot_config="true", so that smarter consumers can disregard them?

(And all that aside, if skipping things in slots for -listxml was a good idea, you could just stop walking the device tree on encountering a card in a non-fixed slot. Changing the behaviour of -listxml doesn’t require an intrusive change to the framework.)

I find this statement very surprising, as it seems that the existence of an emu_options within machine_config for the sole purposes of conveyance to downstream consumers seems to clearly be poor engineering (what ever happened to "anything worth doing is worth doing right?"). Furthermore, there are other usages of machine_config (e.g. - validity.cpp) that should really be neutral to options, even if -listxml should not be so.

All that said, if you feel very strongly that it would be better to have some checks to see whether something is from a slot config and emit from_slot_config="true", I have zero problem pursuing that approach; less work for me ;-)

[ajrhacker]

@cuavas makes a good point about XML listings needing to reflect the default options to accurately demonstrate the system's capabilities for frontends that don't recurse on <slotoption> references. While it's currently possible to configure any system with no options installed in the (non-fixed) slots, many of these configurations are good for nothing except for some very limited unit testing.

I tried rectifying this by getting rid of the machine_config constructor that takes emu_options and added a new machine_config::add_slot_options() method. The weakness of this approach is that if add_slot_options() is not called, it is now necessary to call a complete() method so that the config completed handlers get called.

I'm not sure what we should really be doing here - maybe we should make the constructor private and instead have static methods? I'm open for ideas on better ways to solve this (@ajrhacker)?

The solution I thought of was having the with-options constructor call add_slot_options, followed by whatever does the config_complete pass. Of course, things get complicated if there's a possibility of needing to invoke those independently.

I'm going to see what needs to be happen to extract functionality related to live software list management out of the current device classes. Perhaps this should be called software_list_dispenser or software_list_pool?

I have no confidence in the viability of separating software list management from device classes. The device hierarchy provides modularity, and trying to bypass it will create even more problems.

Furthermore, there are other usages of machine_config (e.g. - validity.cpp) that should really be neutral to options, even if -listxml should not be so.

I believe that the validity checker doesn't even really need to install the default slot options, since it tries to validate every slot option available (within reason).

[cuavas]

All that said, if you feel very strongly that it would be better to have some checks to see whether something is from a slot config and emit from_slot_config="true", I have zero problem pursuing that approach; less work for me ;-)

Just adding tags to them would allow separating the ones that come from slot “cards” on the consuming side, in the same way that you can for the chip and display elements (at the cost of increasing the size substantially). from_slot_config seems awfully specific, and doesn’t let you identify devices added by other subdevices (at least not without working backwards).

The chip element itself is incredibly poorly designed, and we’d do better with child elements on device_ref elements, but at this point too many front-ends depend on it, so deprecating it would be a years-long process.

[npwoods]

@cuavas 's idea of adding tag to <device_ref> was actually extraordinarily easy, so I've submitted a separate PR

My question for this audience - the impetus for this PR is addressed by the addition of tag on <device_ref>. Is it worth pursuing the decoupling of machine_config and emu_options? I still say that they should be decoupled, and that other applications like validation should not be working with default slot configs. But if people think that this is not worth it, then I'm inclined to not bother as well.