a) All DInput effect parameters to need to be translated to linux effect parameters, are either DWORD, or LONG. That means that their maximum absolute values are both equal to 2147483648 (both are 4-byte signed two-complement). All these parameters are limited to a maximum absolute value of DI_FFNOMINALMAX (=10000), except for the time parameters such as period, duration, startdelay, ... and for the elements of "rglDirection" (LONG array). All linux effect parameters do never exceed a size of 16 bits: __s16 and __u16. That means that their maximum absolute values are respectively 32768 and 65535. This means that:
- If the parameters with DI_FFNOMINALMAX limits exceed this limit, we should *consistently* either: - Emit "DIERR_INVALIDPARAM" - Clamp to the correct range => The latter is preferred, and already applied in the ConstantForce case, but overall not consistently: e.g. it is not done in the Periodic case, ...
So, change (e.g. for ramp levels) on dinput/effect_linuxinput.c:500 : This->effect.u.ramp.start_level = (tsp->lStart / 10) * 32; This->effect.u.ramp.end_level = (tsp->lEnd / 10) * 32; to : This->effect.u.ramp.start_level = (max(min(tsp->lStart, 10000), -10000) / 10) * 32; This->effect.u.ramp.end_level = (max(min(tsp->lEnd, 10000), -10000) / 10) * 32; And the same applies to many other lines of code, but I don't write them yet if I don't know whether this proposal will be accepted.
- If the parameters are time-related, plain conversion from dinput to linux could cause parameters to wrap around (even when first dividing by 1000 (to go from millis->micros)), because all time values in linux are maximum 0x7FFF (/include/uapi/linux/input.h:966). We can not emit "DIERR_INVALIDPARAM" when these parameters exceed linux' parameters' range, because it is not forbidden by MSDN. => So we should clamp them before conversion.
So, change dinput/effect_linuxinput.c:424 : if (dwFlags & DIEP_DURATION) { if (peff->dwDuration == -1) This->effect.replay.length = 0; // Infinity else if (peff->dwDuration > 1000) This->effect.replay.length = peff->dwDuration / 1000; else This->effect.replay.length = 1; // Smallest non-zero value } to : if (dwFlags & DIEP_DURATION) { if (peff->dwDuration == -1) This->effect.replay.length = 0; // Infinity else if (peff->dwDuration > 1000) This->effect.replay.length = min(peff->dwDuration / 1000, 0x7FFF); else This->effect.replay.length = 1; // Smallest non-zero value } And the same applies to many other lines of code, but I don't write them yet if I don't know whether this proposal will be accepted.
- If the parameters are direction-related, we should also be careful for parameters to not wrap around, but I did not find such a problem in effect_linuxinput.c
b) Provide a more accurate mapping of force levels from dinput to linux, (the "from linux to dinput" case is less important): Ensure 10000 (dinput) maps exactly to 32767 (linux), this will prevent glitches when envelopes are applied on top of an already maxed out forces. => Variations from 32000 to 32767 can occur in the attack or fade region. These may be small, but when the frequency of effects started is high, this may be significantly perceivable.
Change for all force conversions from dinput to linux (e.g. for constant.level) on dinput/effect_linuxinput.c:494 : This->effect.u.constant.level = (max(min(tsp->lMagnitude, 10000), -10000) / 10) * 32; to : This->effect.u.constant.level = (max(min(tsp->lMagnitude, 10000), -10000) * 0x7FFF) / 10000; And the same applies to many other lines of code, but I don't write them yet if I don't know whether this proposal will be accepted.
c) Optimize code to eliminate (almost) all floating point operations when translating effect parameters from dinput to linux. (the "from linux to dinput" case is less important) If not possible, eliminate it for the common cases.
We will use the uint16_t type, so we need to include stdint.h, change on dinput/effect_linuxinput.c:26 : #include <stdarg.h> #include <string.h> to : #include <stdarg.h> #include <stdint.h> #include <string.h>
Change on dinput/effect_linuxinput.c:415 : This->effect.direction = (unsigned int)((M_PI / 2 + atan2(peff->rglDirection[1], peff->rglDirection[0])) * 0x8000 / M_PI); to : if (!peff->rglDirection[1]) This->effect.direction = (peff->rglDirection[0] >= 0 ? 0x4000 : 0xC000); else if (!peff->rglDirection[0]) This->effect.direction = (peff->rglDirection[1] >= 0 ? 0x8000 : 0); else This->effect.direction = (uint16_t)((M_PI / 2 + atan2(peff->rglDirection[1], peff->rglDirection[0])) * 0x8000 / M_PI);
And change on dinput/effect_linuxinput.c:416 : } else if (peff->dwFlags & DIEFF_SPHERICAL) { This->effect.direction = (unsigned int)(((9000 + (double)peff->rglDirection[0]) / 18000) * 0x8000); } else /* if (peff->dwFlags & DIEFF_POLAR) */ { This->effect.direction = (unsigned int)(((double)peff->rglDirection[0] / 18000) * 0x8000); } to : } else if (peff->dwFlags & DIEFF_SPHERICAL) { /* Avoid floating point arithmetic in most cases, while realizing same precision * (when abs(peff->rglDirection[0]) < 0x00080000 = 524288 which is much larger than 35999), * otherwise, use floating point arithmetic */ if (-0x00080000 < peff->rglDirection[0] && peff->rglDirection[0] < 0x00080000) This->effect.direction = 0x4000 + (uint16_t)((peff->rglDirection[0] * 0x1000) / 2250); else This->effect.direction = 0x4000 + (uint16_t)(((double)peff->rglDirection[0] / 2250) * 0x1000); } else /* if (peff->dwFlags & DIEFF_POLAR) */ { /* Avoid floating point arithmetic in most cases, while realizing same precision * (when abs(peff->rglDirection[0]) < 0x00080000 = 524288 which is much larger than 35999), * otherwise, use floating point arithmetic */ if (-0x00080000 < peff->rglDirection[0] && peff->rglDirection[0] < 0x00080000) This->effect.direction = (uint16_t)((peff->rglDirection[0] * 0x1000) / 2250); else This->effect.direction = (uint16_t)(((double)peff->rglDirection[0] / 2250) * 0x1000); }
d) If previously a successful SetParameters(...) has been called, and then SetParameters(...) is called again, but now with only DIEP_AXES flag set in the 'dwFlags' parameter (and not DIEP_DIRECTION, because it was already done before): => Problem, "effect.direction" is not updated (and instead it should, because it's dependent of the axes config). I did not provide a patch here, because it would require to store extra data. This needs discussion.
e) If previously a successful SetParameters(...) has been called with the DIEP_TYPESPECIFICPARAMS flag set, and then SetParameters(...) is called again, but now with only DIEP_AXES and/or DIEP_DIRECTION flag set: => Problem if type == DIEFT_CONDITION: "effect.u.condition[i].*" are not updated (and instead they should, because they are dependent of the axes config and the direction). I did not provide a patch here, because it would require to store extra data. This needs discussion.
f) We might use 'DIEFFECT.dwGain'/10000 to multiply with all forces of the corresponding effect. In that way, we don't have to set global gain on Linux each time another DIEFFECT.dwGain value occurs. Also this can suffer from the same problem as above: if only the 'dwGain' value is updated in SetParameters(...) after the effect was previously already fully defined, the force values would also need to be updated (because they are dependent of the gain). I did not provide a patch here, because it would require to store extra data. This needs discussion.
Elias