[RFC 11/11] Linux FF: Debug message fixes/additions
Elias Vanderstuyft
elias.vds at gmail.com
Tue Mar 4 13:20:39 CST 2014
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?
Elias
-------------- next part --------------
///////////////////////////// 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