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.

Sign up for GitHub

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

UI: XMU Settings #1315

Merged
merged 34 commits into from
Dec 18, 2023
+769 −142
Merged

UI: XMU Settings #1315

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
96157f6
Added XMU Settings to the Input Screen
faha223 Dec 31, 2022
766409f
Added Peripherals to config
faha223 Jan 2, 2023
3f9b265
Prevent overwriting existing XMUs
faha223 Jan 8, 2023
8e3296b
Added blockdev.h to try to fix the MacOS build
faha223 Jan 10, 2023
51fe070
Merge pull request #12 from xemu-project/master
faha223 Jan 17, 2023
21d467a
Merge pull request #15 from xemu-project/master
faha223 Jan 18, 2023
b1f9085
Fixed some issues that antangelo pointed out
faha223 Jan 19, 2023
a9747a0
Moved the peripheralType and param vars into the loop
faha223 Jan 19, 2023
ac3b36d
Merge branch 'master' into xmu-settings2
faha223 Jan 19, 2023
485cb9c
Moved fatx.h and fatx.c to ui\thirdparty\fatx
faha223 Jan 21, 2023
bd1e773
Merge branch 'xmu-settings2' of https://github.com/faha223/xemu into …
faha223 Jan 21, 2023
bc80676
Added Validation for Peripheral Settings
faha223 Jan 23, 2023
1e0595e
Fixed some nits that were pointed out
faha223 Jun 8, 2023
f397226
Merge pull request #18 from xemu-project/master
faha223 Jun 8, 2023
8423130
Merge branch 'xmu-settings2' into master
faha223 Jun 8, 2023
70881db
Merge pull request #19 from faha223/master
faha223 Jun 8, 2023
afe5146
don't pass NULL into xemu_settings_set_string
faha223 Jun 8, 2023
5def7ad
Changes following Matt's recommendations
faha223 Jun 9, 2023
f966217
Changes to XMU FilePicker
faha223 Jun 12, 2023
3167c43
XMU image auto-bind logic refactor
faha223 Jun 13, 2023
8889047
renamed peripheralType to peripheral_type
faha223 Jun 13, 2023
85b7071
removed unnecessary calls to g_strdup_printf and g_free
faha223 Jun 13, 2023
32fe45a
Cleaned up some comments, removed an unnecessary variable
faha223 Jun 13, 2023
a7f9905
handle overwrite prompt in Windows
faha223 Jun 13, 2023
e372b5a
Merge branch 'master' into xmu-settings2
faha223 Aug 20, 2023
498a44a
Fixed some code format and style inconsistencies
faha223 Oct 30, 2023
e74066e
More formatting fixes
faha223 Oct 30, 2023
97d37c0
Fixed a few memory leaks
faha223 Oct 31, 2023
93fc4a0
qemu_access: check for Read and Write access
faha223 Nov 3, 2023
a8a9678
Merge branch 'master' of https://github.com/xemu-project/xemu into xm…
faha223 Nov 23, 2023
9d43196
Run clang-format
antangelo Dec 17, 2023
51422f0
Remove unused xemu_new_xmu declaration
antangelo Dec 17, 2023
60c2362
Merge remote-tracking branch 'upstream/master' into xmu-settings2
antangelo Dec 17, 2023
4677199
Fix use after free in rebind code
antangelo Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions config_spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,51 @@ input:
port2: string
port3: string
port4: string
peripherals:
port1:
peripheral_type_0:
Copy link
Member

@mborgerson mborgerson Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: The schema could be a little smaller

Some thoughts: genconfig is the config system used in xemu

  • genconfig does support arrays, which could simplify this schema a bit, but might complicate the code a little too
  • genconfig system has an enum type which would be preferable for config checking, or a string type could be used to identify the device type if necessary, enums in gen-config aren't easily re-used

or if you want to keep it fixed as-is, it could be simplified like:

    port2:
      peripheral_type_0: integer
      peripheral_param_0: string
      peripheral_type_1: integer
      peripheral_param_1: string

nit: The manual refers to these peripheral slots as "Expansion Slot A" (top) and "Expansion Slot B" (bottom, nearest the cable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified it as you suggested. I used the word peripheral as it refers to the device plugged into the expansion slot rather than the slot itself, but I can change this if you'd like

type: integer
default: 0
peripheral_param_0:
type: string
peripheral_type_1:
type: integer
default: 0
peripheral_param_1:
type: string
port2:
peripheral_type_0:
type: integer
default: 0
peripheral_param_0:
type: string
peripheral_type_1:
type: integer
default: 0
peripheral_param_1:
type: string
port3:
peripheral_type_0:
type: integer
default: 0
peripheral_param_0:
type: string
peripheral_type_1:
type: integer
default: 0
peripheral_param_1:
type: string
port4:
peripheral_type_0:
type: integer
default: 0
peripheral_param_0:
type: string
peripheral_type_1:
type: integer
default: 0
peripheral_param_1:
type: string
gamecontrollerdb_path: string
auto_bind:
type: bool
Expand Down
1 change: 1 addition & 0 deletions data/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pfiles = [
'controller_mask.png',
'xmu_mask.png',
'logo_sdf.png',
'xemu_64x64.png',
'abxy.ttf',
Expand Down
Binary file added data/xmu_mask.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
240 changes: 240 additions & 0 deletions ui/xemu-input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include "xemu-notifications.h"
#include "xemu-settings.h"

#include "sysemu/blockdev.h"

// #define DEBUG_INPUT

#ifdef DEBUG_INPUT
Expand Down Expand Up @@ -94,6 +96,20 @@ static const char **port_index_to_settings_key_map[] = {
&g_config.input.bindings.port4,
};

static int* peripheral_types_settings_map[4][2] = {
{ &g_config.input.peripherals.port1.peripheral_type_0, &g_config.input.peripherals.port1.peripheral_type_1 },
{ &g_config.input.peripherals.port2.peripheral_type_0, &g_config.input.peripherals.port2.peripheral_type_1 },
{ &g_config.input.peripherals.port3.peripheral_type_0, &g_config.input.peripherals.port3.peripheral_type_1 },
{ &g_config.input.peripherals.port4.peripheral_type_0, &g_config.input.peripherals.port4.peripheral_type_1 }
};

static const char **peripheral_params_settings_map[4][2] = {
{ &g_config.input.peripherals.port1.peripheral_param_0, &g_config.input.peripherals.port1.peripheral_param_1 },
{ &g_config.input.peripherals.port2.peripheral_param_0, &g_config.input.peripherals.port2.peripheral_param_1 },
{ &g_config.input.peripherals.port3.peripheral_param_0, &g_config.input.peripherals.port3.peripheral_param_1 },
{ &g_config.input.peripherals.port4.peripheral_param_0, &g_config.input.peripherals.port4.peripheral_param_1 }
};

static int sdl_kbd_scancode_map[25];

void xemu_input_init(void)
Expand All @@ -113,6 +129,10 @@ void xemu_input_init(void)
new_con->type = INPUT_DEVICE_SDL_KEYBOARD;
new_con->name = "Keyboard";
new_con->bound = -1;
new_con->PeripheralTypes[0] = PERIPHERAL_NONE;
new_con->PeripheralTypes[1] = PERIPHERAL_NONE;
new_con->Peripherals[0] = NULL;
new_con->Peripherals[1] = NULL;

sdl_kbd_scancode_map[0] = g_config.input.keyboard_controller_scancode_map.a;
sdl_kbd_scancode_map[1] = g_config.input.keyboard_controller_scancode_map.b;
Expand Down Expand Up @@ -178,6 +198,25 @@ int xemu_input_get_controller_default_bind_port(ControllerState *state, int star
return -1;
}

void xemu_save_peripheral_settings(int player_index, int peripheral_index, int peripheral_type, const char *peripheral_parameter)
{
fprintf(stderr, "saving peripheral settings for port %d%c: type = %d, filename = %s\r\n", player_index+1, 'A' + peripheral_index, peripheral_type, peripheral_parameter == NULL ? "NULL" : peripheral_parameter);

int *peripheral_type_ptr = peripheral_types_settings_map[player_index][peripheral_index];
const char **peripheral_param_ptr = peripheral_params_settings_map[player_index][peripheral_index];

if(peripheral_type_ptr != NULL && peripheral_param_ptr != NULL)
faha223 marked this conversation as resolved.
Show resolved Hide resolved
{
faha223 marked this conversation as resolved.
Show resolved Hide resolved
*peripheral_type_ptr = peripheral_type;
if(*peripheral_param_ptr != NULL)
free(*peripheral_param_ptr);
*peripheral_param_ptr =
peripheral_parameter == NULL ? strdup("") : strdup(peripheral_parameter);
faha223 marked this conversation as resolved.
Show resolved Hide resolved
}

fprintf(stderr, "saved peripheral settings for port %d%c: type = %d, filename = %s\r\n", player_index+1, 'A' + peripheral_index, peripheral_type, peripheral_parameter == NULL ? "NULL" : peripheral_parameter);
faha223 marked this conversation as resolved.
Show resolved Hide resolved
}

void xemu_input_process_sdl_events(const SDL_Event *event)
{
if (event->type == SDL_CONTROLLERDEVICEADDED) {
Expand All @@ -202,6 +241,10 @@ void xemu_input_process_sdl_events(const SDL_Event *event)
new_con->sdl_joystick_id = SDL_JoystickInstanceID(new_con->sdl_joystick);
new_con->sdl_joystick_guid = SDL_JoystickGetGUID(new_con->sdl_joystick);
new_con->bound = -1;
new_con->PeripheralTypes[0] = PERIPHERAL_NONE;
new_con->PeripheralTypes[1] = PERIPHERAL_NONE;
new_con->Peripherals[0] = NULL;
new_con->Peripherals[1] = NULL;

char guid_buf[35] = { 0 };
SDL_JoystickGetGUIDString(new_con->sdl_joystick_guid, guid_buf, sizeof(guid_buf));
Expand Down Expand Up @@ -255,6 +298,30 @@ void xemu_input_process_sdl_events(const SDL_Event *event)
char buf[128];
snprintf(buf, sizeof(buf), "Connected '%s' to port %d", new_con->name, port+1);
xemu_queue_notification(buf);

// Try to bind peripherals back to controller
enum peripheral_type peripheralType = PERIPHERAL_NONE;
const char *param = NULL;
faha223 marked this conversation as resolved.
Show resolved Hide resolved

for(int i = 0; i < 2; i++)
{
peripheralType = (enum peripheral_type)(*peripheral_types_settings_map[port][i]);
faha223 marked this conversation as resolved.
Show resolved Hide resolved
param = *peripheral_params_settings_map[port][i];

if(peripheralType == PERIPHERAL_XMU) {
if(param != NULL && strlen(param) > 0) {
// This is an XMU and needs to be bound to this controller
bound_controllers[port]->PeripheralTypes[i] = (enum peripheral_type)peripheralType;
bound_controllers[port]->Peripherals[i] = malloc(sizeof(XmuState));
memset(bound_controllers[port]->Peripherals[i], 0, sizeof(XmuState));
xemu_input_bind_xmu(port, i, param);

char buf[128];
snprintf(buf, 128, "Connected XMU %s to port %d%c", param, port + 1, 'A' + i);
faha223 marked this conversation as resolved.
Show resolved Hide resolved
xemu_queue_notification(buf);
}
}
}
}
} else if (event->type == SDL_CONTROLLERDEVICEREMOVED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to unbind the XMU and free the peripherals in this path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Are you asking in relation to the SDL_CONTROLLERDEVICEREMOVED event or the xemu_input_rebind_xmu call?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in relation to the device removed event. When the device is removed, the controller state is freed, so any allocated peripherals will be leaked.

Copy link
Contributor Author

@faha223 faha223 Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It will unbind the XMU/peripherals when we call xemu_input_bind(iter->bind, NULL, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think I see what you mean. It's not the QEMU device that's being leaked, it's the memory for the peripheral state that's being leaked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing it though, the XmuState structs themselves are not freed by xemu_input_unbind_xmu (they are only freed in the UI if the user selects None under the peripheral).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm beginning to think that maybe I should change that. I'm going through and fixing these memory leaks and running out of places where I'm not freeing the XmuState immediately after unbinding it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to leave the freeing of the XmuState structs out of the xemu_input_unbind_xmu function because there are 2 places where I want to unbind the XMU but I don't want to free it: When deselecting the image, and when changing the image. Another option would be to add a boolean argument to the function that lets the caller specify whether or not to free the XmuState and set the peripheral type to PERIPHERAL_NONE

DPRINTF("Controller Removed: %d\n", event->cdevice.which);
Expand Down Expand Up @@ -430,6 +497,11 @@ void xemu_input_bind(int index, ControllerState *state, int save)
if (bound_controllers[index]) {
assert(bound_controllers[index]->device != NULL);
Error *err = NULL;

// Unbind any XMUs
for(int i = 0; i < 2; i++)
xemu_input_unbind_xmu(index, i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to free the XMU/peripherals in this path?

Copy link
Contributor Author

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 I understand what you're asking.

When qdev_unplug is called in xemu_input_bind to free up the underlying QDEV device, it implicitly frees any peripherals connected to the hub. So, we could just set the peripherals[0]->dev and peripherals[1]->dev to NULL and peripheral_types[0] and peripheral_types[1] to PERIPHERAL_NONE. We can't bind them back again when we plug the new controller in though because qdev_unplug doesn't release the XMU image files immediately.

This gave me an idea though.

It might be useful if, instead of freeing the entire QEMU device when we swap controllers, we just reuse the same unerlying QEMU device and move it to the new ControllerState.

This is what it would look like:

If bound_controllers[index] is not null and state is not null, then we're going to be replacing an existing device with a new device. So, instead of freeing the existing device and creating a new one, we call a swap function that does the following:

  1. If state is already bound to another port then call xemu_input_bind(state->bound, NULL, 1) to unbind it from the other port.
  2. Set state->device to bound_controllers[index]->device
  3. Set state->bound to bound_controllers[index]->bound
  4. Set bound_controllers[index]->device to NULL
  5. Iterate over bound_controllers[index]->periperals and bound_controllers[index]->peripheral_types
    4a. for each expansion slot, copy the peripheral and peripheral_type from bound_controllers[index] to state, then set bound_controllers[index]->peripherals[expansion_slot] to NULL and set bound_controllers[index]->peripheral_types[expansion_slot] to PERIPHERAL_NONE
  6. Set bound_controllers[index]->bound to -1
  7. Set bound_controllers[index] to state
  8. Save the changes. In the config, only the controller should have changed.

Concerns:
In this instance, if we call xemu_input_bind(index, bound_controllers[index], 1) in the current implementation, then it would free the exiting XID device from QEMU and create a new one. If we do that with this proposed implementation without any changes then we'll corrupt the ControllerState for this port. So, a check for bound_controllers[index] == state will need to be done and we'll need to decide to either leave the device alone (which is not what we currently do but which would preserve the state of perpherals), or do what we currently do which is free up the device (and the connected peripherals), and then reconnect the device, losing the peripherals in the process.

Any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Initially I thought this might be another leak like the above case, but on second inspection it might not be since the underlying state isn't freed.

From a technical standpoint I don't see an issue with your idea. From a UX standpoint, the current behavior when switching controllers is similar to switching on real hardware (i.e. unplugging one controller and plugging in another one). I don't suspect most users would care about preserving this, but it does add an extra step if this is wanted behavior.

If you did want to attempt it, I would suggest sending it as a separate patch and getting Matt's opinion on the UX (leaving out the peripheral part of it and adjusting one or the other patch appropriately depending on which lands first).


qdev_unplug((DeviceState *)bound_controllers[index]->device, &err);
assert(err == NULL);

Expand Down Expand Up @@ -507,6 +579,121 @@ void xemu_input_bind(int index, ControllerState *state, int save)
}
}

void xemu_input_bind_xmu(int player_index, int peripheral_port_index, const char *filename)
{
assert(player_index >= 0 && player_index < 4);
assert(peripheral_port_index >= 0 && peripheral_port_index < 2);

ControllerState *player = bound_controllers[player_index];
enum peripheral_type peripheralType = player->PeripheralTypes[peripheral_port_index];
if(peripheralType != PERIPHERAL_XMU)
return;

XmuState *xmu = (XmuState*)player->Peripherals[peripheral_port_index];

// Unbind existing XMU
if(xmu->dev != NULL) {
xemu_input_unbind_xmu(player_index, peripheral_port_index);
}

if(filename == NULL)
return;

// Look for any other XMUs that are using this file, and unbind them
for(int player_i = 0; player_i < 4; player_i++) {
ControllerState *state = bound_controllers[player_i];
if(state != NULL) {
for(int peripheral_i = 0; peripheral_i < 2; peripheral_i++) {
if(state->PeripheralTypes[peripheral_i] == PERIPHERAL_XMU) {
XmuState *xmu_i = (XmuState*)state->Peripherals[peripheral_i];
assert(xmu_i);

if(xmu_i->filename != NULL && strcmp(xmu_i->filename, filename) == 0) {
char buf[128];
snprintf(buf, sizeof(buf), "This XMU is already mounted on player %d slot %c\r\n", player_i+1, 'A' + peripheral_i);
xemu_queue_notification(buf);
return;
}
}
}
}
}

xmu->filename = strdup(filename);
faha223 marked this conversation as resolved.
Show resolved Hide resolved

const int port_map[4] = {3, 4, 1, 2};
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated with xemu_input_bind, could you refactor this mapping out so it can be shared by both functions in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled this out into a static const in[] and referenced that in both places

const int xmu_map[2] = {2, 3};
char *tmp;

static int id_counter = 0;
tmp = g_strdup_printf("xmu_%d", id_counter++);

// Add the file as a drive (fixme: Specify the format as raw so that qemu doesn't have to guess)
faha223 marked this conversation as resolved.
Show resolved Hide resolved
QDict *qdict1 = qdict_new();
qdict_put_str(qdict1, "id", tmp);
qdict_put_str(qdict1, "format", "raw");
qdict_put_str(qdict1, "file", filename);

QemuOpts *drvopts = qemu_opts_from_qdict(qemu_find_opts("drive"), qdict1, &error_abort);

xmu->dinfo = drive_new(drvopts, 0, &error_abort);
assert(xmu->dinfo);

// Create the usb-storage device
QDict *qdict2 = qdict_new();

// Specify device driver
qdict_put_str(qdict2, "driver", "usb-storage");

// Specify device identifier
qdict_put_str(qdict2, "drive", tmp);
g_free(tmp);

// Specify index/port
tmp = g_strdup_printf("1.%d.%d", port_map[player_index], xmu_map[peripheral_port_index]);
qdict_put_str(qdict2, "port", tmp);
g_free(tmp);

// Create the device
QemuOpts *opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict2, &error_abort);

DeviceState *dev = qdev_device_add(opts, &error_abort);
assert(dev);

xmu->dev = (void*)dev;

// Unref for eventual cleanup
qobject_unref(qdict1);
qobject_unref(qdict2);

xemu_save_peripheral_settings(player_index, peripheral_port_index, player->PeripheralTypes[peripheral_port_index], xmu->filename);
}

void xemu_input_unbind_xmu(int player_index, int peripheral_port_index)
{
assert(player_index >= 0 && player_index < 4);
assert(peripheral_port_index >= 0 && peripheral_port_index < 2);

ControllerState *state = bound_controllers[player_index];
if(state->PeripheralTypes[peripheral_port_index] != PERIPHERAL_XMU)
return;

XmuState *xmu = (XmuState*)state->Peripherals[peripheral_port_index];
if(xmu != NULL)
{
if(xmu->dev != NULL) {
qdev_unplug((DeviceState*)xmu->dev, &error_abort);
object_unref(OBJECT(xmu->dev));
xmu->dev = NULL;
}

if(xmu->filename != NULL) {
free(xmu->filename);
xmu->filename = NULL;
}
}
}

void xemu_input_set_test_mode(int enabled)
{
test_mode = enabled;
Expand All @@ -516,3 +703,56 @@ int xemu_input_get_test_mode(void)
{
return test_mode;
}

#define FATX_SIGNATURE 0x58544146
faha223 marked this conversation as resolved.
Show resolved Hide resolved

// This is from libfatx
#pragma pack(1)
struct fatx_superblock {
uint32_t signature;
uint32_t volume_id;
uint32_t sectors_per_cluster;
uint32_t root_cluster;
uint16_t unknown1;
uint8_t padding[4078];
};
#pragma pack()

bool xemu_new_xmu(const char* filename, unsigned int size)
faha223 marked this conversation as resolved.
Show resolved Hide resolved
{
unsigned int PartitionId = (unsigned int)rand();
unsigned int SectorsPerCluster = 0x04;
unsigned int RootDirectoryCluster = 0x01;
unsigned char zero = 0x00;
unsigned int empty_fat = 0xfffffff8;

FILE *fp = fopen(filename, "wb");
faha223 marked this conversation as resolved.
Show resolved Hide resolved
if(fp != NULL)
{
struct fatx_superblock superblock;
memset(&superblock, 0xff, sizeof(struct fatx_superblock));

superblock.signature = FATX_SIGNATURE;
superblock.sectors_per_cluster = 4;
superblock.volume_id = PartitionId;
superblock.root_cluster = 1;
superblock.unknown1 = 0;

// Write the fatx superblock.
fwrite(&superblock, sizeof(superblock), 1, fp);

// Write the FAT
fwrite(&empty_fat, sizeof(empty_fat), 1, fp);

// Fill the rest of the space with zeros
for(unsigned int i = ftell(fp); i < size; i++)
fwrite(&zero, 1, 1, fp);

fflush(fp);
fclose(fp);

return true;
}

return false;
}
Loading