Describe the bug
FrameGrabberControls_Responder/Forwarder (the RPC pair backing
IFrameGrabberControls::setFeature, setFeature (2-arg), setActive,
setMode, and setOnePush, used by RGBDSensorClient and
frameGrabber_nwc_yarp) do not propagate the underlying device's actual
success/failure result back to the caller.
FrameGrabberControls_Responder::respond() correctly computes ok from the
real device call (e.g. fgCtrl->setFeature(...)), but never writes it into
the response Bottle for VOCAB_SET commands. On the client side,
FrameGrabberControls_Forwarder::setFeature() (and the other four SET style
methods) return m_port.write(cmd, response) directly, which only reflects
whether the RPC round-trip completed, not whether the device accepted the
request.
Net effect: application code can receive true from setFeature()/
setActive()/setMode()/setOnePush() even when the underlying device's
own implementation returned false.
Found while investigating #3304 (RGBDSensorClient::setFeature() reportedly
returning true without changing the camera). I could not find any code
change between v3.10.0 and v3.12.1 in the actual setFeature() call path
that would explain a version-to-version regression there, so this bug isn't
itself a regression, it's present identically in both versions but it is
a real defect that would produce the exact symptom described in #3304 (RPC
reports success, device silently didn't apply the change). See #3304 for
the original report and additional context.
The sibling protocols in the same file family, RgbVisualParams and
DepthVisualParams, already correctly encode and read back the device's
real result, confirming this is an omission specific to
FrameGrabberControls, not an intentional design choice. Also checked
FrameGrabberControlsDC1394 (uses a different but internally consistent
convention, no bug) and FrameGrabberOf (no SET implementation at all).
To Reproduce
Steps to reproduce the behavior:
- Build
frameGrabber_nwc_yarp talking to frameGrabber_nws_yarp, wrapping
fakeFrameGrabber (whose setFeature/setActive/setMode/setOnePush
stubs always return false).
- Call, e.g.:
IFrameGrabberControls* ictrl = ...;
bool ok = ictrl->setFeature(0, 1.0);
- Observe
ok == true, even though fakeFrameGrabber::setFeature()
returned false.
Expected behavior
setFeature()/setActive()/setMode()/setOnePush() should return
false whenever the underlying device's own implementation returns
false, matching the existing behavior of setRgbResolution()/
setRgbFOV()/etc. in RgbVisualParams.
Screenshots
None (protocol/logic bug, not visual).
Configuration (please complete the following information):
- OS: N/A (reproducible on any platform; not environment-specific)
- yarp version: yarp-3.12 branch (present in v3.12.1, and likely earlier
3.x releases using the same framegrabber_protocol code not checked
further back than v3.10.0)
- compiler: N/A
Additional context
Not present on master : this protocol was replaced entirely by a
Thrift/ReturnValue-based RPC in PR #3184, which correctly propagates the
result. This issue and an accompanying PR are scoped to the yarp-3.12
stable branch only.
I have a fix ready (mirrors the existing RgbVisualParams_Responder/
Forwarder pattern) and a regression test using fakeFrameGrabber
will open a PR referencing this issue shortly.
cc @randaz81
Describe the bug
FrameGrabberControls_Responder/Forwarder(the RPC pair backingIFrameGrabberControls::setFeature,setFeature(2-arg),setActive,setMode, andsetOnePush, used byRGBDSensorClientandframeGrabber_nwc_yarp) do not propagate the underlying device's actualsuccess/failure result back to the caller.
FrameGrabberControls_Responder::respond()correctly computesokfrom thereal device call (e.g.
fgCtrl->setFeature(...)), but never writes it intothe response
BottleforVOCAB_SETcommands. On the client side,FrameGrabberControls_Forwarder::setFeature()(and the other four SET stylemethods) return
m_port.write(cmd, response)directly, which only reflectswhether the RPC round-trip completed, not whether the device accepted the
request.
Net effect: application code can receive
truefromsetFeature()/setActive()/setMode()/setOnePush()even when the underlying device'sown implementation returned
false.Found while investigating #3304 (
RGBDSensorClient::setFeature()reportedlyreturning
truewithout changing the camera). I could not find any codechange between v3.10.0 and v3.12.1 in the actual
setFeature()call paththat would explain a version-to-version regression there, so this bug isn't
itself a regression, it's present identically in both versions but it is
a real defect that would produce the exact symptom described in #3304 (RPC
reports success, device silently didn't apply the change). See #3304 for
the original report and additional context.
The sibling protocols in the same file family,
RgbVisualParamsandDepthVisualParams, already correctly encode and read back the device'sreal result, confirming this is an omission specific to
FrameGrabberControls, not an intentional design choice. Also checkedFrameGrabberControlsDC1394(uses a different but internally consistentconvention, no bug) and
FrameGrabberOf(noSETimplementation at all).To Reproduce
Steps to reproduce the behavior:
frameGrabber_nwc_yarptalking toframeGrabber_nws_yarp, wrappingfakeFrameGrabber(whosesetFeature/setActive/setMode/setOnePushstubs always return
false).ok == true, even thoughfakeFrameGrabber::setFeature()returned
false.Expected behavior
setFeature()/setActive()/setMode()/setOnePush()should returnfalsewhenever the underlying device's own implementation returnsfalse, matching the existing behavior ofsetRgbResolution()/setRgbFOV()/etc. inRgbVisualParams.Screenshots
None (protocol/logic bug, not visual).
Configuration (please complete the following information):
3.x releases using the same
framegrabber_protocolcode not checkedfurther back than v3.10.0)
Additional context
Not present on
master: this protocol was replaced entirely by aThrift/
ReturnValue-based RPC in PR #3184, which correctly propagates theresult. This issue and an accompanying PR are scoped to the
yarp-3.12stable branch only.
I have a fix ready (mirrors the existing
RgbVisualParams_Responder/Forwarderpattern) and a regression test usingfakeFrameGrabberwill open a PR referencing this issue shortly.
cc @randaz81