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

[Elias Vanderstuyft]

On Wed, Mar 5, 2014 at 4:05 PM, Andrew Eikum <aeikum at 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.