Skip to content

Conversation

@Endprodukt
Copy link
Contributor

hooked some outputs for model 2 based on SailorSats work. Added definitions to output names for Lamps and wheel.

output().set_value("Lamp_Race_Leader", BIT(data, 7));
}

if (strncmp(name, "indy500", 7) == 0)
Copy link
Member

@angelosa angelosa Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely no at this. Just generate a function depending on what's the machine_config caller. So for example:

void model2a_state::srallyc(machine_config &config)
{
[...]
	io.out_pf_callback().set(FUNC(model2a_state::srallyc_outputs_w));
}

void model2_state::srallyc_outputs_w(u8 data)
{
     	m_output[1] = data;
		output().set_value("Lamp_Start", BIT(data, 2));
		output().set_value("Lampe_View", BIT(data, 5)); // Lampe?
		output().set_value("Lamp_Race_Leader", BIT(data, 7));
}

Beyond being ugly and prone to breakage, there are two extra reasons about why calling the base system name is a bad juju:

  1. you are iterating for every single string compare;
  2. the clones won't hookup anything (iirc);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clones do hook. Thanks for the feedback.

@Endprodukt
Copy link
Contributor Author

Endprodukt commented Dec 1, 2025

@angelosa I hope these changes will make the integration of the code possible. Thank you again.

@Osso13
Copy link
Member

Osso13 commented Dec 1, 2025

Afaik set_value is deprecated and should be removed in favour of output_finder when encountered. Adding even more shouldn't be the way to go

@happppp
Copy link
Member

happppp commented Dec 1, 2025

output().set_value is deprecated.

Better use 6 generic lamps, like m_lamps(*this, "lamp%u", 0U)
Then just 1 function and set each one in a for loop.

How is the wheel motor shared with m_driveio_comm_data?

@Endprodukt
Copy link
Contributor Author

Endprodukt commented Dec 1, 2025

Uff. Okay I'm tired for today. I did this exactly to avoid generic lamps and define them.

I didn't do the wheel hooks but they work.

@Endprodukt
Copy link
Contributor Author

But defined outputs are not wanted? Will tell people straight what which output does.

@happppp
Copy link
Member

happppp commented Dec 1, 2025

Personal preference: generic output tags in most cases.
But if there's no reference internal .lay (or SVG screen) for the leds and such, then it's nice having comments in the source file explaining what each output is.

@Endprodukt
Copy link
Contributor Author

@happppp changed to generic lamp output. if this works for you I will also fix the outputs for model2o.

@happppp
Copy link
Member

happppp commented Dec 2, 2025

Lamps are on bit 2-7, right? So it's i+2, not i. And you only need one function.
What is m_output for?

And I'm not a fan of "contact MAMEdev" popmessage. To be honest if it was me, I'd use logerror instead of obscuring the game screen. (note, my opinion here may differ from model2 driver authors)

@Endprodukt
Copy link
Contributor Author

Contact mame Dev is not from me. Will fix the rest. I appreciate the time and patience.

@angelosa
Copy link
Member

angelosa commented Dec 2, 2025

At bare minimum a popmessage is for something unlikely that needs some testing that triggers and reporting it to MT/as GH issue/to whoever is the driver maintainer. It doesn't need the contact MAMEdev suffix because it's implicit from the use itself (and should actually be removed from anything that still uses it).

Also, this goes beyond the scope of this PR.

@Endprodukt
Copy link
Contributor Author

okay I think I got this.

@Endprodukt
Copy link
Contributor Author

great now my changes conflict suddenly?

@happppp
Copy link
Member

happppp commented Dec 2, 2025

It means you rebased on an earlier revision and not HEAD of master git branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants