Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899893003:
Add generic mapping for gamepad (Closed)
DescriptionAdd generic mapping for gamepad
There is a group of gamepads designed for joydev interface but will require a mapping for evdev. There was no existing mapping in current code base but if we move on to use evdev gamepad in arc++, user will notice their gamepad does not work anymore.
The purpose of this patch is to generate a mapping as close to joydev as possible while being compatible with xbox and adt gamnepads.
When gamepad vendor id and product id are unknown, we will use generic
mapping. A static generic mapping can only be compatible with one class
of gamepad. This commit implements a dynamic generic gamepad mapping. It
is both compatible with XInput style and ADT style gamepad. And it trys
it best to be compatible with joydev compatible gamepad.
BUG=717246
Review-Url: https://codereview.chromium.org/2899893003
Cr-Commit-Position: refs/heads/master@{#477108}
Committed: https://chromium.googlesource.com/chromium/src/+/310f0bcc8e0e29b48c02c80843463a4d70f4674f
Patch Set 1 #
Total comments: 39
Patch Set 2 : Add generic mapping for gamepad #
Total comments: 8
Patch Set 3 : Add generic mapping for gamepad #Messages
Total messages: 21 (7 generated)
Description was changed from ========== Add generic mapping for gamepad When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 ========== to ========== Add generic mapping for gamepad When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 ==========
jkwang@google.com changed reviewers: + spang@chromium.org
Please take a look when you get some time.
ping
I would like to hear more justification of this approach. I'm worried that this will not work well enough in practice to justify its complexity. I'm particularly concerned that with an algorithm like this, small changes to reported capabilities can cascade into large changes in the computed mapping. This will make it difficult to judge the effects of any changes to this algorithm going forward. An algorithm with two fixed mappings and a single conditional to choose between them could still pass your two test cases. What's leading to the conclusion that we need to build a mapping iteratively? As it stands there are significantly more decisions being made by this algorithm than there are tests to justify those decisions. If there were tests to justify each decision, that would best. Otherwise, it's not clear that the complexity is warranted.
On 2017/05/26 03:24:14, spang wrote:
OK, here's the story.
I was testing gamepads and found one gamepad does not work with ozone gamepad
but will work with /device/gamepad. The ID (vendor and product) of this gamepad
is neither in chromium/src/device/gamepad nor in android keyboard layout. The
gamepad will work without any mapping with joydev interface (/device/gamepad)
but won't work with evdev. I noticed that this is a group of gamepads fit joydev
interface more than evdev. If we move on to use evdev gamepad in arc++, the user
will notice their gamepad does not work anymore. But we will have no clue until
some one complains.
For a more concrete example, here is the key mapping of nintendo switch gamepad:
Event code 304 (BTN_A) -> WG_BUTTON_A 0
Event code 305 (BTN_B) -> WG_BUTTON_B 1
Event code 306 (BTN_C) -> WG_BUTTON_X 2
Event code 307 (BTN_X) -> WG_BUTTON_Y 3
Event code 308 (BTN_Y) -> WG_BUTTON_L1 4
Event code 309 (BTN_Z) -> WG_BUTTON_R1 15
Event code 310 (BTN_TL) -> bit set, but does not actually report this
Event code 311 (BTN_TR) -> bit set, but does not actually report this
Event code 312 (BTN_TL2) -> bit set, but does not actually report this
Event code 313 (BTN_TR2) -> WG_BUTTON_START 9
Event code 314 (BTN_SELECT) -> bit set, but does not actually report this
Event code 315 (BTN_START) -> bit set, but does not actually report this
Event code 316 (BTN_MODE) -> WG_BUTTON_MODE 16
Event code 317 (BTN_THUMBL) -> bit set, but does not actually report this
Event code 318 (BTN_THUMBR)
It is reported that this gamepad work automatically with chrome. It's because it
fit joydev interface very whell. But we do need a mapping for evdev. So the
logic for this generic mapping is:
take care of the special case of Xbox gamepad
take care of the special case of ADT
stick to joydev as close as possible
> I would like to hear more justification of this approach. I'm worried that
this
> will not work well enough in practice to justify its complexity.
>
> I'm particularly concerned that with an algorithm like this, small changes to
> reported capabilities can cascade into large changes in the computed mapping.
Yes, and it is the case for joydev. A small change (adding one button) will
change the whole mapping in joydev and the gamepad will be not usable.
> This will make it difficult to judge the effects of any changes to this
> algorithm going forward.
I agree. But it's not supposed to be the long term support for any gamepad, we
need a static mapping for any explicitly support gamepad. This is supposed to be
the fallback for legacy gamepad or new released xbox compatible gamepad.
(Otherwise, user have to wait to update there chrome to support the new gamepad.
It could take months.)
>
> An algorithm with two fixed mappings and a single conditional to choose
between
> them could still pass your two test cases. What's leading to the conclusion
that
> we need to build a mapping iteratively?
iteratively as joydev. See
https://github.com/spotify/linux/blob/master/drivers/input/joydev.c start from
line 823.
> As it stands there are significantly
> more decisions being made by this algorithm than there are tests to justify
> those decisions. If there were tests to justify each decision, that would
best.
> Otherwise, it's not clear that the complexity is warranted.
On 2017/05/26 20:59:41, jkwang wrote: > On 2017/05/26 03:24:14, spang wrote: > OK, here's the story. > I was testing gamepads and found one gamepad does not work with ozone gamepad > but will work with /device/gamepad. The ID (vendor and product) of this gamepad > is neither in chromium/src/device/gamepad nor in android keyboard layout. The > gamepad will work without any mapping with joydev interface (/device/gamepad) > but won't work with evdev. I noticed that this is a group of gamepads fit joydev > interface more than evdev. If we move on to use evdev gamepad in arc++, the user > will notice their gamepad does not work anymore. But we will have no clue until > some one complains. > This anecdote is very convincing that we need to make a change. Can you put it in your CL description? It's much better than what's there. It's more important to justify the change in the CL description (the why) than to say what the code does (the what). > For a more concrete example, here is the key mapping of nintendo switch gamepad: > > Event code 304 (BTN_A) -> WG_BUTTON_A 0 > Event code 305 (BTN_B) -> WG_BUTTON_B 1 > Event code 306 (BTN_C) -> WG_BUTTON_X 2 > Event code 307 (BTN_X) -> WG_BUTTON_Y 3 > Event code 308 (BTN_Y) -> WG_BUTTON_L1 4 > Event code 309 (BTN_Z) -> WG_BUTTON_R1 15 > Event code 310 (BTN_TL) -> bit set, but does not actually report this > Event code 311 (BTN_TR) -> bit set, but does not actually report this > Event code 312 (BTN_TL2) -> bit set, but does not actually report this > Event code 313 (BTN_TR2) -> WG_BUTTON_START 9 > Event code 314 (BTN_SELECT) -> bit set, but does not actually report this > Event code 315 (BTN_START) -> bit set, but does not actually report this > Event code 316 (BTN_MODE) -> WG_BUTTON_MODE 16 > Event code 317 (BTN_THUMBL) -> bit set, but does not actually report this > Event code 318 (BTN_THUMBR) > > It is reported that this gamepad work automatically with chrome. It's because it > fit joydev interface very whell. But we do need a mapping for evdev. So the > logic for this generic mapping is: > take care of the special case of Xbox gamepad > take care of the special case of ADT > stick to joydev as close as possible If I'm understanding right, the compatibility issue applies only to joydev since that's what the existing fallback path for device/gamepad. If we need to single out the first two cases (xbox and ADT compatible devices), can you prove that we can make this determination correctly? Is it possible for a device to match 2 or more of your buckets (xbox compatible, ADT compatible, joydev compatible)? Do you have a 2nd example of a joydev compatible device that needs a different static mapping than switch controller? Please test the switch case in your unit test since that's the motivating example. > > > I would like to hear more justification of this approach. I'm worried that > this > > will not work well enough in practice to justify its complexity. > > > > I'm particularly concerned that with an algorithm like this, small changes to > > reported capabilities can cascade into large changes in the computed mapping. > Yes, and it is the case for joydev. A small change (adding one button) will > change the whole mapping in joydev and the gamepad will be not usable. > > This will make it difficult to judge the effects of any changes to this > > algorithm going forward. > > I agree. But it's not supposed to be the long term support for any gamepad, we > need a static mapping for any explicitly support gamepad. This is supposed to be > the fallback for legacy gamepad or new released xbox compatible gamepad. > (Otherwise, user have to wait to update there chrome to support the new gamepad. > It could take months.) > > > > An algorithm with two fixed mappings and a single conditional to choose > between > > them could still pass your two test cases. What's leading to the conclusion > that > > we need to build a mapping iteratively? > > iteratively as joydev. See > https://github.com/spotify/linux/blob/master/drivers/input/joydev.c start from > line 823. > > As it stands there are significantly > > more decisions being made by this algorithm than there are tests to justify > > those decisions. If there were tests to justify each decision, that would > best. > > Otherwise, it's not clear that the complexity is warranted.
Description was changed from ========== Add generic mapping for gamepad When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 ========== to ========== Add generic mapping for gamepad There is a group of gamepads designed for joydev interface but will require a mapping for evdev. There was no existing mapping in current code base but if we move on to use evdev gamepad in arc++, user will notice their gamepad does not work anymore. The purpose of this patch is to generate a mapping as close to joydev as possible while being compatible with xbox and adt gamnepads. When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 ==========
On 2017/05/27 14:03:05, spang wrote: > On 2017/05/26 20:59:41, jkwang wrote: > > On 2017/05/26 03:24:14, spang wrote: > > OK, here's the story. > > I was testing gamepads and found one gamepad does not work with ozone gamepad > > but will work with /device/gamepad. The ID (vendor and product) of this > gamepad > > is neither in chromium/src/device/gamepad nor in android keyboard layout. The > > gamepad will work without any mapping with joydev interface (/device/gamepad) > > but won't work with evdev. I noticed that this is a group of gamepads fit > joydev > > interface more than evdev. If we move on to use evdev gamepad in arc++, the > user > > will notice their gamepad does not work anymore. But we will have no clue > until > > some one complains. > > > > This anecdote is very convincing that we need to make a change. Can you put it > in your CL description? It's much better than what's there. It's more important > to justify the change in the CL description (the why) than to say what the code > does (the what). > > > For a more concrete example, here is the key mapping of nintendo switch > gamepad: > > > > Event code 304 (BTN_A) -> WG_BUTTON_A 0 > > Event code 305 (BTN_B) -> WG_BUTTON_B 1 > > Event code 306 (BTN_C) -> WG_BUTTON_X 2 > > Event code 307 (BTN_X) -> WG_BUTTON_Y 3 > > Event code 308 (BTN_Y) -> WG_BUTTON_L1 4 > > Event code 309 (BTN_Z) -> WG_BUTTON_R1 15 > > Event code 310 (BTN_TL) -> bit set, but does not actually report this > > Event code 311 (BTN_TR) -> bit set, but does not actually report this > > Event code 312 (BTN_TL2) -> bit set, but does not actually report this > > Event code 313 (BTN_TR2) -> WG_BUTTON_START 9 > > Event code 314 (BTN_SELECT) -> bit set, but does not actually report this > > Event code 315 (BTN_START) -> bit set, but does not actually report this > > Event code 316 (BTN_MODE) -> WG_BUTTON_MODE 16 > > Event code 317 (BTN_THUMBL) -> bit set, but does not actually report this > > Event code 318 (BTN_THUMBR) > > > > It is reported that this gamepad work automatically with chrome. It's because > it > > fit joydev interface very whell. But we do need a mapping for evdev. So the > > logic for this generic mapping is: > > > take care of the special case of Xbox gamepad > > take care of the special case of ADT > > stick to joydev as close as possible > > If I'm understanding right, the compatibility issue applies only to joydev since > that's what the existing fallback path for device/gamepad. > > If we need to single out the first two cases (xbox and ADT compatible devices), > can you prove that we can make this determination correctly? Is it possible for > a device to match 2 or more of your buckets (xbox compatible, ADT compatible, > joydev compatible)? > Unfortunately, I cannot prove it is "correct". The mapping is based on assumptions which are true for most (if not every) gamepad I have seen. The decision of "xbox" and "adt" does not really conflict with each other. For example, if ABS_GAS and ABS_BRAKE is reported, it will be mapped to l-trigger and r-trigger. If abs_x,abs_y,abs_z, abs_rx, abs_ry, abs_rz is seen, abs_z and abs_rz will be mapped l-trigger and r-trigger. I haven't seen any gamepad report ABS_GAS/brake and z/rz at the same time. If it happens, we simply cannot figure out what we should do. There are too many axis and we cannot get any conclusion. > Do you have a 2nd example of a joydev compatible device that needs a different > static mapping than switch controller? Please test the switch case in your unit > test since that's the motivating example. > I have another joydev compatible gamepad works fine with this generic mapping (But needs a slightly different generic mapping). I am testing more gamepads and will add more static mapping to the library. > > > > > I would like to hear more justification of this approach. I'm worried that > > this > > > will not work well enough in practice to justify its complexity. > > > > > > I'm particularly concerned that with an algorithm like this, small changes > to > > > reported capabilities can cascade into large changes in the computed > mapping. > > Yes, and it is the case for joydev. A small change (adding one button) will > > change the whole mapping in joydev and the gamepad will be not usable. > > > This will make it difficult to judge the effects of any changes to this > > > algorithm going forward. > > > > I agree. But it's not supposed to be the long term support for any gamepad, we > > need a static mapping for any explicitly support gamepad. This is supposed to > be > > the fallback for legacy gamepad or new released xbox compatible gamepad. > > (Otherwise, user have to wait to update there chrome to support the new > gamepad. > > It could take months.) > > > > > > An algorithm with two fixed mappings and a single conditional to choose > > between > > > them could still pass your two test cases. What's leading to the conclusion > > that > > > we need to build a mapping iteratively? > > > > iteratively as joydev. See > > https://github.com/spotify/linux/blob/master/drivers/input/joydev.c start from > > line 823. > > > As it stands there are significantly > > > more decisions being made by this algorithm than there are tests to justify > > > those decisions. If there were tests to justify each decision, that would > > best. > > > Otherwise, it's not clear that the complexity is warranted.
On 2017/05/30 20:15:37, jkwang wrote: > On 2017/05/27 14:03:05, spang wrote: > > On 2017/05/26 20:59:41, jkwang wrote: > > > On 2017/05/26 03:24:14, spang wrote: > > > OK, here's the story. > > > I was testing gamepads and found one gamepad does not work with ozone > gamepad > > > but will work with /device/gamepad. The ID (vendor and product) of this > > gamepad > > > is neither in chromium/src/device/gamepad nor in android keyboard layout. > The > > > gamepad will work without any mapping with joydev interface > (/device/gamepad) > > > but won't work with evdev. I noticed that this is a group of gamepads fit > > joydev > > > interface more than evdev. If we move on to use evdev gamepad in arc++, the > > user > > > will notice their gamepad does not work anymore. But we will have no clue > > until > > > some one complains. > > > > > > > This anecdote is very convincing that we need to make a change. Can you put it > > in your CL description? It's much better than what's there. It's more > important > > to justify the change in the CL description (the why) than to say what the > code > > does (the what). > > > > > For a more concrete example, here is the key mapping of nintendo switch > > gamepad: > > > > > > Event code 304 (BTN_A) -> WG_BUTTON_A 0 > > > Event code 305 (BTN_B) -> WG_BUTTON_B 1 > > > Event code 306 (BTN_C) -> WG_BUTTON_X 2 > > > Event code 307 (BTN_X) -> WG_BUTTON_Y 3 > > > Event code 308 (BTN_Y) -> WG_BUTTON_L1 4 > > > Event code 309 (BTN_Z) -> WG_BUTTON_R1 15 > > > Event code 310 (BTN_TL) -> bit set, but does not actually report this > > > Event code 311 (BTN_TR) -> bit set, but does not actually report this > > > Event code 312 (BTN_TL2) -> bit set, but does not actually report this > > > Event code 313 (BTN_TR2) -> WG_BUTTON_START 9 > > > Event code 314 (BTN_SELECT) -> bit set, but does not actually report > this > > > Event code 315 (BTN_START) -> bit set, but does not actually report this > > > Event code 316 (BTN_MODE) -> WG_BUTTON_MODE 16 > > > Event code 317 (BTN_THUMBL) -> bit set, but does not actually report > this > > > Event code 318 (BTN_THUMBR) > > > > > > It is reported that this gamepad work automatically with chrome. It's > because > > it > > > fit joydev interface very whell. But we do need a mapping for evdev. So the > > > logic for this generic mapping is: > > > > > take care of the special case of Xbox gamepad > > > take care of the special case of ADT > > > stick to joydev as close as possible > > > > If I'm understanding right, the compatibility issue applies only to joydev > since > > that's what the existing fallback path for device/gamepad. > > > > If we need to single out the first two cases (xbox and ADT compatible > devices), > > can you prove that we can make this determination correctly? Is it possible > for > > a device to match 2 or more of your buckets (xbox compatible, ADT compatible, > > joydev compatible)? > > > Unfortunately, I cannot prove it is "correct". The mapping is based on > assumptions which are true for most (if not every) gamepad I have seen. The > decision of "xbox" and "adt" does not really conflict with each other. For > example, if ABS_GAS and ABS_BRAKE is reported, it will be mapped to l-trigger > and r-trigger. If abs_x,abs_y,abs_z, abs_rx, abs_ry, abs_rz is seen, abs_z and > abs_rz will be mapped l-trigger and r-trigger. I haven't seen any gamepad report > ABS_GAS/brake and z/rz at the same time. If it happens, we simply cannot figure > out what we should do. There are too many axis and we cannot get any conclusion. Sorry, I shouldn't have said proof. Good evidence is enough. Trying out lots of devices with the generic mapper is a way to get that. It sounds like you have done the right testing. It would help if you took a representative sample of the devices you've tested, capture the full capabilities, and turn them into test cases for the generic mapper. > > > Do you have a 2nd example of a joydev compatible device that needs a different > > static mapping than switch controller? Please test the switch case in your > unit > > test since that's the motivating example. > > > I have another joydev compatible gamepad works fine with this generic mapping > (But needs a slightly different generic mapping). I am testing more gamepads and > will add more static mapping to the library. To be clear, when you say different generic mapping, you mean the buttons are different but the mapping is computed correctly by the joydev driver? > > > > > > > I would like to hear more justification of this approach. I'm worried that > > > this > > > > will not work well enough in practice to justify its complexity. > > > > > > > > I'm particularly concerned that with an algorithm like this, small changes > > to > > > > reported capabilities can cascade into large changes in the computed > > mapping. > > > Yes, and it is the case for joydev. A small change (adding one button) will > > > change the whole mapping in joydev and the gamepad will be not usable. > > > > This will make it difficult to judge the effects of any changes to this > > > > algorithm going forward. > > > > > > I agree. But it's not supposed to be the long term support for any gamepad, > we > > > need a static mapping for any explicitly support gamepad. This is supposed > to > > be > > > the fallback for legacy gamepad or new released xbox compatible gamepad. > > > (Otherwise, user have to wait to update there chrome to support the new > > gamepad. > > > It could take months.) > > > > > > > > An algorithm with two fixed mappings and a single conditional to choose > > > between > > > > them could still pass your two test cases. What's leading to the > conclusion > > > that > > > > we need to build a mapping iteratively? > > > > > > iteratively as joydev. See > > > https://github.com/spotify/linux/blob/master/drivers/input/joydev.c start > from > > > line 823. > > > > As it stands there are significantly > > > > more decisions being made by this algorithm than there are tests to > justify > > > > those decisions. If there were tests to justify each decision, that would > > > best. > > > > Otherwise, it's not clear that the complexity is warranted.
On 2017/05/30 20:15:37, jkwang wrote: > On 2017/05/27 14:03:05, spang wrote: > > On 2017/05/26 20:59:41, jkwang wrote: > > > On 2017/05/26 03:24:14, spang wrote: > > > OK, here's the story. > > > I was testing gamepads and found one gamepad does not work with ozone > gamepad > > > but will work with /device/gamepad. The ID (vendor and product) of this > > gamepad > > > is neither in chromium/src/device/gamepad nor in android keyboard layout. > The > > > gamepad will work without any mapping with joydev interface > (/device/gamepad) > > > but won't work with evdev. I noticed that this is a group of gamepads fit > > joydev > > > interface more than evdev. If we move on to use evdev gamepad in arc++, the > > user > > > will notice their gamepad does not work anymore. But we will have no clue > > until > > > some one complains. > > > > > > > This anecdote is very convincing that we need to make a change. Can you put it > > in your CL description? It's much better than what's there. It's more > important > > to justify the change in the CL description (the why) than to say what the > code > > does (the what). > > > > > For a more concrete example, here is the key mapping of nintendo switch > > gamepad: > > > > > > Event code 304 (BTN_A) -> WG_BUTTON_A 0 > > > Event code 305 (BTN_B) -> WG_BUTTON_B 1 > > > Event code 306 (BTN_C) -> WG_BUTTON_X 2 > > > Event code 307 (BTN_X) -> WG_BUTTON_Y 3 > > > Event code 308 (BTN_Y) -> WG_BUTTON_L1 4 > > > Event code 309 (BTN_Z) -> WG_BUTTON_R1 15 > > > Event code 310 (BTN_TL) -> bit set, but does not actually report this > > > Event code 311 (BTN_TR) -> bit set, but does not actually report this > > > Event code 312 (BTN_TL2) -> bit set, but does not actually report this > > > Event code 313 (BTN_TR2) -> WG_BUTTON_START 9 > > > Event code 314 (BTN_SELECT) -> bit set, but does not actually report > this > > > Event code 315 (BTN_START) -> bit set, but does not actually report this > > > Event code 316 (BTN_MODE) -> WG_BUTTON_MODE 16 > > > Event code 317 (BTN_THUMBL) -> bit set, but does not actually report > this > > > Event code 318 (BTN_THUMBR) > > > > > > It is reported that this gamepad work automatically with chrome. It's > because > > it > > > fit joydev interface very whell. But we do need a mapping for evdev. So the > > > logic for this generic mapping is: > > > > > take care of the special case of Xbox gamepad > > > take care of the special case of ADT > > > stick to joydev as close as possible > > > > If I'm understanding right, the compatibility issue applies only to joydev > since > > that's what the existing fallback path for device/gamepad. > > > > If we need to single out the first two cases (xbox and ADT compatible > devices), > > can you prove that we can make this determination correctly? Is it possible > for > > a device to match 2 or more of your buckets (xbox compatible, ADT compatible, > > joydev compatible)? > > > Unfortunately, I cannot prove it is "correct". The mapping is based on > assumptions which are true for most (if not every) gamepad I have seen. The > decision of "xbox" and "adt" does not really conflict with each other. For > example, if ABS_GAS and ABS_BRAKE is reported, it will be mapped to l-trigger > and r-trigger. If abs_x,abs_y,abs_z, abs_rx, abs_ry, abs_rz is seen, abs_z and > abs_rz will be mapped l-trigger and r-trigger. I haven't seen any gamepad report > ABS_GAS/brake and z/rz at the same time. If it happens, we simply cannot figure > out what we should do. There are too many axis and we cannot get any conclusion. Sorry, I shouldn't have said proof. Good evidence is enough. Trying out lots of devices with the generic mapper is a way to get that. It sounds like you have done the right testing. It would help if you took a representative sample of the devices you've tested, capture the full capabilities, and turn them into test cases for the generic mapper. > > > Do you have a 2nd example of a joydev compatible device that needs a different > > static mapping than switch controller? Please test the switch case in your > unit > > test since that's the motivating example. > > > I have another joydev compatible gamepad works fine with this generic mapping > (But needs a slightly different generic mapping). I am testing more gamepads and > will add more static mapping to the library. To be clear, when you say different generic mapping, you mean the buttons are different but the mapping is computed correctly by the joydev driver? > > > > > > > I would like to hear more justification of this approach. I'm worried that > > > this > > > > will not work well enough in practice to justify its complexity. > > > > > > > > I'm particularly concerned that with an algorithm like this, small changes > > to > > > > reported capabilities can cascade into large changes in the computed > > mapping. > > > Yes, and it is the case for joydev. A small change (adding one button) will > > > change the whole mapping in joydev and the gamepad will be not usable. > > > > This will make it difficult to judge the effects of any changes to this > > > > algorithm going forward. > > > > > > I agree. But it's not supposed to be the long term support for any gamepad, > we > > > need a static mapping for any explicitly support gamepad. This is supposed > to > > be > > > the fallback for legacy gamepad or new released xbox compatible gamepad. > > > (Otherwise, user have to wait to update there chrome to support the new > > gamepad. > > > It could take months.) > > > > > > > > An algorithm with two fixed mappings and a single conditional to choose > > > between > > > > them could still pass your two test cases. What's leading to the > conclusion > > > that > > > > we need to build a mapping iteratively? > > > > > > iteratively as joydev. See > > > https://github.com/spotify/linux/blob/master/drivers/input/joydev.c start > from > > > line 823. > > > > As it stands there are significantly > > > > more decisions being made by this algorithm than there are tests to > justify > > > > those decisions. If there were tests to justify each decision, that would > > > best. > > > > Otherwise, it's not clear that the complexity is warranted.
https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/evdev/gamep... File ui/events/ozone/evdev/gamepad_event_converter_evdev.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/evdev/gamep... ui/events/ozone/evdev/gamepad_event_converter_evdev.h:110: GamepadMapper* mapper_; std::unique_ptr<> https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... File ui/events/ozone/gamepad/gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.cc:13: GamepadMapper* GetGamepadMapper(const EventDeviceInfo& devinfo) { Should return std::unique_ptr<GamepadMapper> https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.cc:17: result = new GenericGamepadMapping(devinfo); Use base::MakeUnique here. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... File ui/events/ozone/gamepad/gamepad_mapping.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.h:45: uint16_t code, overloading operator() is going to cause syntactic grief when you wrap it in a unique ptr (and you should) Probably just name this Map() https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:20: GenericMappingHelper(const EventDeviceInfo& devinfo, This organization is surprising. Everything happens in the ctor, but once it finishes, the constructed object is not needed. Can you organize this a different way. Something like class GamepadMapperBuilder { public: void MapButton(uint16_t from_button, WebGamepadButtonType to_button) { DCHECK(HasMappingFromButton(from_button)); DCHECK(HasMappingToButton(to_button)); ... } void MapAbsoluteAxis(uint16_t from_absolute_axis, WebGamepadAbsType to_axis) { DCHECK(HasMappingFromAbsoluteAxis(from_absolute_axis)); DCHECK(HasMappingToAxis(to_axis)); ... } // etc std::unique_ptr<GamepadMapping> ToGamepadMapping() { // build final mapper from button_mapping_ & absolute_axis_mapping_ } private: std::bitset<something> mapped_buttons_; std::bitset<something> mapped_absolute_axes_; std::vector<KeyMapEntry> button_mapping_; std::vector<AbsMapEntry> absolute_axis_mapping_; }; std::unique_ptr<GamepadMapping> BuildGamepadMapper(const EventDeviceInfo& info) { GamepadMapperBuilder builder; // Run logic from previous ctor using |info| and |builder| return builder.ToGamepadMapping(); } https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:25: unmapped_wg_btn_.push_back(i); bitset with a vector this should call reserve() if you know the final size. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:37: void MapAbs() { Although linux uses btn and abs Google style is to avoid abbreviations. Possibly "WG" in the constant names should be fixed too. MapAbsoluteAxes ? https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:44: void MapBtns() { MapButtons() https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:53: int code = i + BTN_MISC; Confused by this - what's "i" for here (Why not iterate over code) and why does this test all the keyboard keys? Are there actually gamepads with keyboard keys on them that should get mapped to gamepad buttons? https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:58: if (devinfo_.HasKeyEvent(code) && !mapped_ev_btn_.count(code)) { This would read a lot more clearly if you make a function called bool IsButtonMapped(uint16_t code) and use that (and below) https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:121: abs_map_->push_back(TO_BTN(ABS_Z, WG_BUTTON_LT)); Can you make more functions to make this read well and less error prone. Something like AddButtonMapping(ABS_Z, WG_BUTTON_LT); https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:146: std::list<uint16_t> unmapped_wg_btn_; You can use a std::bitset here. Or std::vector. std::list is very inefficient for this task. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:149: std::set<uint16_t> mapped_ev_btn_; Also bitset https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.h:24: uint16_t* mapped_code) override; const https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:44: struct input_id { InputId https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:49: const input_id test_ids[] = { kTestIds https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:90: void CompareGamepadMapper(GamepadMapper* l_mapper, GamepadMapper* r_mapper) { const& arguments https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:117: GamepadMapper* s_mapper = static_mapper https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:120: FakeDevinfoFromMapper(s_mapper, &dev_info); I'd prefer testing with real data. Can you check in a capabilities literal for these devices instead of computing a partial version of it using a mapping? https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:121: GenericGamepadMapping g_mapper(dev_info); generic_mapper
https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/evdev/gamep... File ui/events/ozone/evdev/gamepad_event_converter_evdev.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/evdev/gamep... ui/events/ozone/evdev/gamepad_event_converter_evdev.h:110: GamepadMapper* mapper_; On 2017/06/01 05:08:12, spang wrote: > std::unique_ptr<> Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... File ui/events/ozone/gamepad/gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.cc:13: GamepadMapper* GetGamepadMapper(const EventDeviceInfo& devinfo) { On 2017/06/01 05:08:12, spang wrote: > Should return > > std::unique_ptr<GamepadMapper> Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.cc:17: result = new GenericGamepadMapping(devinfo); On 2017/06/01 05:08:12, spang wrote: > Use base::MakeUnique here. Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... File ui/events/ozone/gamepad/gamepad_mapping.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gam... ui/events/ozone/gamepad/gamepad_mapping.h:45: uint16_t code, On 2017/06/01 05:08:12, spang wrote: > overloading operator() is going to cause syntactic grief when you wrap it in a > unique ptr (and you should) > > Probably just name this Map() Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:20: GenericMappingHelper(const EventDeviceInfo& devinfo, On 2017/06/01 05:08:12, spang wrote: > This organization is surprising. Everything happens in the ctor, but once it > finishes, the constructed object is not needed. > > Can you organize this a different way. Something like > > class GamepadMapperBuilder { > public: > void MapButton(uint16_t from_button, WebGamepadButtonType to_button) { > DCHECK(HasMappingFromButton(from_button)); > DCHECK(HasMappingToButton(to_button)); > ... > } > > void MapAbsoluteAxis(uint16_t from_absolute_axis, WebGamepadAbsType to_axis) { > DCHECK(HasMappingFromAbsoluteAxis(from_absolute_axis)); > DCHECK(HasMappingToAxis(to_axis)); > ... > } > > // etc > > std::unique_ptr<GamepadMapping> ToGamepadMapping() { > // build final mapper from button_mapping_ & absolute_axis_mapping_ > } > > > private: > std::bitset<something> mapped_buttons_; > std::bitset<something> mapped_absolute_axes_; > std::vector<KeyMapEntry> button_mapping_; > std::vector<AbsMapEntry> absolute_axis_mapping_; > }; > > std::unique_ptr<GamepadMapping> BuildGamepadMapper(const EventDeviceInfo& info) > { > GamepadMapperBuilder builder; > > // Run logic from previous ctor using |info| and |builder| > > return builder.ToGamepadMapping(); > } Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:25: unmapped_wg_btn_.push_back(i); On 2017/06/01 05:08:13, spang wrote: > bitset > > with a vector this should call reserve() if you know the final size. Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:37: void MapAbs() { On 2017/06/01 05:08:13, spang wrote: > Although linux uses btn and abs Google style is to avoid abbreviations. Possibly > "WG" in the constant names should be fixed too. > > MapAbsoluteAxes ? Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:44: void MapBtns() { On 2017/06/01 05:08:12, spang wrote: > MapButtons() Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:53: int code = i + BTN_MISC; On 2017/06/01 05:08:12, spang wrote: > Confused by this - what's "i" for here (Why not iterate over code) and why does > this test all the keyboard keys? Are there actually gamepads with keyboard keys > on them that should get mapped to gamepad buttons? As the comments, it's https://github.com/spotify/linux/blob/master/drivers/input/joydev.c starting from line 816. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:58: if (devinfo_.HasKeyEvent(code) && !mapped_ev_btn_.count(code)) { On 2017/06/01 05:08:13, spang wrote: > This would read a lot more clearly if you make a function called > > bool IsButtonMapped(uint16_t code) > > and use that (and below) Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:121: abs_map_->push_back(TO_BTN(ABS_Z, WG_BUTTON_LT)); On 2017/06/01 05:08:13, spang wrote: > Can you make more functions to make this read well and less error prone. > Something like > > AddButtonMapping(ABS_Z, WG_BUTTON_LT); Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:146: std::list<uint16_t> unmapped_wg_btn_; On 2017/06/01 05:08:13, spang wrote: > You can use a std::bitset here. Or std::vector. std::list is very inefficient > for this task. Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:149: std::set<uint16_t> mapped_ev_btn_; On 2017/06/01 05:08:13, spang wrote: > Also bitset Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping.h (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping.h:24: uint16_t* mapped_code) override; On 2017/06/01 05:08:13, spang wrote: > const Done. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... File ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc (right): https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:44: struct input_id { On 2017/06/01 05:08:13, spang wrote: > InputId Acknowledged. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:49: const input_id test_ids[] = { On 2017/06/01 05:08:13, spang wrote: > kTestIds Acknowledged. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:90: void CompareGamepadMapper(GamepadMapper* l_mapper, GamepadMapper* r_mapper) { On 2017/06/01 05:08:13, spang wrote: > const& arguments Acknowledged. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:120: FakeDevinfoFromMapper(s_mapper, &dev_info); On 2017/06/01 05:08:13, spang wrote: > I'd prefer testing with real data. Can you check in a capabilities literal for > these devices instead of computing a partial version of it using a mapping? Acknowledged. https://codereview.chromium.org/2899893003/diff/1/ui/events/ozone/gamepad/gen... ui/events/ozone/gamepad/generic_gamepad_mapping_unittest.cc:121: GenericGamepadMapping g_mapper(dev_info); On 2017/06/01 05:08:13, spang wrote: > generic_mapper Acknowledged.
one more thing, otherwise lgtm https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... File ui/events/ozone/gamepad/generic_gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:23: GenericGamepadMapper(std::vector<AbsMapEntry>* axis_mapping, Write this as GenericGamepadMapper(std::vector<AbsMapEntry> axis_mapping, std::vector<KeyMapEntry> button_mapping) : axis_mapping_(std::move(axis_mapping)), button_mapping_(std::move(button_mapping)) { } https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:69: GamepadMapperBuilder(const EventDeviceInfo& devinfo) : devinfo_(devinfo) {} explicit https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:72: return base::MakeUnique<GenericGamepadMapper>(&axis_mapping_, return base::MakeUnique<GenericGamepadMapper>(std::move(axis_mapping_), std::move(button_mapping_)); will move the vector for you while still allowing the ctor to accept it by value https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:233: Put everything above in an anonymous namespace (and remove "static" since you won't need it then)
https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... File ui/events/ozone/gamepad/generic_gamepad_mapping.cc (right): https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:23: GenericGamepadMapper(std::vector<AbsMapEntry>* axis_mapping, On 2017/06/02 23:00:58, spang wrote: > Write this as > > GenericGamepadMapper(std::vector<AbsMapEntry> axis_mapping, > std::vector<KeyMapEntry> button_mapping) > : axis_mapping_(std::move(axis_mapping)), > button_mapping_(std::move(button_mapping)) { > } Done. https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:69: GamepadMapperBuilder(const EventDeviceInfo& devinfo) : devinfo_(devinfo) {} On 2017/06/02 23:00:58, spang wrote: > explicit Done. https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:72: return base::MakeUnique<GenericGamepadMapper>(&axis_mapping_, On 2017/06/02 23:00:58, spang wrote: > return base::MakeUnique<GenericGamepadMapper>(std::move(axis_mapping_), > std::move(button_mapping_)); > > will move the vector for you while still allowing the ctor to accept it by value Done. https://codereview.chromium.org/2899893003/diff/20001/ui/events/ozone/gamepad... ui/events/ozone/gamepad/generic_gamepad_mapping.cc:233: On 2017/06/02 23:00:58, spang wrote: > Put everything above in an anonymous namespace (and remove "static" since you > won't need it then) Done.
The CQ bit was checked by jkwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org Link to the patchset: https://codereview.chromium.org/2899893003/#ps40001 (title: "Add generic mapping for gamepad")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496688158237490,
"parent_rev": "0469bc2e31efd074d260f83867e566f17e37f7d5", "commit_rev":
"310f0bcc8e0e29b48c02c80843463a4d70f4674f"}
Message was sent while issue was closed.
Description was changed from ========== Add generic mapping for gamepad There is a group of gamepads designed for joydev interface but will require a mapping for evdev. There was no existing mapping in current code base but if we move on to use evdev gamepad in arc++, user will notice their gamepad does not work anymore. The purpose of this patch is to generate a mapping as close to joydev as possible while being compatible with xbox and adt gamnepads. When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 ========== to ========== Add generic mapping for gamepad There is a group of gamepads designed for joydev interface but will require a mapping for evdev. There was no existing mapping in current code base but if we move on to use evdev gamepad in arc++, user will notice their gamepad does not work anymore. The purpose of this patch is to generate a mapping as close to joydev as possible while being compatible with xbox and adt gamnepads. When gamepad vendor id and product id are unknown, we will use generic mapping. A static generic mapping can only be compatible with one class of gamepad. This commit implements a dynamic generic gamepad mapping. It is both compatible with XInput style and ADT style gamepad. And it trys it best to be compatible with joydev compatible gamepad. BUG=717246 Review-Url: https://codereview.chromium.org/2899893003 Cr-Commit-Position: refs/heads/master@{#477108} Committed: https://chromium.googlesource.com/chromium/src/+/310f0bcc8e0e29b48c02c8084346... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/310f0bcc8e0e29b48c02c8084346... |
