[RFC 6/11] Linux FF: Linux effect: conditional effect's parameters

[Elias Vanderstuyft]

a)  There is some ambiguity about the range of the following Linux
ff_condition_effect struct members:
        right_saturation, left_saturation and deadband
    They are all __u16, but /include/uapi/linux/input.h does not say
what the maximum values are.
    I'm doing a proposal to define and document this (in the Linux
kernel) in the following way, also take a look at "interactive.fig" in
the kernel documentation:
        Max Range of {right_saturation and left_saturation} = 0x7FFF
            Because the maximal value of the saturation bound can be
only half of the total range covered by the max negative to max
positive bounds.
            And also because they are related to some form of force,
and all other forms of force in linux have a maximum value of 0x7FFF
        Max Range of {deadband} = 0xFFFF
            This is a bit harder to explain:
            - First, I would suggest to alter the deadband definition
in figure "interactive.fig":
                I would define deadband as going from a deadband-bound
to the center (=> This is how MSDN defines it: "In other words, the
condition is not active between lOffset minus lDeadBand and lOffset
plus lDeadBand."),
                instead of from a deadband-bound to the other deadband-bound.
                    => No changes needed in Wine.
            - Now, knowing that ff_condition_effect->center is __s16:
                The worst case scenario is that "center = -32768" or
"center = +32767" while still wanting to cover the whole region (this
is actually not possible with DInput's specs: in that case, they can
only cover half of the total region):
                Then, to keep the scale of "center" and "deadband" the
same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
                This means that dinput deadband 10000 maps to 32767.5.
(And dinput deadband 20000 (not possible in dinput) would map to
65535)
                    => No changes needed in Wine (actually, there was,
but applying the changes to the documentation, errors cancel out)

    => After having discussed this (see
http://www.mail-archive.com/linux-input(a)vger.kernel.org/msg08459.html),
the following is true:
        - offset:    [-10000, 10000] (dinput)    ->    [-0x7FFF, 0x7FFF] (linux)
        - deadband:  [     0, 10000] (dinput)    ->    [      0, 0xFFFF] (linux)
        - p/n-coeff: [-10000, 10000] (dinput)    ->    [-0x7FFF, 0x7FFF] (linux)
        - p/n-sat:   [     0, 10000] (dinput)    ->    [      0, 0xFFFF] (linux)

    So change dinput/effect_linuxinput.c:271 :
        for (i = 0; i < 2; ++i) {
            tsp[i].lOffset = (This->effect.u.condition[i].center / 33) * 10;
            tsp[i].lPositiveCoefficient =
(This->effect.u.condition[i].right_coeff / 33) * 10;
            tsp[i].lNegativeCoefficient =
(This->effect.u.condition[i].left_coeff / 33) * 10;
            tsp[i].dwPositiveSaturation =
(This->effect.u.condition[i].right_saturation / 33) * 10;
            tsp[i].dwNegativeSaturation =
(This->effect.u.condition[i].left_saturation / 33) * 10;
            tsp[i].lDeadBand = (This->effect.u.condition[i].deadband / 33) * 10;
        }
    to :
        for (i = 0; i < 2; ++i) {
            tsp[i].lOffset = (This->effect.u.condition[i].center / 33) * 10;
            tsp[i].lPositiveCoefficient =
(This->effect.u.condition[i].right_coeff / 33) * 10;
            tsp[i].lNegativeCoefficient =
(This->effect.u.condition[i].left_coeff / 33) * 10;
            tsp[i].dwPositiveSaturation =
(This->effect.u.condition[i].right_saturation / 66) * 10;
            tsp[i].dwNegativeSaturation =
(This->effect.u.condition[i].left_saturation / 66) * 10;
            tsp[i].lDeadBand = (This->effect.u.condition[i].deadband / 66) * 10;
        }

    And change dinput/effect_linuxinput.c:511 :
        for (i = 0; i < 2; ++i) {
            This->effect.u.condition[i].center = (int)(factor[i] *
(tsp->lOffset / 10) * 32);
            This->effect.u.condition[i].right_coeff = (int)(factor[i]
* (tsp->lPositiveCoefficient / 10) * 32);
            This->effect.u.condition[i].left_coeff = (int)(factor[i] *
(tsp->lNegativeCoefficient / 10) * 32);
            This->effect.u.condition[i].right_saturation =
(int)(factor[i] * (tsp->dwPositiveSaturation / 10) * 32);
            This->effect.u.condition[i].left_saturation =
(int)(factor[i] * (tsp->dwNegativeSaturation / 10) * 32);
            This->effect.u.condition[i].deadband = (int)(factor[i] *
(tsp->lDeadBand / 10) * 32);
        }
    to :
        for (i = 0; i < 2; ++i) {
            This->effect.u.condition[i].center = (int)(factor[i] *
(tsp->lOffset / 10) * 32);
            This->effect.u.condition[i].right_coeff = (int)(factor[i]
* (tsp->lPositiveCoefficient / 10) * 32);
            This->effect.u.condition[i].left_coeff = (int)(factor[i] *
(tsp->lNegativeCoefficient / 10) * 32);
            This->effect.u.condition[i].right_saturation =
(int)(factor[i] * (tsp->dwPositiveSaturation / 10) * 65);
            This->effect.u.condition[i].left_saturation =
(int)(factor[i] * (tsp->dwNegativeSaturation / 10) * 65);
            This->effect.u.condition[i].deadband = (int)(factor[i] *
(tsp->lDeadBand / 10) * 65);
        }

    and change dinput/effect_linuxinput.c:522 :
        for (i = 0; i < 2; ++i) {
            This->effect.u.condition[i].center = (tsp[i].lOffset / 10) * 32;
            This->effect.u.condition[i].right_coeff =
(tsp[i].lPositiveCoefficient / 10) * 32;
            This->effect.u.condition[i].left_coeff =
(tsp[i].lNegativeCoefficient / 10) * 32;
            This->effect.u.condition[i].right_saturation =
(tsp[i].dwPositiveSaturation / 10) * 32;
            This->effect.u.condition[i].left_saturation =
(tsp[i].dwNegativeSaturation / 10) * 32;
            This->effect.u.condition[i].deadband = (tsp[i].lDeadBand / 10) * 32;
        }
    to :
        for (i = 0; i < 2; ++i) {
            This->effect.u.condition[i].center = (tsp[i].lOffset / 10) * 32;
            This->effect.u.condition[i].right_coeff =
(tsp[i].lPositiveCoefficient / 10) * 32;
            This->effect.u.condition[i].left_coeff =
(tsp[i].lNegativeCoefficient / 10) * 32;
            This->effect.u.condition[i].right_saturation =
(tsp[i].dwPositiveSaturation / 10) * 65;
            This->effect.u.condition[i].left_saturation =
(tsp[i].dwNegativeSaturation / 10) * 65;
            This->effect.u.condition[i].deadband = (tsp[i].lDeadBand / 10) * 65;
        }


b)  Spring (or other conditional effects) does not work (aborted in
SetParameters while setting a non-zero envelope) in FEdit:
        This needs discussion: MSDN says: "Typically, you can apply an
envelope to a constant force, a ramp force, or a periodic effect, but
not to a condition."
        Linux's ff_condition_effect struct doesn't contain an
ff_envelope struct,
        so it seems to be logically to say "if (!env) return
DIERR_INVALIDPARAM;" (dinput/effect_linuxinput.c:444 SetParameters),
        but when taking a look at the envelope (that FEdit defines for
such effect), it seems to be 'neutral':
        "Envelope has attack (level: 10000 time: 0), fade (level:
10000 time: 0)"
    So what should we do?
        - Leave it as is, but this will disable effects on hardware
that is capable of applying them (without envelope).
        - Only return an error if the envelope is not 'neutral', but
then we have to provide a rigorous definition of 'neutral'.
        - Don't return an error. This seems to be the behavior of the
Windows Logitech FFB driver (MOMO, MOMO2, ...).

    Choosing the latter:
    change dinput/effect_linuxinput.c:444 :
        if (!env) return DIERR_INVALIDPARAM;
        /* copy the envelope */
        env->attack_length = peff->lpEnvelope->dwAttackTime / 1000;
        env->attack_level = (peff->lpEnvelope->dwAttackLevel / 10) * 32;
        env->fade_length = peff->lpEnvelope->dwFadeTime / 1000;
        env->fade_level = (peff->lpEnvelope->dwFadeLevel / 10) * 32;
    to :
        if (!env) WARN("We get passed an envelope for a type (0x%x)
that doesn't even have one.\n", This->effect.type);
        else {
            /* copy the envelope */
            env->attack_length = peff->lpEnvelope->dwAttackTime / 1000;
            env->attack_level = (peff->lpEnvelope->dwAttackLevel / 10) * 32;
            env->fade_length = peff->lpEnvelope->dwFadeTime / 1000;
            env->fade_level = (peff->lpEnvelope->dwFadeLevel / 10) * 32;
        }


Elias

[Andrew Eikum]

On Tue, Mar 04, 2014 at 08:20:13PM +0100, Elias Vanderstuyft wrote:
> for (i = 0; i < 2; ++i) {
>             This->effect.u.condition[i].center = (tsp[i].lOffset / 10) * 32;
>             This->effect.u.condition[i].right_coeff =
> (tsp[i].lPositiveCoefficient / 10) * 32;
>             This->effect.u.condition[i].left_coeff =
> (tsp[i].lNegativeCoefficient / 10) * 32;
>             This->effect.u.condition[i].right_saturation =
> (tsp[i].dwPositiveSaturation / 10) * 65;
>             This->effect.u.condition[i].left_saturation =
> (tsp[i].dwNegativeSaturation / 10) * 65;
>             This->effect.u.condition[i].deadband = (tsp[i].lDeadBand / 10) * 65;
>         }

I agree with this in principle, but I think I would change the
implementation to be a bit more explicit. Something like,
    left_coeff = (lNegativeCoefficient / (double)10000) * 32767;
    right_saturation = (dwPositiveSaturation / (double)10000) * 65535;
which shows an explicit mapping between full ranges. (This might need
some more thinking at the extreme edges of the bounds, so we don't go
out of bounds during type coersion.)
> - Don't return an error. This seems to be the behavior of the
> Windows Logitech FFB driver (MOMO, MOMO2, ...).

We should match Windows. Sounds like they ignore envelopes for effects
which don't support them, so we should too. Your change looks good. I
would probably use a TRACE instead of WARN, since we're doing the
right thing.

Andrew

[Elias Vanderstuyft]

On Wed, Mar 5, 2014 at 4:05 PM, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> On Tue, Mar 04, 2014 at 08:20:13PM +0100, Elias Vanderstuyft wrote:
> > for (i = 0; i < 2; ++i) {
> >             This->effect.u.condition[i].center = (tsp[i].lOffset / 10) * 32;
> >             This->effect.u.condition[i].right_coeff =
> > (tsp[i].lPositiveCoefficient / 10) * 32;
> >             This->effect.u.condition[i].left_coeff =
> > (tsp[i].lNegativeCoefficient / 10) * 32;
> >             This->effect.u.condition[i].right_saturation =
> > (tsp[i].dwPositiveSaturation / 10) * 65;
> >             This->effect.u.condition[i].left_saturation =
> > (tsp[i].dwNegativeSaturation / 10) * 65;
> >             This->effect.u.condition[i].deadband = (tsp[i].lDeadBand / 10) * 65;
> >         }
>
> I agree with this in principle, but I think I would change the
> implementation to be a bit more explicit. Something like,
>     left_coeff = (lNegativeCoefficient / (double)10000) * 32767;
>     right_saturation = (dwPositiveSaturation / (double)10000) * 65535;
> which shows an explicit mapping between full ranges. (This might need
> some more thinking at the extreme edges of the bounds, so we don't go
> out of bounds during type coersion.)

That's a good idea, in one of my further RFC mails, I say: "Provide a
more accurate mapping of force levels from dinput to linux", and this
should actually also apply to the conditional effect parameters.
I would change your code a little bit, like this (also to avoid
floating point operations (and maintaining precision) and thus avoid
floating point context switching, ~maintaining performance):
    left_coeff = (max(min(lNegativeCoefficient, 0), 10000) * 0x7FFF) / 10000;
    right_saturation = (max(min(dwPositiveSaturation, 0), 10000) *
0xFFFF) / 10000;
No overflow will happen, because the type of the return-value of max()
will be LONG (= 32bit signed), so 10000*0xFFFF < 2^31-1
> > - Don't return an error. This seems to be the behavior of the
> > Windows Logitech FFB driver (MOMO, MOMO2, ...).
>
> We should match Windows. Sounds like they ignore envelopes for effects
> which don't support them, so we should too.

It's also possible that drivers other than Logitech *do* return an
error, but I think they won't.
> Your change looks good. I
> would probably use a TRACE instead of WARN, since we're doing the
> right thing.

OK.