-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rethink consistency checker in control board NWS/YARP #2939
Comments
Perhaps the consistency check makes more sense for the |
@randaz81 would it be possible to consider this for the next release (3.9.x)? |
An example of the consistency checker causing NWS to stop streaming: |
I thought about this issue for quite some time, thinking about possible solutions (one at the end of this comment). Of course, code can be always improved and new checks can be introduced to make it more resilient to unexpected situations. However, let's analyze the issue from its fundamentals.
No. This is something that my old master discouraged me from doing always. Under no situation you should allow a robot to operate if its status is (partially) undefined because of hardware failures in one of its parts. Under no situation, you should fix in software a problem of a hardware nature, hiding the problem with a watchdog and possibly causing a malfunctioning of a higher layer of the stack. Consider a coordinated movement of multiple joints, a differential system, or a cartesian controller. What will be the final trajectory of the end effector, if one controller receives a position feedback older than 1 second? Now, considering possible improvements to the system, I'm currently working to improve return codes in Yarp interfaces and this is very related to your point 1:
A draft of PR is already open #3051, It was originally scheduled for yarp 3.10 but unfortunately, it is a pretty large change, and it will require some extra development time to be propagated in all affected devices (especially the motorcontroller which implements a huge amount of methods). I'll keep you updated on the advancements on this. |
Patch #2841 introduced a check in YARP 3.7.0 that, under certain circumstances, prevents controlBoard_nws_yarp from broadcasting state (ports
/state:o
and/stateExt:o
) to clients (namely its NWC counterpart, i.e. remote_controlboard). This encompasses encoder data as well as torques, currents, control modes, etc. The check itself consists of a comparison of timestamps as returned byIEncodersTimed::getEncodersTimed
. If any of these timestamps (each corresponding to one wrapped joint) is more than one second apart from the others, the check claims this situation is inconsistent and prevents from dispatching the port write further down in the code:yarp/src/devices/controlBoard_nws_yarp/ControlBoard_nws_yarp.cpp
Lines 511 to 520 in 920ecde
As I observed in #2862 (comment), this consistency checking can break some workflows:
The first point I'd like to bring up in this issue is: what was the rationale behind the introduction of this consistency check and what problems does it aim to solve?
In my opinion, disabling state broadcasting is a bit draconian (clients can no longer attend to robot state just because one single joint is off sync) and inconsistency could be handled differently, i.e. through return codes. These are already broadcast through the same port along with proper data (see Boolean fields here).
The
times
vector as it appears in the previous snippet is only used as the envelope of both state ports after averaging:yarp/src/devices/controlBoard_nws_yarp/ControlBoard_nws_yarp.cpp
Lines 522 to 527 in 920ecde
This envelope is read by the NWC counterpart, RemoteControlBoard, and stored in a class variable named
lastStamp
here. Clients that want to retrieve certain data (broadcast by said state ports, such as current torques) will trigger a local save of the timestamp enveloping the last state message: ref.Although this operation (retrieve time envelope) is repeated in several places throughout the NWC's code,
lastStamp
is only consumed (read from) in two situations:IEncodersTimed::{getEncoderTimed,getEncodersTimed}
andIMotorEncoders::{getMotorEncoderTimed,getMotorEncodersTimed}
. Please note the timestamps we get here are the average of all timestamps, contrary to the interface contract which says "stamps
: pointer to the array that will contain individual timestamps".IPreciselyTimed::getLastInputStamp
here. The contract specifies that this is the "time stamp relative to the last acquisition", which seems correct, but......I believe
lastStamp
is still being misused in both situations. In the former, since we don't expect an average stamp according to the documentation. In the latter, becauselastStamp
is also set whenever the client performs an RPC call that contains timestamp information, seegetTimestamp
. Example: if a client callsgetRefTorque()
, the NWC will internally updatelastStamp
(for further use as an average stamp of encoder data and last acquisition stamp; see the two points explained earlier) despite not having received any data through the state ports for a while. As far as I can guess the purpose behind the consistency checker, I believe this behavior invalidates it. Interestingly, the stamp returned by RPC calls, generated on the NWS side, is simply the current timestamp at the time said RPC call was performed: ref.Proposed course of action (depending on the first point: what was the purpose of the consistency checker)
getEncoder
and similar. This might require action on the implementor's side: if my joint is not responding, returnfalse
and let it flow through the NWS/NWC pair down to remote callers./state:o
and/stateExt:o
use the current stamp as the port envelope instead of averaging anything, consistently with the behavior of current RPC calls.VectorOfDouble stamps
field in thejointData
struct and populate it with the return values of the second out array parameter ingetEncodersTimed
.IPreciselyTimed::getLastInputStamp
: in order to be consistent with the contract, use the most recent stamp obtained in the previous point (assuming "time stamp relative to the last acquisition" means "the last time we got a response from the real hardware", not "the last time we called the raw subdevice").cc @randaz81
The text was updated successfully, but these errors were encountered: