Discussion and Improvement of Wine's Linux Force Feedback implementation

Elias Vanderstuyft elias.vds at gmail.com
Tue Mar 4 02:59:20 CST 2014


Hi,


In the past few weeks I've been reviewing Wine's Linux Force Feedback
implementation. I annotated questions and proposals for improvement
and bundled them in a file "Improvements.txt".
Now, I think it's time to post my results, before someone begins to
work on the dinput files I started from (untouched since
wine-1.7.11-206-g82b3813).

The reason why I don't send this to wine-patches, is because it is
more a review and discussion, than a set of actual diff-generated
patch files.

Because of the rather massive amount of text I've written, I decided
to split them up in separate mails in subjects, each containing 3
questions/proposals on average:
Subjects [11]
-------------
- Joystick control panel program [2]
- Linux effect status management [3]
- Linux effect parameter: start delay [1]
- Linux effect parameter: periodic effect's phase [1]
- Linux effect parameters: time parameters [3]
- Linux effect: conditional effect's parameters [2]
- Linux effect: axes [1]
- Linux effect direction [7]
- Linux effect parameters (global) [6]
- Linux joystick initialization [1]
- Debug message fixes/additions [6]

I'll send them with the prefix "[RFC x/11] Linux FF: <subject>".
Am I allowed to send this?


Thanks,

Elias
-------------- next part --------------
Tested using wine-1.7.11-206-g82b3813
(but none of the files of which I propose changes to be made in,
 are affected in the current (2014-03-04) wine-version 1.7.13)


Because I haven't created any (formal) patch yet (and instead first request for comments),
I tried to keep the line numbers to reference the affected files
more or less based to the version I started from, where I could.
However, the orig code of a (informal) patch that has to be applied after (and thus is dependent of) a previous patch, 
may be different from the initial code of the wine-version I started from.

After having discussed everything, I will send a proper patchset.

In this document, sometimes 'FEdit' is mentioned:
it's a MS application that allows to create all FF effect types (except CustomForce and rumble/vibration effects),
and therefore the ideal test application.
Physical behavior is compared between Windows7 (using Logitech MOMO wheel with latest drivers installed) running in Virtualbox,
and my patched Wine-version.
In the end, (using a dummy FF driver on the Linux side),
the behavior of Wine is almost (limitations caused by the dummy ff driver, such as updating an effect while playing is not implemented) identical,
if not better (in some aspects) then the Windows equivalent.

Sometimes I refer to linux-input mailing lists to prove a statement,
because the links may vanish (unlikely though),
I provide the titles corresponding with the links:
    (http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html):
        "ff-core effect id handling in case of a failed effect upload"
    (http://www.mail-archive.com/linux-input@vger.kernel.org/msg08459.html):
        "Questions about the documentation/specification of Linux ForceFeedback input.h"
    

Subjects [11]
-------------
- Joystick control panel program [2]
- Linux effect status management [3]
- Linux effect parameter: start delay [1]
- Linux effect parameter: periodic effect's phase [1]
- Linux effect parameters: time parameters [3]
- Linux effect: conditional effect's parameters [2]
- Linux effect: axes [1]
- Linux effect direction [7]
- Linux effect parameters (global) [6]
- Linux joystick initialization [1]
- Debug message fixes/additions [6]
    




/////////////////////////////    Joystick control panel program    /////////////////////////////

a)  When unloading conditional effects in joy.cpl, the program crashes:
        When unloading the conditional effect array entries,
        Wine tries to access an invalid (high) address because
        dieffect.lpvTypeSpecificParams was not set at the time the effect was created.
        So declare a valid 'standard' (maxed out) conditional effect.
        For other unknown effects:
            - The same can happen when CustomForce effects are available,
                but because Wine's FF dinput->linux translation does not support this yet,
                we simply print a FIXME to indicate a possible cause if a crash would happen.
            - The same for other (unsupported) effects (this is probably not safe?),
                but then we print a WARN.
            => This is probably not safe, but these effects won't be unloaded,
                because they will never be allowed to be created:
                I tested this by using a dummy FF device that advertised to support CustomForce,
                but because JoystickAImpl_EnumEffects() does not handle CustomForce,
                no CustomForce entry was generated in the joy.cpl application,
                and there was no crash at exit.
    
    Solution: After the code of joy.cpl/main.c:766 :
        else if (IsEqualGUID(&pdei->guid, &GUID_Sine) ||
                IsEqualGUID(&pdei->guid, &GUID_Square) ||
                IsEqualGUID(&pdei->guid, &GUID_Triangle) ||
                IsEqualGUID(&pdei->guid, &GUID_SawtoothUp) ||
                IsEqualGUID(&pdei->guid, &GUID_SawtoothDown))
        {
            DIPERIODIC pforce;

            pforce.dwMagnitude = DI_FFNOMINALMAX;
            pforce.lOffset = 0;
            pforce.dwPhase = 0;
            pforce.dwPeriod = FF_PERIOD_TIME;

            dieffect.cbTypeSpecificParams = sizeof(pforce);
            dieffect.lpvTypeSpecificParams = &pforce;
            dieffect.dwFlags |= DIEP_TYPESPECIFICPARAMS;
        }
    add the following code :
        else if (IsEqualGUID(&pdei->guid, &GUID_Spring) ||
                IsEqualGUID(&pdei->guid, &GUID_Damper) ||
                IsEqualGUID(&pdei->guid, &GUID_Inertia) ||
                IsEqualGUID(&pdei->guid, &GUID_Friction))
        {
            DICONDITION cxyforce[2];

            int i;
            for (i = 0; i < 2; ++i) {
                cxyforce[i].lOffset = 0;
                cxyforce[i].lPositiveCoefficient = DI_FFNOMINALMAX;
                cxyforce[i].lNegativeCoefficient = DI_FFNOMINALMAX;
                cxyforce[i].dwPositiveSaturation = DI_FFNOMINALMAX;
                cxyforce[i].dwNegativeSaturation = DI_FFNOMINALMAX;
                cxyforce[i].lDeadBand = 0;
            }

            dieffect.cbTypeSpecificParams = sizeof(cxyforce);
            dieffect.lpvTypeSpecificParams = cxyforce;
            dieffect.dwFlags |= DIEP_TYPESPECIFICPARAMS;
        }
        else if (IsEqualGUID(&pdei->guid, &GUID_CustomForce))
        {
            FIXME("Unhandled type specific parameters of CustomForce effect, this may lead to memory errors later on.\n");
        }
        else
            WARN("Possibly unhandled type specific parameters of effect with guid %x.\n", pdei->guid);


b)  Weirdly, I had to add these commands that evaluates the DIEFFECT pointers at the end of each if-case,
    otherwise the pointers seemed to point to other address or data on that address was getting overwritten:
        TRACE("&rforce=0x%x\n", &rforce);
        TRACE("&cforce=0x%x\n", &cforce);
        TRACE("&pforce=0x%x\n", &pforce);
        TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n", &(cxyforce[0]), &(cxyforce[1]));





/////////////////////////////    Linux effect status management    /////////////////////////////

a)  This is a bug that is either the responsibility of the Linux kernel, or of Wine:
        It is caused by the following situation:
            When uploading an effect, the specific kernel device driver may return an error,
            e.g. EINVAL for example when a periodic's effect period is set to zero.
            This error will then be returned by "ioctl(*(This->fd), EVIOCSFF, &This->effect)".
            With or without error, one can find out that /drivers/input/ff-core.c:input_ff_upload(...) is called,
            which will set effect->id >= 0, if it was set to -1 (=> new effect created) by the user.
            
            But in case of an error:
            - Assume effect->id was set to -1 by the user:
                The error is reported by ff->upload(...) at /drivers/input/ff-core.c:167,
                the effect->id will also be set >= 0 (*).
                The offending effect will not be saved in the ff->effects[] array (***).
            - Assume effect->id was set >= 0 by the user (and ff->effects[effect->id] is a valid existing effect):
                The error is reported by ff->upload(...) at /drivers/input/ff-core.c:167,
                the effect->id will remain unchanged (**).
                The offending effect will not overwrite the ff->effects[effect->id] element (****).
            
            Is this (see *, **, *** and ****) desired behavior?
            - If yes:
                Change the following in Wine's dinput/effect_linuxinput.c:84 :
                    LinuxInputEffectImpl *This = impl_from_IDirectInputEffect(iface);

                    TRACE("(this=%p)\n", This);

                    if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
                to :
                    LinuxInputEffectImpl *This = impl_from_IDirectInputEffect(iface);
                    int effectId_old;

                    TRACE("(this=%p)\n", This);
                    
                    effectId_old = This->effect.id;
                    if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
                        This->effect.id = effectId_old;
            - If no for *:
                Kernel code /drivers/input/ff-core.c:input_ff_upload(...) has to be patched to revert "effect->id" back to its original value set by the user,
                which is only needed when the initial (by user) value of "effect->id" was equal to -1.
            - If no for **** (or maybe also ***):
                ff->effects[effect->id] could be replaced by an 'empty' effect (however this can get complex because the effect's type has to remain unchanged)
                This would be a change in the kernel code /drivers/input/ff-core.c:input_ff_upload(...).
            - If no for **:
                I don't really know. Discussion is needed.
            
            - In my opinion, **, *** and **** are desired behavior, while * should leave the effect->id at -1.
                In that case, Wine's dinput implementation does not have to be patched, and the kernel code only should apply a minor patch.
    
    => This has been discussed (see http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html), and the following is true:
        My opinion appeared to be correct, but for to be sure, I was recommended to apply the change in Wine as well.


b)  Be more precise in returning errors.
    In dinput/effect_linuxinput.c:89, change :
        if (errno == ENOMEM) {
            return DIERR_DEVICEFULL;
        } else {
            FIXME("Could not upload effect. Assuming a disconnected device %d \"%s\".\n", *This->fd, strerror(errno));
            return DIERR_INPUTLOST;
        }
    to :
        switch (errno) {
            case EINVAL:
                TRACE("Could not upload effect: Invalid argument.\n");
                return DIERR_INVALIDPARAM;
            case ENOSPC:
                TRACE("Could not upload effect: No space left on device.\n");
                return DIERR_DEVICEFULL;
            case ENOMEM:
                TRACE("Could not upload effect: Out of memory.\n");
                return DIERR_OUTOFMEMORY;
            default:
                FIXME("Could not upload effect. Assuming a disconnected device %d \"%s\".\n", *This->fd, strerror(errno));
                return DIERR_INPUTLOST;
        }


c)  The following in dinput/effect_linuxinput.c:336 :
        if (res != DI_OK)
    should be probably :
        if (FAILED(res))
    for example if a device reports S_FALSE because it has already updated an identical effect.
    
    The same for line 549:
        if (retval != DI_OK)
    should be then :
        if (FAILED(retval))





/////////////////////////////    Linux effect parameter: start delay    /////////////////////////////

a)  Keep in mind that startDelay is not supported in DX5 or lower:
    In the SetParameters(...) case, we're sure This->effect.replay.delay is set to zero,
    because the HEAP_ZERO_MEMORY flag was set when allocating the LinuxInputEffectImpl 'This' struct in linuxinput_create_effect(...).
    So change dinput/effect_linuxinput.c:223 :
        if (dwFlags & DIEP_STARTDELAY) {
            peff->dwStartDelay = This->effect.replay.delay * 1000;
        }
    to :
        if ((dwFlags & DIEP_STARTDELAY) && (peff->dwSize > sizeof(DIEFFECT_DX5))) {
            peff->dwStartDelay = This->effect.replay.delay * 1000;
        }
    
    and change dinput/effect_linuxinput.c:463 :
        if (dwFlags & DIEP_STARTDELAY)
            This->effect.replay.delay = peff->dwStartDelay / 1000;
    to :
        if ((dwFlags & DIEP_STARTDELAY) && (peff->dwSize > sizeof(DIEFFECT_DX5)))
            This->effect.replay.delay = peff->dwStartDelay / 1000;





/////////////////////////////    Linux effect parameter: periodic effect's phase    /////////////////////////////

a)  Periodic phase is not implemented conform most efficient implementations of linux FF-devices:
    phase should be related to time, not to angle:
    - HW interface example: In /drivers/input/joystick/iforce/iforce-ff.c :
        make_period_modifier(...): "iforce_send_packet(iforce, FF_CMD_PERIOD, data)" where data[4] = HI(phase);
        make_core(...): "iforce_send_packet(iforce, FF_CMD_EFFECT, data)" where data[4] = HI(duration);
        => Conclusion: phase likely to be related to time, not to angle
    - SW kernel interface example, which is going to be used for all memless FF devices in the future:
        (http://permalink.gmane.org/gmane.linux.kernel/1620326) in ff-memless-next.c:149 :
        mlnxeff->playback_time = effect->u.periodic.phase
    - SW interface example: SDL2-2.0.1/src/haptic/linux/SDL_syshaptic.c:640
    
    There is also written on /include/uapi/linux/input.h:1058 :
        "@phase: 'horizontal' shift"
        where 'horizontal' most likely means time dimension, not phase of the waveform.
    => To be sure, I asked this at http://www.mail-archive.com/linux-input@vger.kernel.org/msg08459.html,
        but didn't receive any answer yet.
    
    So change the following in dinput/effect_linuxinput.c:259 :
        tsp->lOffset = (This->effect.u.periodic.offset / 33) * 10;
        tsp->dwPhase = (This->effect.u.periodic.phase / 33) * 36;
        tsp->dwPeriod = (This->effect.u.periodic.period * 1000);
    to :
        tsp->lOffset = (This->effect.u.periodic.offset / 33) * 10;
        if (!This->effect.u.periodic.period) {
            /* Avoid division by zero period, set tsp->dwPhase arbitrarily to zero. */
            tsp->dwPhase = 0;
        } else
            tsp->dwPhase = (((int)This->effect.u.periodic.phase * 36000) / This->effect.u.periodic.period) % 36000;
        tsp->dwPeriod = (This->effect.u.periodic.period * 1000);
    
    and change the following in dinput/effect_linuxinput.c:486 :
        This->effect.u.periodic.offset = (tsp->lOffset / 10) * 32;
        This->effect.u.periodic.phase = (tsp->dwPhase / 9) * 8; /* == (/ 36 * 32) */
        This->effect.u.periodic.period = tsp->dwPeriod / 1000;
    to :
        This->effect.u.periodic.offset = (tsp->lOffset / 10) * 32;
        This->effect.u.periodic.period = tsp->dwPeriod / 1000;
        This->effect.u.periodic.phase = ((int)(tsp->dwPhase % 36000) * This->effect.u.periodic.period) / 36000;





/////////////////////////////    Linux effect parameters: time parameters    /////////////////////////////

a)  Because of the higher time resolution of DInput parameters (in microseconds instead of milliseconds), e.g. dwPeriod,
    and because truncating may result in "periodic.period == 0", while "1 <= dwPeriod (non-zero) < 1000",
    and because "periodic.period == 0" is an invalid argument,
    we might set periodic.period to the smallest non-zero value (that is 1) when dwPeriod is non-zero, to avoid unnecessary exceptions.
    So change in dinput/effect_linuxinput.c:488 :
        This->effect.u.periodic.period = tsp->dwPeriod / 1000;
    to :
        if (0 < tsp->dwPeriod && tsp->dwPeriod <= 1000)
            This->effect.u.periodic.period = 1;
        else
            This->effect.u.periodic.period = tsp->dwPeriod / 1000;


b)  Anticipate if dwPeriod < 0, otherwise periodic.period will incorrectly wrap around because it's unsigned.
    We can choose to let periodic.period be proportional to:
    - 0    (driver will generate EINVAL)
    - abs(dwPeriod)    (no error, and is mathematically more correct)
    
    Choosing the latter, so change in dinput/effect_linuxinput.c:488 :
        if (0 < tsp->dwPeriod && tsp->dwPeriod <= 1000)
            This->effect.u.periodic.period = 1;
        else
            This->effect.u.periodic.period = tsp->dwPeriod / 1000;
    to :
        if (tsp->dwPeriod && abs(tsp->dwPeriod) <= 1000)
            This->effect.u.periodic.period = 1;
        else
            This->effect.u.periodic.period = abs(tsp->dwPeriod) / 1000;


c)  Something similar is needed for 'dwDuration':
    Set infinite replay-length in linux by "replay.length == 0": discussion: http://www.mail-archive.com/linux-input@vger.kernel.org/msg08459.html
    Knowing that DInput "INFINITE == -1", we convert -1 to 0.
    If "peff->dwDuration == 0", we can:
    - not start the effect
    - set the effect duration to the absolute minimum
    
    I would choose the latter, because otherwise more behaviour of the code would change.
    And lastly, we don't throw exceptions when "dwDuration < 0": MSDN does not require us to throw DIERR_INVALIDPARAM.
    So, change in dinput/effect_linuxinput.c:191 :
        if (dwFlags & DIEP_DURATION) {
            peff->dwDuration = (DWORD)This->effect.replay.length * 1000;
        }
    to :
        if (dwFlags & DIEP_DURATION) {
            peff->dwDuration = (This->effect.replay.length ? (DWORD)This->effect.replay.length * 1000 : -1);
        }
    
    And change in dinput/effect_linuxinput.c:424 :
        if (dwFlags & DIEP_DURATION)
            This->effect.replay.length = peff->dwDuration / 1000;
    to :
        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
        }





/////////////////////////////    Linux effect: conditional effect's parameters    /////////////////////////////

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;
        }





/////////////////////////////    Linux effect: axes    /////////////////////////////

a)  In dinput/effect_linuxinput.c, the GetParameters(...) and SetParameters(...) functions implicitly assume that
    (DIEFFECT.dwFlags & DIEFF_OBJECTOFFSETS) != 0, but the other flag 'DIEFF_OBJECTIDS' is also possible according to MSDN:
        The semantics of dwTriggerButton and rgdwAxes are determined by the flags DIEFF_OBJECTIDS and DIEFF_OBJECTOFFSETS.
        (http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.reference.dieffect%28v=vs.85%29.aspx)
    But I don't know exactly how to convert values determined by DIEFF_OBJECTIDS to values determined by DIEFF_OBJECTOFFSETS.
    
    => Suggestions?
    (the reason why this hasn't yet resulted in problems, is because the usage of 'DIEFF_OBJECTIDS' seems to be rare)





/////////////////////////////    Linux effect direction    /////////////////////////////

a)  Some common mistakes about linux directions:
    - using 0x7FFF as ~359 degrees for linux direction, instead of 0xFFFF (look at /include/uapi/linux/input.h:1117)
    - the everlasting confusion about how to map dinput to linux directions,
        this proves to be a difficult topic, only by finding out that the SDL2 haptic implementation does a different conversion than Wine does,
        and since only one possibility is the right one, at least one of the implementations is wrong.
        It turns out they're both wrong.
        With this explanation, I try to finally come up with the right implementation, by deriving it in a systematical way.
        This explanation has been verified by the linux input mailing lists: http://www.mail-archive.com/linux-input@vger.kernel.org/msg08459.html
    
        Directions
        ==========

        For conversions between different APIs, the physical behaviour should be the same.

        DInput
        ______

        Extracted from http://msdn.microsoft.com/en-us/library/windows/desktop/ee417536%28v=vs.85%29.aspx :

        Physical situation:


                 North (polar 0, spher[0] 270)
                    (0, -1)
                    -y Far

          West              East (polar 90, spher[0] 0)
        (-1, 0)        O       (+1, 0)
        -x Left                +x Right

                     South
                    (0, +1)
                    +y Near


                    .------.
                    | User |
                    '------'

        Remember these are the 'directions':
            as defined as the direction the user should exert force in to counteract the force.
            So the direction in which the force is applied by the joystick, is opposite.

        If we're interested in the direction in which the force is applied by the joystick,
        the physical situation becomes point-flipped:


                          (polar 0, spher[0] 270)
                                  (0, +1)
                                    +y

        (polar 270, spher[0] 180)           (polar 90, spher[0] 0)
                (+1, 0)              O             (-1, 0)
                   +x                                -x

                          (polar 180, spher[0] 90)
                                  (0, -1)
                                    -y

        Linux
        _____

        Extracted from Linux /include/uapi/linux/input.h:1113, and from the discussion with Anssi Hannula in the mailing lists :

        Physical situation:


                    Up (dir 180)
                      (0, -1)
                        -y
                      
        Left (dir 90)           Right (dir 270)
          (-1, 0)        O          (+1, 0)
            -x                        +x 
                  
                  Down (dir 0)
                      (0, +1)
                        +y


                     .------.
                     | User |
                     '------'

        It is unclear what is meant by 'directions' in this case:
            Possibility #1: counteraction direction of user
            Possibility #2: direction of force applied by joystick

        Anssi Hannula confirmed that Possibility #2 is the right one.

        Mapping
        _______

        In the "Carts" column, you can find the Cartesian coordinates which indicate the counteraction direction of user.
        (This choice is arbitrary ('direction of force applied by joystick' could also have been chosen), just to have a physical reference.)
        These "Carts" tuples should be equal across APIs to ensure equal behavior.

        #Assume Possibility #1:
        #
        #          DInput         <->     Linux
        #    _________________     |   ___________
        #    Carts|Polar|Spher     |   Carts|Direc
        #    -----+-----+-----     |   -----+-----
        #    0, -1|   0 | 270      |   0, -1| 180 
        #    +1, 0|  90 |   0      |   +1, 0| 270 
        #    0, +1| 180 |  90      |   0, +1|   0 
        #    -1, 0| 270 | 180      |   -1, 0|  90 

        Assume Possibility #2:
            => Linux Cartesian coordinates are opposite in respect to the "Carts" reference.
            
                  DInput         <->     Linux
            _________________     |   ___________
            Carts|Polar|Spher     |   Carts|Direc
            -----+-----+-----     |   -----+-----
            0, -1|   0 | 270      |   0, -1|   0 
            +1, 0|  90 |   0      |   +1, 0|  90 
            0, +1| 180 |  90      |   0, +1| 180 
            -1, 0| 270 | 180      |   -1, 0| 270 

        This conversion table will be used in this document.
    
    On effect_linuxinput.c:385, there is written:
        /* some of this may look funky, but it's 'cause the linux driver and directx have
         * different opinions about which way direction "0" is.  directx has 0 along the x
         * axis (left), linux has it along the y axis (down). */
    The part "directx has 0 along the x axis (left)" contradicts with "angle bases: 0 -> -y (down) (linux) -> 0 -> +x (right) (windows)" on line 148,
    because 'left' != 'right'.
    Also, this comment seems to be about the dinput spherical coordinates, instead of polar.
    Applying the conversion table on dinput/effect_linuxinput.c:385, change :
        /* some of this may look funky, but it's 'cause the linux driver and directx have
         * different opinions about which way direction "0" is.  directx has 0 along the x
         * axis (left), linux has it along the y axis (down). */
        if (dwFlags & DIEP_DIRECTION) {
            if (peff->cAxes == 1) {
                if (peff->dwFlags & DIEFF_CARTESIAN) {
                    if (dwFlags & DIEP_AXES) {
                        if (peff->rgdwAxes[0] == DIJOFS_X && peff->rglDirection[0] >= 0)
                            This->effect.direction = 0x4000;
                        else if (peff->rgdwAxes[0] == DIJOFS_X && peff->rglDirection[0] < 0)
                            This->effect.direction = 0xC000;
                        else if (peff->rgdwAxes[0] == DIJOFS_Y && peff->rglDirection[0] >= 0)
                            This->effect.direction = 0;
                        else if (peff->rgdwAxes[0] == DIJOFS_Y && peff->rglDirection[0] < 0)
                            This->effect.direction = 0x8000;
    to :
        /* some of this may look funky, but it's 'cause the linux driver and directx have
         * different opinions about which way direction "0" is.  directx has 0 along the y
         * axis (north/far, counteract), linux has it along the y axis (down). */
        if (dwFlags & DIEP_DIRECTION) {
            if (peff->cAxes == 1) {
                if (peff->dwFlags & DIEFF_CARTESIAN) {
                    if (peff->rgdwAxes[0] == DIJOFS_X && peff->rglDirection[0] >= 0)
                        This->effect.direction = 0x4000;
                    else if (peff->rgdwAxes[0] == DIJOFS_X && peff->rglDirection[0] < 0)
                        This->effect.direction = 0xC000;
                    else if (peff->rgdwAxes[0] == DIJOFS_Y && peff->rglDirection[0] >= 0)
                        This->effect.direction = 0x8000;
                    else if (peff->rgdwAxes[0] == DIJOFS_Y && peff->rglDirection[0] < 0)
                        This->effect.direction = 0;
    
    Change dinput/effect_linuxinput.c:148 :
        /* Major conversion factors are:
         * times: millisecond (linux) -> microsecond (windows) (x * 1000)
         * forces: scale 0x7FFF (linux) -> scale 10000 (windows) approx ((x / 33) * 10)
         * angles: scale 0x7FFF (linux) -> scale 35999 (windows) approx ((x / 33) * 36)
         * angle bases: 0 -> -y (down) (linux) -> 0 -> +x (right) (windows)
         */
    to :
        /* Major conversion factors are:
         * times: millisecond (linux) -> microsecond (windows) (x * 1000)
         * forces: scale 0x7FFF (linux) -> scale 10000 (windows) approx ((x / 33) * 10)
         * angles: scale 0xFFFF (linux) -> scale 35999 (windows) approx ((x / 66) * 36)
         * angle bases: 0 -> +y (down) (linux) -> 0 -> -y (far: counteract direction) (windows polar)
         */
    
    and change dinput/effect_linuxinput.c:184 :
        peff->rglDirection[0] = (This->effect.direction / 33) * 36 + 9000;
        if (peff->rglDirection[0] > 35999)
            peff->rglDirection[0] -= 35999;
    to :
        peff->rglDirection[0] = (This->effect.direction / 66) * 36;
    
    and change dinput/effect_linuxinput.c:415 :
        This->effect.direction = (int)((3 * M_PI / 2 - atan2(y, x)) * -0x7FFF / M_PI);
    to :
        This->effect.direction = (unsigned int)((M_PI / 2 + atan2(y, x)) * 0x8000 / M_PI);
    
    and change dinput/effect_linuxinput.c:505 :
        /* One condition block.  This needs to be rotated to direction,
         * and expanded to separate x and y conditions. */
        int i;
        double factor[2];
        factor[0] = asin((This->effect.direction * 3.0 * M_PI) / 0x7FFF);
        factor[1] = acos((This->effect.direction * 3.0 * M_PI) / 0x7FFF);
    to :
        /* One condition block.  This needs to be rotated to direction,
         * using dinput Cartesian coordinates, and expanded to separate x and y conditions. */
        int i;
        double factor[2];
        factor[0] = +sin((This->effect.direction * M_PI) / 0x8000);
        factor[1] = -cos((This->effect.direction * M_PI) / 0x8000);


b)  Applying direction seems to have no influence on X and Y force. (It's always polar 0 degrees) Tested in FEdit
    => Multiple things in the implementation are wrong:
        - typo in the bracket placement for the double-to-int conversion
        - forgetting (typo) that dinput works with 9000 instead of 90 (inconsistent with linux->dinput conversion of GetParameters)
        - common mistakes about linux directions, see above
        - it is more readable and accurate to use "* (0x8000 / 18000)" than "* (0xFFFF / 35999)",
        and wrapping of this (effect.direction) u16 is intentional.
    Solution: Change dinput/effect_linuxinput.c:419 :
        This->effect.direction = (int)(((double)peff->rglDirection[0] - 90) / 35999) * 0x7FFF;
    to :
        This->effect.direction = (unsigned int)(((double)peff->rglDirection[0] / 18000) * 0x8000);


c)  Multiple things are weird/wrong here:
    - common mistakes about linux directions, see above
    - "M_PI * 3", why '3' ???
    - Why "1000" as max magnitude of direction? type(rglDirection[i]) == 'LONG' => You can pick at least 10000 (DI_FFNOMINALMAX).
    So, change dinput/effect_linuxinput.c:174 :
        if (peff->dwFlags & DIEFF_CARTESIAN) {
            peff->rglDirection[0] = sin(M_PI * 3 * This->effect.direction / 0x7FFF) * 1000;
            peff->rglDirection[1] = cos(M_PI * 3 * This->effect.direction / 0x7FFF) * 1000;
    to :
        if (peff->dwFlags & DIEFF_CARTESIAN) {
            peff->rglDirection[0] = +sin(This->effect.direction * M_PI / 0x8000) * DI_FFNOMINALMAX;
            peff->rglDirection[1] = -cos(This->effect.direction * M_PI / 0x8000) * DI_FFNOMINALMAX;


d)  The statement "Polar and spherical are the same for 2 axes" is wrong:
        http://msdn.microsoft.com/en-us/library/windows/desktop/ee417536%28v=vs.85%29.aspx :
            "Polar coordinates are expressed as a single angle, in hundredths of a degree clockwise from whatever zero-point, or true north, has been established for the effect.
                Normally this is the negative y-axis; that is, away from the user."
            => (0, -1) direction is Polar starting direction (angle = 0)
        http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.reference.dieffect%28v=vs.85%29.aspx :
            "If spherical, the first angle is measured in hundredths of a degree from the (1, 0) direction, rotated in the direction of (0, 1)."
            => (1, 0) direction is Spherical starting direction (angle = 0)
        (also take a look at the conversion table)
    
    So, change dinput/effect_linuxinput.c:177 :
        } else {
            /* Polar and spherical coordinates are the same for two or less
             * axes.
             * Note that we also use this case if NO flags are marked.
             * According to MSDN, we should return the direction in the
             * format that it was specified in, if no flags are marked.
             */
            peff->rglDirection[0] = (This->effect.direction / 66) * 36;
        }
    to :
        } else if (peff->dwFlags & DIEFF_SPHERICAL) {
            peff->rglDirection[0] = 27000 + (This->effect.direction / 66) * 36;
            if (peff->rglDirection[0] >= 36000)
                peff->rglDirection[0] -= 36000;
        } else /* if (peff->dwFlags & DIEFF_POLAR) */ {
            /* Note that we return polar coordinates if NO flags are marked.
             * According to MSDN, we should return the direction in the
             * format that it was specified in, if no flags are marked.
             * TODO: Return the direction in the format that it was specified in,
             *       if no flags are marked.
             */
            peff->rglDirection[0] = (This->effect.direction / 66) * 36;
        }
    
    And change dinput/effect_linuxinput.c:416 :
        } else {
            /* Polar and spherical are the same for 2 axes */
            /* Precision is important here, so we do double math with exact constants */
            This->effect.direction = (unsigned int)(((double)peff->rglDirection[0] / 18000) * 0x8000);
        }
    to :
        } 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);
        }


e)  If "first_axis_is_x" in effect_linuxinput.c happens to be "false" and "cAxes == 2",
    doesn't the assignment of "effect.direction" in SetParameters(...) need additional transformations if "(peff->dwFlags & DIEFF_POLAR) != 0"?
    Because then x and y axis are swapped.
    With swapped x and y axes, then the following conversions hold:
    
        #Assume Possibility #1:
        #
        #          DInput         <->     Linux
        #    _________________     |   ___________
        #    Carts|Polar|Spher     |   Carts|Direc
        #    -----+-----+-----     |   -----+-----
        #    -1, 0|   0 | 270      |   -1, 0|  90 
        #    0, +1|  90 |   0      |   0, +1|   0 
        #    +1, 0| 180 |  90      |   +1, 0| 270 
        #    0, -1| 270 | 180      |   0, -1| 180 

        Assume Possibility #2:
            => Linux Cartesian coordinates are opposite in respect to the "Carts" reference.
            
                  DInput         <->     Linux
            _________________     |   ___________
            Carts|Polar|Spher     |   Carts|Direc
            -----+-----+-----     |   -----+-----
            -1, 0|   0 | 270      |   -1, 0| 270 
            0, +1|  90 |   0      |   0, +1| 180 
            +1, 0| 180 |  90      |   +1, 0|  90 
            0, -1| 270 | 180      |   0, -1|   0 

        (Possibility #2 is the correct one)

    So change 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) {
            This->effect.direction = (unsigned int)(((9000 + (double)peff->rglDirection[0]) / 18000) * 0x8000);
            if (!This->first_axis_is_x)
                This->effect.direction = 0xC000 - This->effect.direction;
        } else /* if (peff->dwFlags & DIEFF_POLAR) */ {
            This->effect.direction = (unsigned int)(((double)peff->rglDirection[0] / 18000) * 0x8000);
            if (!This->first_axis_is_x)
                This->effect.direction = 0xC000 - This->effect.direction;
        }
    
    Then we can simplify the following code of dinput/effect_linuxinput.c:405 :
        } else { /* two axes */
            if (peff->dwFlags & DIEFF_CARTESIAN) {
                LONG x, y;
                if (This->first_axis_is_x) {
                    x = peff->rglDirection[0];
                    y = peff->rglDirection[1];
                } else {
                    x = peff->rglDirection[1];
                    y = peff->rglDirection[0];
                }
                This->effect.direction = (unsigned int)((M_PI / 2 + atan2(y, x)) * 0x8000 / M_PI);
            } else if (peff->dwFlags & DIEFF_SPHERICAL) {
                This->effect.direction = (unsigned int)(((9000 + (double)peff->rglDirection[0]) / 18000) * 0x8000);
                if (!This->first_axis_is_x)
                    This->effect.direction = 0xC000 - This->effect.direction;
            } else /* if (peff->dwFlags & DIEFF_POLAR) */ {
                This->effect.direction = (unsigned int)(((double)peff->rglDirection[0] / 18000) * 0x8000);
                if (!This->first_axis_is_x)
                    This->effect.direction = 0xC000 - This->effect.direction;
            }
        }
    to :
        } else { /* two axes */
            if (peff->dwFlags & DIEFF_CARTESIAN) {
                This->effect.direction = (unsigned int)((M_PI / 2 + atan2(peff->rglDirection[1], peff->rglDirection[0])) * 0x8000 / M_PI);
            } 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);
            }
            if (!This->first_axis_is_x)
                This->effect.direction = 0xC000 - This->effect.direction;
        }


f)  Condition effects (with struct-size of 2*sizeof(DICONDITION)) should keep "first_axis_is_x" into account:
    Change dinput/effect_linuxinput.c:520 :
        /* Two condition blocks.  Direct parameter copy. */
        int i;
        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;
        }
    to :
        /* Two condition blocks.  Direct parameter copy, after a small change of axes if needed. */
        int i, j;
        for (i = 0; i < 2; ++i) {
            j = (first_axis_is_x ? i : 1-i);
            This->effect.u.condition[j].center = (tsp[i].lOffset / 10) * 32;
            This->effect.u.condition[j].right_coeff = (tsp[i].lPositiveCoefficient / 10) * 32;
            This->effect.u.condition[j].left_coeff = (tsp[i].lNegativeCoefficient / 10) * 32;
            This->effect.u.condition[j].right_saturation = (tsp[i].dwPositiveSaturation / 10) * 65;
            This->effect.u.condition[j].left_saturation = (tsp[i].dwNegativeSaturation / 10) * 65;
            This->effect.u.condition[j].deadband = (tsp[i].lDeadBand / 10) * 65;
        }
    
    The condition effects with struct-size of 1*sizeof(DICONDITION) don't need changes regarding "first_axis_is_x",
    because "factor[i]" depends on "effect.direction", which already incorporates "first_axis_is_x".


g)  Condition effects (with struct-size of 1*sizeof(DICONDITION)) are not transformed exactly right,
    look at "interactive.fig" for more understanding of the suggested code:
        - "deadband" can't be negative
        - for "left/right_coeff" and "left/right_saturation",
            if a direction along an axis is negative, "left" and "right" should be swapped
   
    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) * 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);
        }
    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)(abs(factor[i]) * ((factor[i] >= 0 ? tsp->lPositiveCoefficient : tsp->lNegativeCoefficient) / 10) * 32);
            This->effect.u.condition[i].left_coeff = (int)(abs(factor[i]) * ((factor[i] >= 0 ? tsp->lNegativeCoefficient : tsp->lPositiveCoefficient) / 10) * 32);
            This->effect.u.condition[i].right_saturation = (int)(abs(factor[i]) * ((factor[i] >= 0 ? tsp->dwPositiveSaturation : tsp->dwNegativeSaturation) / 10) * 65);
            This->effect.u.condition[i].left_saturation = (int)(abs(factor[i]) * ((factor[i] >= 0 ? tsp->dwNegativeSaturation : tsp->dwPositiveSaturation) / 10) * 65);
            This->effect.u.condition[i].deadband = (int)(abs(factor[i]) * (tsp->lDeadBand / 10) * 65);
        }





/////////////////////////////    Linux effect parameters (global)    /////////////////////////////

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.





/////////////////////////////    Linux joystick initialization    /////////////////////////////

a)  In dinput/joystick_linuxinput.c:546 :
    Why not change (to fill in additional device capabilities) :
        if (newDevice->joydev->has_ff)
            newDevice->generic.devcaps.dwFlags |= DIDC_FORCEFEEDBACK;
    to :
        if (newDevice->joydev->has_ff) {
            newDevice->generic.devcaps.dwFlags |= DIDC_FORCEFEEDBACK;
            
            newDevice->generic.devcaps.dwFlags |= DIDC_STARTDELAY;
            if ( test_bit(newDevice->joydev->ffbits, FF_CONSTANT) || 
                        test_bit(newDevice->joydev->ffbits, FF_PERIODIC) || 
                        test_bit(newDevice->joydev->ffbits, FF_RAMP) )
                newDevice->generic.devcaps.dwFlags |= DIDC_FFATTACK | DIDC_FFFADE;
            if ( test_bit(newDevice->joydev->ffbits, FF_SPRING) || 
                        test_bit(newDevice->joydev->ffbits, FF_DAMPER) || 
                        test_bit(newDevice->joydev->ffbits, FF_INERTIA) || 
                        test_bit(newDevice->joydev->ffbits, FF_FRICTION) )
                newDevice->generic.devcaps.dwFlags |= DIDC_POSNEGCOEFFICIENTS | DIDC_POSNEGSATURATION | DIDC_SATURATION;
        }





/////////////////////////////    Debug message fixes/additions    /////////////////////////////

a)  An effectSpecific structure corresponding with a condition effect, can have 2 possible valid sizes, instead of 1:
        sizeof(DICONDITION) and 2*sizeof(DICONDITION); the latter is even more common.
    Solution: change in dinput/joystick.c:216 :
        if (eff->cbTypeSpecificParams != sizeof(DICONDITION)) {
            WARN("Effect claims to be a condition but the type-specific params are the wrong size!\n");
        } else {
            _dump_DICONDITION(eff->lpvTypeSpecificParams);
        }
    to :
        if (eff->cbTypeSpecificParams == sizeof(DICONDITION)) {
            _dump_DICONDITION(eff->lpvTypeSpecificParams);
        } else if (eff->cbTypeSpecificParams == 2 * sizeof(DICONDITION)) {
            TRACE("Effect has two condition blocks:\n");
            _dump_DICONDITION(eff->lpvTypeSpecificParams);
            _dump_DICONDITION(eff->lpvTypeSpecificParams + sizeof(DICONDITION));
        } else {
            WARN("Effect claims to be a condition but the type-specific params are the wrong size (0x%x instead of 0x%x)!\n",
                    eff->cbTypeSpecificParams, sizeof(DICONDITION));
        }


b)  Question: consider following code in dinput/effect_linuxinput:369 :
        if ((dwFlags & ~DIEP_NORESTART & ~DIEP_NODOWNLOAD & ~DIEP_START) == 0) {
            /* set everything */
            dwFlags = DIEP_AXES | DIEP_DIRECTION | DIEP_DURATION | DIEP_ENVELOPE |
                DIEP_GAIN | DIEP_SAMPLEPERIOD | DIEP_STARTDELAY | DIEP_TRIGGERBUTTON |
                DIEP_TRIGGERREPEATINTERVAL | DIEP_TYPESPECIFICPARAMS;
    
    Q1: Why is everything set, if dwFlags has no bits set (except DIEP_NORESTART, DIEP_NODOWNLOAD or DIEP_START)?
        I can see no mention of it on MSDN.
    
    Q2: Why "dwFlags = " and not "dwFlags |= "?
        E.g. assume dwFlags = DIEP_START, then the if-test will cause the DIEP_START flag to be lost, resulting in an effect not started.


c)  If Q1 is right, then the order of "dump_DIEFFECT" on dinput/effect_linuxinput:367 should be changed, from :
        dump_DIEFFECT(peff, &This->guid, dwFlags);
        
        if ((dwFlags & ~DIEP_NORESTART & ~DIEP_NODOWNLOAD & ~DIEP_START) == 0) {
            /* set everything */
            dwFlags = DIEP_AXES | DIEP_DIRECTION | DIEP_DURATION | DIEP_ENVELOPE |
                DIEP_GAIN | DIEP_SAMPLEPERIOD | DIEP_STARTDELAY | DIEP_TRIGGERBUTTON |
                DIEP_TRIGGERREPEATINTERVAL | DIEP_TYPESPECIFICPARAMS;
        }
    to :
        if ((dwFlags & ~DIEP_NORESTART & ~DIEP_NODOWNLOAD & ~DIEP_START) == 0) {
            /* set everything */
            dwFlags = DIEP_AXES | DIEP_DIRECTION | DIEP_DURATION | DIEP_ENVELOPE |
                DIEP_GAIN | DIEP_SAMPLEPERIOD | DIEP_STARTDELAY | DIEP_TRIGGERBUTTON |
                DIEP_TRIGGERREPEATINTERVAL | DIEP_TYPESPECIFICPARAMS;
        }
        
        dump_DIEFFECT(peff, &This->guid, dwFlags);


d)  Add debug info for rglDirection:
    Change in dinput/joystick.c:169 :
        TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);
        TRACE("  - dwTriggerRepeatInterval: %d\n", eff->dwTriggerRepeatInterval);
        TRACE("  - rglDirection: %p\n", eff->rglDirection);
        TRACE("  - cbTypeSpecificParams: %d\n", eff->cbTypeSpecificParams);
        TRACE("  - lpvTypeSpecificParams: %p\n", eff->lpvTypeSpecificParams);

        /* Only trace some members if dwFlags indicates they have data */
        if (dwFlags & DIEP_AXES) {
            TRACE("  - cAxes: %d\n", eff->cAxes);
            TRACE("  - rgdwAxes: %p\n", eff->rgdwAxes);

            if (TRACE_ON(dinput) && eff->rgdwAxes) {
                TRACE("    ");
                for (i = 0; i < eff->cAxes; ++i)
                    TRACE("%d ", eff->rgdwAxes[i]);
                TRACE("\n");
            }
        }
    to :
        TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);
        TRACE("  - dwTriggerRepeatInterval: %d\n", eff->dwTriggerRepeatInterval);
        TRACE("  - cbTypeSpecificParams: %d\n", eff->cbTypeSpecificParams);
        TRACE("  - lpvTypeSpecificParams: %p\n", eff->lpvTypeSpecificParams);

        /* Only trace some members if dwFlags indicates they have data */
        if ((dwFlags & DIEP_AXES) || (dwFlags & DIEP_DIRECTION)) {
            TRACE("  - cAxes: %d\n", eff->cAxes);
        
            if (dwFlags & DIEP_AXES) {
                TRACE("  - rgdwAxes: %p\n", eff->rgdwAxes);

                if (TRACE_ON(dinput) && eff->rgdwAxes) {
                    TRACE("    ");
                    for (i = 0; i < eff->cAxes; ++i)
                        TRACE("%d ", eff->rgdwAxes[i]);
                    TRACE("\n");
                }
            }

            if (dwFlags & DIEP_DIRECTION) {
                TRACE("  - rglDirection: %p\n", eff->rglDirection);

                if (TRACE_ON(dinput) && eff->rglDirection) {
                    TRACE("    ");
                    for (i = 0; i < eff->cAxes; ++i)
                        TRACE("%d ", eff->rglDirection[i]);
                    TRACE("\n");
                }
            }
        }


e)  Add debug info for dwSamplePeriod:
    Change in dinput/joystick.c:166 :
        if (eff->dwGain > 10000)
            WARN("dwGain is out of range (>10,000)\n");
        
        TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);
    to :
        if (eff->dwGain > 10000)
            WARN("dwGain is out of range (>10,000)\n");
        
        if (dwFlags & DIEP_SAMPLEPERIOD)
            TRACE("  - dwSamplePeriod: %d\n", eff->dwSamplePeriod);
        TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);


f)  Why not consistently only trace set (and probably thus valid) parameters in dump_DIEFFECT?
    
    Change in dinput/joystick.c:163 :
        TRACE("  - dwDuration: %d\n", eff->dwDuration);
        TRACE("  - dwGain: %d\n", eff->dwGain);
            
        if (eff->dwGain > 10000)
            WARN("dwGain is out of range (>10,000)\n");

        if (dwFlags & DIEP_SAMPLEPERIOD)
            TRACE("  - dwSamplePeriod: %d\n", eff->dwSamplePeriod);
        TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);
        TRACE("  - dwTriggerRepeatInterval: %d\n", eff->dwTriggerRepeatInterval);
        TRACE("  - cbTypeSpecificParams: %d\n", eff->cbTypeSpecificParams);
        TRACE("  - lpvTypeSpecificParams: %p\n", eff->lpvTypeSpecificParams);
    to :
        if (dwFlags & DIEP_DURATION)
            TRACE("  - dwDuration: %d\n", eff->dwDuration);
        if (dwFlags & DIEP_GAIN) {
            TRACE("  - dwGain: %d\n", eff->dwGain);
            
            if (eff->dwGain > 10000)
                WARN("dwGain is out of range (>10,000)\n");
        }

        if (dwFlags & DIEP_SAMPLEPERIOD)
            TRACE("  - dwSamplePeriod: %d\n", eff->dwSamplePeriod);
        if (dwFlags & DIEP_TRIGGERBUTTON)
            TRACE("  - dwTriggerButton: %d\n", eff->dwTriggerButton);
        if (dwFlags & DIEP_TRIGGERREPEATINTERVAL)
            TRACE("  - dwTriggerRepeatInterval: %d\n", eff->dwTriggerRepeatInterval);
        if (dwFlags & DIEP_TYPESPECIFICPARAMS) {
            TRACE("  - cbTypeSpecificParams: %d\n", eff->cbTypeSpecificParams);
            TRACE("  - lpvTypeSpecificParams: %p\n", eff->lpvTypeSpecificParams);
        }
    
    and change on dinput/joystick.c:194 :
        if (eff->dwSize > sizeof(DIEFFECT_DX5))
            TRACE("  - dwStartDelay: %d\n", eff->dwStartDelay);
    to :
        if ((dwFlags & DIEP_STARTDELAY) && (eff->dwSize > sizeof(DIEFFECT_DX5)))
            TRACE("  - dwStartDelay: %d\n", eff->dwStartDelay);
    
    Note: If applying that patch, maybe we should use TRACE_ON(dinput) to circumvent all these ifs for performance reasons?


More information about the wine-devel mailing list