-
Notifications
You must be signed in to change notification settings - Fork 196
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
Removed version0 parameters (as much as possible) #89
Removed version0 parameters (as much as possible) #89
Conversation
…multi-group parameter Removed need for state yaml file, uses multi-group parameter
…sed on the existance of parameters
…licated in motion_interface.yaml)
…arameter to select joint_trajectory_action node
…roup compatable nodes are launched
…parameter, which is confusing
@ipa-mirb, can someone at IPA test this PR your sda10f? The controller side should stay as is. |
* | ||
* \return true on success, false if parameter not found | ||
*/ | ||
bool getJointGroups(std::string topic_param, std::map<int, RobotGroup> robot_groups); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think robot_groups
should be a reference, as it's now passed by value. This causes callers to never get their robot_groups
variables updated by this method, resulting in zero groups everywhere.
I quickly ran into an
I think I tracked it down to these lines (from here): this->pub_controls_[robot_id].publish(*control_state);
this->pub_states_[robot_id].publish(*sensor_state); Commenting out these lines lets the node continue, but I don't get any messages out of it (which makes sense, since I disabled publishing). But I can't really understand why it's crashing there. Afaict, this PR doesn't change anything connected to these lines (the publishers in I haven't spent too much time trying to figure out what is going on, so any ideas? |
@gavanderhoorn: The numbering of these should start with 0, so when the joint states of the last group needs to be published, it tries to index a publisher which does not exist. Once this problem is resolved, the next problem (which is also present in
This can be surpressed by commenting line 89 of |
ah. So it uses the |
That seems like it is the case. As for the other problem, a workaround/fix has been implemented here: We should probably put that into another PR with the complete description of what is happening. |
@shaun-edwards: the assertion I reported is not related to this PR, and we should probably move the discussion to a separate issue. |
@jettan, @shaun-edwards: I've created #91 for the issue observed by @jettan. |
@gavanderhoorn, @jettan once I fix the pass-by-reference (newbie) mistake ;), can this be merged or should it wait on #91? |
imo it can be merged after the pass by reference fix |
Yes, this can be merged. Afaict this is actually a pretty nice clean-up, although without unit-tests we can't know for sure there aren't any regressions introduced by this. I only have access to a single system, and it's not a dual-arm, so I can't say anything about the changes in this PR when used with a dual-arm setup. |
@gavanderhoorn, corrected my issue and change has been tested on a second multi-group setup (not dual arm though) |
Ok to merge? |
Ok for me |
Thanks @jettan, merging. Thanks for all the review help. In combination with a few more PR's, we should have an |
The
version0
parameter is no longer required for joint streaming/state nodes. The nodes determine whether a multi-group message should be handled based on whether groups are specified via parameter.The
joint_trajectory_action
did not change behavior based on theversion0
parameter as I expected. Instead, different nodes are started when multiple group control is required. I created a new launchrobot_multigroup_interface_streaming_<controller>.launch
to hide this detail from the user.If/when this PR is accepted, the multi-group tutorial should be updated.