From: "Rémi Bernon" Subject: Re: [PATCH 5/6] xinput.sys: Create an internal PDO, on the XINPUT bus. Message-Id: <9a06335e-4a09-865c-a68c-dc206d455931@codeweavers.com> Date: Mon, 30 Aug 2021 18:54:09 +0200 In-Reply-To: <9261d117-2adb-37e2-6740-9c297b468c55@codeweavers.com> References: <20210826055904.828578-1-rbernon@codeweavers.com> <20210826055904.828578-5-rbernon@codeweavers.com> <09351bcc-3338-2d03-3256-55e6885031fe@codeweavers.com> <7808547a-0589-aa59-c17a-f4044fe4381e@codeweavers.com> <9261d117-2adb-37e2-6740-9c297b468c55@codeweavers.com> On 8/30/21 6:22 PM, Zebediah Figura (she/her) wrote: > On 8/27/21 3:55 PM, Rémi Bernon wrote: >> On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote: >>>> wcsrchr(device_id, '\\') + 1); >>>>        wcscpy(ext->instance_id, instance_id); >>>> +    ext->interface_guid = !wcsncmp(device_id, L"XINPUT\\", 7) ? >>>> &GUID_DEVINTERFACE_XINPUT >>>> +                                                              : >>>> &GUID_DEVINTERFACE_HID; >>> >>> It's a matter of taste, I guess, but personally if I have to wrap a >>> ternary operator, I take it as a sign I should be using if/else >>> instead ;-) >>> >> >> Yeah, I spent a lot of time hesitating on the best looking way to write >> that. >> >>> It also occurs to me that since this is very internal it might be a good >>> idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to >>> make a difference, of course, but it also sort of communicates that this >>> is internal, and more clearly tells anyone looking for the code where to >>> look for the driver that reports WINEXINPUT devices (viz. elsewhere in >>> the code tree.) >>> >> >> Works for me, I'm still not convinced that winexinput.sys is much >> clearer though, but I can probably live with it. > > I'm fine with xinput.sys, but I'd rather see the device ID named > WINEXINPUT. > That's what I did in the new version, although the driver is also named winexinput.sys now. I think it'd be less confusing that way. >> >>>> @@ -52,14 +54,24 @@ struct device_state >>>>    { >>>>        DEVICE_OBJECT *bus_pdo; >>>>        DEVICE_OBJECT *gamepad_pdo; >>>> +    DEVICE_OBJECT *xinput_pdo; >>>>        WCHAR bus_id[MAX_DEVICE_ID_LEN]; >>>>        WCHAR instance_id[MAX_DEVICE_ID_LEN]; >>>> + >>>> +    /* everything below requires holding the cs */ >>>> +    CRITICAL_SECTION cs; >>>> +    IRP *pending_read; >>>> +    BOOL pending_is_gamepad; >>> >>> Honestly after thinking about it I don't dislike "internal" as much I >>> dislike "xinput" now. Because you'd think that "xinput" refers to the >>> device we access from xinput, but actually it's the other way around. >> >> It's still the case there, but eventually (ie: in an eventual PATCH 7/6 >> where xinput is switched to the internal device interface class), this >> is how it will indeed be. So to me it's actually intuitive. > > Well, right, I'm talking about the eventual situation. > > And, I hate to say it, but "to me it's actually intuitive" isn't great :-( > >> >> In any case, I don't think the driver is supposed to become more >> complicated than these patches (it just misses the report translation >> and that's it), it should be quite straightforward to get the whole >> picture. With a few comments maybe? > > I think a comment near the top is definitely going to be necessary > regardless of what terminology we eventually use. > > My logic with "real"/"fake" is that the "real" device reports the "real" > data that we get from winebus; the "fake" device fixes it up and hides > some of it. But yeah, it's clearly not unambiguous either. The best > thing it has going for it is that they're clearly attributes of the > child devices rather than namespace prefixes or referring to internal > IOCTLs or anything else. > In the new version I kept the names "gamepad_device" -for the bogus HID gamepad- with the "IG_" (as in I-something G-amepad) suffix, and "xinput_device" which will have an "XI_" (as in X-I-nput) suffix to distinguish them. Both will have the WINEXINPUT\ bus namespace. I think it's the best I can come up with that doesn't sound like something too generic (fake/real is completely generic IMHO and could mean anything depending on the reader current understanding). Cheers, -- Rémi Bernon