Menu
▾
▴
1 Attachments
#50 support for long press detection
Hi.
I'd like to add the long keypress detection
functionality into lirc. There are some work-arounds:
http://forum.xbmc.org/showthread.php?tid=115023
but they look clumsy.
I wanted to code up something clean and simple.
Patch attached.
Hi!
Thanks for this patch! There is no need to convince me that this is a problem...
To be frank, this patch touches parts I'm yet not familiar with, so to speak. Together with the lack of test cases this means that reviewing might take some time... please stay tuned.
Hi,
C'mon, I simply provided all the information
I could on that subject, including the link
to other's workaround. :)
And yes, I forgot the use-case, here it is:
begin
prog=devlirc
button=POWER
config=SLEEP
end
begin
prog=devlirc
button=POWER
delay=25
delay_start=0
config=POWEROFF
end
In this example I define the reaction to the
short press of POWER (this will be SLEEP) and
the long press (this will be POWEROFF). Repeat
is disabled in both cases.
Yes, I'm not complaining about the patch. I'm complaining about my limited brain, which still is not understanding all this code. It's improving, though.
What I see so far is that we need to add some documentation. Could you possibly:
Incorporate your use-case information above in doc/html-source/configure.html? Look for "lircrc_format", there is a list where it seems to fit for now. Don't hesitate to actually mention the Apple remotes in this context.
Add doxygen comment blocks to the different static rep_filter() functions? We are in the process of documenting the code, and you are no doubt the person who knows these best by now ;)
The patches runs the test suite and the regression test OK, so it looks good. Need a night's sleep and whatever other input there is besides the documentation update to merge.
OK, done, attached.
I have pushed a feature branch 'longpress' with these patches for continued review.
Here's the patch for easier review.
Adds comments and handles possible misconfiguration.
Hm... here are really just two remarks.
The first is about the naming, which results in the default value 1. Although not really "wrong", values in lirc more or less always have 0/disabled as default value. Given that there is no real usecase for delay:_start > 1 (?) it would probably be better to rename it to to something like "ignore_first_events" and adjust the patch according to that. Or something like that.
The other thing is that we need a testcase. However, we miss the tools to generate that. I'm on it, and if I can complete a sketch we should be able to create a testcase or two in a few days. Will not delay this patch more than that on this issue.
If the delay or repeat is also enabled - certainly.
But if they are disabled, the user can limit the
amount of generated events. For example, if some
player has 10 volume levels, you may decide to
limit the amount of generated events to 10.
But I agree, this is still nearly useless, but
nearly is not entirely. :)
As for "ignore_first_events" proposal. This can be
done but this "ignore_first_events" will be entirely
similar to "delay" with just one difference: "delay"
counts from 1 and "ignore_first_events" counts from 0.
I wanted to code up something that at least pretends
to be newish. :)
That said, I am not against your proposal. If
the above didn't convince you (and it actually
shouldn't) then I'll do it your way.
I propose to apply my cleanup patch anyway, so
that I can start from cleaner code.
;)
Actually, I thought I had replied to this, but it was seemingly lost before I pushed Post. Sorry for that.
That said, it seems that we agree to do this "my" way, not adding more complexity to the semantics than required to solve the problem at hand. As for 'ignore_first_events' being very similar to 'delay' I think this is actually an advantage, it makes it easier to explain. And explaining is one of the core problems here...
We also need a testcase. I have pushed a feature branch called 'api'. Your patches applies also on that. In this branch, there is a new tool irtestcase which occasionally is able to collect a testcase: timing info as of mode2, symbols as of irw and application strings as of ircat. Everything goes to /tmp/irtestcase (hardcoded). Could you give it a try using your patches and remote?
OK, lets agree to do it your way, but why
not to apply my clean-up patch first? This
will allow me to change the cleaner code.
Oh... please do!
You didn't apply it to git. :)
OK, I can ignore that fact and assume you
will apply it later.
Patch here.
Yup, that was my intention...
Now, this is becoming a bit messy. Might be part of it, sorry.
The cleanup patch is in the first page
of that thread. And here it is again:
https://sourceforge.net/p/lirc/tickets/_discuss/thread/f8e533ba/3f41/attachment/0001-ignore-delay_start-parameter-if-delay-is-not-set.patch
Sorry, I can't build api branch for target:
checking python3 module: yaml... no
configure: error: failed to find required python module yaml
This package is simply not in the BSP we have
and not in its upstream repositories.
So it would be better if I just give you a test-case
in a form of:
begin
prog=devlirc
button=POWER
config=SLEEP
end
begin
prog=devlirc
button=POWER
ignore_first_events=20
config=POWEROFF
end
so that you can pass that through your tools yourself.
Is this acceptable? :)
I will really suggest making the dependencies like
yaml optional. lirc is used on an embedded systems,
and I doubt if our upstream will be very happy adding
things like yaml to their BSP.
OK, I just didn't understand it was this you are referring to. It's in longpress branch, NP
NAK, I need the raw mode 2 input + your lircrc file + expected output which is something very different. Also, you are the only one with the related remote which can generate the raw input.
The yaml package is more or less ubiquotious. It's named python3-PyYAML in many distros. It's also available as a pypi package at https://pypi.python.org/pypi/PyYAML/3.11
There is a short note in CONTRIBUTE.m how to use irtestcase...
I don't see it there.
I don't see the point.
You can configure the long press on any button
from any remote. So what's so valuable in exactly
my configs?
Now, here are too many patches... I need to know in which order you apply them.
Yes, I can configure anything. But the one who really knows how to do this is you, and I think it makes a lot of sense that you are providing this. This is really the "new rules of the game" I try to establish... you can of course walk around it by just adding a testcase, but I presume this is more work (?)
Exactly in order they are attached here
(the today's reference to the prev patch is
disregarded).
I am not sure what "just adding a test-case" mean.
Could you please give me a brief instructions how
to provide you 3 logs (mode2, irw, ircat) separately,
without the use of irtestcase?
Just copy/paste what they write to console?
Indeed, had to rewrite the longpress branch. Re-pushed it after rebase.
Now, this is unchartered territory. It might work, the thing is to be able to give the same input from the remote twice. In short:
gives a log of raw pulse/spaces in printable form. It's probably too much to
paste, redirect it to a file instead.
another. Push the same button presses as you gave to mode2 and store output
from irw and ircat to files (this can most likely be copy-pasted).
Submit the mode2 log, irw log, ircat log, lircd.conf and lircrc config files.
OK, I managed to run irtestcase.
Now this actually landed on top of the api branch. Many thanks for this patch and your work. Not to mention your patience...
I rewrote some the patches somewhat, hiding the original detour. Hope this is OK.
Fixed in [2108d7], [62bf63], [09fca1], [9d7057] and [ad38a4].
leaving ticket open for more comments, but will close unless there is more input.
BTW: Was there any particular problem running irtestcase that needs to be fixed?
Related
Commit: [09fca1]
Commit: [2108d7]
Commit: [62bf63]
Commit: [9d7057]
Commit: [ad38a4]
I think comments can go to the closed ticket
as well (unless some check-box is checked IIRC).
Initially I was only replacing the client lib.
My patch worked well with the old lircd.
But irtestcase refused, so I had to disable
yaml in configure and build things by hands.
What needs to be "fixed" (or just clarified)
is the "Driver default not supported" message.
Since I was compiling and installing things
by hands, something like this would help a lot:
"Unable to find /usr/lib/lirc/plugins/default.so, exiting!"
With "Driver default not supported" I spent
almost the whole day. The biggest gotcha was
that you have drivers/default/* which looked
like a solution. I built it, but it didn't
work at all. It appears plugins/default.c should
be used instead. Suggesting to remove drivers/*
No, its not there. I don't see it in api branch.
Any plans for the release?
If release is done, I'll ask our upstream to
add that feature to their BSP.
Last edit: Stas Sergeev 2014-10-07
But you can find it in top of master, it's merged. The api branch is dead.
Yes, that silly message, I must fix it. Yet one of those things I've thought about without actually doing it. A really spoiled day for you, that was. Feel somewhat guilty.
The plans for next releasae includes handling most of the open tickets. This means also an overhaul of the config system + the remotes database. This will take some time.
Before that there will probably be a 0.9.1 bugfix release (backported patches from master). Certainly not this stuff, though.
Closing. Please try to make some comment so we can make sure that users can comment in closed bugs. If you don't comment, I take as it's impossible :)