Page MenuHomePhabricator

ecore-drm: Add APIs to support key remap functionality
ClosedPublic

Authored by input.hacker on Dec 17 2015, 1:34 AM.

Details

Summary

This adds two new APIs to enable/set key remap functionality and
a number of keys to be remapped to the other keys. As of now there is no
api to do this therefore we need to remap using linux utility such as
'setkeycodes'. By adding/calling these apis, each Ecore_Drm_Evdev device
will have its specific key remap hash and we can apply each remapping keys
for each key/keyboard device.

Test Plan

(1) Enable key remap and set remapping of a key on a specific keyboard device
(2) Plug in the keyboard device and check the key is being remapped or not
(3) Check the other keys are coming normally
(4) Check the the remapping key on a specific keyboard doesn't affect to any other devices

Signed-off-by: Sung-Jin Park <input.hacker@gmail.com>

Diff Detail

Repository
rEFL core/efl
Branch
work
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 915
Build 980: arc lint + arc unit
input.hacker retitled this revision from to ecore-drm: Add APIs to support key remap functionality.
input.hacker updated this object.
input.hacker edited the test plan for this revision. (Show Details)
input.hacker added subscribers: cedric, ohduna, JHyun.

Dear all, please kindly review this changes.
IMO, its better to have key remap functionality in libinput as one of the device configuration stuff.
Now libinput doesn't have the key remap feature, therefore I added it into ecore drm evdev.
Once if libinput has the support for key remap functionality, I'll rewrite these apis to use libinput's.
Actually, as I added key remap feature as a libinput device configuration stuff in my local enviroment,
it works properly and seems better.

Thanks and regards,
Sung-Jin Park

devilhorns requested changes to this revision.Dec 17 2015, 9:57 AM
devilhorns edited edge metadata.
devilhorns added inline comments.
src/lib/ecore_drm/ecore_drm_evdev.c
973

Missing @return here to describe the return value

1002

Missing @return here to describe the return value

This revision now requires changes to proceed.Dec 17 2015, 9:57 AM
ManMower edited edge metadata.Dec 17 2015, 4:38 PM

Unfortunately, it looks like libinput will only accept such things if they're very hardware specific, so what you've done is the right way to go.

libinput doesn't want to have to solve the API issues, would rather leave that to compositors.

input.hacker edited edge metadata.

Two missing lines are added (@return).

input.hacker marked 2 inline comments as done.Dec 17 2015, 7:09 PM

devilhorns, thank you for your review.
I added the two missing lines. :)

Unfortunately, it looks like libinput will only accept such things if they're very hardware specific, so what you've done is the right way to go.

libinput doesn't want to have to solve the API issues, would rather leave that to compositors.

ManMower, thanks for your comment.
As we have a specific USB TV tuner named "au0828 IR (Hauppauge HVR950Q)" and when it's been plugged in a linux machine,
a keys were making a bad keycode(KEY_EXIT) different from its original purpose of keycode (KEY_BACK).
I thought there can be more H/Ws in addition to our H/W and this is why I think key remap feature can be a part of libinput device configuration.

That's as hardware specific as it gets. :)

I'll talk to Peter again - I'm not sure he'll be flexible.

This seems like a kernel driver bug to me though - can't we just fix up the HVR950Q input driver to generate the right buttons in the first place?

Hey, if this is only for static remapping... have you seen udev's scancode to keycode remapping stuff?

look for 60-keyboard.hwdb

https://wiki.archlinux.org/index.php/Map_scancodes_to_keycodes
has some examples..

ManMower, thanks for the information.
This change is for both static and dynamic remapping.
In static remapping cases, this allows compositor to do remapping just with keycode information
and we don't need to know the scancode information.

Anyway, I didn't know udev hwdb and it'll be very helpful to us.
It seems we can deal with many remapping cases with this. :)
Thanks.

devilhorns requested changes to this revision.Dec 20 2015, 8:57 PM
devilhorns edited edge metadata.

With udev, you can assign/reassign based on any rules that are implemented. This means: create the rules, put them in the proper directories, and they will be there when the machine is booted (assuming they are in the proper place and copied over during install)...

That being said, I am NOT against having APIs to do this also.

If it makes life easier for programmers to be able to implement these in other ways, then OK APIs are fine !! ;)

That being said, once we have an API that's been published we CAN NOT change it !!!

SO MAKE SURE THE APIS HERE ARE WHAT YOU WANT !!! (that includes paramaters required)

src/lib/ecore_drm/ecore_drm_evdev.c
1024

Why not just create this hash on Init ???

1034

Personally would prefer double parans here:

if ((!from_keys) || (!to_keys))

1044

Don't really need these brackets...its a 1 liner:

for (...)

eina_hash_add(...)
src/lib/ecore_drm/ecore_drm_private.h
229

struct
{

Eina_Hash hash:
Eina Bool enabled :1';

} key_remap;

Just me thoughts. Structure allows better organization. Easier readability. Easier adabtion for future changes...

This revision now requires changes to proceed.Dec 20 2015, 8:57 PM

Allowing compositor to do remapping is probably something we are going to want anyway. Enlightenment has a BUTT load of remapping possibilities !! Let's just make sure that the API is flexible enough to cover 99% of use cases.

Having read this code a bit, I am ok with it (in the sense that .. it makes sense to have it). Lets just be sure about the API...

Requires Changes !!

src/lib/ecore_drm/Ecore_Drm.h
951

Can we make this a bit more clear ???

EINA_TRUE on success... WHAT IS SUCCESS ?? What is Failure ??

What does OK mean ? what does failure mean ?? Maybe a little bit more about the return.

What CAUSES failure ?? (same for all NEW api here)...

968

Again...what is FAIL ?? what is SUCCESS ??

How about: "returns TRUE when remap works"...

src/lib/ecore_drm/ecore_drm_evdev.c
287

This should be ERROR checked !! What happens if libinput_event_keyboard_get fails and code == 0 ????or -1 or whatever....

No error checking here :(

973

@return needs more detail .. what causes EINA_FALSE here ?? Let the user know (in the docs)...

@ManMower @zmike...

I will VETO any of this that goes in where @return is not explained for the API user...

If we are adding API (which we are going to have to stick with), then let's make sure the docs explain the returns adequately

input.hacker added inline comments.Dec 20 2015, 9:35 PM
src/lib/ecore_drm/Ecore_Drm.h
951

"EINA_TRUE on success" means that the given arguments are valid and we're succeed to enable and are ready to get key remap information on the given evdev device.
"EINA_FALSE on failure" means that the given arguments are invalid and we're not succeed to enable it on the given evdev device.

I'll make it more clear.

968

FAIL means that any one or more given arguments are invalid and therefore we're not ready for setting keymap information on the given evdev device.

I'll make it more clear, too.
I also think it better to change as you recommended. :)

src/lib/ecore_drm/ecore_drm_evdev.c
287

Can you tell me what kinds of error check should be done here ? :)
I think no error check is needed here.

  1. libinput_event_keyboard_get_key() returns uint32_t, therefore no negative value will be returned.
  2. Currently we are using the code getting from libinput_event_keyboard_get_key(). No matter what libinput_event_keyboard_get_key() returns, _device_remapped_key_get() will try to find the coming keycode in a remap hash and will returns the code as it is if there is no remapping key found in it.
1024

eina_hash_new() will do memory allocation even it's small.
I though we can create this hash only when the remap is enable and the information is being set.
One more thing is the following.
When ecore_drm_evdev_key_remap_enable() is called with EINA_FALSE, it'll free the hash.
Therefore we need to create a hash newly when ecore_drm_evdev_key_remap_set() is called.

1034

I'll modify it. :)

1044

I don't need to use it. I'll modify it. :)

src/lib/ecore_drm/ecore_drm_private.h
229

I agree with you.

Yes I understand the "litterals" of EINA_FALSE/TRUE ..

MY POINT IS MORE ALONG LINES OF WHAT THE API READS... Someone quickly reading the docs for the given API functions just MIGHT be checking the return values (and what they stand for) .. (not necessarily reading the whole docs for the function. Just a quick read of usage. Thereforce, (imo) it would be "better" if the docs read: EINA_FALSE means "failed to remap the keys" ... RATHER THAN (Ein_False means something failed). Just a bit more CLARITY is all ;)

Lets make those Minor changes to DOCS, then this is OK !! :)

src/lib/ecore_drm/Ecore_Drm.h
951

I Understand what You meant by docs ;)

But other users May Not !! ;)

Let just be clear on the return value ;)

Example: EINA_FALSE is returned when:

  1. Parameters are not good
  2. Key remapping failed because (... explain)..

I MAY also be good idea to use %m in err msg:

ERR("FAILED TO SET KEYMAP BECAUSE:", %m) <--- This allow error message from previous library function to be printed ;) So if you call "libinput..." function, then THAT error message gets printed ;)

968

I Understand what You meant by docs ;)

But other users May Not !! ;)

Let just be clear on the return value ;)

src/lib/ecore_drm/ecore_drm_evdev.c
257

Please move like "key_remap_hash" check ABOVE "enable" check..

reason:

EINA_SAFETY_ON_NULL_RETURN_VAL(evdev->key_remap_hash...) .. ==== BIGGER FAIL !!!

evdev->key_remap_enable == Maybe remap just not enabled for this evdev. Not REALLY a "big error" (no reason to EINA_SAFETY really) ..

IMO:

EINA_SAFETY_ON_NULL_RETURN_VAL(edev, code);
if (!edev->key_remap_enabled) return code;
EINA_SAFETY_ON_NULL_RETURN(edev->keymap_remap_hash, code)

287
  1. What happens if "libinput_event_keyboard_get_key" return ZERO ???
  1. YES, but WHY call that if ZERO is return above ?? We already know (from ZERO) that is invalid key ... Why make Extra function call if fail above ?>?
1024

Alright (I AM OK WITH THAT) ;)

BUT ..

Lets do:

If (!evdev->key_remap_hash) ..

Not point in ( == NULL) ;)

the ! is good ;)..

So:

if (!evdev->key_remap_hash) ;)

input.hacker added inline comments.Dec 21 2015, 12:27 AM
src/lib/ecore_drm/ecore_drm_evdev.c
287

You're right. :) We can avoid calling _device_remapped_key_get() for ZERO returned by libinput_event_keyboard_get_key() :).

:) It's just minor things ... some Doc cleanup, small (easy) code changes, etc ;) Should not take much more than 5-10 minutes to make this all Right ;)

input.hacker added inline comments.Dec 21 2015, 12:51 AM
src/lib/ecore_drm/ecore_drm_evdev.c
287

As I know, KEY_RESERVED is zero. I think we need to return immediately when libinput_event_keyboard_get_key() return zero, right ? :)

input.hacker edited edge metadata.

fix docs and codes

devilhorns added inline comments.Dec 21 2015, 1:09 AM
src/lib/ecore_drm/ecore_drm_evdev.c
287

If we get a ZERO key code, then likely everything else after that is going to fail anyway (or be using an improper keycode anyway), so probably best to just error out if we get a ZERO from libinput

input.hacker edited edge metadata.

modify to return if we got ZERO as a keycode from libinput

devilhorns requested changes to this revision.Dec 21 2015, 3:07 AM
devilhorns edited edge metadata.
devilhorns added inline comments.
src/lib/ecore_drm/Ecore_Drm.h
951

I still don't like this doc for @return...

"@return EINA_FALSE is returned when the parameters are not good, EINA_TRUE otherwise" ...

Not entirely the case. EINA_FALSE could ALSO be returned if the EvDev (the Ecore_Drm_Evdev passed in) has no actual "libinput device" associated with it.

To just say "paramaters are not good" .. well, to me that sounds Too generic, and IMO @return should provide a bit more information:

I will write this one (as I think it should be) for you ;) Please update any @returns to have more detail like this:

"@return EINA_FALSE is returned If the Ecore_Drm_Evdev is not valid, or if no libinput device has been assigned to it yet. EINA_TRUE will be returned if enabling key remap for this device succeeded."

src/lib/ecore_drm/ecore_drm_evdev.c
288

This is good for checking the return of "libinput_event_keyboard_get_key" .. BUT where "code" is defined above:

uint32_t code..

we should probably Init that with a value...

uint32_t code = KEY_RESERVED ??

or uint32_t code = 0...

973

Please add more detail to @return (as I show above)

This revision now requires changes to proceed.Dec 21 2015, 3:07 AM
input.hacker edited edge metadata.

updated doc for @return sections for both apis

input.hacker added inline comments.Dec 22 2015, 9:57 PM
src/lib/ecore_drm/ecore_drm_evdev.c
288

Do we need to init code value to 0 or KEY_RESERVED ?
I think it probably doesn't have any meaning because the value of code will be overriden by the value from libinput_event_keyboard_get_key().
Anyway from the initialization point of view, it's okay. I'll initialize it with zero. :)

input.hacker edited edge metadata.

initialized code value to zero in _device_handle_key()

updated doc for @return

devilhorns accepted this revision.Dec 28 2015, 6:21 AM
devilhorns edited edge metadata.
This revision is now accepted and ready to land.Dec 28 2015, 6:21 AM
devilhorns closed this revision.Dec 28 2015, 6:25 AM