GCAdapter: Code cleanups. #11187
GCAdapter: Code cleanups. #11187
Conversation
jordan-woyak
force-pushed
the
wiiu-adapter-cleanups
branch
from
27d7451
to
57b3fa2
Compare
| struct PortState | ||
| { | ||
| GCPadStatus origin = {}; | ||
| GCPadStatus status = {}; | ||
|
|
||
| ControllerType controller_type = ControllerType::None; | ||
| bool is_new_connection = false; | ||
| }; |
There was a problem hiding this comment.
I'm not completely sure I understand what the origin field is used for. It roughly seems like it's only used once, and in that situation status wouldn't have been used. What problem is it trying to solve?
There was a problem hiding this comment.
It solves the future problem of ControllerInterface reading inputs simultaneously with CSIDevice_GCAdapter. CSIDevice_GCAdapter needs the "origin" after the "controller_type" is reset so it needs to be saved.
In my ControllerInterface GCAdapter PR I'm adding a PeekInput function to read status without changing the is_new_connection flag.
The whole GET_ORIGIN thing is a mess, but it's all tied into NetPlay and I'm trying to avoid touching all that fragile code.
There was a problem hiding this comment.
That sounds reasonable (though I'm still not entirely sure why it would need to care about a specific origin value - if the stick isn't centered on startup, bad things are always going to happen, and the same would make sense if the user immediately messed with it after startup I think...)
There was a problem hiding this comment.
Some games seem to actually use the "origin" value and can function properly with a non-centered stick on attachment (Mario Kart Double Dash seems to be one).
And allegedly the controller "origin" can be read from the GCAdapter but we aren't doing that yet: https://bugs.dolphin-emu.org/issues/12956
It's a bit strange now to store the origin separately when it's currently just a random pad state but It should make more sense if the issue's suggestion is implemented.
jordan-woyak
force-pushed
the
wiiu-adapter-cleanups
branch
from
57b3fa2
to
a710a51
Compare
jordan-woyak
force-pushed
the
wiiu-adapter-cleanups
branch
from
a710a51
to
5ed0543
Compare
| // Current adapter status: detected/not detected/in error (libusb error codes are negative) | ||
| static std::atomic<int> s_status = NO_ADAPTER_DETECTED; | ||
| static std::atomic<AdapterStatus> s_status = AdapterStatus::NotDetected; | ||
| static std::atomic<libusb_error> s_adapter_error = LIBUSB_SUCCESS; |
There was a problem hiding this comment.
Another thought here - does it make sense to have 2 atomic variables like this? I don't think it'd be atomic anymore when done this way. Though IsDetected also reads (and previously read) s_status multiple times. (You were the one who initially made s_status atomic in #8933, for what it's worth; also see #10540 and #10604.)
Other than that, looks good to me.
There was a problem hiding this comment.
They need to be at least atomic because they are read/written from different threads.
Even though their combined usage is not atomic it should be fine since it just generates UI error text.
A race should ultimately resolve to the correct UI message after the next adapter callback.
This is in preparation for #10489 (GCAdapter ControllerInterface support).
The main thing is processing input in the read thread rather than the
Inputfunction so device changes and input work when a game isn't running.I also did some other general cleanups.