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@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
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 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. Your change looks good. I would probably use a TRACE instead of WARN, since we're doing the right thing.
Andrew
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.
-
Andrew Eikum -
Elias Vanderstuyft