[input] Add 'Enable Joystick' to GUI settings. Fix for http://trac.xbmc.org/ticket/13146 #1111

Merged
merged 5 commits into from Jul 8, 2012

6 participants

@Karlson2k
Team Kodi member

Added GUI settings for control Joystick usage.
Some addition to CJoystick class - identical to CStatMouse.
Now Joystick can be enabled/disabled (and Initialized/deinitialized) by calling g_Joystick.SetEnable (true / false)

Also can be used as workaround for popular iMON (SoundGraph) hardware on Windows, which with latest DirectInput patches falls into endless Remove/Insert loop. That's make remote iMon control and display unusable.

@wsoltys
Team Kodi member

I don't think that I like that. As I can see from the imon thread this is a bug in the imon firmware and I don't want to fix their shit in our code.
Since I can't test it I would appreciate a real and more general fix if possible.

@wsoltys
Team Kodi member

btw for your next pr please make sure that you don't have the merge branch thingies in. Try something like git pull --rebase upstream master to get your changes on top without the merge.

@Karlson2k
Team Kodi member

iMon claims that this it the bug in DirectX. See 'Cause' in http://www.soundgraph.com/forums/faq.php?faq=sg_faq_sw_imon_manager_g_01#faq_sg_faq_software_products_gamecontrol
But anyway - it's unfixable because firmware isn't upgradeable. At least until Microsoft rewrote DirectInput code. :)
So, until that it's not possible to use iMON hardware with XBMC (with DirectInput Joystick). I'm sure that a lot of users will be unhappy if this will go to release.
My solution is relatively simple, doesn't broke anything and add small feature, that can be used even by users without iMON hardware.

@jmarshallnz
Team Kodi member

Putting aside the issue of whether we want it, the above 3 commits could be squashed into one, plus some of the checks (in ProcessEventClient) aren't needed - they don't actually use the joystick hardware stuff, rather they use the joystick keymapping stuff. Also, there's some whitespace issues on the last commit.

To squash into one, git rebase -i HEAD~3 and change the last 2 commits to squash. You'll then get to rewrite the commit message.

@wsoltys
Team Kodi member

To get you into thinking, is there a possibility to detect the firmware in question? Maybe via a key in the registry or a certain dll which is there?

@Karlson2k
Team Kodi member

OK, I'll combine all commits in one, fix whitespaces and remove some checks.
It's possible to detect hardware via USB HID properties, but I don't have a lists of buggy and bug-free firmwares.
I'm ready to write detection module. Could you advise something about lists of the hardware?

@Karlson2k
Team Kodi member

Started development thread for discussion: http://forum.xbmc.org/showthread.php?tid=135218

@Karlson2k
Team Kodi member

Rewrote from the scratch.
Now it looks like elegant solution, not like a hack as before

@hippojay hippojay pushed a commit that referenced this pull request Jul 5, 2012
@davilla davilla [osx] bring in 'SDL: Fixed bug #1111', invalid inline assembly in src…
…/video/mmx.h
dbff1c0
@wsoltys
Team Kodi member

I like the idea with the gui settings but you mixed too many different changes into one commit which makes it difficult to review. One commit for the gui setting and one commit for the cosmetic changes please.
I don't know what the asm fix has to do with the joystick class nor this pr in general.

@Karlson2k
Team Kodi member

There are just a few cosmetic points.
One of them - duplicated Reset, other - code for one function move from .h to .cpp and third one - move g_Joystick to SystemGlobal.
Do you really need separate commits?

@wsoltys
Team Kodi member

@jmarshallnz what do you think? can this go in?

@Karlson2k
Team Kodi member

OK. I'll split it to several commits. :)

@wsoltys
Team Kodi member

Just two are fine :)

@Karlson2k
Team Kodi member

Too late. :)

@Karlson2k
Team Kodi member

@jmarshallnz . Fine?

@jmarshallnz
Team Kodi member

Karlson2k: yes - please squash that one down, remove the russian translation, fix the Reset issue, and separate out your one-line if()'s onto two lines.

@Karlson2k
Team Kodi member

It was done already, except squash.

@jmarshallnz
Team Kodi member

Looks fine - please squash the fixups into the appropriate commits and I'll pull it in.

@Karlson2k
Team Kodi member

@jmarshallnz finally done!

@jmarshallnz jmarshallnz was assigned Jul 8, 2012
@jmarshallnz jmarshallnz merged commit ba554f1 into xbmc:master Jul 8, 2012
@jmarshallnz
Team Kodi member

Thanks :)

@gyunaev

This breaks the configurations with --disable-joystick:

CPP xbmc/SystemGlobals.o
SystemGlobals.cpp:68:3: error: ‘CJoystick’ does not name a type

Team Kodi member

Looks like just the || defined HAS_EVENT_SERVER needs removing on the include and g_Joystick?

That indeed fixed it for me and so far the compiled binary seem to work fine. Is there any other reason for having the HAS_EVENT_SERVER there?

Team Kodi member

HAS_EVENT_SERVER is there because the apple remote uses the joystick interface. If you break that, then the Apple remote will not work anymore if --disable-joystick is used which ios does in configure.in

arm-apple-darwin*)
use_joystick=no

Team Kodi member

Doesn't it just use the joystick translation stuff, not the actual joystick object (g_Joystick) ?

was that moved out ? from what I remember, there are functions there that the eventserver called which is why there was an || HAS_EVENT_SERVER

and it's osx not ios that uses the joystick/eventserver. other eventclients might also be using it.

Team Kodi member

Looks like "|| HAS_EVENT_SERVER" could be removed from SystemGlobals.cpp and from Application.cpp.
Now SystemGlobals just reflect include schema from Application.cpp.
CApplication::ProcessEventServer calls CApplication::ProcessJoystickEvent which one calls g_Joystick.Reset() (only ifdef HAS_SDL_JOYSTICK) and calls CButtonTranslator::GetInstance().TranslateJoystickString which one is using only defines JACTIVE_ from WIN/SDLJoystick.h
So now it doesn't break anything, but could be optimized.
Could someone test compilation on MacOS/iOS and Linux platforms to be sure?

I can confirm that Linux builds and works fine with || HASEVENT_SERVER removed when configured with --disabled-joystick (my standard build configuration)

Team Kodi member

@gyunaev thanks!
Still need MacOS/iOS confirmation.

Team Kodi member

Confirmed - removing the || HASEVENT_SERVER fixes compilation on ios/osx too.

Team Kodi member

Created PR #1140

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