Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved joystick handling for Game OSD #12366

Merged
merged 11 commits into from Jul 5, 2017
Merged

Conversation

garbear
Copy link
Member

@garbear garbear commented Jun 26, 2017

This PR is the second part of #12367. It introduces a new system for handling Kodi input in and out of games. It introduces the ability to specify combos in joystick.xml. It also specializes the use of the <FullscreenGame> tag in joystick.xml so that game combos can be used from within the in-game menu.

Description

See the intro from the new joystick.xml:

<?xml version="1.0" encoding="UTF-8"?>
<!-- This file contains the mapping of joystick buttons to actions within    -->
<!-- Kodi.                                                                   -->
<!--                                                                         -->
<!-- The format is:                                                          -->
<!--   <window>                                                              -->
<!--     <joystick profile="game.controller.default">                        -->
<!--       <button>action</button>                                           -->
<!--     </joystick>                                                         -->
<!--   </window>                                                             -->
<!--                                                                         -->
<!-- The <global> section is a fall through - they will only be used if the  -->
<!-- button is not used in the current window's section.                     -->
<!--                                                                         -->
<!-- The "profile" attribute specifies the controller profile whose buttons  -->
<!-- are being mapped. Currently, the default controller profile is used for -->
<!-- every controller. In the future it may be possible to specify mappings  -->
<!-- for the controller profile most similar to the user's physical          -->
<!-- controller.                                                             -->
<!--                                                                         -->
<!-- Actions can be built-in functions.                                      -->
<!--   eg <b>ActivateWindow(Music)</b>                                       -->
<!-- would automatically go to Music on the press of the B button.           -->
<!--                                                                         -->
<!-- Buttons can be overloaded with hold durations, in miliseconds:          -->
<!--   <joystick profile="game.controller.default">                          -->
<!--     <a>Select</a>                                                       -->
<!--     <a holdtime="500">ContextMenu</a>                                   -->
<!--   </joystick>                                                           -->
<!--                                                                         -->
<!-- Buttons can be also require hotkeys to be pressed:                      -->
<!--   <joystick profile="game.controller.default">                          -->
<!--     <start hotkey="back">Stop</start>                                   -->
<!--   </joystick>                                                           -->
<!--                                                                         -->
<!-- Due to limitations in the button mapper, buttons can be overloaded with -->
<!-- different hold durations, but not different hotkeys for the same        -->
<!-- duration.                                                               -->
<!--                                                                         -->
<!-- More documentation on keymaps can be found on                           -->
<!-- http://kodi.wiki/view/keymaps                                           -->
<!--                                                                         -->

Requires (and builds upon) the improvements made in #12365.

How Has This Been Tested?

Tested controller input in and out of games.

@garbear garbear mentioned this pull request Jun 26, 2017
3 tasks
@garbear garbear changed the title Improved joystick handling for game OSD Improved joystick handling for Game OSD Jun 26, 2017
@garbear garbear force-pushed the keymap-handling branch 5 times, most recently from 17d09ae to 72e2792 Compare June 27, 2017 17:28
@garbear garbear added this to the L 18.0-alpha1 milestone Jun 28, 2017
@garbear garbear force-pushed the keymap-handling branch 2 times, most recently from 168d4b2 to 95788be Compare June 28, 2017 13:56
@garbear
Copy link
Member Author

garbear commented Jun 28, 2017

I've rebased on top of #12365. I've also added three commits to fix a crash-on-exit bug that I observed. The problem was due (yet again) to the order of deinitialization. To fix this, I:

  • Moved the global CInputManager to the service manager
  • Moved the global button mapper (and its variants for other devices) to CInputManager
  • Made joystick handling aware of button map changes

I used the Observer interface. I haven't really been keeping up on which parts of Kodi are deprecated, so let me know if I should use another notification strategy.

*
* \param keyName The key name
* \param keyNames The key names

This comment was marked as spam.

This comment was marked as spam.

<!-- Buttons can be also require hotkeys to be pressed: -->
<!-- <joystick profile="game.controller.default"> -->
<!-- <start hotkey="back">Stop</start> -->
<!-- </jotstick> -->

This comment was marked as spam.

@AlwinEsch
Copy link
Member

From the whole looks your work very good, but have not direct experience with them to give more, Sorry.

But thanks a lot for this work :-) . To have the Joystick working helps a lot.

@garbear garbear force-pushed the keymap-handling branch 2 times, most recently from 04c445b to b049f7d Compare July 4, 2017 23:40
@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

jenkins build this please

@garbear garbear force-pushed the keymap-handling branch 2 times, most recently from 9dbcabc to 41bb746 Compare July 5, 2017 02:08
@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

grr missing includes. jenkins build this please

@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

jenkins build and merge

@garbear garbear merged commit 08e9a41 into xbmc:master Jul 5, 2017
@garbear garbear deleted the keymap-handling branch July 5, 2017 06:09
@AlwinEsch
Copy link
Member

Have not tested but there is a label "No-Jenkins", maybe this can prevent auto builds.

@MartijnKaijser
Copy link
Member

if auto jenkins fails the manual kicked one will also fail.

@popcornmix
Copy link
Member

This causes a crash on boot error:

(gdb) bt
#0  CServiceManager::GetInputManager (this=0x0) at /home/dc4/xbmc_holder/xbmc/xbmc/ServiceManager.cpp:265
#1  0x00d8d7f8 in CServiceBroker::GetInputManager () at /home/dc4/xbmc_holder/xbmc/xbmc/ServiceBroker.cpp:110
#2  0x00d0ac2c in CAppParamParser::Parse (this=0x7e928fe0, this@entry=0x7e928fd8, argv=argv@entry=0x7e929304, nArgs=nArgs@entry=5) at /home/dc4/xbmc_holder/xbmc/xbmc/AppParamParser.cpp:59
#3  0x004e7c5c in main (argc=5, argv=0x7e929304) at /home/dc4/xbmc_holder/xbmc/xbmc/platform/posix/main.cpp:113

I've bisected and "Move CInputManager into service manager" causes the crash.

@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

thanks for the report, I'll check it out and push a fix

popcornmix added a commit to popcornmix/xbmc that referenced this pull request Jul 5, 2017
This reverts commit 08e9a41, reversing
changes made to 306d9bd.
@garbear garbear mentioned this pull request Jul 5, 2017
@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

@popcornmix can you try #12426?

@garbear
Copy link
Member Author

garbear commented Jul 5, 2017

Have not tested but there is a label "No-Jenkins", maybe this can prevent auto builds.

I applied this label to #12367, force-pushed, then removed this label. Now, 5 minutes later, I can't trigger a build. @MartijnKaijser is there any way to make jenkins forget that it saw this label?

EDIT: Ah, jenkins is being restarted. NVM.

popcornmix added a commit to popcornmix/xbmc that referenced this pull request Jul 5, 2017
This reverts commit 08e9a41, reversing
changes made to 306d9bd.
@darebee
Copy link

darebee commented Aug 3, 2017

@garbear

I am using Kodi via LibreELEC on Celeron Chomebox. Ever since Milhouse build LibreELEC-Generic.x86_64-9.0-Milhouse-20170705211737-#705-g0ab0d73.tar which included PR:12366 and PR:12367 I get a black screen and "crashes". https://forum.kodi.tv/showthread.php?tid=298462&pid=2613326#pid2613326

I have attached the debug log here:
http://sprunge.us/MZOO

Mihouse mentioned it was most likely the joystick mapper and asked me to remove my joystick but I have nothing connected to my Chromebox.

Is there anything I can do to assist in testing a correct fix for this as I have been frozen on the 2017-07-04 build and cannot upgrade beyond that without causing the same error.

@garbear
Copy link
Member Author

garbear commented Aug 3, 2017

@darebee Can you try a more recent build? Several fixes have gone in for crashes this PR caused. Specifically, #12426, #12427, #12432 and #12435

@darebee
Copy link

darebee commented Aug 3, 2017

@garbear

I tried the most recent build from LibreELEC-Generic.x86_64-9.0-Milhouse-20170802210357-#802-g072425f.tar with the same behavior.

My crashlog is located at http://sprunge.us/SKQO

@darebee
Copy link

darebee commented Aug 4, 2017

@garbear

#12426, #12427, #12432 and #12435 are all in Milhouse builds since 7 July 2017 builds and I am still having the crash. What can I do to help squish this bug?

@garbear
Copy link
Member Author

garbear commented Aug 4, 2017

I found a spot in the code that will crash if a button in joystick.xml has no action. Can you upload your joystick.xml? I'll open a PR for Milhouse to include to see if this spot is the problem.

@darebee
Copy link

darebee commented Aug 4, 2017

Where is the joystick.xml file located? I see a Joystick.AppleRemote.xml under /storage/.kodi/userdata/keymaps/

EDIT: Also, a reminder, I don't actually have a joystick attached.

@garbear
Copy link
Member Author

garbear commented Aug 4, 2017

Now the gears click into place :) Joystick.AppleRemote.xml is left over from the old system, and I remember a report about this crashing the keymapper. The problem is we expect the new schema and get the old schema. The solution is to safely reject it, which we failed to do. I'll run a test with this file and verify that it is properly rejected.

@darebee
Copy link

darebee commented Aug 4, 2017

@garbear

Good news! I deleted the Joystick.AppleRemote.xml I had in that folder as I saw none of these files on my Raspberry Pis and the Chromebox was updated to 03 August build without a crash. It had to do with that erroneous file I had in there from who knows where. If you need me to test after the PR is resolved I will put that file back in and let you know how it goes.

Thanks for all your help with this.

@garbear
Copy link
Member Author

garbear commented Aug 4, 2017

Awesome, try putting the xml file back once #12626 is included to verify that there's no more problems

@darebee
Copy link

darebee commented Aug 5, 2017

@garbear

I put Joystick.AppleRemote.xml back in with build 0803 build and it crashed as expected.

I updated to build 0804 which has #12626 in it. Rebooted and after the upgrade, everything came up okay. Thanks for resolving this issue!

I will be removing the joystick file again as I don't use it...

@garbear
Copy link
Member Author

garbear commented Aug 5, 2017

with the new change, not only does it not crash, but joystick.AppleRemote.xml is completely ignored. thanks for reporting, i'll merge the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants