Refactor GCAdapter, part 1 #10604

Pokechu22 · 2022-04-23T19:44:42Z

In particular, the android and non-android versions are now in a single file, with the parts where they differ separated using the preprocessor. I also removed some old special cases, which may or may not be relevant.

I don't own a GC adapter, so this PR has not been tested at all (and thus I'm submitting it as a draft). Further work will probably be done as a separate PR.

Pokechu22 · 2022-04-23T20:00:34Z

I'm dropping b84b146, as it still causes issues with the freebsd builder.

pizuz · 2022-04-24T08:23:40Z

Working fine on macOS Catalina x86_64.

shuffle2 · 2022-04-25T02:02:10Z

tbh i think splicing 2 files into 1 with a bunch of #ifdefs is a step backward.

Pokechu22 · 2022-04-25T02:10:17Z

I think the current 2-file version with large amounts of overlapping code is pretty bad, especially since the different files diverged for various bugfixes and other behaviors. But it would make sense to eventually split out the libusb and android code into other files, once everything else is merged nicely.

Code like

#ifndef ANDROID
  if (s_handle == nullptr)
    return;
#else
  if (!s_detected || !s_fd)
    return;
#endif

is something that I wouldn't want to keep permanently, but is a bit awkward to merge together.

(I'm also not sure what the deal is with s_fd on android, as it's obtained from Java code, but then the value is never sent back to the Java code or used for any IO operations - it seems like it's effectively just treated as a (redundant?) boolean flag.)

jordan-woyak · 2022-05-06T01:35:27Z

The ifdefs are unfortunate but better than completely separate files. At least the duplicate code is merged.

Ideally I think we'd want an interface for the basic libusb functionality with Android/PC implementations.

JMC47 · 2022-05-22T04:12:51Z

This fixes LibUSB issues on certain hardware, so we should probably get this cleaned up and reviewed.

JaredWhiteOne · 2022-05-22T04:27:42Z

Tested on Windows with the Super Smash Bros. GameCube Adapter for Wii U and MAYFLASH 2 Ports Wii U GC GameCube Controller Adapter for Wii U/PC. Adapter was detected in settings and controller was working as intended.

Pokechu22 · 2022-05-23T05:28:34Z

I've got an adapter now and have done some testing, and things seem good on Windows at least. I still need to test on Android, though.

pizuz · 2022-05-23T20:54:18Z

Tested again on macOS Catalina. Adapter working as it should.

Pokechu22 · 2022-06-01T01:14:23Z

I did some testing with the CI build on android, and it seems to work fine there too (although it was a debug build so performance wasn't great). This PR is ready for review.

JMC47 · 2022-06-01T03:19:41Z

This build works fine for me on windows and Android, forgot to comment.

iwubcode · 2022-06-01T04:01:54Z

Need to look over the exit-early commit some more, not super familiar with the input code but from what I could tell all seems like a solid refactor.

Pokechu22 · 2022-06-01T04:06:14Z

That commit's easier to look at if you disable whitespace (git show -w or ?w=1 in the GitHub URL: b69b579?w=1)

iwubcode · 2022-06-01T05:04:11Z

Ah neat, did not know that was a thing. That is much easier to read!

Dentomologist

I'm not done reviewing but wanted to make some comments in case I didn't finish. Mostly nitpicky except for casting the type byte for each controller.

Dentomologist · 2022-04-23T19:50:49Z

Source/Core/InputCommon/GCAdapter.cpp

+#ifndef ANDROID
+#include <libusb.h>
+#else
+#include <jni.h>
+#endif


[nitpick] I think if-else statements are slightly easier to follow when the condition is positive:

#ifdef ANDROID #include <jni.h> #else #include <libusb.h> #endif

There are a few similar occurrences I would swap, if I'm not missing a reason to do it this way.

I was mostly trying to put the non-android code on the top (though there isn't a strong reason for doing so). I could add a second define for using the libusb implementation instead, or I could just swap them. Do you have any preference between these?

Almost called this out as well. I agree about the positiveness being more readable though I also feel like if statements are easier to read if the smaller block is on top.

As for using a second define or swap them, I don't personally have a preference.

In the long run I don't think it really matters because the plan is to split the android USB code and the libusb code into separate files again in a later PR (leaving the common logic in GCAdapter), so it doesn't really matter which comes first. I'll swap them.

... actually, while trying to swap them, I've found that there are a lot of cases where it's #ifndef ANDROID without a corresponding #else (at least initially), so a second define is probably better (otherwise it'll just end up being a mix of #ifdef ANDROID and #ifndef ANDROID which is much more confusing than almost entirely #ifndef ANDROID).

Source/Core/InputCommon/GCAdapter.cpp

Dentomologist · 2022-06-01T18:44:39Z

Source/Core/InputCommon/GCAdapter.cpp

-    int err = libusb_interrupt_transfer(s_handle, s_endpoint_in, s_controller_payload_swap,
+    int err = libusb_interrupt_transfer(s_handle, s_endpoint_in, s_controller_payload_swap.data(),
                                        sizeof(s_controller_payload_swap), &payload_size, 16);


err can be const.

[Opinion/Bikeshed] Not that I expect anyone to be confused by err, but in the spirit of updating unnecessarily abbreviated names from old code this could be error.

Done. I've also added a new fmt::formatter that calls libusb_strerror and libusb_error_name to improve the related log messages.

Dentomologist · 2022-06-01T18:53:03Z

Source/Core/InputCommon/GCAdapter.cpp

-static u8 s_controller_payload[37];
-static u8 s_controller_payload_swap[37];
+static std::array<u8, 37> s_controller_payload;
+static std::array<u8, 37> s_controller_payload_swap;


Since _swap and _copy are defined to have the same size as _payload, would it make sense to make that explicit with std::array<u8, s_controller_payload.size()>?

I'll create constexpr size_t CONTROLER_PAYLOAD_SIZE = 37 instead.

Dentomologist · 2022-06-02T00:13:34Z

Source/Core/InputCommon/GCAdapter.cpp

+    // TODO: What do the other bits here indicate?  Does casting to an enum like this make sense?
+    const auto type = static_cast<ControllerType>(controller_payload_copy[1 + (9 * chan)] >> 4);


I haven't seen any explicit documentation on the adapter's protocol, so flagging this for anybody who knows it better.

From what I can tell everything else assumes 0 <= type <= 2 which implies the top two bits are 0. If that's not necessarily the case we need to deal with that somehow.

iwubcode · 2022-06-02T23:03:26Z

Source/Core/InputCommon/GCAdapter.cpp

@@ -87,10 +87,14 @@ static std::array<u8, SerialInterface::MAX_SI_CHANNELS> s_controller_type = {
    ControllerTypes::CONTROLLER_NONE, ControllerTypes::CONTROLLER_NONE};
 static std::array<u8, SerialInterface::MAX_SI_CHANNELS> s_controller_rumble{};

+constexpr size_t CONTROLER_INPUT_PAYLOAD_EXPECTED_SIZE = 37;


Does it make sense to move these above their respective usages (at least until there are extra scenarios where they are used) so as to improve readability?

They're already used in several places (both for declaring the variables and when reading into them), and IMO it's better to have them all together to highlight that the sizes are different (and that different sizes are used for rumble and init).

Source/Core/InputCommon/GCAdapter.cpp

iwubcode

Untested. LGTM

This is mostly a brainless merge, #ifdef-ing anything that doesn't match between the two while preserving common logic. I didn't rename any variables (although similar ones do exist), but I did change one log that was ERROR on android and NOTICE elsewhere to just always be NOTICE. Further merging will follow.

I believe the setting already existed in the UI; it just wasn't implemented in GCAdapter_Android.cpp.

This hack was added in 8f0cbef, and the part of it in SI_DeviceGCAdapter is present on Android already, so I don't see any reason why this part doesn't apply to Android.

It was removed for non-android in 56239d1, and android already uses a separate thread, so presumably this isn't needed anymore.

This is only so that indentation is consistent with the non-android code.

This was done for Android in 6cc40b1.

Pokechu22 force-pushed the gcadapter-refactor branch 2 times, most recently from 76a12e2 to 6f0d855 Compare April 23, 2022 20:33

Pokechu22 force-pushed the gcadapter-refactor branch from 6f0d855 to f94d28b Compare April 25, 2022 01:54

Pokechu22 force-pushed the gcadapter-refactor branch from f94d28b to 18a743c Compare April 25, 2022 04:58

Pokechu22 force-pushed the gcadapter-refactor branch from 18a743c to e2cc950 Compare May 23, 2022 05:26

Pokechu22 marked this pull request as ready for review May 23, 2022 05:27

Pokechu22 force-pushed the gcadapter-refactor branch from e2cc950 to 2cc944d Compare June 1, 2022 00:36

Dentomologist reviewed Jun 2, 2022

View reviewed changes

Pokechu22 force-pushed the gcadapter-refactor branch 6 times, most recently from b27075c to bcb5969 Compare June 2, 2022 22:38

iwubcode reviewed Jun 2, 2022

View reviewed changes

iwubcode reviewed Jun 3, 2022

View reviewed changes

Source/Core/InputCommon/GCAdapter.cpp Outdated Show resolved Hide resolved

iwubcode approved these changes Jun 3, 2022

View reviewed changes

Pokechu22 added 14 commits June 2, 2022 19:39

GCAdapter: Use std::array for controller read and write payloads

36d4ee0

GCAdapter: Fix rumble enabled config on Android

682d86f

I believe the setting already existed in the UI; it just wasn't implemented in GCAdapter_Android.cpp.

GCAdapter: Convert ControllerType to an enum class

55922e6

GCAdapter: Use determinism hack on Android

3ae775e

This hack was added in 8f0cbef, and the part of it in SI_DeviceGCAdapter is present on Android already, so I don't see any reason why this part doesn't apply to Android.

GCAdapter: Remove unused includes

279888d

GCAdapter: Harmonize read/write thread variable names

2794704

GCAdapter: Remove check on write size on android

749a4ad

It was removed for non-android in 56239d1, and android already uses a separate thread, so presumably this isn't needed anymore.

GCAdapter: Exit early if the adapter fails to open on Android

0fa9269

This is only so that indentation is consistent with the non-android code.

GCAdapter: Have the read thread control the write thread

9ec65ba

This was done for Android in 6cc40b1.

GCAdapter: Merge Read logic

cd9edea

GCAdapter: Make local variables const where possible

0d8772c

LibusbUtils: Create ErrorWrap

457fcba

GCAdapter: Use LibusbUtils::ErrorWrap in log messages

6823b4d

Pokechu22 force-pushed the gcadapter-refactor branch from bcb5969 to 6823b4d Compare June 3, 2022 02:40

JMC47 merged commit 664663e into dolphin-emu:master Jun 4, 2022
10 checks passed

Pokechu22 mentioned this pull request Jun 8, 2022

Improve libusb error logging #10729

Merged

Pokechu22 mentioned this pull request Oct 22, 2022

GCAdapter: Code cleanups. #11187

Merged

Refactor GCAdapter, part 1 #10604

Refactor GCAdapter, part 1 #10604

Pokechu22 commented Apr 23, 2022

Pokechu22 commented Apr 23, 2022

pizuz commented Apr 24, 2022

shuffle2 commented Apr 25, 2022

Pokechu22 commented Apr 25, 2022

jordan-woyak commented May 6, 2022

JMC47 commented May 22, 2022

JaredWhiteOne commented May 22, 2022

Pokechu22 commented May 23, 2022

pizuz commented May 23, 2022

Pokechu22 commented Jun 1, 2022

JMC47 commented Jun 1, 2022

iwubcode commented Jun 1, 2022

Pokechu22 commented Jun 1, 2022

iwubcode commented Jun 1, 2022

Dentomologist left a comment

Dentomologist Apr 23, 2022

Pokechu22 Jun 2, 2022

iwubcode Jun 2, 2022

Pokechu22 Jun 2, 2022

Pokechu22 Jun 2, 2022

Dentomologist Jun 1, 2022

Pokechu22 Jun 2, 2022

Dentomologist Jun 1, 2022

Pokechu22 Jun 2, 2022

Dentomologist Jun 2, 2022

iwubcode Jun 2, 2022

Pokechu22 Jun 2, 2022

iwubcode left a comment

		// TODO: What do the other bits here indicate? Does casting to an enum like this make sense?
		const auto type = static_cast<ControllerType>(controller_payload_copy[1 + (9 * chan)] >> 4);

Refactor GCAdapter, part 1 #10604

Refactor GCAdapter, part 1 #10604

Conversation

Pokechu22 commented Apr 23, 2022

Pokechu22 commented Apr 23, 2022

pizuz commented Apr 24, 2022

shuffle2 commented Apr 25, 2022

Pokechu22 commented Apr 25, 2022

jordan-woyak commented May 6, 2022

JMC47 commented May 22, 2022

JaredWhiteOne commented May 22, 2022

Pokechu22 commented May 23, 2022

pizuz commented May 23, 2022

Pokechu22 commented Jun 1, 2022

JMC47 commented Jun 1, 2022

iwubcode commented Jun 1, 2022

Pokechu22 commented Jun 1, 2022

iwubcode commented Jun 1, 2022

Dentomologist 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwubcode left a comment

Choose a reason for hiding this comment