Fixes for using generate-state_database #1412
Conversation
|
Please target the |
|
This pull request is in conflict. Could you fix it @ejalaa12? |
ejalaa12
force-pushed
the
bug-generate-state-database
branch
from
de7ab12
to
c04f9ac
Compare
Codecov Report
@@ Coverage Diff @@
## main #1412 +/- ##
==========================================
+ Coverage 50.99% 51.00% +0.01%
==========================================
Files 380 380
Lines 31743 31744 +1
==========================================
+ Hits 16184 16187 +3
+ Misses 15559 15557 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
@ejalaa12 thanks a lot for your fixes. Could you run pre-commit to fix formatting and ideally rebase the three fixes as three individual commits on main?
| @@ -537,7 +537,7 @@ bool constructConstraints(const rclcpp::Node::SharedPtr& node, const std::string | |||
| return false; | |||
|
|
|||
| for (auto& constraint_id : constraint_ids) | |||
| constraint_id.insert(0, constraints_param); | |||
| constraint_id.insert(0, constraints_param + "."); | |||
There was a problem hiding this comment.
If you add "." here, wouldn't it be redundant in line 488 etc?
There was a problem hiding this comment.
constraint_param is passed as parameter to the function, and its value is constraints (see here and here)
The purpose of this for-loop is to prefix the contraint_id with the correct param "namespace".
Assuming that before the for-loop,
constraint_ids = ["contraintA", "constraintB"];After the loop, and without the fix, it becomes:
contraint_ids = ["contraintscontraintA", "contraintsconstraintB"];Instead of (with the fix)
contraint_ids = ["contraints.contraintA", "contraints.constraintB"];Line 488, also prefix (again) those constraint_ids, without the second fix, it becomes
contraint_ids = [".constraints.contraints.contraintA", ".constraints.contraints.constraintB"];where the first . is not necessary, that's why I removed it.
I find the second prefixing (inside collectConstraints) confusing anyway. I guess it all boils down to how should the yaml file be structured.
Do you guys have an example for this file ?
There was a problem hiding this comment.
To continue on my last remark, we could have the yaml look like this:
generate_state_database:
ros__parameters:
planning_group: mygroup
use_current_scene: false
constraints:
name: some_constraints
constraint_ids: [constraintA]
constraintA:
type: joint
frame_id: base_link
joint_name: jr_link3_robot
position: 0.0
tolerance: 0.3
weight: 1.0In which case, the for-loop in the constructConstraints method would look like this:
for (auto& constraint_id : constraint_ids)
constraint_id.insert(0, constraints_param + ".");and inside the collectConstraint method we change line 479 to
const auto constraint_param = constraint_id;instead of const auto constraint_param = ".constraints." + constraint_id
henningkayser
added
backport-humble
ejalaa12
force-pushed
the
bug-generate-state-database
branch
from
f7e859e
to
cf714d5
Compare
|
Hey @henningkayser, thanks for the review. I've rebased my changes onto main, and squashed them to have one commit per fix. |
On galactic the generate_state_database script is not working.
This PR solves 3 issues that were blocking it from working properly.
Issue 1: planning_group parameter not declared
To reproduce simply run the generate_state_database node as follows
The error you get is
This happens because none of the parameters that this node uses are declared before the
get_parametermethod is called.The first commit simply adds the
automatically_declare_paramters_from_override(true)to the node option.This should be fine for such an isolated script.
Issue 2: planning constraints parameters are not found
Once the previous issue is fixed, you get the following error, even with the parameters passed correctly.
The params file being:
This is because the constraints parameters are not looked up correctly. The second commit fixes that.
However I'm not 100% sure about what format what expected by the constraint loader. After my fix, the above param file now works.
Issue 3: Failed to serialize message when writing the database file
Once issue2 is fixed, we get the following error:
This is because the
rmw_serialized_message_tis not instantiated correctly (see here).Third commit fixes it.