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

Controller input #8807

Merged
merged 44 commits into from Mar 11, 2016
Merged

Controller input #8807

merged 44 commits into from Mar 11, 2016

Conversation

garbear
Copy link
Member

@garbear garbear commented Jan 8, 2016

Here it is... the first half of RetroPlayer!

Relavant links:

Controller window

This PR contains a new system for controller and keyboard input used by RetroPlayer. Games aren't included in this PR, so you'll have to use a RetroPlayer build to test the full extent of the input system. Still, the input system offers many improvements in Kodi outside of games, and lets us start collecting button map data for when games hit.

This PR is accompanied by a binary add-on, peripheral.joystick. It warrants review alongside the PR. This add-on was created to contain all of Kodi's platform-specific joystick code and keymap data.

Here are the blocking issues:

  • Compilation on all platforms (RPi needs an #ifdef HAS_LIBUDEV around the touchscreen fix)
  • Android support (thread) (thanks montellese)
  • The controller window's "get more" button fails silently (report)
  • Breaks some EventServer clients (Apple IR Remotes, some Yatse buttons, etc) (report)
  • Erratic button presses while mapping buttons (report)
  • Empty keyboard settings breaks GUI (report) (thanks montelllese)
  • Missing scroll bars in controller dialog (thanks hitcher)
  • Broken volume commands (report)
  • Open separate PR for [guilib] fix (done: PR 8932)
  • Open separate PR for [addon] changes (thanks montellese)
  • Broken controller input on RPi and some linux boxes (report)
  • Erratic controller behavior in GUI (report)
  • Controller dialog skips down and left for some controllers (report)
  • OK dialog when peripheral.joystick is not focused on opening (report) (thanks Montellese)
  • Move peripheral event processing to another thread to fix hangs (report)
  • Notification for add-on updates clears the controller profiles list (thanks montellese)
  • Labels disappear on first install (report) (thanks montellese)
  • Canceling the prompt doesn't absorb keyboard input
  • Kodi doesn't fail gracefully when peripheral.joystick isn't found (report) (thanks montellese)
  • Possible deadlock on shutdown (report) (thanks montellese)

Here are the issues that aren't blocking the merge:

  • iOS Game Controller Framework support (thread)
  • Broken touchscreen on RPi (report)
  • Controller button doesn't stay focused while user is mapping buttons (report)
  • Move focusing logic to CGUIViewControl (report) (skinner advice please?)
  • Incompatible with Universal Remote server (report)
  • Incompatible with iMON receiver (report)
  • Problems with accelerometers (report and linux solution)
  • Controller input not ignored when Kodi is minimized (report)
  • CPeripheralAddon requires refactoring (report)
  • Add "Help" button
  • Refocus controller when user ends mapping
  • The "Reset" button resets all controllers instead of asking for a specific one
  • Anomalous trigger detector misidentifies axes that are fully pressed when controller is connected
  • Erroneous drivers with no joystick name can't be mapped

@garbear garbear force-pushed the controller-input branch 2 times, most recently from 8b103fa to 1718f3d Compare January 8, 2016 04:32
@natethomas natethomas added the Type: Feature non-breaking change which adds functionality label Jan 8, 2016
@razzeee razzeee added WIP PR that is still being worked on v17 Krypton Component: Settings labels Jan 8, 2016
@ace20022
Copy link
Member

ace20022 commented Jan 8, 2016

Congratulations!

@MilhouseVH
Copy link
Contributor

If this PR is included in a build of master (say you just wanted to use a joystick to control Kodi, and aren't including the rest of Retroplayer), then it doesn't appear possible to access the configuration settings - there's no Games > Input settings as per the "How to". Are the settings in another PR/commit?

@ace20022
Copy link
Member

ace20022 commented Jan 8, 2016

Imo it might be a good idea to review an merge the addon callback commits in a separate pr.

@MilhouseVH
Copy link
Contributor

Aha... it's in Settings > System > Input devices > Configure controller input... silly me, I believed the How-To. :)

@garbear
Copy link
Member Author

garbear commented Jan 8, 2016

Imo it might be a good idea to review an merge the addon callback commits in a separate pr.

you mean #8450?

Aha... it's in Settings > System > Input devices > Configure controller input

I'll update the post. didn't make much sense adding a game category if there's no game settings ;)

@ace20022
Copy link
Member

ace20022 commented Jan 8, 2016

@garbear sorry ;)

@MilhouseVH
Copy link
Contributor

@garbear: Are you able to confirm if Kodi with this PR responds to Yatse navigation inputs?

My RPi/RPi2 and Generic #108 build no longer responds to Yatse navigation - up/down/left/right, select, volume up/down all no longer work after including PR8807.

If I don't include PR8807, then Yatse works normally. I've made sure I'm using the latest PR8807, but that still breaks Yatse.

When building with PR8807 I'm including the peripheral.joystick binary addon package, and nothing else.

Also ping @Tolriq to better understand the methods Yatse is using for the no longer functioning inputs.

@Montellese
Copy link
Member

Someone (might have been @Tolriq) has already reported that input through EventServer doesn't work anymore in RetroPlayer. Should probably be added to the TODOs.

@Tolriq
Copy link
Contributor

Tolriq commented Jan 9, 2016

For basic navigation Yatse use Event Server and sends remote codes as R1 (And if I remember correctly it's XBox controller emulation) (I've just checked Kore quickly from github and they have reference to R1 and R2 but it's the same system)

And it's what most people using EventServer use to have consistent behavior between remotes and Kodi keymaps.

Event Server is mandatory for a lot of things that are not possible from JSON, I can do test and anything needed, but it must not be killed or there must be a way for remotes to use keymaps configuration.

About @Montellese what I did ask before was support for EventServer as a Retroplayer Controller, @garbear said it was missing button up code but in fact it's not. Then we moved to it should be added a binary addon. (But I wonder how we could split event server into normal event server and binary addon one).

@garbear
Copy link
Member Author

garbear commented Jan 9, 2016

I've gathered a bunch of known issues in the releases thread. I'll copy them to the PR description.

One issue is a broken Event Server. It relied on a "gamepad" abstraction, which was somehow parallel to the old joystick stuff and wasn't removed with everything else.

If we want to avoid breaking these apps, we should just re-route the gamepad stuff to the keymap handling system. EventServer doesn't send "button release" events needed for the new joystick library, so the gamepad stuff can't be used with the new system.

To replace the old gamepad system, we should extend the Joystick API to EventServer. Is this something that is worth the effort? At some point we should allow controllers over IP.

@Tolriq
Copy link
Contributor

Tolriq commented Jan 9, 2016

Event Server have the flags for button up, down, duration all possible axis too (

PT_BUTTON = 0x03,
)

So I do not think there's missing things at least on the way it should work, there's maybe bugs that prevent it to work but that's another story.

The problem about Event Server is that there's tons of things in it, it's also used to send keyboard key press. (And just remember that Kore do support EventServer by default now as it was the main problem that users had with it ;) xbmc/Kore#46 )

There's also the possibility to create a new keymap for remotes to rely on, and access them via something else than R1 / R2.

I do not think migration to a new system to allow cleanup is a problem, but not having a way to preserve access to user Keymap is a critical one.

I still personally think that access to those via JSON should be possible, since it's opening user configuration to remotes and despite all answers I get there was never a correct reason to refuse that.

(And yes as explained a lot Input.Right should do right as it's named like that, but something should be available if the user have changed right to toggle subs during video playing)

@garbear
Copy link
Member Author

garbear commented Jan 10, 2016

@Tolriq I think I've fixed the broken EventServer. Can you confirm that it was only the four (White, Black, Right stick and Left stick) buttons that were broken? It looks like these got broken when changing Kodi's controller abstraction from Xbox to Xbox 360. I've added some backwards compatibility to the button translator in the latest commit so these should be working again.

@Tolriq
Copy link
Contributor

Tolriq commented Jan 10, 2016

I'll check tomorrow what use the R1 mapping but I do use event server for multiple dozen of commands and have opened it to virtually anything via custom command :(

Possible to have a windows test build with addons in it ?

@garbear
Copy link
Member Author

garbear commented Jan 10, 2016

Sure, I'll post back with a windows build of this PR, and I'll upload a new RetroPlayer build to the builds thread with the fix included.

@garbear
Copy link
Member Author

garbear commented Jan 11, 2016

@Tolriq
Copy link
Contributor

Tolriq commented Jan 11, 2016

Ok so this build miss the libKODI_peripheral.dll so not sure this is the cause of the problem, but no buttons does work.

Also tested your latest retroplayer build (2016-01-09) and also none of the button works, so not only 4 fixed here.

Here's the list of R1 button code that Yatse use :

"power", "up", "left", "right", "up", "down", "select", "back", "info", "title", "display", "menu"
"volumeplus", "volumeminus", "skipminus", "reverse", "stop", "play", "pause", "forward", "skipplus", "hash"

None works, and as explained user can use any R1 button code mapping.

But worse is that even Keyboard mapping does not work.

Logs are full of :

11:28:38 T:12208   DEBUG: AddOnLog: Joystick Support: No XInput devices on port 0
11:28:38 T:12208   DEBUG: AddOnLog: Joystick Support: No XInput devices on port 1
11:28:38 T:12208   DEBUG: AddOnLog: Joystick Support: No XInput devices on port 2
11:28:38 T:12208   DEBUG: AddOnLog: Joystick Support: No XInput devices on port 3

So this sounds like If there's no connected controller, nothing will work.

And after reading EventServer code again (was a long time since I did play with it to add unicode support) I do not see why it should be impacted with your joystick code.

R1 mapping use correct ButtonTranslator that translate to XINPUT_IR_REMOTE_XXX so not joystick related. (But your code can have impact for XG mapping and for joysticks named input).

Your last fix could fix some XG mapping buttons.

But when reading again the logs that talks about Xinput, I suppose your code try to match the IR_REMOTE with a connected joystick and obviously fails.

This needs to have someone that use LIRC too to see if it works. (Maybe LIRC is seen as a external controller and works)

If someone can help on http://forum.kodi.tv/showthread.php?tid=251917 I could debug that, but until then I can only report what I found from logs.

And another repeated problem maybe for @Montellese :) (Kodi default webserver port on Windows is still port 80, while wiki says it should be 8080 to address the Windows 10 bug, should really be fixed for 16.0)

@razzeee
Copy link
Member

razzeee commented Jan 11, 2016

@Tolriq
Haven't done much with setting so far, but I can't find where that would be the case.
Windows specific settings don't mention ports
https://github.com/xbmc/xbmc/blob/master/system/settings/win32.xml

And the others are 8080
https://github.com/xbmc/xbmc/blob/master/system/settings/settings.xml#L2100
https://github.com/xbmc/xbmc/blob/master/system/settings/settings.xml#L2798

@Tolriq
Copy link
Contributor

Tolriq commented Jan 11, 2016

Well all fresh portable install on Windows have 80. This is not a recovered value from a previous install since all my installs have 90 as port.

On the MAC it was correctly 8080. (But not related to this PR at all was just another reminder on a place where people do read :) ) I can give more details on forum is there's a follow up ;)

@Montellese
Copy link
Member

@razzeee @Tolriq: offtopic so please don't continue discussing. But here is the default override

((CSettingInt*)m_settingsManager->GetSetting(CSettings::SETTING_SERVICES_WEBSERVERPORT))->SetDefault(80);
. It's not win32 specific but it certainly applies to win32.

@Tolriq
Copy link
Contributor

Tolriq commented Jan 11, 2016

@garbear

Ok so now that I can compile it's easier :)

Culprit is garbear@a90fca1#diff-c1e123244f01a3701552f8b893b3445cL333

You removed code with bazooka when it required scalpel ;) EventServer is not simple to understand and highly tied to lot of things.

You removed 3/4 of Event Server ;)

Here's a patch to restore EventServer at least for remotes and keyboard : http://yatse.tv/0001-Fix-Event-Server.patch

Patch is tested and works, since I choose the fast path to fix, I have no idea how to properly make a PR from a branch on another remote. But I suppose you can apply .patch easily and I uploaded to yatse.tv so if I have enough credibility here it should be considered a secure source ;)

As a side note, I did add support for Unicode to Event Server long ago (#1612) so well understand this part of the code ;)

As the Patch title says, there's the need to either fix Joystick code or remove support for it from EventServer and advertise about this because I have no idea how many things rely on this.
I have not tested either EventServer LIRC integration, have no idea how and by who it's used.

@garbear garbear force-pushed the controller-input branch 3 times, most recently from 750b6cb to 4212b2b Compare January 11, 2016 17:43
@garbear
Copy link
Member Author

garbear commented Mar 11, 2016

@Montellese correct, it needs to be added to the repo. I'll do so now

@phil65 I sent the confluence changes to xbmc/skin.confluence#5. the help button addition got squashed, but you can still see it in the xml. also needed is an icon for peripheral add-ons

here goes nothing...

garbear added a commit that referenced this pull request Mar 11, 2016
@garbear garbear merged commit e851a38 into xbmc:master Mar 11, 2016
@razzeee razzeee added this to the Krypton 17.0-alpha1 milestone Mar 11, 2016
@Hedda
Copy link
Contributor

Hedda commented Mar 14, 2016

Have the GUI elements for this been ported to the new Estuary skin?

@razzeee
Copy link
Member

razzeee commented Mar 14, 2016

Like phil said, yes. Only missing some minors, if he hasn't added them already, as he's pretty quick.

garbear added a commit to garbear/xbmc that referenced this pull request Mar 17, 2016
garbear added a commit to garbear/xbmc that referenced this pull request Mar 17, 2016
@garbear garbear mentioned this pull request Apr 5, 2016
@Hitcher
Copy link
Contributor

Hitcher commented Apr 8, 2016

@garbear Noticed this doesn't display any info for skin debugging instead the underlying windows' info is still shown.

screenshot045

@Memphiz
Copy link
Member

Memphiz commented Apr 10, 2016

@garbear sorry for the late comment. I finally realised that this PR broke apple remote handling on osx. We use the eventserver there for injecting jostick events.

Because of this:

https://github.com/xbmc/xbmc/blob/master/xbmc/input/InputManager.cpp#L208

This doesn't work anymore. What am i supposed to do to fix it? We had this approach to be able to have a custom appleremote.xml key map which was independend from keyboard map. I could of course change XBMCHelper so it fires keyboard events instead. But that is not backward compatible (users have their appleremote.xml in place already for years).

@Memphiz
Copy link
Member

Memphiz commented Apr 10, 2016

Mhhh na i could not really switch over to keyboard events because dependend on the used apple remote we have multiple distinguishable codes for right, left and so on ... thats not possible with keyboard map

@garbear
Copy link
Member Author

garbear commented Apr 11, 2016

@HitcherUK probably because it's a dialog, not a window

@Memphiz I think the line if (joystickName.length() > 0) was a hack so that if the joystick had a name, it would bypass the EventServer code. Does XBMCHelper give the joystick a name?

@Hitcher
Copy link
Contributor

Hitcher commented Apr 11, 2016

@garbear doesn't matter if it's a window or a dialog ;)

screenshot062

Maybe @ronie has an idea on how to fix this?

@ronie
Copy link
Member

ronie commented Apr 11, 2016

@HitcherUK works fine here

screenshot000

@Memphiz
Copy link
Member

Memphiz commented Apr 12, 2016

@garbear yes it gives it a name - see:
https://github.com/xbmc/xbmc/blob/master/tools/EventClients/Clients/OSXRemote/xbmcclientwrapper.mm#L264

I thought this is needed for being able to map the keycodes to kodi keys/actions (and the map xml file is called like the joystick no?)
Still unclear to me how i can regain that feature...

@Hitcher
Copy link
Contributor

Hitcher commented Apr 12, 2016

@ronie thanks, just checked and it does work with the latest nightly.

@garbear
Copy link
Member Author

garbear commented Apr 12, 2016

The line you highlighted shows ATV commands mapped to an entry in joystick.AppleRemote.xml. This file was removed in #8807 when the old keymaps were replaced by peripheral.joystick.

Kodi on OSX now uses the cocoa framework to interface with controllers. If the Apple Remote can be made to show up as a cocoa controller, it can be mapped within the GUI.

@garbear
Copy link
Member Author

garbear commented Apr 12, 2016

Alternatively, EventServer has a "gamepad" interface. It's mappings can be found here: gamepad.xml

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