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 }
Elias
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?
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. Is this how Windows behaves for massive dwPeriod? I would expect E_INVALIDARG, since it's out of the range [0,36000).
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(). You should also use INFINITE instead of -1.
Andrew
On Wed, Mar 5, 2014 at 4:03 PM, Andrew Eikum aeikum@codeweavers.com 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
On Sun, Mar 09, 2014 at 11:38:06AM +0100, Elias Vanderstuyft wrote:
On Wed, Mar 5, 2014 at 4:03 PM, Andrew Eikum aeikum@codeweavers.com 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)).
Okay. If using floating point is costly, then I agree the tiny (<1ms) increase in accuracy isn't worth it.
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.
Yes, you're right. Your updated patch seems fine.
Maybe we should also change all occurrences of 10000 in a force context to DI_FFNOMINALMAX, in the whole FF dinput implementation?
I would support a patch like that, yes.
(I've been busy lately, but I'll get to the rest of your mail sequence soon!)
Andrew
On Mon, Mar 10, 2014 at 5:19 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Sun, Mar 09, 2014 at 11:38:06AM +0100, Elias Vanderstuyft wrote:
On Wed, Mar 5, 2014 at 4:03 PM, Andrew Eikum aeikum@codeweavers.com 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)).
Okay. If using floating point is costly, then I agree the tiny (<1ms) increase in accuracy isn't worth it.
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.
Yes, you're right. Your updated patch seems fine.
Maybe we should also change all occurrences of 10000 in a force context to DI_FFNOMINALMAX, in the whole FF dinput implementation?
I would support a patch like that, yes.
OK.
(I've been busy lately, but I'll get to the rest of your mail sequence soon!)
No problem, thanks for all feedback already!
Elias
-
Andrew Eikum -
Elias Vanderstuyft