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