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

[Linux] add libinput backend for non window system platforms #13176

Merged
merged 14 commits into from Apr 25, 2018

Conversation

@lrusak
Copy link
Contributor

commented Dec 12, 2017

I'm expecting to get a bit of push back here but let me explain.

Positives:

  1. Better (and simpler) input handling
  2. Lower maintenance due to offloading to a support library
  3. Future features including keymap selection and other options (natural scrolling, mouse acceleration, etc)
  4. Improved hotplugging (no more 10 second delay)
  5. Following mainstream standards (wayland input handling, new x11 input handling)

Negatives:

  1. Adding dependencies on external libraries
  2. Possible higher CPU load (unmeasured)

So far this PR adds feature parity to the current code. I did not want to extend the feature set in the initial PR as that may add complexity to the testing and code review. This input handling should be the same or better (I've noticed an improvement in touchscreen accuracy).

This input handling doesn't support remotes you must use something like eventlircd for that.

In the future (outside the scope of this PR) I'd like to add a settings selection for things like keymap selection, input options (input tapping, mouse acceleration, natural scrolling, left handed, etc).

This PR now builds for any projects that use CLinuxInputDevices so GBM, RPi, and AML are the candidates. This does not apply to any other platforms.

I've separated the commits into semi-logical groups for easier review. I plan to squash into one commit if this is merged.

This PR has a dependency on libinput and libxkbcommon (for keyboard handling).

Relevant changes to LE are here, LibreELEC/LibreELEC.tv@master...lrusak:libinput-kodi

If this is approved then we probably need to add libinput and it's dependencies to the build depends.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

please move the folders xbmc/input/linux and input/linux to xbmc/platform/linux

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

@FernetMenta can you please explain the purpose?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

  1. the folder xbmc/linux is a relic and wrong. You don't won't to have a platform folder for all platforms in the root folder, do you? That's why there is xbmc/platform where all platforms but linux already maintain their stuff.

  2. Having a platform specific folder for every component like input is not maintainable. This would end up in having dozens of i.e. linux folders everywhere in the source tree. The platform specific folders under windowing will go to platform too.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

Ok, but I don't touch xbmc/linux at all. Maybe adjusting all that is outside the scope of this PR? I can do all that in a separate PR from this one.

set(LIBINPUT_VERSION ${PC_LIBINPUT_VERSION})

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(LIBINPUT

This comment has been minimized.

Copy link
@Rechi

Rechi Dec 12, 2017

Member

LibInput

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

Ok but please move input/linux then. This is what this PR touches

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

This is working fine on RPi2 (LibreELEC).

@lrusak lrusak force-pushed the lrusak:libinput branch 2 times, most recently from 1045a04 to 07a9867 Dec 15, 2017
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

added a commit to allow reading the default kernel repeat and delay rate and use those.

{
public:
CWinEventsLinux();
~CWinEventsLinux();

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Add override to the destructor

std::queue<XBMC_Event> m_events;
std::mutex m_mutex;

std::unique_ptr<CLibInputHandler> m_li;

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Can you use a more verbose name?

return m_devices.IsRemoteLowBattery();
if (m_li)
{
m_li.reset();

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

This whole destructor body isn't needed. That's the beauty of a smart pointer.

class CLibInputHandler
{
public:
CLibInputHandler(CWinEventsLinux *winEvents);

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Add a new interface and only expose the methods from CWinEventsLinux that you need.

}

libinput_log_set_handler(m_li, LogHandler);
libinput_log_set_priority(m_li, LIBINPUT_LOG_PRIORITY_DEBUG);

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Just a thought, should this match Kodi's log level?


if (libinput_device_has_capability(dev, LIBINPUT_DEVICE_CAP_TOUCH))
{
auto device = find (m_devices.begin(), m_devices.end(), dev);

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Change to std::find and drop the space

if (m_winEvents)
{
delete m_winEvents;
m_winEvents = nullptr;

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

don't need to set nullptr in destructor

};

CLibInputKeyboard::CLibInputKeyboard(CWinEventsLinux *winEvents)
: m_ctx(nullptr)

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

C++11 lets you initialize members in the header


CLibInputTouch::CLibInputTouch()
{
CGenericTouchInputHandler::GetInstance().RegisterHandler(&CGenericTouchActionHandler::GetInstance());

This comment has been minimized.

Copy link
@garbear

garbear Dec 16, 2017

Member

Globals are evil. At least hide the global behind an interface

@graham8 graham8 referenced this pull request Dec 19, 2017
4 of 10 tasks complete
@lrusak lrusak force-pushed the lrusak:libinput branch from 6bf135e to 1450e65 Jan 1, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

how can I go about adding depends suport for this? the problem is that the chain of dependencies is quite large.

libinput
  -> udev
  -> mtdev
  -> evdev
  -> libxkbcommon

Also, this is only needed for the RPi in jenkins.

@lrusak lrusak force-pushed the lrusak:libinput branch from 1450e65 to 7e2cd93 Jan 23, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

almost got there,

/home/jenkins/workspace/LINUX-RBPI/xbmc/platform/linux/input/LibInputKeyboard.cpp:30:37: fatal error: linux/input-event-codes.h: No such file or directory
 #include <linux/input-event-codes.h>

I assume we include the kernel headers for rpi in jenkins. How should I be including them in cmake?
http://jenkins.kodi.tv/job/LINUX-RBPI/7827

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@FernetMenta can you look at my last fixup commit here. It seems that I don't need any of the cruft that was in CWinEventsLinux before (I'm not even sure what MessagePump is for).

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

I have a busy week at Hannover Fair. I will look at it on Saturday.

@lrusak lrusak force-pushed the lrusak:libinput branch 2 times, most recently from 26a331b to bead073 Apr 25, 2018
@lrusak lrusak force-pushed the lrusak:libinput branch from bead073 to beb5ea7 Apr 25, 2018
@lrusak lrusak force-pushed the lrusak:libinput branch from cf9c37e to c33044e Apr 25, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

jenkins build this please

@lrusak lrusak merged commit d233545 into xbmc:master Apr 25, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details

CLibInputHandler::CLibInputHandler(CWinEventsLinux *winEvents)
{
m_udev = udev_new();

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Apr 25, 2018

Contributor

so, now eudev/systemd is not an optional build time dependency anymore

This comment has been minimized.

Copy link
@lrusak

lrusak Apr 25, 2018

Author Contributor

Correct, one or the other

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Apr 26, 2018

Contributor

there are few issues with this.

  • udev is marked as opional dependency, kodi wont build with -DENABLE_UDEV=OFF -so that should be fixed in cmakefiles now
  • libusb is (was) an optional dependency, only if udev was NOT found. see xbmc/peripherals/bus/PeripheralBusUSB.h and xbmc/peripherals/bus/PeripheralBusUSB.*. should this be completely removed now ?

This comment has been minimized.

Copy link
@wsnipex

wsnipex Apr 26, 2018

Member

this is only true for platforms without "windowing": RPi, AML, GBM
For those udev should be mandatory

@@ -19,27 +19,23 @@
*/

#pragma once

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 26, 2018

Member

cut the dependency to windowing. read input events by a dedicated thread (like lirc does) and send to CApp's inbound port.

@mkreisl

This comment has been minimized.

Copy link

commented May 2, 2018

Not sure if this is the right place to post an libinput related issue here

Since merging, inputs provided via /dev/uinput interface are not longer working (Raspberry Pi)

Neither keyboard events nor mouse events injected by our vnc-server are recognized by Kodi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.