| Message ID | 20250119131356.1006582-3-tomasz.pakula.oficjalny@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | HID: Upgrade the generic pidff driver and add hid-universal-pidff | expand |
On 19.01.25 14:12, Tomasz Pakuła wrote: > Envelope struct is always initialized, but the envelope itself is > optional as described in USB PID Device class definition 1.0. > > 5.1.1.1 Type Specific Block Offsets > ... > 4) Effects that do not use Condition Blocks use 1 Parameter Block and > an *optional* Envelope Block. > > Sending out "empty" envelope breaks force feedback on some devices with > games that use SINE effect + offset to emulate constant force effect, as > well as generally breaking Constant/Periodic effects. One of the affected > brands is Moza Racing. [..] > @@ -261,10 +261,19 @@ static void pidff_set_envelope_report(struct pidff_device *pidff, > static int pidff_needs_set_envelope(struct ff_envelope *envelope, > struct ff_envelope *old) > { > - return envelope->attack_level != old->attack_level || > - envelope->fade_level != old->fade_level || > + bool needs_new_envelope; > + needs_new_envelope = envelope->attack_level != 0 || > + envelope->fade_level != 0 || > + envelope->attack_length != 0 || > + envelope->fade_length != 0; > + > + if (!needs_new_envelope || !old) > + return needs_new_envelope; I am afraid this is the most convoluted piece of boolean algebra I've seen in a long time. In particular because it mixes things that do not belong together. Regards Oliver
On Tue, 21 Jan 2025 at 10:59, Oliver Neukum <oneukum@suse.com> wrote: > I am afraid this is the most convoluted piece of boolean algebra I've seen > in a long time. In particular because it mixes things that do not belong together. Could you elaborate on that? What here does not belong? I think the diff is a bit unfortunate and doesn't make it justice, but this is based on code that was already there. Instead of just checking if the new values differ from old values, we first check if any values are different from 0. If neither are != 0 OR the effect didn't contain an envelope previously (NULL here), we return the value of the check. Only if envelope isn't empty and if the effect already had an envelope, we compare the new and old values to decide if we need to send an envelope to the device. if (!needs_new_envelope || !old) return needs_new_envelope; Could be represented as: if (!needs_new_envelope) // empty envelope return 0; if (!old) // effect doesn't have an envelope yet return needs_new_envelope; The last part works exactly as it did before. Just to make sure we're on the same page here, here's the resulting code: static int pidff_needs_set_envelope(struct ff_envelope *envelope, struct ff_envelope *old) { bool needs_new_envelope; needs_new_envelope = envelope->attack_level != 0 || envelope->fade_level != 0 || envelope->attack_length != 0 || envelope->fade_length != 0; if (!needs_new_envelope || !old) return needs_new_envelope; return envelope->attack_level != old->attack_level || envelope->fade_level != old->fade_level || envelope->attack_length != old->attack_length || envelope->fade_length != old->fade_length; }
On 21.01.25 11:17, Tomasz Pakuła wrote: > On Tue, 21 Jan 2025 at 10:59, Oliver Neukum <oneukum@suse.com> wrote: >> I am afraid this is the most convoluted piece of boolean algebra I've seen >> in a long time. In particular because it mixes things that do not belong together. > > Could you elaborate on that? What here does not belong? Hi, > > I think the diff is a bit unfortunate and doesn't make it justice, but > this is based on > code that was already there. Instead of just checking if the new > values differ from > old values, we first check if any values are different from 0. If > neither are != 0 OR > the effect didn't contain an envelope previously (NULL here), we > return the value > of the check. Indeed. And that is the problem. You could see the evaluation to contain either two or three main cases. First view: A - everything is 0. We return false. B - we compare the old and the new and return the comparison. With a subcase of no old old values existing. In the first view you are mixing the test for no old values of the second case with the test for no old values existing. Or you split up the second condition into two independent cases. [..] > > if (!needs_new_envelope || !old) > return needs_new_envelope; This boolean statement stems from a common result, not from a common logical reason for acting so. This is clear because if the first half is true, you are returning itself. This statement would be so much more clear as: if (!needs_new_envelope) return false; if (!old) return needs_new_envelope; Regards Oliver
On Tue, 21 Jan 2025 at 14:01, Oliver Neukum <oneukum@suse.com> wrote: > > This boolean statement stems from a common result, not from a common > logical reason for acting so. This is clear because if the first half > is true, you are returning itself. > > This statement would be so much more clear as: > > if (!needs_new_envelope) > return false; > > if (!old) > return needs_new_envelope; > Okay, thanks for the clarification! I'll simplify and include this suggestion in the next version. Tomasz
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c index 3899d72a0b02..cf8163d92ea4 100644 --- a/drivers/hid/usbhid/hid-pidff.c +++ b/drivers/hid/usbhid/hid-pidff.c @@ -261,10 +261,19 @@ static void pidff_set_envelope_report(struct pidff_device *pidff, static int pidff_needs_set_envelope(struct ff_envelope *envelope, struct ff_envelope *old) { - return envelope->attack_level != old->attack_level || - envelope->fade_level != old->fade_level || + bool needs_new_envelope; + needs_new_envelope = envelope->attack_level != 0 || + envelope->fade_level != 0 || + envelope->attack_length != 0 || + envelope->fade_length != 0; + + if (!needs_new_envelope || !old) + return needs_new_envelope; + + return envelope->attack_level != old->attack_level || + envelope->fade_level != old->fade_level || envelope->attack_length != old->attack_length || - envelope->fade_length != old->fade_length; + envelope->fade_length != old->fade_length; } /* @@ -579,11 +588,9 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect, pidff_set_effect_report(pidff, effect); if (!old || pidff_needs_set_constant(effect, old)) pidff_set_constant_force_report(pidff, effect); - if (!old || - pidff_needs_set_envelope(&effect->u.constant.envelope, - &old->u.constant.envelope)) - pidff_set_envelope_report(pidff, - &effect->u.constant.envelope); + if (pidff_needs_set_envelope(&effect->u.constant.envelope, + &old->u.constant.envelope)) + pidff_set_envelope_report(pidff, &effect->u.constant.envelope); break; case FF_PERIODIC: @@ -618,11 +625,9 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect, pidff_set_effect_report(pidff, effect); if (!old || pidff_needs_set_periodic(effect, old)) pidff_set_periodic_report(pidff, effect); - if (!old || - pidff_needs_set_envelope(&effect->u.periodic.envelope, - &old->u.periodic.envelope)) - pidff_set_envelope_report(pidff, - &effect->u.periodic.envelope); + if (pidff_needs_set_envelope(&effect->u.periodic.envelope, + &old->u.periodic.envelope)) + pidff_set_envelope_report(pidff, &effect->u.periodic.envelope); break; case FF_RAMP: @@ -636,11 +641,9 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect, pidff_set_effect_report(pidff, effect); if (!old || pidff_needs_set_ramp(effect, old)) pidff_set_ramp_force_report(pidff, effect); - if (!old || - pidff_needs_set_envelope(&effect->u.ramp.envelope, - &old->u.ramp.envelope)) - pidff_set_envelope_report(pidff, - &effect->u.ramp.envelope); + if (pidff_needs_set_envelope(&effect->u.ramp.envelope, + &old->u.ramp.envelope)) + pidff_set_envelope_report(pidff, &effect->u.ramp.envelope); break; case FF_SPRING:
Envelope struct is always initialized, but the envelope itself is optional as described in USB PID Device class definition 1.0. 5.1.1.1 Type Specific Block Offsets ... 4) Effects that do not use Condition Blocks use 1 Parameter Block and an *optional* Envelope Block. Sending out "empty" envelope breaks force feedback on some devices with games that use SINE effect + offset to emulate constant force effect, as well as generally breaking Constant/Periodic effects. One of the affected brands is Moza Racing. This change prevents the envelope from being sent if it contains all 0 values while keeping the old behavior of only sending it, if it differs from the old one. Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com> --- drivers/hid/usbhid/hid-pidff.c | 39 ++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 18 deletions(-)