Skip to content
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

Added support for memory unit devices #2246

Merged
merged 9 commits into from Jun 29, 2021

Conversation

ergo720
Copy link
Member

@ergo720 ergo720 commented Jun 21, 2021

This adds support for a new input device, the memory unit (MU), which becomes available in the input gui when selecting the duke or S controller. For this cxbxr will now create a new folder called EmuMu in the same location where EmuDisk is created, and inside EmuMu there will be 8 folders, one for each MU. Regarding the internal code changes, this implements XMountMUA, XMountMURootA, XUnmountMU, XReadMUMetaData and extends the kernel functions NtQueryVolumeInformationFile, NtDeviceIoControlFile and NtFsControlFile to support MUs, in addition to the HDD. Below is a pic of the dashboard recognizing the MUs:

Capture

Testing is welcome.

@github-actions github-actions bot added file-system file-system issue graphics GPU and/or game graphics related HLE High Level Emulation input USB and/or game input related kernel xbox kernel related modules submodules user interface GUI related labels Jun 21, 2021
@LanHikariDS
Copy link
Contributor

LanHikariDS commented Jun 22, 2021

Can confirm that TimeSplitters 2 and TimeSplitters: Future Perfect read from/write to them flawlessly-
image
image

@ergo720 ergo720 force-pushed the mem_units branch 2 times, most recently from 0ddb7e8 to de9913f Compare June 22, 2021 14:45
Copy link
Member

@RadWolfie RadWolfie left a comment

Choose a reason for hiding this comment

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

I had not made a full review, there's some changes need some fixes before I can re-review again.

I notice there are bunch of dev1 and port1 and the like which is not suitable to be use and lead confusion. Intead of using dev# and port#, they are actually iterator (in most cases), so using iDev and iPort, or i_dev and i_port, are preferred to be used.

src/core/hle/D3D8/Direct3D9/Direct3D9.h Outdated Show resolved Hide resolved
src/core/kernel/exports/EmuKrnlNt.cpp Show resolved Hide resolved
src/core/kernel/support/EmuFile.cpp Outdated Show resolved Hide resolved
@RadWolfie
Copy link
Member

Beside code review, I tested the pull request's build with dashboard first, then Monopoly Party title.

I can confirm personalize MU names does work. And able to display the MU name in Monopoly Party title as seen below:

MU1A with personalize

MU1A with personalize

MU1B without personalize (initialized by game title)

MU1B without personalize

@ergo720 ergo720 force-pushed the mem_units branch 2 times, most recently from 35578ce to 09843d3 Compare June 24, 2021 08:46
@ergo720 ergo720 force-pushed the mem_units branch 2 times, most recently from d63058d to b9b2213 Compare June 25, 2021 14:04
Copy link
Member

@RadWolfie RadWolfie left a comment

Choose a reason for hiding this comment

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

Few code style changes and base on numerous to input mechanical not relative to memory unit devices. I recommended to perform full test with various titles that had been known to break in the past.

src/common/input/InputDevice.cpp Outdated Show resolved Hide resolved
src/common/input/InputManager.h Outdated Show resolved Hide resolved
src/common/input/SdlJoystick.cpp Show resolved Hide resolved
extern const std::string DriveZ;
extern const std::string DevicePrefix;
extern const std::string DeviceCdrom0;
extern const std::string DeviceHarddisk0;
extern const std::string DeviceMU;
Copy link
Member

Choose a reason for hiding this comment

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

Normally, DeviceMU should be move down to above DeviceMU0. But I can understand the group purpose.

src/core/kernel/init/CxbxKrnl.cpp Show resolved Hide resolved
src/common/input/InputWindow.h Outdated Show resolved Hide resolved
src/common/Settings.hpp Outdated Show resolved Hide resolved
src/common/input/InputManager.h Outdated Show resolved Hide resolved
src/common/input/InputManager.h Outdated Show resolved Hide resolved
{ NULL, PORT_INVALID, XBOX_INPUT_DEVICE::DEVICE_INVALID, &g_InState[3], false, false, false, false, false, { 0, 0, 0, 0, 0 } },
};
// Protects access to xpp types
std::atomic<bool> g_bXppGuard = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we need this additional guard?

// Guard against the unfortunate case where XGetDevices or XGetDeviceChanges have already checked for g_bIsDevicesInitializing
// and g_bIsDevicesEmulating and a thread switch happens to this function

Quote from while (g_bXppGuard) {} line.

Can't we simply check both booleans than rely on additional variable, g_bXppGuard?

Copy link
Member Author

Choose a reason for hiding this comment

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

UpdateXppState is only called from ConstructHleInputDevice and DestructHleInputDevice, both of which set g_bIsDevicesEmulating to true. The unfortunate case happens when XGetDevices or XGetDeviceChanges are called, find g_bIsDevicesEmulating = false, proceed, but then a thread switch happens to UpdateXppState. Without the guard, the latter will concurrently access the xpp type, which might cause issues depending on which xpp type XGetDevices and XGetDeviceChanges are using.

Copy link
Member

@RadWolfie RadWolfie left a comment

Choose a reason for hiding this comment

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

Known titles experienced regression in the past has pass test by our tester.

@RadWolfie RadWolfie merged commit cd768c6 into Cxbx-Reloaded:master Jun 29, 2021
@ergo720 ergo720 deleted the mem_units branch June 30, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-system file-system issue graphics GPU and/or game graphics related HLE High Level Emulation input USB and/or game input related kernel xbox kernel related modules submodules user interface GUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants