-
Notifications
You must be signed in to change notification settings - Fork 2.2k
CD-i: Add Audio Mixing #14481
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
base: master
Are you sure you want to change the base?
CD-i: Add Audio Mixing #14481
Conversation
|
Compiling src/mame/philips/cdislavehle.cpp... Also, this is kind of atrocious code. What are you trying to do exactly? |
| { | ||
| case 0xc0: case 0xc1: case 0xc2: case 0xc3: case 0xc4: case 0xc5: case 0xc6: case 0xc7: | ||
| case 0xc8: case 0xc9: case 0xca: case 0xcb: case 0xcc: case 0xcd: case 0xce: case 0xcf: | ||
| dynamic_cast<cdi_state*>(m_owner)->m_cdic->atten_w(m_in_buf); |
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.
This is unportable, you should rather provide this communication thru devcb_write8::array<4>.
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.
Acknowledged. If it is acceptable, I will use a follow up PR on this detail.
MooglyGuy
left a comment
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.
Covered some finer points on both OG's and Angelo's comments.
| m_atten[0] = ((args[0] & 0x8) << 7) | args[1]; | ||
| m_atten[1] = ((args[0] & 0x4) << 7) | args[2]; | ||
| m_atten[2] = ((args[0] & 0x2) << 7) | args[3]; | ||
| m_atten[3] = ((args[0] & 0x1) << 7) | args[4]; |
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.
Following on from Kale's comment, it's pretty unlikely that the real hardware connection between the SLAVE and CDIC devices is 20 individual traces for the 20 bits of state represented by args here.
The correct way to do this would be to have atten_w take a single uint8_t, have a private member that increments on each call from 0 to 4 and wrapping around, in order to determine how to apply the incoming byte to the m_atten array. You would then have a devcb_write8 with a public callback-registration function in cdislavehle_device - there are plenty of examples throughout the codebase on how to do this - and then in the machine configuration function for cdi_state::cdimono1_base, do -
m_slave_hle->atten_callback().set(m_cdic, FUNC(cdicdic_device::atten_w));
rather than grabbing into a protected class member in a way that doesn't compile.
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.
Added a temporary fix to expose the cdic for dynamic casting just so that it compiles so it can be validated it worked. Thanks for the feedback, I'll look into the callback 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.
Thank you for the example. Excluding the Mute bits here will result in popping sounds. Hotel Mario shows that this solution is close, but not entirely correct. I don't exactly know what the correct technical implementation is supposed to be yet.
If it is acceptable, I will commit this in the (largely) working state. I have left a TODO here.
-cdicdic: Add audio attenuation. Fixes mamedev#12852 [Vincent Halver]
7470928 to
5a39587
Compare
-cdicdic: Add audio attenuation (fixes #12852) [Vincent Halver]