| Message ID | 20190610213106.19342-1-mail@joaomoreno.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: apple: Fix stuck function keys when using FN | expand |
Hi Jiri & Benjamin, Let me know if you need something else to get this patch moving forward. This fixes an issue I hit daily, it would be great to get it fixed. Thanks. On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > This fixes an issue in which key down events for function keys would be > repeatedly emitted even after the user has raised the physical key. For > example, the driver fails to emit the F5 key up event when going through > the following steps: > - fnmode=1: hold FN, hold F5, release FN, release F5 > - fnmode=2: hold F5, hold FN, release F5, release FN > > The repeated F5 key down events can be easily verified using xev. > > Signed-off-by: Joao Moreno <mail@joaomoreno.com> > --- > drivers/hid/hid-apple.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > index 1cb41992aaa1..81867a6fa047 100644 > --- a/drivers/hid/hid-apple.c > +++ b/drivers/hid/hid-apple.c > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > trans = apple_find_translation (table, usage->code); > > if (trans) { > - if (test_bit(usage->code, asc->pressed_fn)) > - do_translate = 1; > - else if (trans->flags & APPLE_FLAG_FKEY) > - do_translate = (fnmode == 2 && asc->fn_on) || > - (fnmode == 1 && !asc->fn_on); > + int fn_on = value ? asc->fn_on : > + test_bit(usage->code, asc->pressed_fn); > + > + if (!value) > + clear_bit(usage->code, asc->pressed_fn); > + else if (asc->fn_on) > + set_bit(usage->code, asc->pressed_fn); > + > + if (trans->flags & APPLE_FLAG_FKEY) > + do_translate = (fnmode == 2 && fn_on) || > + (fnmode == 1 && !fn_on); > else > do_translate = asc->fn_on; > > if (do_translate) { > - if (value) > - set_bit(usage->code, asc->pressed_fn); > - else > - clear_bit(usage->code, asc->pressed_fn); > - > input_event(input, usage->type, trans->to, > value); > > -- > 2.19.1 >
Hi João, On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > Hi Jiri & Benjamin, > > Let me know if you need something else to get this patch moving forward. This > fixes an issue I hit daily, it would be great to get it fixed. Sorry for the delay, I am very busy with internal corporate stuff, and I tried setting up a new CI system at home, and instead of spending a couple of ours, I am down to 2 weeks of hard work, without possibility to switch to the new right now :( Anyway. > > Thanks. > > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > > > This fixes an issue in which key down events for function keys would be > > repeatedly emitted even after the user has raised the physical key. For > > example, the driver fails to emit the F5 key up event when going through > > the following steps: > > - fnmode=1: hold FN, hold F5, release FN, release F5 > > - fnmode=2: hold F5, hold FN, release F5, release FN Ouch :/ > > > > The repeated F5 key down events can be easily verified using xev. > > > > Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > --- > > drivers/hid/hid-apple.c | 21 +++++++++++---------- > > 1 file changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > index 1cb41992aaa1..81867a6fa047 100644 > > --- a/drivers/hid/hid-apple.c > > +++ b/drivers/hid/hid-apple.c > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > trans = apple_find_translation (table, usage->code); > > > > if (trans) { > > - if (test_bit(usage->code, asc->pressed_fn)) > > - do_translate = 1; > > - else if (trans->flags & APPLE_FLAG_FKEY) > > - do_translate = (fnmode == 2 && asc->fn_on) || > > - (fnmode == 1 && !asc->fn_on); > > + int fn_on = value ? asc->fn_on : > > + test_bit(usage->code, asc->pressed_fn); > > + > > + if (!value) > > + clear_bit(usage->code, asc->pressed_fn); > > + else if (asc->fn_on) > > + set_bit(usage->code, asc->pressed_fn); I have the feeling that this is not the correct fix here. I might be wrong, but the following sequence might also mess up the driver state, depending on how the reports are emitted: - hold FN, hold F4, hold F5, release F4, release FN, release F5 The reason is that the driver only considers you have one key pressed with the modifier, and as the code changed its state based on the last value. IMO a better fix would: - keep the existing `trans` mapping lookout - whenever a `trans` mapping gets found: * get both translated and non-translated currently reported values (`test_bit(keycode, input_dev->key)`) * if one of them is set to true, then consider the keycode to be the one of the key (no matter fn_on) -> deal with `value` with the corrected keycode * if the key was not pressed: -> chose the keycode based on `fn_on` and `fnmode` states and report the key press event This should remove the nasty pressed_fn state which depends on the other pressed keys. Cheers, Benjamin > > + > > + if (trans->flags & APPLE_FLAG_FKEY) > > + do_translate = (fnmode == 2 && fn_on) || > > + (fnmode == 1 && !fn_on); > > else > > do_translate = asc->fn_on; > > > > if (do_translate) { > > - if (value) > > - set_bit(usage->code, asc->pressed_fn); > > - else > > - clear_bit(usage->code, asc->pressed_fn); > > - > > input_event(input, usage->type, trans->to, > > value); > > > > -- > > 2.19.1 > >
Hi Benjamin, No worries, also pretty busy over here. Didn't mean to press. On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi João, > > On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > > > Hi Jiri & Benjamin, > > > > Let me know if you need something else to get this patch moving forward. This > > fixes an issue I hit daily, it would be great to get it fixed. > > Sorry for the delay, I am very busy with internal corporate stuff, and > I tried setting up a new CI system at home, and instead of spending a > couple of ours, I am down to 2 weeks of hard work, without possibility > to switch to the new right now :( > Anyway. > > > > > Thanks. > > > > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > > > > > This fixes an issue in which key down events for function keys would be > > > repeatedly emitted even after the user has raised the physical key. For > > > example, the driver fails to emit the F5 key up event when going through > > > the following steps: > > > - fnmode=1: hold FN, hold F5, release FN, release F5 > > > - fnmode=2: hold F5, hold FN, release F5, release FN > > Ouch :/ > Right?! > > > > > > The repeated F5 key down events can be easily verified using xev. > > > > > > Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > > --- > > > drivers/hid/hid-apple.c | 21 +++++++++++---------- > > > 1 file changed, 11 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > > index 1cb41992aaa1..81867a6fa047 100644 > > > --- a/drivers/hid/hid-apple.c > > > +++ b/drivers/hid/hid-apple.c > > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > trans = apple_find_translation (table, usage->code); > > > > > > if (trans) { > > > - if (test_bit(usage->code, asc->pressed_fn)) > > > - do_translate = 1; > > > - else if (trans->flags & APPLE_FLAG_FKEY) > > > - do_translate = (fnmode == 2 && asc->fn_on) || > > > - (fnmode == 1 && !asc->fn_on); > > > + int fn_on = value ? asc->fn_on : > > > + test_bit(usage->code, asc->pressed_fn); > > > + > > > + if (!value) > > > + clear_bit(usage->code, asc->pressed_fn); > > > + else if (asc->fn_on) > > > + set_bit(usage->code, asc->pressed_fn); > > I have the feeling that this is not the correct fix here. > > I might be wrong, but the following sequence might also mess up the > driver state, depending on how the reports are emitted: > - hold FN, hold F4, hold F5, release F4, release FN, release F5 > I believe this should be fine. Following the code: - hold FN, sets asc->fn_on to true - hold F4, in the trans block fn_on will be true and we'll set the F4 bit in the bitmap - hold F5, in the trans block fn_on will be true and we'll set the F5 bit - release F4, in the trans block fn_on will be true (because of the bitmap) and we'll clear the F4 bit - release FN, asc->fn_on will be false, but it doesn't matter since... - release F5, in the trans block we'll look into the bitmap (instead of asc->fn_on), so fn_on will be true and we'll clear the F5 bit I tested it in practice using my changes: Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is pressed, seems like a hardware limitation. But F6 does work. So, when I execute these events in that order, everything works as it should: xev reports the following: KeyPress F4 KeyPress F6 KeyRelease F4 KeyRelease F6 > The reason is that the driver only considers you have one key pressed > with the modifier, and as the code changed its state based on the last > value. > I believe the bitmap takes care of storing the FN state per key press. The trick I did was to check on the global `asc->fn_on` state only when a key is pressed, but check on the bitmap instead when it's released. Let me know what you think. Am I missing something here? Cheers, João. > IMO a better fix would: > > - keep the existing `trans` mapping lookout > - whenever a `trans` mapping gets found: > * get both translated and non-translated currently reported values > (`test_bit(keycode, input_dev->key)`) > * if one of them is set to true, then consider the keycode to be the > one of the key (no matter fn_on) > -> deal with `value` with the corrected keycode > * if the key was not pressed: > -> chose the keycode based on `fn_on` and `fnmode` states > and report the key press event > > This should remove the nasty pressed_fn state which depends on the > other pressed keys. > > Cheers, > Benjamin > > > > + > > > + if (trans->flags & APPLE_FLAG_FKEY) > > > + do_translate = (fnmode == 2 && fn_on) || > > > + (fnmode == 1 && !fn_on); > > > else > > > do_translate = asc->fn_on; > > > > > > if (do_translate) { > > > - if (value) > > > - set_bit(usage->code, asc->pressed_fn); > > > - else > > > - clear_bit(usage->code, asc->pressed_fn); > > > - > > > input_event(input, usage->type, trans->to, > > > value); > > > > > > -- > > > 2.19.1 > > >
Hi Benjamin, On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: > > Hi Benjamin, > > No worries, also pretty busy over here. Didn't mean to press. > > On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Hi João, > > > > On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > > > > > Hi Jiri & Benjamin, > > > > > > Let me know if you need something else to get this patch moving forward. This > > > fixes an issue I hit daily, it would be great to get it fixed. > > > > Sorry for the delay, I am very busy with internal corporate stuff, and > > I tried setting up a new CI system at home, and instead of spending a > > couple of ours, I am down to 2 weeks of hard work, without possibility > > to switch to the new right now :( > > Anyway. > > > > > > > > Thanks. > > > > > > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > > > > > > > This fixes an issue in which key down events for function keys would be > > > > repeatedly emitted even after the user has raised the physical key. For > > > > example, the driver fails to emit the F5 key up event when going through > > > > the following steps: > > > > - fnmode=1: hold FN, hold F5, release FN, release F5 > > > > - fnmode=2: hold F5, hold FN, release F5, release FN > > > > Ouch :/ > > > > Right?! > > > > > > > > > The repeated F5 key down events can be easily verified using xev. > > > > > > > > Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > > > --- > > > > drivers/hid/hid-apple.c | 21 +++++++++++---------- > > > > 1 file changed, 11 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > > > index 1cb41992aaa1..81867a6fa047 100644 > > > > --- a/drivers/hid/hid-apple.c > > > > +++ b/drivers/hid/hid-apple.c > > > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > > trans = apple_find_translation (table, usage->code); > > > > > > > > if (trans) { > > > > - if (test_bit(usage->code, asc->pressed_fn)) > > > > - do_translate = 1; > > > > - else if (trans->flags & APPLE_FLAG_FKEY) > > > > - do_translate = (fnmode == 2 && asc->fn_on) || > > > > - (fnmode == 1 && !asc->fn_on); > > > > + int fn_on = value ? asc->fn_on : > > > > + test_bit(usage->code, asc->pressed_fn); > > > > + > > > > + if (!value) > > > > + clear_bit(usage->code, asc->pressed_fn); > > > > + else if (asc->fn_on) > > > > + set_bit(usage->code, asc->pressed_fn); > > > > I have the feeling that this is not the correct fix here. > > > > I might be wrong, but the following sequence might also mess up the > > driver state, depending on how the reports are emitted: > > - hold FN, hold F4, hold F5, release F4, release FN, release F5 > > > > I believe this should be fine. Following the code: > > - hold FN, sets asc->fn_on to true > - hold F4, in the trans block fn_on will be true and we'll set the F4 > bit in the bitmap > - hold F5, in the trans block fn_on will be true and we'll set the F5 bit > - release F4, in the trans block fn_on will be true (because of the bitmap) and > we'll clear the F4 bit > - release FN, asc->fn_on will be false, but it doesn't matter since... > - release F5, in the trans block we'll look into the bitmap (instead > of asc->fn_on), > so fn_on will be true and we'll clear the F5 bit > > I tested it in practice using my changes: > > Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is > pressed, seems like a hardware limitation. But F6 does work. So, when I execute > these events in that order, everything works as it should: xev reports > the following: > > KeyPress F4 > KeyPress F6 > KeyRelease F4 > KeyRelease F6 > > > The reason is that the driver only considers you have one key pressed > > with the modifier, and as the code changed its state based on the last > > value. > > > > I believe the bitmap takes care of storing the FN state per key press. The > trick I did was to check on the global `asc->fn_on` state only when a key > is pressed, but check on the bitmap instead when it's released. > > Let me know what you think. Am I missing something here? > > Cheers, > João. > > > IMO a better fix would: > > > > - keep the existing `trans` mapping lookout > > - whenever a `trans` mapping gets found: > > * get both translated and non-translated currently reported values > > (`test_bit(keycode, input_dev->key)`) > > * if one of them is set to true, then consider the keycode to be the > > one of the key (no matter fn_on) > > -> deal with `value` with the corrected keycode > > * if the key was not pressed: > > -> chose the keycode based on `fn_on` and `fnmode` states > > and report the key press event > > > > This should remove the nasty pressed_fn state which depends on the > > other pressed keys. > > > > Cheers, > > Benjamin > > > > > > + > > > > + if (trans->flags & APPLE_FLAG_FKEY) > > > > + do_translate = (fnmode == 2 && fn_on) || > > > > + (fnmode == 1 && !fn_on); > > > > else > > > > do_translate = asc->fn_on; > > > > > > > > if (do_translate) { > > > > - if (value) > > > > - set_bit(usage->code, asc->pressed_fn); > > > > - else > > > > - clear_bit(usage->code, asc->pressed_fn); > > > > - > > > > input_event(input, usage->type, trans->to, > > > > value); > > > > > > > > -- > > > > 2.19.1 > > > > Gave this another look and I still haven't found any issues, let me know if you still think I'm missing something. Thanks! Cheers, João
Hi João, On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote: > > Hi Benjamin, > > On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: > > > > Hi Benjamin, > > > > No worries, also pretty busy over here. Didn't mean to press. > > > > On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Hi João, > > > > > > On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > > > > > > > Hi Jiri & Benjamin, > > > > > > > > Let me know if you need something else to get this patch moving forward. This > > > > fixes an issue I hit daily, it would be great to get it fixed. > > > > > > Sorry for the delay, I am very busy with internal corporate stuff, and > > > I tried setting up a new CI system at home, and instead of spending a > > > couple of ours, I am down to 2 weeks of hard work, without possibility > > > to switch to the new right now :( > > > Anyway. > > > > > > > > > > > Thanks. > > > > > > > > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > > > > > > > > > This fixes an issue in which key down events for function keys would be > > > > > repeatedly emitted even after the user has raised the physical key. For > > > > > example, the driver fails to emit the F5 key up event when going through > > > > > the following steps: > > > > > - fnmode=1: hold FN, hold F5, release FN, release F5 > > > > > - fnmode=2: hold F5, hold FN, release F5, release FN > > > > > > Ouch :/ > > > > > > > Right?! > > > > > > > > > > > > The repeated F5 key down events can be easily verified using xev. > > > > > > > > > > Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > > > > --- > > > > > drivers/hid/hid-apple.c | 21 +++++++++++---------- > > > > > 1 file changed, 11 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > > > > index 1cb41992aaa1..81867a6fa047 100644 > > > > > --- a/drivers/hid/hid-apple.c > > > > > +++ b/drivers/hid/hid-apple.c > > > > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > > > trans = apple_find_translation (table, usage->code); > > > > > > > > > > if (trans) { > > > > > - if (test_bit(usage->code, asc->pressed_fn)) > > > > > - do_translate = 1; > > > > > - else if (trans->flags & APPLE_FLAG_FKEY) > > > > > - do_translate = (fnmode == 2 && asc->fn_on) || > > > > > - (fnmode == 1 && !asc->fn_on); > > > > > + int fn_on = value ? asc->fn_on : > > > > > + test_bit(usage->code, asc->pressed_fn); > > > > > + > > > > > + if (!value) > > > > > + clear_bit(usage->code, asc->pressed_fn); > > > > > + else if (asc->fn_on) > > > > > + set_bit(usage->code, asc->pressed_fn); > > > > > > I have the feeling that this is not the correct fix here. > > > > > > I might be wrong, but the following sequence might also mess up the > > > driver state, depending on how the reports are emitted: > > > - hold FN, hold F4, hold F5, release F4, release FN, release F5 > > > > > > > I believe this should be fine. Following the code: > > > > - hold FN, sets asc->fn_on to true > > - hold F4, in the trans block fn_on will be true and we'll set the F4 > > bit in the bitmap > > - hold F5, in the trans block fn_on will be true and we'll set the F5 bit > > - release F4, in the trans block fn_on will be true (because of the bitmap) and > > we'll clear the F4 bit > > - release FN, asc->fn_on will be false, but it doesn't matter since... > > - release F5, in the trans block we'll look into the bitmap (instead > > of asc->fn_on), > > so fn_on will be true and we'll clear the F5 bit > > > > I tested it in practice using my changes: > > > > Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is > > pressed, seems like a hardware limitation. But F6 does work. So, when I execute > > these events in that order, everything works as it should: xev reports > > the following: > > > > KeyPress F4 > > KeyPress F6 > > KeyRelease F4 > > KeyRelease F6 > > > > > The reason is that the driver only considers you have one key pressed > > > with the modifier, and as the code changed its state based on the last > > > value. > > > > > > > I believe the bitmap takes care of storing the FN state per key press. The > > trick I did was to check on the global `asc->fn_on` state only when a key > > is pressed, but check on the bitmap instead when it's released. > > > > Let me know what you think. Am I missing something here? > > > > Cheers, > > João. > > > > > IMO a better fix would: > > > > > > - keep the existing `trans` mapping lookout > > > - whenever a `trans` mapping gets found: > > > * get both translated and non-translated currently reported values > > > (`test_bit(keycode, input_dev->key)`) > > > * if one of them is set to true, then consider the keycode to be the > > > one of the key (no matter fn_on) > > > -> deal with `value` with the corrected keycode > > > * if the key was not pressed: > > > -> chose the keycode based on `fn_on` and `fnmode` states > > > and report the key press event > > > > > > This should remove the nasty pressed_fn state which depends on the > > > other pressed keys. > > > > > > Cheers, > > > Benjamin > > > > > > > > + > > > > > + if (trans->flags & APPLE_FLAG_FKEY) > > > > > + do_translate = (fnmode == 2 && fn_on) || > > > > > + (fnmode == 1 && !fn_on); > > > > > else > > > > > do_translate = asc->fn_on; > > > > > > > > > > if (do_translate) { > > > > > - if (value) > > > > > - set_bit(usage->code, asc->pressed_fn); > > > > > - else > > > > > - clear_bit(usage->code, asc->pressed_fn); > > > > > - > > > > > input_event(input, usage->type, trans->to, > > > > > value); > > > > > > > > > > -- > > > > > 2.19.1 > > > > > > > Gave this another look and I still haven't found any issues, let me > know if you still > think I'm missing something. Thanks! > OK, I am tempted to take the patch, but I'll need to add this as a regression test in my test-suite. This is too complex that it can easily break next time someone looks at it. Can you send me the report descriptors of the device using hid-recorder from hid-tools[0]. Ideally, if you can put the faulty sequence in the logs, that would help writing down the use case. Cheers, Benjamin [0] https://gitlab.freedesktop.org/libevdev/hid-tools
Hi João, On 8/12/19 6:57 PM, Benjamin Tissoires wrote: > Hi João, > > On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote: >> >> Hi Benjamin, >> >> On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: >>> >>> Hi Benjamin, >>> >>> No worries, also pretty busy over here. Didn't mean to press. >>> >>> On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires >>> <benjamin.tissoires@redhat.com> wrote: >>>> >>>> Hi João, >>>> >>>> On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: >>>>> >>>>> Hi Jiri & Benjamin, >>>>> >>>>> Let me know if you need something else to get this patch moving forward. This >>>>> fixes an issue I hit daily, it would be great to get it fixed. >>>> >>>> Sorry for the delay, I am very busy with internal corporate stuff, and >>>> I tried setting up a new CI system at home, and instead of spending a >>>> couple of ours, I am down to 2 weeks of hard work, without possibility >>>> to switch to the new right now :( >>>> Anyway. >>>> >>>>> >>>>> Thanks. >>>>> >>>>> On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: >>>>>> >>>>>> This fixes an issue in which key down events for function keys would be >>>>>> repeatedly emitted even after the user has raised the physical key. For >>>>>> example, the driver fails to emit the F5 key up event when going through >>>>>> the following steps: >>>>>> - fnmode=1: hold FN, hold F5, release FN, release F5 >>>>>> - fnmode=2: hold F5, hold FN, release F5, release FN >>>> >>>> Ouch :/ >>>> >>> >>> Right?! >>> >>>>>> >>>>>> The repeated F5 key down events can be easily verified using xev. >>>>>> >>>>>> Signed-off-by: Joao Moreno <mail@joaomoreno.com> >>>>>> --- >>>>>> drivers/hid/hid-apple.c | 21 +++++++++++---------- >>>>>> 1 file changed, 11 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c >>>>>> index 1cb41992aaa1..81867a6fa047 100644 >>>>>> --- a/drivers/hid/hid-apple.c >>>>>> +++ b/drivers/hid/hid-apple.c >>>>>> @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, >>>>>> trans = apple_find_translation (table, usage->code); >>>>>> >>>>>> if (trans) { >>>>>> - if (test_bit(usage->code, asc->pressed_fn)) >>>>>> - do_translate = 1; >>>>>> - else if (trans->flags & APPLE_FLAG_FKEY) >>>>>> - do_translate = (fnmode == 2 && asc->fn_on) || >>>>>> - (fnmode == 1 && !asc->fn_on); >>>>>> + int fn_on = value ? asc->fn_on : >>>>>> + test_bit(usage->code, asc->pressed_fn); >>>>>> + >>>>>> + if (!value) >>>>>> + clear_bit(usage->code, asc->pressed_fn); >>>>>> + else if (asc->fn_on) >>>>>> + set_bit(usage->code, asc->pressed_fn); >>>> >>>> I have the feeling that this is not the correct fix here. >>>> >>>> I might be wrong, but the following sequence might also mess up the >>>> driver state, depending on how the reports are emitted: >>>> - hold FN, hold F4, hold F5, release F4, release FN, release F5 >>>> >>> >>> I believe this should be fine. Following the code: >>> >>> - hold FN, sets asc->fn_on to true >>> - hold F4, in the trans block fn_on will be true and we'll set the F4 >>> bit in the bitmap >>> - hold F5, in the trans block fn_on will be true and we'll set the F5 bit >>> - release F4, in the trans block fn_on will be true (because of the bitmap) and >>> we'll clear the F4 bit >>> - release FN, asc->fn_on will be false, but it doesn't matter since... >>> - release F5, in the trans block we'll look into the bitmap (instead >>> of asc->fn_on), >>> so fn_on will be true and we'll clear the F5 bit >>> >>> I tested it in practice using my changes: >>> >>> Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is >>> pressed, seems like a hardware limitation. But F6 does work. So, when I execute >>> these events in that order, everything works as it should: xev reports >>> the following: >>> >>> KeyPress F4 >>> KeyPress F6 >>> KeyRelease F4 >>> KeyRelease F6 >>> >>>> The reason is that the driver only considers you have one key pressed >>>> with the modifier, and as the code changed its state based on the last >>>> value. >>>> >>> >>> I believe the bitmap takes care of storing the FN state per key press. The >>> trick I did was to check on the global `asc->fn_on` state only when a key >>> is pressed, but check on the bitmap instead when it's released. >>> >>> Let me know what you think. Am I missing something here? >>> >>> Cheers, >>> João. >>> >>>> IMO a better fix would: >>>> >>>> - keep the existing `trans` mapping lookout >>>> - whenever a `trans` mapping gets found: >>>> * get both translated and non-translated currently reported values >>>> (`test_bit(keycode, input_dev->key)`) >>>> * if one of them is set to true, then consider the keycode to be the >>>> one of the key (no matter fn_on) >>>> -> deal with `value` with the corrected keycode >>>> * if the key was not pressed: >>>> -> chose the keycode based on `fn_on` and `fnmode` states >>>> and report the key press event >>>> >>>> This should remove the nasty pressed_fn state which depends on the >>>> other pressed keys. >>>> >>>> Cheers, >>>> Benjamin >>>> >>>>>> + >>>>>> + if (trans->flags & APPLE_FLAG_FKEY) >>>>>> + do_translate = (fnmode == 2 && fn_on) || >>>>>> + (fnmode == 1 && !fn_on); >>>>>> else >>>>>> do_translate = asc->fn_on; >>>>>> >>>>>> if (do_translate) { >>>>>> - if (value) >>>>>> - set_bit(usage->code, asc->pressed_fn); >>>>>> - else >>>>>> - clear_bit(usage->code, asc->pressed_fn); >>>>>> - >>>>>> input_event(input, usage->type, trans->to, >>>>>> value); >>>>>> >>>>>> -- >>>>>> 2.19.1 >>>>>> >> >> Gave this another look and I still haven't found any issues, let me >> know if you still >> think I'm missing something. Thanks! >> > > OK, I am tempted to take the patch, but I'll need to add this as a > regression test in my test-suite. This is too complex that it can > easily break next time someone looks at it. > > Can you send me the report descriptors of the device using > hid-recorder from hid-tools[0]. > Ideally, if you can put the faulty sequence in the logs, that would > help writing down the use case. > I finally managed to get the regression tests up in https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/52 This allowed me to better understand the code, and yes, for the F-keys, I could not find a faulty situation with your patch. However, the same problem is happening with the arrow keys, as arrow up is translated to page up. We *could* adapt your patch to solve this, but I find it really hard to understand. How about the following diff: --- diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c index 81df62f48c4c..ef916c0f3c0b 100644 --- a/drivers/hid/hid-apple.c +++ b/drivers/hid/hid-apple.c @@ -54,7 +54,6 @@ MODULE_PARM_DESC(swap_opt_cmd, "Swap the Option (\"Alt\") and Command (\"Flag\") struct apple_sc { unsigned long quirks; unsigned int fn_on; - DECLARE_BITMAP(pressed_fn, KEY_CNT); DECLARE_BITMAP(pressed_numlock, KEY_CNT); }; @@ -181,6 +180,8 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, { struct apple_sc *asc = hid_get_drvdata(hid); const struct apple_key_translation *trans, *table; + bool do_translate; + u16 code = 0; if (usage->code == KEY_FN) { asc->fn_on = !!value; @@ -189,8 +190,6 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, } if (fnmode) { - int do_translate; - if (hid->product >= USB_DEVICE_ID_APPLE_WELLSPRING4_ANSI && hid->product <= USB_DEVICE_ID_APPLE_WELLSPRING4A_JIS) table = macbookair_fn_keys; @@ -202,25 +201,33 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, trans = apple_find_translation (table, usage->code); if (trans) { - if (test_bit(usage->code, asc->pressed_fn)) - do_translate = 1; - else if (trans->flags & APPLE_FLAG_FKEY) - do_translate = (fnmode == 2 && asc->fn_on) || - (fnmode == 1 && !asc->fn_on); - else - do_translate = asc->fn_on; - - if (do_translate) { - if (value) - set_bit(usage->code, asc->pressed_fn); - else - clear_bit(usage->code, asc->pressed_fn); - - input_event(input, usage->type, trans->to, - value); - - return 1; + if (test_bit(trans->from, input->key)) + code = trans->from; + if (test_bit(trans->to, input->key)) + code = trans->to; + + if (!code) { + if (trans->flags & APPLE_FLAG_FKEY) { + switch (fnmode) { + case 1: + do_translate = !asc->fn_on; + break; + case 2: + do_translate = asc->fn_on; + break; + default: + /* should never happen */ + do_translate = false; + } + } else { + do_translate = asc->fn_on; + } + + code = do_translate ? trans->to : trans->from; } + + input_event(input, usage->type, code, value); + return 1; } if (asc->quirks & APPLE_NUMLOCK_EMULATION && --- This is more or less what I suggested, minus the bug fixes. I find this easier to understand as the .pressed_fn field is not there anymore and we just rely on the actual state of the input subsystem. Comments? Cheers, Benjamin > Cheers, > Benjamin > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools >
Hi Benjamin, First of all, sorry for the late reply. Turns out a newborn baby can keep one quite busy, who would have known? :) On Tue, 27 Aug 2019 at 18:57, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi João, > > On 8/12/19 6:57 PM, Benjamin Tissoires wrote: > > Hi João, > > > > On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote: > >> > >> Hi Benjamin, > >> > >> On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: > >>> > >>> Hi Benjamin, > >>> > >>> No worries, also pretty busy over here. Didn't mean to press. > >>> > >>> On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires > >>> <benjamin.tissoires@redhat.com> wrote: > >>>> > >>>> Hi João, > >>>> > >>>> On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > >>>>> > >>>>> Hi Jiri & Benjamin, > >>>>> > >>>>> Let me know if you need something else to get this patch moving forward. This > >>>>> fixes an issue I hit daily, it would be great to get it fixed. > >>>> > >>>> Sorry for the delay, I am very busy with internal corporate stuff, and > >>>> I tried setting up a new CI system at home, and instead of spending a > >>>> couple of ours, I am down to 2 weeks of hard work, without possibility > >>>> to switch to the new right now :( > >>>> Anyway. > >>>> > >>>>> > >>>>> Thanks. > >>>>> > >>>>> On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > >>>>>> > >>>>>> This fixes an issue in which key down events for function keys would be > >>>>>> repeatedly emitted even after the user has raised the physical key. For > >>>>>> example, the driver fails to emit the F5 key up event when going through > >>>>>> the following steps: > >>>>>> - fnmode=1: hold FN, hold F5, release FN, release F5 > >>>>>> - fnmode=2: hold F5, hold FN, release F5, release FN > >>>> > >>>> Ouch :/ > >>>> > >>> > >>> Right?! > >>> > >>>>>> > >>>>>> The repeated F5 key down events can be easily verified using xev. > >>>>>> > >>>>>> Signed-off-by: Joao Moreno <mail@joaomoreno.com> > >>>>>> --- > >>>>>> drivers/hid/hid-apple.c | 21 +++++++++++---------- > >>>>>> 1 file changed, 11 insertions(+), 10 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > >>>>>> index 1cb41992aaa1..81867a6fa047 100644 > >>>>>> --- a/drivers/hid/hid-apple.c > >>>>>> +++ b/drivers/hid/hid-apple.c > >>>>>> @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > >>>>>> trans = apple_find_translation (table, usage->code); > >>>>>> > >>>>>> if (trans) { > >>>>>> - if (test_bit(usage->code, asc->pressed_fn)) > >>>>>> - do_translate = 1; > >>>>>> - else if (trans->flags & APPLE_FLAG_FKEY) > >>>>>> - do_translate = (fnmode == 2 && asc->fn_on) || > >>>>>> - (fnmode == 1 && !asc->fn_on); > >>>>>> + int fn_on = value ? asc->fn_on : > >>>>>> + test_bit(usage->code, asc->pressed_fn); > >>>>>> + > >>>>>> + if (!value) > >>>>>> + clear_bit(usage->code, asc->pressed_fn); > >>>>>> + else if (asc->fn_on) > >>>>>> + set_bit(usage->code, asc->pressed_fn); > >>>> > >>>> I have the feeling that this is not the correct fix here. > >>>> > >>>> I might be wrong, but the following sequence might also mess up the > >>>> driver state, depending on how the reports are emitted: > >>>> - hold FN, hold F4, hold F5, release F4, release FN, release F5 > >>>> > >>> > >>> I believe this should be fine. Following the code: > >>> > >>> - hold FN, sets asc->fn_on to true > >>> - hold F4, in the trans block fn_on will be true and we'll set the F4 > >>> bit in the bitmap > >>> - hold F5, in the trans block fn_on will be true and we'll set the F5 bit > >>> - release F4, in the trans block fn_on will be true (because of the bitmap) and > >>> we'll clear the F4 bit > >>> - release FN, asc->fn_on will be false, but it doesn't matter since... > >>> - release F5, in the trans block we'll look into the bitmap (instead > >>> of asc->fn_on), > >>> so fn_on will be true and we'll clear the F5 bit > >>> > >>> I tested it in practice using my changes: > >>> > >>> Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is > >>> pressed, seems like a hardware limitation. But F6 does work. So, when I execute > >>> these events in that order, everything works as it should: xev reports > >>> the following: > >>> > >>> KeyPress F4 > >>> KeyPress F6 > >>> KeyRelease F4 > >>> KeyRelease F6 > >>> > >>>> The reason is that the driver only considers you have one key pressed > >>>> with the modifier, and as the code changed its state based on the last > >>>> value. > >>>> > >>> > >>> I believe the bitmap takes care of storing the FN state per key press. The > >>> trick I did was to check on the global `asc->fn_on` state only when a key > >>> is pressed, but check on the bitmap instead when it's released. > >>> > >>> Let me know what you think. Am I missing something here? > >>> > >>> Cheers, > >>> João. > >>> > >>>> IMO a better fix would: > >>>> > >>>> - keep the existing `trans` mapping lookout > >>>> - whenever a `trans` mapping gets found: > >>>> * get both translated and non-translated currently reported values > >>>> (`test_bit(keycode, input_dev->key)`) > >>>> * if one of them is set to true, then consider the keycode to be the > >>>> one of the key (no matter fn_on) > >>>> -> deal with `value` with the corrected keycode > >>>> * if the key was not pressed: > >>>> -> chose the keycode based on `fn_on` and `fnmode` states > >>>> and report the key press event > >>>> > >>>> This should remove the nasty pressed_fn state which depends on the > >>>> other pressed keys. > >>>> > >>>> Cheers, > >>>> Benjamin > >>>> > >>>>>> + > >>>>>> + if (trans->flags & APPLE_FLAG_FKEY) > >>>>>> + do_translate = (fnmode == 2 && fn_on) || > >>>>>> + (fnmode == 1 && !fn_on); > >>>>>> else > >>>>>> do_translate = asc->fn_on; > >>>>>> > >>>>>> if (do_translate) { > >>>>>> - if (value) > >>>>>> - set_bit(usage->code, asc->pressed_fn); > >>>>>> - else > >>>>>> - clear_bit(usage->code, asc->pressed_fn); > >>>>>> - > >>>>>> input_event(input, usage->type, trans->to, > >>>>>> value); > >>>>>> > >>>>>> -- > >>>>>> 2.19.1 > >>>>>> > >> > >> Gave this another look and I still haven't found any issues, let me > >> know if you still > >> think I'm missing something. Thanks! > >> > > > > OK, I am tempted to take the patch, but I'll need to add this as a > > regression test in my test-suite. This is too complex that it can > > easily break next time someone looks at it. > > > > Can you send me the report descriptors of the device using > > hid-recorder from hid-tools[0]. > > Ideally, if you can put the faulty sequence in the logs, that would > > help writing down the use case. > > > > I finally managed to get the regression tests up in > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/52 > This is great, really cool that you can write proper tests for this. I assume you don't need me to send you the report descriptors any more? > This allowed me to better understand the code, and yes, for the F-keys, > I could not find a faulty situation with your patch. > > However, the same problem is happening with the arrow keys, as arrow up > is translated to page up. > Great catch, I can easily repro that issue as well on my keyboard. > We *could* adapt your patch to solve this, but I find it really hard to understand. > > How about the following diff: > --- > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > index 81df62f48c4c..ef916c0f3c0b 100644 > --- a/drivers/hid/hid-apple.c > +++ b/drivers/hid/hid-apple.c > @@ -54,7 +54,6 @@ MODULE_PARM_DESC(swap_opt_cmd, "Swap the Option (\"Alt\") and Command (\"Flag\") > struct apple_sc { > unsigned long quirks; > unsigned int fn_on; > - DECLARE_BITMAP(pressed_fn, KEY_CNT); > DECLARE_BITMAP(pressed_numlock, KEY_CNT); > }; > > @@ -181,6 +180,8 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > { > struct apple_sc *asc = hid_get_drvdata(hid); > const struct apple_key_translation *trans, *table; > + bool do_translate; > + u16 code = 0; > > if (usage->code == KEY_FN) { > asc->fn_on = !!value; > @@ -189,8 +190,6 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > } > > if (fnmode) { > - int do_translate; > - > if (hid->product >= USB_DEVICE_ID_APPLE_WELLSPRING4_ANSI && > hid->product <= USB_DEVICE_ID_APPLE_WELLSPRING4A_JIS) > table = macbookair_fn_keys; > @@ -202,25 +201,33 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > trans = apple_find_translation (table, usage->code); > > if (trans) { > - if (test_bit(usage->code, asc->pressed_fn)) > - do_translate = 1; > - else if (trans->flags & APPLE_FLAG_FKEY) > - do_translate = (fnmode == 2 && asc->fn_on) || > - (fnmode == 1 && !asc->fn_on); > - else > - do_translate = asc->fn_on; > - > - if (do_translate) { > - if (value) > - set_bit(usage->code, asc->pressed_fn); > - else > - clear_bit(usage->code, asc->pressed_fn); > - > - input_event(input, usage->type, trans->to, > - value); > - > - return 1; > + if (test_bit(trans->from, input->key)) > + code = trans->from; > + if (test_bit(trans->to, input->key)) > + code = trans->to; I see, this is what you meant before. I still don't quite understand what information `input->key` actually holds. Can you elaborate on what it contains? How does it differ from `usage->code`? Also, should the second `if` be an `else if` instead? We shouldn't have to call `test_bit` more than once here, right? If right, then the next `if` would could simply be an `else`, since the translation tables should not have zeros in them. > + > + if (!code) { > + if (trans->flags & APPLE_FLAG_FKEY) { > + switch (fnmode) { > + case 1: > + do_translate = !asc->fn_on; > + break; > + case 2: > + do_translate = asc->fn_on; > + break; > + default: > + /* should never happen */ > + do_translate = false; > + } > + } else { > + do_translate = asc->fn_on; > + } > + > + code = do_translate ? trans->to : trans->from; > } > + > + input_event(input, usage->type, code, value); > + return 1; > } > > if (asc->quirks & APPLE_NUMLOCK_EMULATION && > --- > > This is more or less what I suggested, minus the bug fixes. > > I find this easier to understand as the .pressed_fn field is not there > anymore and we just rely on the actual state of the input subsystem. > > Comments? Love it. Seems to catch all the cases on my hardware. Thanks for following up with the patch! > > Cheers, > Benjamin > > > > Cheers, > > Benjamin > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > Cheers, João
Hi João, On Mon, Sep 2, 2019 at 4:44 PM João Moreno <mail@joaomoreno.com> wrote: > > Hi Benjamin, > > First of all, sorry for the late reply. Turns out a newborn baby can > keep one quite busy, who would have known? :) heh. Well, congrats! > > On Tue, 27 Aug 2019 at 18:57, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Hi João, > > > > On 8/12/19 6:57 PM, Benjamin Tissoires wrote: > > > Hi João, > > > > > > On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote: > > >> > > >> Hi Benjamin, > > >> > > >> On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: > > >>> > > >>> Hi Benjamin, > > >>> > > >>> No worries, also pretty busy over here. Didn't mean to press. > > >>> > > >>> On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires > > >>> <benjamin.tissoires@redhat.com> wrote: > > >>>> > > >>>> Hi João, > > >>>> > > >>>> On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > >>>>> > > >>>>> Hi Jiri & Benjamin, > > >>>>> > > >>>>> Let me know if you need something else to get this patch moving forward. This > > >>>>> fixes an issue I hit daily, it would be great to get it fixed. > > >>>> > > >>>> Sorry for the delay, I am very busy with internal corporate stuff, and > > >>>> I tried setting up a new CI system at home, and instead of spending a > > >>>> couple of ours, I am down to 2 weeks of hard work, without possibility > > >>>> to switch to the new right now :( > > >>>> Anyway. > > >>>> > > >>>>> > > >>>>> Thanks. > > >>>>> > > >>>>> On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > >>>>>> > > >>>>>> This fixes an issue in which key down events for function keys would be > > >>>>>> repeatedly emitted even after the user has raised the physical key. For > > >>>>>> example, the driver fails to emit the F5 key up event when going through > > >>>>>> the following steps: > > >>>>>> - fnmode=1: hold FN, hold F5, release FN, release F5 > > >>>>>> - fnmode=2: hold F5, hold FN, release F5, release FN > > >>>> > > >>>> Ouch :/ > > >>>> > > >>> > > >>> Right?! > > >>> > > >>>>>> > > >>>>>> The repeated F5 key down events can be easily verified using xev. > > >>>>>> > > >>>>>> Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > >>>>>> --- > > >>>>>> drivers/hid/hid-apple.c | 21 +++++++++++---------- > > >>>>>> 1 file changed, 11 insertions(+), 10 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > >>>>>> index 1cb41992aaa1..81867a6fa047 100644 > > >>>>>> --- a/drivers/hid/hid-apple.c > > >>>>>> +++ b/drivers/hid/hid-apple.c > > >>>>>> @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > >>>>>> trans = apple_find_translation (table, usage->code); > > >>>>>> > > >>>>>> if (trans) { > > >>>>>> - if (test_bit(usage->code, asc->pressed_fn)) > > >>>>>> - do_translate = 1; > > >>>>>> - else if (trans->flags & APPLE_FLAG_FKEY) > > >>>>>> - do_translate = (fnmode == 2 && asc->fn_on) || > > >>>>>> - (fnmode == 1 && !asc->fn_on); > > >>>>>> + int fn_on = value ? asc->fn_on : > > >>>>>> + test_bit(usage->code, asc->pressed_fn); > > >>>>>> + > > >>>>>> + if (!value) > > >>>>>> + clear_bit(usage->code, asc->pressed_fn); > > >>>>>> + else if (asc->fn_on) > > >>>>>> + set_bit(usage->code, asc->pressed_fn); > > >>>> > > >>>> I have the feeling that this is not the correct fix here. > > >>>> > > >>>> I might be wrong, but the following sequence might also mess up the > > >>>> driver state, depending on how the reports are emitted: > > >>>> - hold FN, hold F4, hold F5, release F4, release FN, release F5 > > >>>> > > >>> > > >>> I believe this should be fine. Following the code: > > >>> > > >>> - hold FN, sets asc->fn_on to true > > >>> - hold F4, in the trans block fn_on will be true and we'll set the F4 > > >>> bit in the bitmap > > >>> - hold F5, in the trans block fn_on will be true and we'll set the F5 bit > > >>> - release F4, in the trans block fn_on will be true (because of the bitmap) and > > >>> we'll clear the F4 bit > > >>> - release FN, asc->fn_on will be false, but it doesn't matter since... > > >>> - release F5, in the trans block we'll look into the bitmap (instead > > >>> of asc->fn_on), > > >>> so fn_on will be true and we'll clear the F5 bit > > >>> > > >>> I tested it in practice using my changes: > > >>> > > >>> Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is > > >>> pressed, seems like a hardware limitation. But F6 does work. So, when I execute > > >>> these events in that order, everything works as it should: xev reports > > >>> the following: > > >>> > > >>> KeyPress F4 > > >>> KeyPress F6 > > >>> KeyRelease F4 > > >>> KeyRelease F6 > > >>> > > >>>> The reason is that the driver only considers you have one key pressed > > >>>> with the modifier, and as the code changed its state based on the last > > >>>> value. > > >>>> > > >>> > > >>> I believe the bitmap takes care of storing the FN state per key press. The > > >>> trick I did was to check on the global `asc->fn_on` state only when a key > > >>> is pressed, but check on the bitmap instead when it's released. > > >>> > > >>> Let me know what you think. Am I missing something here? > > >>> > > >>> Cheers, > > >>> João. > > >>> > > >>>> IMO a better fix would: > > >>>> > > >>>> - keep the existing `trans` mapping lookout > > >>>> - whenever a `trans` mapping gets found: > > >>>> * get both translated and non-translated currently reported values > > >>>> (`test_bit(keycode, input_dev->key)`) > > >>>> * if one of them is set to true, then consider the keycode to be the > > >>>> one of the key (no matter fn_on) > > >>>> -> deal with `value` with the corrected keycode > > >>>> * if the key was not pressed: > > >>>> -> chose the keycode based on `fn_on` and `fnmode` states > > >>>> and report the key press event > > >>>> > > >>>> This should remove the nasty pressed_fn state which depends on the > > >>>> other pressed keys. > > >>>> > > >>>> Cheers, > > >>>> Benjamin > > >>>> > > >>>>>> + > > >>>>>> + if (trans->flags & APPLE_FLAG_FKEY) > > >>>>>> + do_translate = (fnmode == 2 && fn_on) || > > >>>>>> + (fnmode == 1 && !fn_on); > > >>>>>> else > > >>>>>> do_translate = asc->fn_on; > > >>>>>> > > >>>>>> if (do_translate) { > > >>>>>> - if (value) > > >>>>>> - set_bit(usage->code, asc->pressed_fn); > > >>>>>> - else > > >>>>>> - clear_bit(usage->code, asc->pressed_fn); > > >>>>>> - > > >>>>>> input_event(input, usage->type, trans->to, > > >>>>>> value); > > >>>>>> > > >>>>>> -- > > >>>>>> 2.19.1 > > >>>>>> > > >> > > >> Gave this another look and I still haven't found any issues, let me > > >> know if you still > > >> think I'm missing something. Thanks! > > >> > > > > > > OK, I am tempted to take the patch, but I'll need to add this as a > > > regression test in my test-suite. This is too complex that it can > > > easily break next time someone looks at it. > > > > > > Can you send me the report descriptors of the device using > > > hid-recorder from hid-tools[0]. > > > Ideally, if you can put the faulty sequence in the logs, that would > > > help writing down the use case. > > > > > > > I finally managed to get the regression tests up in > > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/52 > > > > This is great, really cool that you can write proper tests for this. I > assume you don't need me to send you the report descriptors any more? It would be nice to have yours too. Just in case they differ from the one I have here. > > > This allowed me to better understand the code, and yes, for the F-keys, > > I could not find a faulty situation with your patch. > > > > However, the same problem is happening with the arrow keys, as arrow up > > is translated to page up. > > > > Great catch, I can easily repro that issue as well on my keyboard. > > > We *could* adapt your patch to solve this, but I find it really hard to understand. > > > > How about the following diff: > > --- > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > index 81df62f48c4c..ef916c0f3c0b 100644 > > --- a/drivers/hid/hid-apple.c > > +++ b/drivers/hid/hid-apple.c > > @@ -54,7 +54,6 @@ MODULE_PARM_DESC(swap_opt_cmd, "Swap the Option (\"Alt\") and Command (\"Flag\") > > struct apple_sc { > > unsigned long quirks; > > unsigned int fn_on; > > - DECLARE_BITMAP(pressed_fn, KEY_CNT); > > DECLARE_BITMAP(pressed_numlock, KEY_CNT); > > }; > > > > @@ -181,6 +180,8 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > { > > struct apple_sc *asc = hid_get_drvdata(hid); > > const struct apple_key_translation *trans, *table; > > + bool do_translate; > > + u16 code = 0; > > > > if (usage->code == KEY_FN) { > > asc->fn_on = !!value; > > @@ -189,8 +190,6 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > } > > > > if (fnmode) { > > - int do_translate; > > - > > if (hid->product >= USB_DEVICE_ID_APPLE_WELLSPRING4_ANSI && > > hid->product <= USB_DEVICE_ID_APPLE_WELLSPRING4A_JIS) > > table = macbookair_fn_keys; > > @@ -202,25 +201,33 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > trans = apple_find_translation (table, usage->code); > > > > if (trans) { > > - if (test_bit(usage->code, asc->pressed_fn)) > > - do_translate = 1; > > - else if (trans->flags & APPLE_FLAG_FKEY) > > - do_translate = (fnmode == 2 && asc->fn_on) || > > - (fnmode == 1 && !asc->fn_on); > > - else > > - do_translate = asc->fn_on; > > - > > - if (do_translate) { > > - if (value) > > - set_bit(usage->code, asc->pressed_fn); > > - else > > - clear_bit(usage->code, asc->pressed_fn); > > - > > - input_event(input, usage->type, trans->to, > > - value); > > - > > - return 1; > > + if (test_bit(trans->from, input->key)) > > + code = trans->from; > > + if (test_bit(trans->to, input->key)) > > + code = trans->to; > > I see, this is what you meant before. I still don't quite understand > what information `input->key` actually holds. Can you elaborate on > what it contains? How does it differ from `usage->code`? input is the input device itself (from the kernel point of view) and key is the bitmask of the currently reported pressed keys. usage->code is the HID usage (so from the hardware point of view) and identifies which physical key is being pressed. And last, asc->pressed_fn contained the bitmask of which physical key was pressed, while here we store which reported key is sent. > > Also, should the second `if` be an `else if` instead? We shouldn't > have to call `test_bit` more than once here, right? If right, then the > next `if` would could simply be an `else`, since the translation > tables should not have zeros in them. right we can do that. > > > + > > + if (!code) { > > + if (trans->flags & APPLE_FLAG_FKEY) { > > + switch (fnmode) { > > + case 1: > > + do_translate = !asc->fn_on; > > + break; > > + case 2: > > + do_translate = asc->fn_on; > > + break; > > + default: > > + /* should never happen */ > > + do_translate = false; > > + } > > + } else { > > + do_translate = asc->fn_on; > > + } > > + > > + code = do_translate ? trans->to : trans->from; > > } > > + > > + input_event(input, usage->type, code, value); > > + return 1; > > } > > > > if (asc->quirks & APPLE_NUMLOCK_EMULATION && > > --- > > > > This is more or less what I suggested, minus the bug fixes. > > > > I find this easier to understand as the .pressed_fn field is not there > > anymore and we just rely on the actual state of the input subsystem. > > > > Comments? > > Love it. Seems to catch all the cases on my hardware. Thanks for > following up with the patch! Thanks I'll properly resend the patch. I guess I should keep your from and you can later add your signed-off-by line if you agree. (but likely not today, I have to pick up my girls at school, things you'll discover in a few years, or months for daycare :-P). Cheers, Benjamin > > > > > Cheers, > > Benjamin > > > > > > > Cheers, > > > Benjamin > > > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > > > Cheers, > João
Hi Benjamin, On Mon, 2 Sep 2019 at 17:23, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi João, > > On Mon, Sep 2, 2019 at 4:44 PM João Moreno <mail@joaomoreno.com> wrote: > > > > Hi Benjamin, > > > > First of all, sorry for the late reply. Turns out a newborn baby can > > keep one quite busy, who would have known? :) > > heh. Well, congrats! > > > > > On Tue, 27 Aug 2019 at 18:57, Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Hi João, > > > > > > On 8/12/19 6:57 PM, Benjamin Tissoires wrote: > > > > Hi João, > > > > > > > > On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote: > > > >> > > > >> Hi Benjamin, > > > >> > > > >> On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote: > > > >>> > > > >>> Hi Benjamin, > > > >>> > > > >>> No worries, also pretty busy over here. Didn't mean to press. > > > >>> > > > >>> On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires > > > >>> <benjamin.tissoires@redhat.com> wrote: > > > >>>> > > > >>>> Hi João, > > > >>>> > > > >>>> On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote: > > > >>>>> > > > >>>>> Hi Jiri & Benjamin, > > > >>>>> > > > >>>>> Let me know if you need something else to get this patch moving forward. This > > > >>>>> fixes an issue I hit daily, it would be great to get it fixed. > > > >>>> > > > >>>> Sorry for the delay, I am very busy with internal corporate stuff, and > > > >>>> I tried setting up a new CI system at home, and instead of spending a > > > >>>> couple of ours, I am down to 2 weeks of hard work, without possibility > > > >>>> to switch to the new right now :( > > > >>>> Anyway. > > > >>>> > > > >>>>> > > > >>>>> Thanks. > > > >>>>> > > > >>>>> On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote: > > > >>>>>> > > > >>>>>> This fixes an issue in which key down events for function keys would be > > > >>>>>> repeatedly emitted even after the user has raised the physical key. For > > > >>>>>> example, the driver fails to emit the F5 key up event when going through > > > >>>>>> the following steps: > > > >>>>>> - fnmode=1: hold FN, hold F5, release FN, release F5 > > > >>>>>> - fnmode=2: hold F5, hold FN, release F5, release FN > > > >>>> > > > >>>> Ouch :/ > > > >>>> > > > >>> > > > >>> Right?! > > > >>> > > > >>>>>> > > > >>>>>> The repeated F5 key down events can be easily verified using xev. > > > >>>>>> > > > >>>>>> Signed-off-by: Joao Moreno <mail@joaomoreno.com> > > > >>>>>> --- > > > >>>>>> drivers/hid/hid-apple.c | 21 +++++++++++---------- > > > >>>>>> 1 file changed, 11 insertions(+), 10 deletions(-) > > > >>>>>> > > > >>>>>> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > > >>>>>> index 1cb41992aaa1..81867a6fa047 100644 > > > >>>>>> --- a/drivers/hid/hid-apple.c > > > >>>>>> +++ b/drivers/hid/hid-apple.c > > > >>>>>> @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > >>>>>> trans = apple_find_translation (table, usage->code); > > > >>>>>> > > > >>>>>> if (trans) { > > > >>>>>> - if (test_bit(usage->code, asc->pressed_fn)) > > > >>>>>> - do_translate = 1; > > > >>>>>> - else if (trans->flags & APPLE_FLAG_FKEY) > > > >>>>>> - do_translate = (fnmode == 2 && asc->fn_on) || > > > >>>>>> - (fnmode == 1 && !asc->fn_on); > > > >>>>>> + int fn_on = value ? asc->fn_on : > > > >>>>>> + test_bit(usage->code, asc->pressed_fn); > > > >>>>>> + > > > >>>>>> + if (!value) > > > >>>>>> + clear_bit(usage->code, asc->pressed_fn); > > > >>>>>> + else if (asc->fn_on) > > > >>>>>> + set_bit(usage->code, asc->pressed_fn); > > > >>>> > > > >>>> I have the feeling that this is not the correct fix here. > > > >>>> > > > >>>> I might be wrong, but the following sequence might also mess up the > > > >>>> driver state, depending on how the reports are emitted: > > > >>>> - hold FN, hold F4, hold F5, release F4, release FN, release F5 > > > >>>> > > > >>> > > > >>> I believe this should be fine. Following the code: > > > >>> > > > >>> - hold FN, sets asc->fn_on to true > > > >>> - hold F4, in the trans block fn_on will be true and we'll set the F4 > > > >>> bit in the bitmap > > > >>> - hold F5, in the trans block fn_on will be true and we'll set the F5 bit > > > >>> - release F4, in the trans block fn_on will be true (because of the bitmap) and > > > >>> we'll clear the F4 bit > > > >>> - release FN, asc->fn_on will be false, but it doesn't matter since... > > > >>> - release F5, in the trans block we'll look into the bitmap (instead > > > >>> of asc->fn_on), > > > >>> so fn_on will be true and we'll clear the F5 bit > > > >>> > > > >>> I tested it in practice using my changes: > > > >>> > > > >>> Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is > > > >>> pressed, seems like a hardware limitation. But F6 does work. So, when I execute > > > >>> these events in that order, everything works as it should: xev reports > > > >>> the following: > > > >>> > > > >>> KeyPress F4 > > > >>> KeyPress F6 > > > >>> KeyRelease F4 > > > >>> KeyRelease F6 > > > >>> > > > >>>> The reason is that the driver only considers you have one key pressed > > > >>>> with the modifier, and as the code changed its state based on the last > > > >>>> value. > > > >>>> > > > >>> > > > >>> I believe the bitmap takes care of storing the FN state per key press. The > > > >>> trick I did was to check on the global `asc->fn_on` state only when a key > > > >>> is pressed, but check on the bitmap instead when it's released. > > > >>> > > > >>> Let me know what you think. Am I missing something here? > > > >>> > > > >>> Cheers, > > > >>> João. > > > >>> > > > >>>> IMO a better fix would: > > > >>>> > > > >>>> - keep the existing `trans` mapping lookout > > > >>>> - whenever a `trans` mapping gets found: > > > >>>> * get both translated and non-translated currently reported values > > > >>>> (`test_bit(keycode, input_dev->key)`) > > > >>>> * if one of them is set to true, then consider the keycode to be the > > > >>>> one of the key (no matter fn_on) > > > >>>> -> deal with `value` with the corrected keycode > > > >>>> * if the key was not pressed: > > > >>>> -> chose the keycode based on `fn_on` and `fnmode` states > > > >>>> and report the key press event > > > >>>> > > > >>>> This should remove the nasty pressed_fn state which depends on the > > > >>>> other pressed keys. > > > >>>> > > > >>>> Cheers, > > > >>>> Benjamin > > > >>>> > > > >>>>>> + > > > >>>>>> + if (trans->flags & APPLE_FLAG_FKEY) > > > >>>>>> + do_translate = (fnmode == 2 && fn_on) || > > > >>>>>> + (fnmode == 1 && !fn_on); > > > >>>>>> else > > > >>>>>> do_translate = asc->fn_on; > > > >>>>>> > > > >>>>>> if (do_translate) { > > > >>>>>> - if (value) > > > >>>>>> - set_bit(usage->code, asc->pressed_fn); > > > >>>>>> - else > > > >>>>>> - clear_bit(usage->code, asc->pressed_fn); > > > >>>>>> - > > > >>>>>> input_event(input, usage->type, trans->to, > > > >>>>>> value); > > > >>>>>> > > > >>>>>> -- > > > >>>>>> 2.19.1 > > > >>>>>> > > > >> > > > >> Gave this another look and I still haven't found any issues, let me > > > >> know if you still > > > >> think I'm missing something. Thanks! > > > >> > > > > > > > > OK, I am tempted to take the patch, but I'll need to add this as a > > > > regression test in my test-suite. This is too complex that it can > > > > easily break next time someone looks at it. > > > > > > > > Can you send me the report descriptors of the device using > > > > hid-recorder from hid-tools[0]. > > > > Ideally, if you can put the faulty sequence in the logs, that would > > > > help writing down the use case. > > > > > > > > > > I finally managed to get the regression tests up in > > > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/52 > > > > > > > This is great, really cool that you can write proper tests for this. I > > assume you don't need me to send you the report descriptors any more? > > It would be nice to have yours too. Just in case they differ from the > one I have here. > You got it. Here's the output of the F5 faulty sequence: # Apple, Inc Apple Keyboard # 0x05, 0x01, // Usage Page (Generic Desktop) 0 # 0x09, 0x06, // Usage (Keyboard) 2 # 0xa1, 0x01, // Collection (Application) 4 # 0x05, 0x07, // Usage Page (Keyboard) 6 # 0x19, 0xe0, // Usage Minimum (224) 8 # 0x29, 0xe7, // Usage Maximum (231) 10 # 0x15, 0x00, // Logical Minimum (0) 12 # 0x25, 0x01, // Logical Maximum (1) 14 # 0x75, 0x01, // Report Size (1) 16 # 0x95, 0x08, // Report Count (8) 18 # 0x81, 0x02, // Input (Data,Var,Abs) 20 # 0x95, 0x01, // Report Count (1) 22 # 0x75, 0x08, // Report Size (8) 24 # 0x81, 0x01, // Input (Cnst,Arr,Abs) 26 # 0x05, 0x08, // Usage Page (LEDs) 28 # 0x19, 0x01, // Usage Minimum (1) 30 # 0x29, 0x05, // Usage Maximum (5) 32 # 0x95, 0x05, // Report Count (5) 34 # 0x75, 0x01, // Report Size (1) 36 # 0x91, 0x02, // Output (Data,Var,Abs) 38 # 0x95, 0x01, // Report Count (1) 40 # 0x75, 0x03, // Report Size (3) 42 # 0x91, 0x01, // Output (Cnst,Arr,Abs) 44 # 0x05, 0x07, // Usage Page (Keyboard) 46 # 0x19, 0x00, // Usage Minimum (0) 48 # 0x2a, 0xff, 0x00, // Usage Maximum (255) 50 # 0x95, 0x05, // Report Count (5) 53 # 0x75, 0x08, // Report Size (8) 55 # 0x15, 0x00, // Logical Minimum (0) 57 # 0x26, 0xff, 0x00, // Logical Maximum (255) 59 # 0x81, 0x00, // Input (Data,Arr,Abs) 62 # 0x05, 0xff, // Usage Page (Vendor Usage Page 0xff) 64 # 0x09, 0x03, // Usage (Vendor Usage 0x03) 66 # 0x75, 0x08, // Report Size (8) 68 # 0x95, 0x01, // Report Count (1) 70 # 0x81, 0x02, // Input (Data,Var,Abs) 72 # 0xc0, // End Collection 74 # R: 75 05 01 09 06 a1 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 95 01 75 08 81 01 05 08 19 01 29 05 95 05 75 01 91 02 95 01 75 03 91 01 05 07 19 00 2a ff 00 95 05 75 08 15 00 26 ff 00 81 00 05 ff 09 03 75 08 95 01 81 02 c0 N: Apple, Inc Apple Keyboard I: 3 05ac 0221 # LeftControl: 0 | LeftShift: 0 | LeftAlt: 0 | Left GUI: 0 | RightControl: 0 | RightShift: 0 | RightAlt: 0 | Right GUI: 0 | # |Keyboard ['00', '00', '00', '00', '00'] | 0xff0003: 1 E: 000000.000000 8 00 00 00 00 00 00 00 01 # LeftControl: 0 | LeftShift: 0 | LeftAlt: 0 | Left GUI: 0 | RightControl: 0 | RightShift: 0 | RightAlt: 0 | Right GUI: 0 | # |Keyboard ['F5', '00', '00', '00', '00'] | 0xff0003: 1 E: 000000.216000 8 00 00 3e 00 00 00 00 01 # LeftControl: 0 | LeftShift: 0 | LeftAlt: 0 | Left GUI: 0 | RightControl: 0 | RightShift: 0 | RightAlt: 0 | Right GUI: 0 | # |Keyboard ['F5', '00', '00', '00', '00'] | 0xff0003: 0 E: 000000.391958 8 00 00 3e 00 00 00 00 00 # LeftControl: 0 | LeftShift: 0 | LeftAlt: 0 | Left GUI: 0 | RightControl: 0 | RightShift: 0 | RightAlt: 0 | Right GUI: 0 | # |Keyboard ['00', '00', '00', '00', '00'] | 0xff0003: 0 E: 000000.591974 8 00 00 00 00 00 00 00 00 # LeftControl: 1 | LeftShift: 0 | LeftAlt: 0 | Left GUI: 0 | RightControl: 0 | RightShift: 0 | RightAlt: 0 | Right GUI: 0 | # |Keyboard ['00', '00', '00', '00', '00'] | 0xff0003: 0 E: 000001.255952 8 01 00 00 00 00 00 00 00 > > > > > This allowed me to better understand the code, and yes, for the F-keys, > > > I could not find a faulty situation with your patch. > > > > > > However, the same problem is happening with the arrow keys, as arrow up > > > is translated to page up. > > > > > > > Great catch, I can easily repro that issue as well on my keyboard. > > > > > We *could* adapt your patch to solve this, but I find it really hard to understand. > > > > > > How about the following diff: > > > --- > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c > > > index 81df62f48c4c..ef916c0f3c0b 100644 > > > --- a/drivers/hid/hid-apple.c > > > +++ b/drivers/hid/hid-apple.c > > > @@ -54,7 +54,6 @@ MODULE_PARM_DESC(swap_opt_cmd, "Swap the Option (\"Alt\") and Command (\"Flag\") > > > struct apple_sc { > > > unsigned long quirks; > > > unsigned int fn_on; > > > - DECLARE_BITMAP(pressed_fn, KEY_CNT); > > > DECLARE_BITMAP(pressed_numlock, KEY_CNT); > > > }; > > > > > > @@ -181,6 +180,8 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > { > > > struct apple_sc *asc = hid_get_drvdata(hid); > > > const struct apple_key_translation *trans, *table; > > > + bool do_translate; > > > + u16 code = 0; > > > > > > if (usage->code == KEY_FN) { > > > asc->fn_on = !!value; > > > @@ -189,8 +190,6 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > } > > > > > > if (fnmode) { > > > - int do_translate; > > > - > > > if (hid->product >= USB_DEVICE_ID_APPLE_WELLSPRING4_ANSI && > > > hid->product <= USB_DEVICE_ID_APPLE_WELLSPRING4A_JIS) > > > table = macbookair_fn_keys; > > > @@ -202,25 +201,33 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, > > > trans = apple_find_translation (table, usage->code); > > > > > > if (trans) { > > > - if (test_bit(usage->code, asc->pressed_fn)) > > > - do_translate = 1; > > > - else if (trans->flags & APPLE_FLAG_FKEY) > > > - do_translate = (fnmode == 2 && asc->fn_on) || > > > - (fnmode == 1 && !asc->fn_on); > > > - else > > > - do_translate = asc->fn_on; > > > - > > > - if (do_translate) { > > > - if (value) > > > - set_bit(usage->code, asc->pressed_fn); > > > - else > > > - clear_bit(usage->code, asc->pressed_fn); > > > - > > > - input_event(input, usage->type, trans->to, > > > - value); > > > - > > > - return 1; > > > + if (test_bit(trans->from, input->key)) > > > + code = trans->from; > > > + if (test_bit(trans->to, input->key)) > > > + code = trans->to; > > > > I see, this is what you meant before. I still don't quite understand > > what information `input->key` actually holds. Can you elaborate on > > what it contains? How does it differ from `usage->code`? > > input is the input device itself (from the kernel point of view) and > key is the bitmask of the currently reported pressed keys. > usage->code is the HID usage (so from the hardware point of view) and > identifies which physical key is being pressed. > And last, asc->pressed_fn contained the bitmask of which physical key > was pressed, while here we store which reported key is sent. > Oh, I see. So `input->key` contains whatever value we called `input_event` with previously? Makes sense! > > > > Also, should the second `if` be an `else if` instead? We shouldn't > > have to call `test_bit` more than once here, right? If right, then the > > next `if` would could simply be an `else`, since the translation > > tables should not have zeros in them. > > right we can do that. > > > > > > + > > > + if (!code) { > > > + if (trans->flags & APPLE_FLAG_FKEY) { > > > + switch (fnmode) { > > > + case 1: > > > + do_translate = !asc->fn_on; > > > + break; > > > + case 2: > > > + do_translate = asc->fn_on; > > > + break; > > > + default: > > > + /* should never happen */ > > > + do_translate = false; > > > + } > > > + } else { > > > + do_translate = asc->fn_on; > > > + } > > > + > > > + code = do_translate ? trans->to : trans->from; > > > } > > > + > > > + input_event(input, usage->type, code, value); > > > + return 1; > > > } > > > > > > if (asc->quirks & APPLE_NUMLOCK_EMULATION && > > > --- > > > > > > This is more or less what I suggested, minus the bug fixes. > > > > > > I find this easier to understand as the .pressed_fn field is not there > > > anymore and we just rely on the actual state of the input subsystem. > > > > > > Comments? > > > > Love it. Seems to catch all the cases on my hardware. Thanks for > > following up with the patch! > > Thanks I'll properly resend the patch. I guess I should keep your from > and you can later add your signed-off-by line if you agree. (but > likely not today, I have to pick up my girls at school, things you'll > discover in a few years, or months for daycare :-P). > Sure, that works for me. Hehe, no spoilers! > Cheers, > Benjamin > > > > > > > > > Cheers, > > > Benjamin > > > > > > > > > > Cheers, > > > > Benjamin > > > > > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > > > > > > Cheers, > > João Cheers, João
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c index 1cb41992aaa1..81867a6fa047 100644 --- a/drivers/hid/hid-apple.c +++ b/drivers/hid/hid-apple.c @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input, trans = apple_find_translation (table, usage->code); if (trans) { - if (test_bit(usage->code, asc->pressed_fn)) - do_translate = 1; - else if (trans->flags & APPLE_FLAG_FKEY) - do_translate = (fnmode == 2 && asc->fn_on) || - (fnmode == 1 && !asc->fn_on); + int fn_on = value ? asc->fn_on : + test_bit(usage->code, asc->pressed_fn); + + if (!value) + clear_bit(usage->code, asc->pressed_fn); + else if (asc->fn_on) + set_bit(usage->code, asc->pressed_fn); + + if (trans->flags & APPLE_FLAG_FKEY) + do_translate = (fnmode == 2 && fn_on) || + (fnmode == 1 && !fn_on); else do_translate = asc->fn_on; if (do_translate) { - if (value) - set_bit(usage->code, asc->pressed_fn); - else - clear_bit(usage->code, asc->pressed_fn); - input_event(input, usage->type, trans->to, value);
This fixes an issue in which key down events for function keys would be repeatedly emitted even after the user has raised the physical key. For example, the driver fails to emit the F5 key up event when going through the following steps: - fnmode=1: hold FN, hold F5, release FN, release F5 - fnmode=2: hold F5, hold FN, release F5, release FN The repeated F5 key down events can be easily verified using xev. Signed-off-by: Joao Moreno <mail@joaomoreno.com> --- drivers/hid/hid-apple.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)