Skip to content

fix check motion done + remove warning #3321

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.

Sign up for GitHub

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

Open
valegagge wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −6
Open

fix check motion done + remove warning #3321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/devices/networkWrappers/RemoteControlBoard/RemoteControlBoard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2905,7 +2905,7 @@ yarp::dev::ReturnValue RemoteControlBoard::isJointBraked(int j, bool& braked) co
//std::lock_guard<std::mutex> lg(m_mutex);
auto ret = m_RPC.isJointBrakedRPC(j);
if (!ret.ret) {
yCError(REMOTECONTROLBOARD, "Unable to isJointBraked");
//yCError(REMOTECONTROLBOARD, "Unable to isJointBraked");
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Commenting out error logs is generally not a good practice as it can hide underlying issues and make debugging more difficult. If this error is being triggered frequently for non-critical reasons (e.g., an optional interface not being present), consider downgrading it to yCWarning or yCDebug. Alternatively, the calling code could handle this failure more gracefully, perhaps by checking for the interface's availability once and then avoiding repeated calls.

        yCError(REMOTECONTROLBOARD, "Unable to isJointBraked");

return ret.ret;
}
braked = ret.isBraked;
Expand Down
2 changes: 1 addition & 1 deletion src/devices/networkWrappers/controlBoard_nws_yarp/ControlBoardServerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ return_isJointBraked ControlBoardRPCd::isJointBrakedRPC(const std::int32_t j) co
ret.ret = m_iJointBrake->isJointBraked(j, ret.isBraked);
if (!ret.ret)
{
yCError(CB_RPC, "isJointBraked() failed");
//yCError(CB_RPC, "isJointBraked() failed");
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Silencing error logs can make it difficult to diagnose problems. This log indicates a failure in the underlying isJointBraked() implementation. If this failure is expected in some cases (e.g., the interface is optional), it might be better to handle it by logging at a lower severity level like yCWarning or yCDebug. Simply commenting it out hides the issue entirely.

Suggested change
//yCError(CB_RPC, "isJointBraked() failed");
yCError(CB_RPC, "isJointBraked() failed");

}
return ret;
}
Expand Down
2 changes: 1 addition & 1 deletion src/devices/networkWrappers/controlBoard_nws_yarp/ControlBoard_nws_yarp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ bool ControlBoard_nws_yarp::setDevice(yarp::dev::DeviceDriver* driver, bool owne

subdevice_ptr->view(iJointBrake);
if (!iJointBrake) {
yCWarning(CONTROLBOARD, "Part <%s>: iJointBrake interface was not found in subdevice.", partName.c_str());
yCError(CONTROLBOARD, "Part <%s>: iJointBrake interface was not found in subdevice.", partName.c_str());
}

subdevice_ptr->view(iVelocityDirect);
Expand Down
6 changes: 3 additions & 3 deletions src/guis/yarpmotorgui/partitem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2339,8 +2339,8 @@ bool PartItem::updatePart()
if (m_iPos)
{
bool boolval = true;
m_iPos->checkMotionDone(m_slow_k, &boolval); // using k to save bandwidth
m_done[m_slow_k] = boolval;
ret_motdone = m_iPos->checkMotionDone(m_slow_k, &boolval); // using k to save bandwidth
m_done[m_slow_k] = boolval; // @randaz: are you sure about set true if ret_motdone==false ?
Comment on lines +2342 to +2343
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The value of the output parameter boolval is only guaranteed to be valid if checkMotionDone returns a success code. The current implementation assigns boolval to m_done[m_slow_k] regardless of the return value, which can lead to an incorrect state if the call fails. The assignment should be conditional on the success of the function call. Also, the comment left for @randaz should be removed and replaced with the correct logic.

        ret_motdone = m_iPos->checkMotionDone(m_slow_k, &boolval); // using k to save bandwidth
        if (ret_motdone)
        {
            m_done[m_slow_k] = boolval;
        }

}
if (m_ijointbrake)
{
Expand Down Expand Up @@ -2392,7 +2392,7 @@ bool PartItem::updatePart()
}
if (!ret_jntbrk)
{
yWarning() << "Missing Implementation of isJointBraked()";
// yWarning() << "Missing Implementation of isJointBraked()";
Copy link

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Commenting out this warning hides a recurring issue. If isJointBraked() is failing because the feature is not available on some hardware, the GUI should detect this and avoid calling it repeatedly in the update loop. A better approach would be to log the warning once and then set a flag to disable future calls for that part. For example, you could add a member variable m_jointBrakeIsAvailable to PartItem, initialized to true. If isJointBraked() fails, set this flag to false and log a warning. In subsequent updates, only call isJointBraked() if the flag is true.

        yWarning() << "Missing Implementation of isJointBraked()";

}

// *** update the widget every cycle ***
Expand Down
Loading