UI: XMU Settings #1315

faha223 · 2023-01-02T19:40:12Z

Added support for creating and mounting/unmounting XMU images in the Input settings menu.

ui/xemu-input.c

faha223 · 2023-01-08T01:49:39Z

This is related to these issues:
#1128
#389

Merged recent changes from xemu-project to master

Merged recent changes from remote

ui/xui/main-menu.cc

ui/xemu-input.c

ui/xui/main-menu.cc

…xmu-settings2

antangelo

A few more comments, mostly nits

ui/xemu-input.c

ui/xemu-input.h

ui/xui/main-menu.cc

antangelo · 2023-06-08T01:03:37Z

ui/xui/main-menu.cc

+            selectedType = bound_state->PeripheralTypes[i];
+            if(selectedType == PERIPHERAL_XMU) {
+                // We are using the same texture for all buttons, but ImageButton
+                // uses the texture as a unique ID. Push a new ID now to resolve


Is this referring to the ID that is pushed later in this block (as opposed to now)?

Yes. I've moved this comment to the relevant code.

ui/xui/main-menu.cc

Updating my master branch to match xemu master

Merged the recent changes from master to xmu-settings2

mborgerson · 2023-06-08T23:44:43Z

ui/thirdparty/fatx/fatx.c

+        superblock.unknown1 = 0;
+
+        // Write the fatx superblock.
+        fwrite(&superblock, sizeof(superblock), 1, fp);


This implementation depends on host byte order being little endian. Can you rework it so it will also work on BE hosts?

I have changed the code to use cpu_to_le32(u32) on the unsigned integers that were being written to the superblock. I think that will work but I don't have a BE machine to test on. It doesn't break this functionality on my machine.

mborgerson · 2023-06-08T23:49:53Z

ui/thirdparty/fatx/fatx.c

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


We can avoid this loop and writing a bunch of zeros with seek/truncate, which will create a sparse file (logically, but not physically allocating space for all the zeros)

I changed this to fseek to size - sizeof(unsigned byte), and I just write one byte of 0x00 at the end. I initially tried ftruncate(size) but that didn't do the job.

mborgerson · 2023-06-08T23:53:08Z

ui/xemu-input.c

+    xmu->dev = (void*)dev;
+
+    // Unref for eventual cleanup
+    //g_free(dinfo);


dinfo is allocated in drive_new by a call to g_malloc0. I don't use it again, but I'm not sure what to do with it. Calling g_free here on dinfo causes a segfault when unmounting the XMU image. I didn't mean to leave this commented.

It's tied to the blockdev and freed when the blockdev is deleted. You shouldn't have to do anything

ui/xemu-input.c

mborgerson · 2023-06-09T00:10:19Z

config_spec.yml

@@ -25,6 +25,51 @@ input:
    port2: string
    port3: string
    port4: string
+  peripherals:
+    port1:
+      peripheral_type_0:


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)

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

mborgerson · 2023-06-09T00:25:27Z

ui/xemu-input.c

+
+    xmu->filename = strdup(filename);
+
+    const int port_map[4] = {3, 4, 1, 2};


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

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

mborgerson · 2023-06-09T00:38:36Z

ui/xui/main-menu.cc

+                            g_free(msg);
+                        } else {
+                            // Create an 8MB formatted XMU image. We can create smaller or larger ones, but 8 MB is the default
+                            if (create_fatx_image(new_path, 8*1024*1024)) {


Let's move this number out to a constant definition and drop the above comment

I moved this to a preprocessor definition called DEFAULT_XMU_SIZE

mborgerson · 2023-06-09T00:39:33Z

ui/xui/main-menu.cc

+                    ImGui::GetCursorPosX() +
+                    (int)((ImGui::GetColumnWidth() - xmu_display_size.x) / 2.0));
+
+                ImGui::Text("Peripheral %c", 'A' + i);


"Expansion Slot" is the manual terminology

I changed the labels in the UI to follow the Expansion Slot wording, but I left the variables saying peripheral as they refer to the device rather than the slot in which the device is installed

mborgerson · 2023-06-09T00:45:56Z

ui/xui/main-menu.cc

+                        if (qemu_access(new_path, F_OK) == 0) {
+                            // The file already exists
+                            char *msg = g_strdup_printf("%s already exists", new_path);
+                            xemu_queue_notification(msg);


There's xemu_queue_error_message for errors

Thanks, I changed over the xemu_queue_notification calls to xemu_queue_error_message where I was using it for error messages.

mborgerson · 2023-06-09T00:47:23Z

ui/xui/main-menu.cc

+                g_free((void*)id);
+                g_free((void*)xmu_port_path);
+
+                ImGui::PushID(i+2); // Add 2 because ID value of i was already used and the for loop iterating on i stops at 2


Should be able to have just the one PushID at the start and PopID at the end of the loop, dropping the middle Pop/Push

I removed those lines and it doesn't appear to have broken anything

mborgerson · 2023-06-12T08:12:31Z

ui/xui/main-menu.cc

+                        if (qemu_access(new_path, F_OK) == 0) {
+                            // The file already exists
+                            char *msg = g_strdup_printf("%s already exists", new_path);
+                            xemu_queue_error_message(msg);


As an alternative to showing an error message in this way, we can request that the native file dialog prompt users for overwrite by adding the flag NOC_FILE_DIALOG_OVERWRITE_CONFIRMATION with a bitwise OR above on line 383

This doesn't appear to work on Windows. I changed it to work this way, but when testing it just overwrites the file without a confirmation and, after looking through the implementation of that function, it doesn't appear that NOC_FILE_DIALOG_OVERWRITE_CONFIRMATION is used in the Windows implementation.

Oh wow, you are right! It looks like it's also missing on macOS, or maybe its the default.

On Windows we will need to support NOC_FILE_DIALOG_OVERWRITE_CONFIRMATION and if set pass the OFN_OVERWRITEPROMPT flag in when creating the save prompt. Can you try it?

That worked, I committed the change

mborgerson · 2023-06-12T08:14:47Z

ui/xui/main-menu.cc

+                    xmu_port_path = g_strdup("");
+                else 
+                    xmu_port_path = g_strdup(xmu->filename);
+                if (FilePicker(id, &xmu_port_path, img_file_filters)) {


If the user presses the "X" button on the file picker widget, this will cause xemu to crash because file will be unspecified when binding the xmu. In this case, we can unbind the xmu and eliminate the need for the "Clear Selection" button below

I made the X button unbind the XMU and clear the filename, and I removed the "Clear Selection" button

mborgerson · 2023-06-12T08:25:30Z

ui/xemu-input.c

@@ -255,6 +293,39 @@ 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
+            for(int i = 0; i < 2; i++) {


Unfortunately binding here wont detect the keyboard binds. We can probably just refactor this code out and add a call in xemu_input_bind

I refactored the code into a new function called xemu_input_rebind_xmu(int port) but calling it from xemu_input_bind causes xemu to crash when switching between Keyboard and Xbox Controller. This is because unbinding the controller unplugs the XMUs, and trying to bind the XMUs again right after unplugging them causes an error because the file is still in use.

Instead I call this function where the code was originally and also on line 180 just after it auto-binds the keyboard.

mborgerson · 2023-06-12T08:25:40Z

ui/xemu-input.c

+
+            // Try to bind peripherals back to controller
+            for(int i = 0; i < 2; i++) {
+                enum peripheral_type peripheralType = (enum peripheral_type)(*peripheral_types_settings_map[port][i]);


I renamed this variable to peripheral_type, though that might be confusing because it's the same as its type.

mborgerson · 2023-06-12T08:29:10Z

ui/xui/main-menu.cc

+                    RenderXmu(x, y, 0x1f1f1f00, 0x0f0f0f00);
+                }
+
+                ImVec2 cur = ImGui::GetCursorPos();


Set but unused

mborgerson · 2023-06-12T08:36:02Z

ui/xui/main-menu.cc

+                    ImGui::GetCursorPosX() +
+                    (int)((ImGui::GetColumnWidth() - xmu_display_size.x) / 2.0));
+
+                ImGui::Text("Expansion Slot %c", 'A' + i);


Let's drop this extra text

mborgerson · 2023-06-12T08:37:28Z

ui/xui/main-menu.cc

+                    }
+                }
+
+                const char *id = g_strdup_printf("MU_%d%c File Path", active + 1, 'A' + i);


Let's change this to something simpler, like "XMU" or "Image" or something. The FilePicker icon already tells the user that it's a file path

I changed it from "MU_1A File Path" to "Image"

antangelo

Please run clang-format on the code (see https://xemu.app/docs/dev/#contributing for details on how to do that).

There are also a few spots where camelCase variable naming conventions are used, can you update these to follow xemu's lower_snake_case convention?

antangelo · 2023-10-29T00:39:26Z

ui/xemu-input.c

@@ -254,6 +293,7 @@ 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);
+            xemu_input_rebind_xmu(port);
        }
    } else if (event->type == SDL_CONTROLLERDEVICEREMOVED) {


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

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

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.

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

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.

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).

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

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

antangelo · 2023-10-29T00:39:42Z

ui/xemu-input.c

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


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

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:

If state is already bound to another port then call xemu_input_bind(state->bound, NULL, 1) to unbind it from the other port.

Set state->device to bound_controllers[index]->device

Set state->bound to bound_controllers[index]->bound

Set bound_controllers[index]->device to NULL

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

Set bound_controllers[index]->bound to -1

Set bound_controllers[index] to state

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?

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).

antangelo · 2023-10-29T00:47:35Z

ui/xemu-input.c

+                // This is an XMU and needs to be bound to this controller
+                if(qemu_access(param, F_OK) == 0) {
+                    bound_controllers[port]->peripheral_types[i] = peripheral_type;
+                    bound_controllers[port]->peripherals[i] = malloc(sizeof(XmuState));


malloc is used to allocate peripherals here, but g_malloc is used in the UI. They must be consistent.

antangelo · 2023-10-29T00:55:52Z

ui/xemu-input.c

@@ -460,7 +505,6 @@ void xemu_input_bind(int index, ControllerState *state, int save)
        bound_controllers[index] = state;
        bound_controllers[index]->bound = index;

-        const int port_map[4] = {3, 4, 1, 2};
        char *tmp;

        // Create controller's internal USB hub.


In my testing if you disconnect and reconnect a controller in the input UI the XMU is not rebound. Do we need to rebind peripherals here?

This is how it behaves when we unplug a device and plug it back in with auto-bind enabled so we should probably do that here as well.

If I remember correctly, this will cause Xemu to crash if the user selects a different controller with one already connected though because QEMU will not have finished releasing the XMU image after freeing the existing controller when it goes to rebind the XMU to the hub created for the new controller. If we want to do this then we might want to consider reusing the QEMU device that's already bound when changing from one controller to another instead freeing it and creating a new one.

ui/xui/main-menu.cc

ui/xemu-input.c

antangelo · 2023-10-30T22:28:55Z

ui/xui/main-menu.cc

+                                // Another peripheral was already bound.
+                                // Unplugging
+                                xemu_input_unbind_xmu(active, i);
+                            }


Do we need to free bound_state->peripherals[i] here?

Yes, otherwise the existing XmuState will be lost when a new one is allocated

…u-settings2

antangelo · 2023-12-18T06:38:00Z

I've created #1572 to track the detach/reattach issue as a followup

…ect#1315)" This reverts commit 03f40b1. Revert "ui: Warn user about no output when AV Pack set to "None"" This reverts commit 800eb46. Revert "ui: Use only one option for settings window (xemu-project#1122)" This reverts commit b605381.

faha223 added 2 commits December 31, 2022 00:00

Added XMU Settings to the Input Screen

96157f6

Added Peripherals to config

766409f

GXTX reviewed Jan 3, 2023

View reviewed changes

ui/xemu-input.c Outdated Show resolved Hide resolved

faha223 changed the title ~~Xmu settings2~~ UI: XMU Settings Jan 5, 2023

faha223 requested a review from GXTX January 7, 2023 21:25

Prevent overwriting existing XMUs

3f9b265

Triticum0 mentioned this pull request Jan 9, 2023

Adding missing peripherals #389

Open

8 tasks

faha223 added 3 commits January 10, 2023 16:41

Added blockdev.h to try to fix the MacOS build

8e3296b

Merge pull request #12 from xemu-project/master

51fe070

Merged recent changes from xemu-project to master

Merge pull request #15 from xemu-project/master

21d467a

Merged recent changes from remote

antangelo reviewed Jan 18, 2023

View reviewed changes

faha223 added 6 commits January 18, 2023 21:09

Fixed some issues that antangelo pointed out

b1f9085

Moved the peripheralType and param vars into the loop

a9747a0

Merge branch 'master' into xmu-settings2

ac3b36d

Moved fatx.h and fatx.c to ui\thirdparty\fatx

485cb9c

Merge branch 'xmu-settings2' of https://github.com/faha223/xemu into …

bd1e773

…xmu-settings2

Added Validation for Peripheral Settings

bc80676

antangelo reviewed Jun 8, 2023

View reviewed changes

faha223 added 5 commits June 8, 2023 18:42

Fixed some nits that were pointed out

1e0595e

Merge pull request #18 from xemu-project/master

f397226

Updating my master branch to match xemu master

Merge branch 'xmu-settings2' into master

8423130

Merge pull request #19 from faha223/master

70881db

Merged the recent changes from master to xmu-settings2

don't pass NULL into xemu_settings_set_string

afe5146

mborgerson reviewed Jun 8, 2023

View reviewed changes

mborgerson reviewed Jun 9, 2023

View reviewed changes

ui/xemu-input.c Outdated Show resolved Hide resolved

mborgerson reviewed Jun 9, 2023

View reviewed changes

Changes following Matt's recommendations

5def7ad

mborgerson reviewed Jun 12, 2023

View reviewed changes

faha223 added 7 commits June 12, 2023 19:21

Changes to XMU FilePicker

f966217

XMU image auto-bind logic refactor

3167c43

renamed peripheralType to peripheral_type

8889047

removed unnecessary calls to g_strdup_printf and g_free

85b7071

Cleaned up some comments, removed an unnecessary variable

32fe45a

handle overwrite prompt in Windows

a7f9905

Merge branch 'master' into xmu-settings2

e372b5a

antangelo self-assigned this Oct 18, 2023

antangelo reviewed Oct 29, 2023

View reviewed changes

faha223 added 2 commits October 30, 2023 00:36

Fixed some code format and style inconsistencies

498a44a

More formatting fixes

e74066e

antangelo reviewed Oct 30, 2023

View reviewed changes

faha223 added 3 commits October 30, 2023 23:03

Fixed a few memory leaks

97d37c0

qemu_access: check for Read and Write access

93fc4a0

Merge branch 'master' of https://github.com/xemu-project/xemu into xm…

a8a9678

…u-settings2

dekay mentioned this pull request Dec 9, 2023

Xbox Controller Memory card mborgerson/fatx#57

Open

antangelo added 4 commits December 17, 2023 14:21

Run clang-format

9d43196

Remove unused xemu_new_xmu declaration

51422f0

Merge remote-tracking branch 'upstream/master' into xmu-settings2

60c2362

Fix use after free in rebind code

4677199

antangelo merged commit 03f40b1 into xemu-project:master Dec 18, 2023
15 checks passed

antangelo mentioned this pull request Dec 18, 2023

XMU Devices are not preserved when switching between controllers on an input #1572

Open

faha223 deleted the xmu-settings2 branch March 10, 2024 02:53


		xmu->filename = strdup(filename);

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

UI: XMU Settings #1315

UI: XMU Settings #1315

Conversation

faha223 commented Jan 2, 2023

faha223 commented Jan 8, 2023

antangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mborgerson Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mborgerson Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faha223 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faha223 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antangelo commented Dec 18, 2023

mborgerson Jun 9, 2023 •

edited

Loading

mborgerson Jun 9, 2023 •

edited

Loading

faha223 Oct 30, 2023 •

edited

Loading

faha223 Oct 30, 2023 •

edited

Loading