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

Already on GitHub? Sign in to your account

Joystick abstraction and XInput support #2370

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
8 participants
Member

garbear commented Mar 4, 2013

New joystick abstraction - make input handling separate from joystick APIs

This allows for APIs to coexist, such as DirectInput alongside XInput for full 360-like controller support. Multiple joysticks are supported. The current behavior is retained, where the most recent action across all controllers is repeated at a 0.1s delay.

The interface is

class IJoystick
{
public:
  // Implementers should provide the following functions (See CJoystickManager::Initialize()):
  // static void Initialize(JoystickArray &joysticks);
  // static void DeInitialize(JoystickArray &joysticks);
  virtual ~IJoystick() { }
  virtual void Update() = 0;
  virtual const SJoystick &GetState() const = 0;
};

Adding a new joystick requires implementing these functions using the new system's API, and JoystickManager does the rest. One benefit is that much of the joystick handling code is removed from CApplication.

The second commit adds XInput joystick support. This lets XBMC support all 360-like XInput-compatible controllers out of the box using the Xbox 360 keymap, regardless of controller name.

Tested XInput functionality on win32 with logitech XInput controller (linux compiles and runs but wouldn't pick my XInput or DirectInput controllers, I'll keep trying)

The third commit adds Linux Joystick API support

Member

elupus commented Mar 4, 2013

To be honest I'd like to have seen the sdl droppage branch in before this.
That may be some time off though.

Member

theuni commented Mar 5, 2013

Agreed with @elupus. This looks like a really good start, but I'm afraid that an abstraction that leaves out ios/android/embedded-linux isn't quite abstract enough. What would it take to nuke the sdl dependency in the interface itself so that sdl could be hooked up as a client instead?

Member

garbear commented Mar 5, 2013

Yes, I considered rebasing on your patch and waiting til then, but the patch doesn't deal with the joystick code so I figured this wouldn't clash. Besides, the input handling has been separated from the API so dropping SDL will be changing ~240 lines instead of ~580 when we switch to linux input.

When I dig into trac 14130 i'll probably add the API for linux input to the PR. The JoystickManager can track linux input and SDL joysticks at the same time, so the interchangeability should make the transition a lot easier.

Member

garbear commented Mar 5, 2013

@elupus nuking SDL is dropping virtual void Update(SDL_Event *joyEvent) = 0;. This function noops outside of the SDL joystick, and even now isn't used unless --disable-sdl is provided (which currently breaks the build...)

The SDL_Event enum is forward declared, so no code pulls in the headers except for SDLJoystick.cpp

Is this ok? Is there a better way to do it?

Member

garbear commented Mar 5, 2013

Re: the enum SDL_Event, it gets called from CWinEventsSDL::MessagePump(). I don't know the history, does someone know when this code gets hit? Is Update(SDL_Event*) still necessary?

@ghost

ghost commented Mar 6, 2013

that code is hit about once per frame ;) it sits in the very inner loop of xbmc, Application.cpp ~ l2958

Member

garbear commented Mar 10, 2013

@cptspiff is polling joysticks every frame bad? it was done that way before

I updated the PR with a Linux Joystick API client. SDL joysticks have been dropped.

Member

theuni commented Mar 10, 2013

@garbear: before, each implementation did its own polling, and we checked the results once per frame. That's probably what you meant, just worth pointing out the difference.

Owner

koying commented Mar 15, 2013

Android has a "push", event oriented approach, where events are generated when the axis values changes.
This one keeps a "poll" approach only, right?

Member

garbear commented Mar 16, 2013

Right now, yes. Earlier it also did event-based joystick handling, but it kind of muddied up the interface, and the client API supported both polling and events so I dropped the events part.

For android we'll probably want IJoystick::Update() to noop and do our own event-based handling, and implement IJoystick::GetState() so that the joystick manager can still process the input.

Owner

koying commented Mar 16, 2013

k. Would you mind rebasing so that I can start building android support on top of this one?

Member

garbear commented Mar 16, 2013

gladly :)

@koying koying commented on the diff Mar 16, 2013

xbmc/input/linux/Makefile
@@ -1,5 +1,7 @@
SRCS=LIRC.cpp \
- LinuxInputDevices.cpp
+ LinuxInputDevices.cpp \
+ LinuxJoystick.cpp \
@koying

koying Mar 16, 2013

Owner

Android is also linux, but don't have the linux joystick interface (fails on "linux/joystick.h" as a start).
So, LinuxJoystick and LinuxJoystickSDL shouldn't be compiled for Android, somehow

@garbear

garbear Mar 16, 2013

Member

Previously the entire LinuxJoystickSDL file was #define-guarded by HAS_SDL_JOYSTICK

Member

garbear commented Mar 16, 2013

@koying rebased

sorry for the spam there, the rebase nuked all of theuni's comments!

some configure checks and #define overhauls are in order. Koying, I don't have an android platform to test, could you look into this?

@koying koying and 1 other commented on an outdated diff Mar 20, 2013

xbmc/input/JoystickManager.h
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with XBMC; see the file COPYING. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#pragma once
+
+#include "IJoystick.h"
+#include "utils/StdString.h"
+
+class CAction;
@koying

koying Mar 20, 2013

Owner

You have to include "guilib/Key.h" if you are using references to CAction rather than pointers

@theuni

theuni Mar 20, 2013

Member

That's kinda the point of a forward-declare ;)

@koying

koying Mar 20, 2013

Owner

?
See line 68. He's using a reference, so the compiler needs to know the size of the class, so a forwards def is not enough...

[EDIT] My mistake.

@koying koying and 1 other commented on an outdated diff Mar 20, 2013

xbmc/input/JoystickManager.h
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#pragma once
+
+#include "IJoystick.h"
+#include "utils/StdString.h"
+
+class CAction;
+
+// Class to manage all connected joysticks
+class CJoystickManager
+{
+private:
+ CJoystickManager() : m_bEnabled(true), m_bWakeupChecked(false), m_actionTracker() { }
@koying

koying Mar 20, 2013

Owner

??
If you initialize "m_bEnabled" to true, "SetEnabled(true)" won't execute "Initialize()".

@garbear

garbear Mar 20, 2013

Member

good catch, fixed below

@davilla davilla and 1 other commented on an outdated diff Mar 20, 2013

xbmc/input/JoystickManager.h
+ SJoystick m_states[GAMEPAD_MAX_CONTROLLERS];
+ bool m_bEnabled;
+ bool m_bWakeupChecked; // true if WakeupCheck() has been called
+
+ struct ActionTracker
+ {
+ ActionTracker() { Reset(); }
+ void Reset() { actionID = targetTime = 0; name.clear(); }
+ void Track(const CAction &action);
+ int actionID;
+ CStdString name;
+ uint32_t targetTime;
+ };
+
+ ActionTracker m_actionTracker;
+};
@davilla

davilla Mar 20, 2013

Contributor

there is too much code in this include, move the code to the .cpp file where it belongs.

and line out things like;
Reset() { actionID = targetTime = 0; name.clear(); }

these are a bitch to debug as you can't set a bp and walk through them

@garbear

garbear Mar 20, 2013

Member

Good point, I got a bit carried away. I'll make another cpp/h

edit, done, and I thought you were talking about IJoystick.h, so I moved all the non-pure-virtual code from that into new .cpp/.h files

@koying koying commented on an outdated diff Mar 20, 2013

xbmc/input/JoystickManager.cpp
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with XBMC; see the file COPYING. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "system.h" // for HAS_SDL_JOYSTICK
+#if defined(HAS_SDL_JOYSTICK)
+
+#include "JoystickManager.h"
+#include "Application.h"
+#include "ButtonTranslator.h"
+#include "utils/log.h"
+#include "MouseStat.h"
+
@koying

koying Mar 20, 2013

Owner

Have to include "guilib/Key.h" for CAction. My Android implementation fails to compile otherwise.
It's probably included somewhere via the windows or linux include's

@garbear garbear referenced this pull request in garbear/xbmc Apr 6, 2013

Closed

Retroplayer4 - Improvements #7

romanlum commented Apr 7, 2013

theuni said:

Should use udev to grab device names

I implemented the initialization with udev
romanlum/xbmc@e077dd6

Is there a way in xbmc to get an event on device changes, or must i implement a udev monitor??

Can anybody explain me peripherials?? could i use that??

Member

garbear commented Apr 9, 2013

The PR has been updated, some previous comments have been included and

  • ./configure checks for the Linux Joystick API and, if present, that will override SDL joysticks (regardless of whether --enable-joystick was provided. The docstring for --enable-joystick is "enable SDL joystick support (default is yes)")
  • @romanlum has fixed Suspend/Resume support

Also, @theuni was asking if we should poll() on the fd in a separate thread for the linux joystick api client. I don't think that's necessary because we don't poll() for keyboard events, right?

Finally, @romanlum hooked up udev above but I'm not able to test ATM

Owner

koying commented Aug 22, 2013

Mmm... Is this going anywhere?

Member

garbear commented Aug 22, 2013

I think I stalled out at the udev stuff

Owner

koying commented Aug 22, 2013

@garbear Thanks. I guess I'll implement droid joystick independently, then ;)

Contributor

davilla commented Aug 22, 2013

fyi, pivos has some droid joystick handling pending, should appear in a few weeks.

garbear and others added some commits Mar 7, 2013

[joystick] New joystick abstraction - make input handling separate fr…
…om joystick APIs

This allows for APIs to coexist, such as DirectInput alongside XInput for full 360-like controller support. The current behavior is retained, where the most recent button action across all controllers is repeated at a 0.1s delay.

Analog axes can now imitate buttons, allowing easy GUI navigation. For example: `<axis id="1" limit="+1">Right</axis>`. This has the effect that pushing the analog stick >= 50% right will send a single Right action. After an initial delay (500ms) it will then send more Right actions at a 100ms period until the right stick is moved < 50%. This behavior mirrors keyboard keys.
[joystick] Added XInput joystick support. XBMC now supports all 360-l…
…ike XInput-compatible controllers out of the box using the Xbox 360 keymap, regardless of controller name.
[Joystick] implemented Suspend - Resume
LinuxJoystick now ignores the initial events, because they mess up the buttons
Member

garbear commented Oct 25, 2013

@davilla is your joystick code still expected at the end of the october? some friends of mine are looking to get into xbmc hacking, and making this pr less awful is the prefect place for them to start :)

Contributor

davilla commented Oct 26, 2013

Yes, on track for around the 29th.

herrnst added a commit to herrnst/xbmc that referenced this pull request Oct 31, 2013

herrnst added a commit to herrnst/xbmc that referenced this pull request Nov 13, 2013

revert "X11: add SDL joystick until we have a better solution", funct…
…ionality should be handled by "Joystick API (PR:2370 - xbmc#2370)"

garbear added a commit to garbear/xbmc that referenced this pull request Feb 11, 2014

garbear added a commit to garbear/xbmc that referenced this pull request Feb 17, 2014

eibma added a commit to eibma/xbmc that referenced this pull request Mar 11, 2014

tuxity added a commit to pixl-project/xbmc that referenced this pull request Apr 28, 2014

garbear added a commit to garbear/xbmc that referenced this pull request May 11, 2014

Joystick API (PR:2370 - xbmc#2370) with the following improvements:
* Fix scrolling being detected as a digital action
* Don't default input.enablejoystick setting to false on linux
* Default SDL joysticks to no - use the linux joystick api instead
* Fix compile warning (possible bug)
* Prefer linux joystick API over SDL joysticks, and some cosmetics
* Enable joystick manager on startup
* Register joystick manager with CPeripherals

garbear added a commit to garbear/xbmc that referenced this pull request Jul 4, 2014

Joystick API (PR:2370 - xbmc#2370) with the following improvements:
* Fix scrolling being detected as a digital action
* Don't default input.enablejoystick setting to false on linux
* Default SDL joysticks to no - use the linux joystick api instead
* Fix compile warning (possible bug)
* Prefer linux joystick API over SDL joysticks, and some cosmetics
* Enable joystick manager on startup
* Register joystick manager with CPeripherals

garbear added a commit to garbear/xbmc that referenced this pull request Dec 10, 2014

Joystick API (PR:2370 - xbmc#2370) with the following improvements:
* Fix scrolling being detected as a digital action
* Don't default input.enablejoystick setting to false on linux
* Default SDL joysticks to no - use the linux joystick api instead
* Fix compile warning (possible bug)
* Prefer linux joystick API over SDL joysticks, and some cosmetics
* Enable joystick manager on startup
* Register joystick manager with CPeripherals
Member

garbear commented Feb 14, 2015

superseded by recent joystick work on retroplayer branch

@garbear garbear closed this Feb 14, 2015

garbear added a commit to garbear/xbmc-retroplayer that referenced this pull request Feb 15, 2015

@MartijnKaijser MartijnKaijser modified the milestones: Temporary freezer until devs have time, Abandoned, obsolete or superseeded Feb 17, 2015

garbear added a commit to garbear/xbmc that referenced this pull request Feb 17, 2015

@garbear garbear deleted the garbear:joystick-xinput branch Sep 21, 2016

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