Warning message for position control PID when config not consistent #716

AlexAntn · 2021-02-03T17:04:55Z

This pull request introduces a warning message when the configuration of position control PID is not consistent across the different joints, resulting in wrong values for the PIDs (mismatching between machine_units and metric_units).

This PR addresses this issue.

@randaz81 : This implementation covers only position control, since the configuration for other types of control is slightly different. Should this be implemented for all different types of control?

…is not consistent across joints

pattacini · 2021-02-03T21:46:30Z

cc @lrapetti

pattacini

A few hints.

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

lrapetti · 2021-02-04T08:17:39Z

Thanks @AlexAntn for taking care.

@randaz81 : This implementation covers only position control, since the configuration for other types of control is slightly different. Should this be implemented for all different types of control?

If I can give my opinion regarding this point, in general I think this needs to be implemented for all the control types. For example, my issue was opened after I experienced that problem in torque control mode.

pattacini · 2021-02-04T08:59:06Z

If I can give my opinion regarding this point, in general I think this needs to be implemented for all the control types. For example, my issue was opened after I experienced that problem in torque control mode.

But the case of torque PID should have been already covered if I got it right (cc @AlexAntn).
Maybe because of the logic I pointed out here the warning wasn't generated.

lrapetti · 2021-02-04T09:06:30Z

If I can give my opinion regarding this point, in general I think this needs to be implemented for all the control types. For example, my issue was opened after I experienced that problem in torque control mode.

But the case of torque PID should have been already covered if I got it right (cc @AlexAntn).
Maybe because of the logic I pointed out here the warning wasn't generated.

Should it be a Warning or should it return an Error? I did't notice anything, but it could be possible I was missing the Warning.

AlexAntn · 2021-02-04T09:15:44Z

If I can give my opinion regarding this point, in general I think this needs to be implemented for all the control types. For example, my issue was opened after I experienced that problem in torque control mode.

But the case of torque PID should have been already covered if I got it right (cc @AlexAntn).
Maybe because of the logic I pointed out here the warning wasn't generated.

Should it be a Warning or should it return an Error? I did't notice anything, but it could be possible I was missing the Warning.

The message is an error (for both torque and position control), not a warning, so my guess is it would be very visible if it showed up. As @pattacini mentioned, there is an error in the logic of the code (regarding the firstjoint), which would lead to no message being printed in any case. I will fix it and push.

pattacini · 2021-02-04T09:30:33Z

which would lead to no message being printed in any case

In some cases, not all, but I think fixing this will make sure to deal with the correct joint number.

…d the previous error messages (for torque and position control)

AlexAntn · 2021-02-04T10:33:41Z

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

+            }
+        }
+    }
+


Here I introduced an error message for current control type

AlexAntn · 2021-02-04T10:34:00Z

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

+            }
+        }
+    }
+


here I introduced a similar message for speed control type

AlexAntn · 2021-02-04T10:39:36Z

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

@@ -1246,8 +1310,6 @@ bool Parser::getCorrectPidForEachJoint(PidInfo *ppids/*, PidInfo *vpids*/, TrqPi
        //eomc_ctrl_out_type_vel = 2,
        //eomc_ctrl_out_type_cur = 3

-    return true;
-


We noticed there was this return before any of the error messages were printed, was this intentional or a small bug, @randaz81 ?

Nice catch @AlexAntn!
Because of this, there was no hope to see error messages popping up.

AlexAntn · 2021-02-04T10:40:18Z

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

            firstjoint = i;
+            break;
+        }


Introduced a break on the firstjoint loop, so now it correctly gets the first joint (instead of the last enabled one)

AlexAntn · 2021-02-04T10:41:40Z

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

+            if(ppids[firstjoint].fbk_PidUnits != ppids[i].fbk_PidUnits ||
+               ppids[firstjoint].out_PidUnits != ppids[i].out_PidUnits)
+            {
+                yError() << "embObjMC BOARD " << _boardname << "all joints with POSITION enabled should have same controlunits type. Joint " << firstjoint << " differs from joint " << i;


finally, added the error message for position control (not just torque). Also, made the origin of the messages more evident by putting in caps (e.g.: POSITION) the type of control that is reporting the error

Co-authored-by: Ugo Pattacini <ugo.pattacini@iit.it>

pattacini · 2021-02-05T10:20:11Z

@randaz81 @lrapetti the PR is somewhat ready for your reviews.

randaz81 · 2021-02-09T23:30:43Z

Nice fix!
Just an additional question: at first glance it seems to me that the code of that performs the check is identical in all the three different cases (only the output string changes?). If this is the case, maybe a more compact solution to avoid code duplication (if in the future additional control blocks are added) could be to have a call to a common method that takes in input the pid and its name (string) and returns true/false to the upper level? Just an idea..

pattacini · 2021-02-10T07:36:53Z

I think it's valuable feedback @randaz81 👍🏻
Thanks!

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

AlexAntn · 2021-02-10T15:37:25Z

As @randaz81 suggested, I put the print into a function that is now called (instead of having the same code spread all over).

pattacini

Minor comments

src/libraries/icubmod/embObjMotionControl/eomcParser.h

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp

pattacini · 2021-02-10T16:41:11Z

Thanks @AlexAntn 👍🏻
I'll merge the PR once checks are passed.

lrapetti · 2021-02-10T17:19:18Z

Thanks all!

…obotology#716)

added warning message to position control PID when the configuration …

5a63e9e

…is not consistent across joints

pattacini assigned AlexAntn Feb 3, 2021

pattacini requested a review from randaz81 February 3, 2021 21:45

pattacini reviewed Feb 3, 2021

View reviewed changes

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp Outdated Show resolved Hide resolved

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp Outdated Show resolved Hide resolved

added the unit types error message to current and speed control; fixe…

4f58884

…d the previous error messages (for torque and position control)

AlexAntn commented Feb 4, 2021

View reviewed changes

made error messages due to unit types mismatch more evident

3129ab8

AlexAntn commented Feb 4, 2021

View reviewed changes

revert character change in a comment

0f1b87a

Co-authored-by: Ugo Pattacini <ugo.pattacini@iit.it>

pattacini requested a review from lrapetti February 5, 2021 10:19

pattacini approved these changes Feb 5, 2021

View reviewed changes

pattacini linked an issue Feb 10, 2021 that may be closed by this pull request

embObjMotionControl: issue in CONTROL configuration parsing #712

Closed

lrapetti reviewed Feb 10, 2021

View reviewed changes

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp Show resolved Hide resolved

lrapetti approved these changes Feb 10, 2021

View reviewed changes

AlexAntn and others added 2 commits February 10, 2021 16:29

merged all the prints into a single function (checkJointTypes)

2452165

Inserted new-line at the end

f6b1b41

pattacini requested changes Feb 10, 2021

View reviewed changes

src/libraries/icubmod/embObjMotionControl/eomcParser.h Outdated Show resolved Hide resolved

src/libraries/icubmod/embObjMotionControl/eomcParser.cpp Outdated Show resolved Hide resolved

AlexAntn added 2 commits February 10, 2021 17:22

changed the declaration of pid_type to const std::string

3f0b1c3

passing pid_type by reference

67036c6

pattacini approved these changes Feb 10, 2021

View reviewed changes

pattacini merged commit 122e11a into robotology:master Feb 10, 2021

Nicogene mentioned this pull request Feb 23, 2021

embobjMotionControl fails to open after adding the consistency check of torque PIDs #718

Closed

AlexAntn added a commit to AlexAntn/icub-main that referenced this pull request Mar 3, 2021

Warning message for position control PID when config not consistent (r…

c47d12b

…obotology#716)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning message for position control PID when config not consistent #716

Warning message for position control PID when config not consistent #716

AlexAntn commented Feb 3, 2021 •

edited by pattacini

Loading

pattacini commented Feb 3, 2021

pattacini left a comment

lrapetti commented Feb 4, 2021

pattacini commented Feb 4, 2021 •

edited

Loading

lrapetti commented Feb 4, 2021

AlexAntn commented Feb 4, 2021

pattacini commented Feb 4, 2021

AlexAntn Feb 4, 2021

AlexAntn Feb 4, 2021

AlexAntn Feb 4, 2021

pattacini Feb 4, 2021 •

edited

Loading

AlexAntn Feb 4, 2021

AlexAntn Feb 4, 2021

pattacini commented Feb 5, 2021

randaz81 commented Feb 9, 2021

pattacini commented Feb 10, 2021 •

edited

Loading

AlexAntn commented Feb 10, 2021

pattacini left a comment

pattacini commented Feb 10, 2021

lrapetti commented Feb 10, 2021

Warning message for position control PID when config not consistent #716

Warning message for position control PID when config not consistent #716

Conversation

AlexAntn commented Feb 3, 2021 • edited by pattacini Loading

pattacini commented Feb 3, 2021

pattacini left a comment

Choose a reason for hiding this comment

lrapetti commented Feb 4, 2021

pattacini commented Feb 4, 2021 • edited Loading

lrapetti commented Feb 4, 2021

AlexAntn commented Feb 4, 2021

pattacini commented Feb 4, 2021

AlexAntn Feb 4, 2021

Choose a reason for hiding this comment

AlexAntn Feb 4, 2021

Choose a reason for hiding this comment

AlexAntn Feb 4, 2021

Choose a reason for hiding this comment

pattacini Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

AlexAntn Feb 4, 2021

Choose a reason for hiding this comment

AlexAntn Feb 4, 2021

Choose a reason for hiding this comment

pattacini commented Feb 5, 2021

randaz81 commented Feb 9, 2021

pattacini commented Feb 10, 2021 • edited Loading

AlexAntn commented Feb 10, 2021

pattacini left a comment

Choose a reason for hiding this comment

pattacini commented Feb 10, 2021

lrapetti commented Feb 10, 2021

AlexAntn commented Feb 3, 2021 •

edited by pattacini

Loading

pattacini commented Feb 4, 2021 •

edited

Loading

pattacini Feb 4, 2021 •

edited

Loading

pattacini commented Feb 10, 2021 •

edited

Loading