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

fix lirc cross-compilation and non-working left/right keys #1281

Merged
merged 3 commits into from Feb 7, 2017

Conversation

Projects
None yet
6 participants
@HiassofT
Copy link
Member

commented Feb 5, 2017

don't merge yet, please review and test

Since diagonal key support was added in kernel 4.7 via commit torvalds/linux@4883269 using the default release suffix of _UP for transporting key-up events from lircd to lircd.-uinput is broken.

KEY_LEFT_UP and KEY_RIGHT_UP are now valid keycodes and lircd-uinput will interpret them differently (diagonal-key-down instead of left/right key up) leading to non-working left/right keys. See for example this irw output: https://forum.libreelec.tv/thread-4413-post-31915.html#pid31915

This problem went unnoticed so far because of another issue: during cross-compilation the lirc build picked up the linux include with valid keycodes from the build host instead of the target. So when compiling on a system with older userspace kernel headers installed lircd-uinput worked as before because it didn't know about KEY_LEFT/RIGHT_UP. But when compiling on a system with newer userspace kernel headers installed realase handling of left/right keys broke.

The first commit fixes lircd->lircd-uinput communication by using a different suffix.

The second commit tries to fix lirc cross-compilation:

During a normal (non-cross) build lib/input_map.inc and lib/lirc/input_map.inc in the source folder will be automatically regenerated matching the keycodes from /usr/ijnclude/linux/input-event-codes.h (or /usr/include/linux/input.h on older kernels). This part of the Makefile is nasty, though, when build is started from the object folder input_map.inc will end up there. The build seems to pick it up fine (instead of the one(s) in the source folder), but still it's not nice to have 2 different input_map.inc files sitting around.

I've fixed this by disabling automatic regeneration of input_map.inc during make and doing it manually in package.mk instead.

For removing the build-host-include issue there are 2 options: either patch the lirc-make-devinput script to point it to the target includes or add a parameter to the lirc-make-devinput, pointing it to the location of the kernel include (the input-event-codes.h). I choose the first approach, although it's a little nasty to patch files it'll still work if the script is called from the lirc build so it's more robust. Also it's a little bit easier, otherwise I we might have to re-implement the input-event-code.h->input.h fallback in package.mk.

The third commit disables generation of LIRC release events in eventlircd. Kodi doesn't seem to handle them (they just show up in the logs) and since _UP is now ambiguous we'd have to use another suffix for kodi anyways..

lirc: use _LIRCUP as key-release suffix instead of _UP
Since diagonal key support was added in kernel 4.7 KEY_LEFT_UP
and KEY_RIGHT_UP are valid input symbols.

This means we can no longer use _UP as a suffix for transporting
release events from lircd to lircd-uinput, lircd-uinput now
interprets KEY_LEFT_UP/KEY_RIGHT_UP as a diagnoal key down event
instead of left/right key release.
@InuSasha

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

i will make a test. builds are running (incl. cherrypick for LE8)

@InuSasha
Copy link
Member

left a comment

Test with a former broken configuration.
After update to this PR, all keys are working.
The build artifacts shows, that now the LE-linux header files are used.

Also the cherry-pick for the LE8 is working.
Please backport.

@HiassofT HiassofT force-pushed the HiassofT:le-lirc branch from 1ba01de to 9415eb5 Feb 6, 2017

@HiassofT

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

I've changed the cross-compilation commit to only fixup lirc-make-devinput. That's simpler and seems to be the right way to do.

@InuSasha could you please retest?

@InuSasha

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

@HiassofT master and cherry-pick to LE8 is working.
But the lirc-make-devinput is installed in the image, too. And with the actual solution it is not working on target (i am not sure if it needed on target, or not)

@InuSasha

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

@HiassofT : May an other solution for the crosscompile patch InuSasha@21624c2
only build tested jet, functional test will done soon
functional test was succesful (incl. check for correct used headers)

@HiassofT

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

Thanks a lot for testing!

lirc-make-devinput is also installed by default in debian https://packages.debian.org/stretch/amd64/lirc/filelist . It doesn't make too much sense on a LE install (there are no kernel headers anyway) but also won't hurt much. Sure, it's not nice that it contains the host build path - if there are concerns we can just remove it completely from the install.

As for your patch: we'd need to do that slightly differently, input-event-codes.h was added in kernel 4.4 and there are several platforms /(wetek, amlogic) that use quite ancient 3.x kernels. So we'd have to add a check to package.mk and then add that as a parameter to lirc-make-devinput -i. Best place for that would be in pre_make_target, patching the generated Makefile in the objdir.

Not sure if that is more robust than patching lirc-make-devinput, both could change in the next lirc release :)

Ideally we'd have some configure parameter we could pass in (or let configure autodetect it), I'll contact the lirc guys about that.

@InuSasha

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

@HiassofT you are right... i forgotten the proprietary cpus with historical linux kernels...
and with out the linux-header, the lirc-make-devinput make no sense (we can drop it completely from image)
We can let as is.

Now, i make a build test with wetek_play and press the review button.

@InuSasha

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

For WeTek_Play the build of package lirc was successful

@InuSasha
Copy link
Member

left a comment

Build and Run tested for RPi2
Package lirc Build tested for WeTek_Play

On buildsystems with linux kernel 3.8 and 4.8

@HiassofT

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

FYI: just created 2 tickets about the issues on the lirc tracker
https://sourceforge.net/p/lirc/tickets/263/
https://sourceforge.net/p/lirc/tickets/264/

@@ -37,6 +37,13 @@ PKG_CONFIGURE_OPTS_TARGET="ac_cv_func_forkpty=no \
--with-gnu-ld \
--without-x"

pre_configure_target() {
# patch lirc-make-devinput to use target kernel include
sed -i \

This comment has been minimized.

Copy link
@lrusak

lrusak Feb 7, 2017

Member

mind changing this sed statement so it is more readable? like

  sed -e "s|/usr/include/linux/|${SYSROOT_PREFIX}/usr/include/linux/|g" \
      -i ${ROOT}/${PKG_BUILD}/tools/lirc-make-devinput

It's just a nitpick :P

@lrusak

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

This works for me 👍 if we could just get the little cleanup in this is good to go and needs a backport

HiassofT added some commits Feb 4, 2017

lirc: fix cross compilation
Make sure lirc uses kernel headers from target, not the build
host, when generating input_map.inc.
eventlircd: dont' send _UP release events
Kodi doesn't support the artificial lirc release events and
_UP leads to a clash since KEY_LEFT_UP and KEY_RIGHT_UP are valid
linux input events since the diagonal key support in kernel 4.7.

@HiassofT HiassofT force-pushed the HiassofT:le-lirc branch from 9415eb5 to d209f33 Feb 7, 2017

@HiassofT

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

thanks for the feedback, updated the sed statement as suggested

@chewitt

chewitt approved these changes Feb 7, 2017

Copy link
Member

left a comment

Thanks :)

@chewitt chewitt merged commit 1af6e3d into LibreELEC:master Feb 7, 2017

chewitt added a commit that referenced this pull request Feb 7, 2017

Merge pull request #1290 from HiassofT/le8-lirc
backport of #1281 (fix lirc cross-compilation and non-working left/right keys)

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Feb 27, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281
@LongChair

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

Sorry for being late on that one ... although i agree that the default _UP release suffix leads to confusion, removing the up event causes some troubles.
We use the Up event in our tree because of the slow response of the LIRC API.
Basically the down will trigger autorepeat until the up is sent in our code. So removing the up causes autorepeat frenzyness.

I understand that kodi doesn't use this, but is there anything else that would require to remove it ?
I would think we could keep it using a proper suffix ie _LIRCUP or something; would be nice not to have a specific patch for that in our tree :)

@HiassofT

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

@LongChair the _UP events just cluttered kodi.log so I saw very little point in keeping them. If you want them in your tree I think it'd be best to add a distribution/project specific eventlircd systemd file. Or maybe think about some way to make the eventlircd options configurable. But just overriding the LE systemd file would probably be the easiest solution.

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Mar 2, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Mar 11, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281

@HiassofT HiassofT deleted the HiassofT:le-lirc branch Mar 12, 2017

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Mar 29, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Apr 2, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Apr 7, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281

LongChair added a commit to plexinc/plex-media-player that referenced this pull request Apr 8, 2017

InputLIRC : fix keyup detection.
Upstream changed the suffix for up events as per LibreELEC/LibreELEC.tv#1281
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.