Re: [RFC 5/11] Linux FF: Linux effect parameters: time parameters

[Elias Vanderstuyft]

On Wed, Mar 5, 2014 at 4:03 PM, Andrew Eikum  wrote:
> On Tue, Mar 04, 2014 at 08:20:09PM +0100, Elias Vanderstuyft wrote:
> > 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;
>
> Seems good. While we're here, any reason not to use round(3) instead
> of always rounding down in the >1000 case?

That's a good question, the reason is that I want to avoid floating
point arithmetic (round(3)).
But you also have to see this in perspective:
The behaviour of my code is indeed inconsistent around the dwPeriod ==
1000 +- 500 region (in respect with higher values of dwPeriod), but
this is practically irrelevant: a dwPeriod of 1000 means 1ms; a period
of a periodic effect should be minimum 2*sampleperiod, so this
inconsistency will only be perceived (and I'm even not sure a human
would perceive this) when the Linux FF driver has a sampleperiod of
0.5ms, and this is unlikely (lowest value I have seen is 8ms). (Except
maybe for realtime systems like real-motion simulators, but in that
case, I don't think DInput (or other software) will be used as low
level interface, it's more likely that would be implemented in
hardware.)

So can I leave it as I suggested before?
> > 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)
>
> DWORDs are usually treated as unsigned.

Thanks for clarifying, I'll fix it.
For some odd reason, I remembered them as signed 32bit values ;)

My code should then become:
    if (tsp->dwPeriod && tsp->dwPeriod <= 1000)
        This->effect.u.periodic.period = 1;
    else
        This->effect.u.periodic.period = tsp->dwPeriod / 1000;
> Is this how Windows behaves
> for massive dwPeriod? I would expect E_INVALIDARG, since it's out of
> the range [0,36000).

I think you're mixing up dwPeriod with dwPhase. The range of dwPeriod
is not defined in MSDN.
> > 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
> >     }
>
> This seems okay, though again, maybe use round().

Again, practically an inconsistency in duration at the 1ms bound, will
not be perceivable, see my comment above.
> You should also use
> INFINITE instead of -1.

OK.

Maybe we should also change all occurrences of 10000 in a force
context to DI_FFNOMINALMAX, in the whole FF dinput implementation?

Elias