Skip to content
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

Submitted models/robotika x2 sensor config 2 #380

Closed
wants to merge 9 commits into from

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Apr 9, 2020

Model submission of X2 sensor configuration 2 for Robotika
Add breadcrumbs to robotika_x2 to match default x2 configuration.

@caguero caguero self-requested a review June 12, 2020 13:59
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

I made a few suggestions and with them I was able to test it locally, deploy breadcrumbs and visualize your sensor information.

rviz_robotika

" <right_joint>rear_right_wheel_joint</right_joint>\n"\
" <wheel_separation>#{0.33559 * 1.23}</wheel_separation>\n"\
" <wheel_radius>0.098</wheel_radius>\n"\
" <topic>/model/#{_name}/cmd_vel_relay</topic>\n"\
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, consider adding this block to limit the maximum velocities and accelerations:

"      <min_velocity>-2</min_velocity>\n"\
"      <max_velocity>2</max_velocity>\n"\
"      <min_acceleration>-6</min_acceleration>\n"\
"      <max_acceleration>6</max_acceleration>\n"\

These parameters have been introduced here. You should also add it to the same file under robotica_x2_sensor_config_1.

@@ -0,0 +1,113 @@
def spawner(_name, _modelURI, _worldName, _x, _y, _z, _roll, _pitch, _yaw)
"<plugin name=\"ignition::launch::GazeboFactory\"\n"\
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace this line with "<spawn name=\"#{_name}\">\n"\

" <publish_sensor_pose>true</publish_sensor_pose>\n"\
" <publish_collision_pose>false</publish_collision_pose>\n"\
" <publish_visual_pose>false</publish_visual_pose>\n"\
" <publish_nested_model_pose>#{$enableGroundTruth}</publish_nested_model_pose>\n"\
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add the following block with parameters added since this pull request:

"      <use_pose_vector_msg>true</use_pose_vector_msg>\n"\
"      <static_publisher>true</static_publisher>\n"\
"      <static_update_frequency>1</static_update_frequency>\n"\

" </plugin>\n"\
" </include>\n"\
" </sdf>\n"\
"</plugin>\n"\
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to replace this line with </spawn>\n"\

pkg="ros_ign_bridge"
type="parameter_bridge"
name="ros_ign_bridge_pose"
args="/model/$(arg name)/pose@geometry_msgs/TransformStamped[ignition.msgs.Pose">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, update the arguments from #402:

args="/model/$(arg name)/pose@tf2_msgs/TFMessage[ignition.msgs.Pose_V">

name="ros_ign_bridge_pose"
args="/model/$(arg name)/pose@geometry_msgs/TransformStamped[ignition.msgs.Pose">
<remap from="/model/$(arg name)/pose" to="pose"/>
</node>
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to add the following node:

<node
  pkg="ros_ign_bridge"
  type="parameter_bridge"
  name="ros_ign_bridge_pose_static"
  args="/model/$(arg name)/pose_static@tf2_msgs/TFMessage[ignition.msgs.Pose_V">
  <remap from="/model/$(arg name)/pose_static" to="pose_static"/>
</node>

name="ros_ign_bridge_battery_state"
args="/model/$(arg name)/battery/linear_battery/state@sensor_msgs/BatteryState[ignition.msgs.BatteryState">
<remap from="/model/$(arg name)/battery/linear_battery/state" to="battery_state"/>
</node>
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to add the next node, otherwise you won't be able to deploy breadcrumbs:

<node
  pkg="ros_ign_bridge"
  type="parameter_bridge"
  name="ros_ign_bridge_breadcrumbs"
  args="/model/$(arg name)/breadcrumb/deploy@std_msgs/Empty]ignition.msgs.Empty">
  <remap from="/model/$(arg name)/breadcrumb/deploy" to="breadcrumb/deploy"/>
</node>

@nkoenig nkoenig changed the base branch from master to cave_feature_release3 June 15, 2020 14:57
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero
Copy link
Contributor

caguero commented Jun 18, 2020

I went ahead and made all the changes in b061be5.

@m3d
Copy link
Contributor

m3d commented Jun 18, 2020

@caguero thanks a lot - we are really not sure what to change to align with the latest X2 with breadcrumbs receivers. I know that it evolved ... thanks Martin (like the speeds used to be 2m/s)

@caguero
Copy link
Contributor

caguero commented Jun 18, 2020

@caguero thanks a lot - we are really not sure what to change to align with the latest X2 with breadcrumbs receivers. I know that it evolved ... thanks Martin (like the speeds used to be 2m/s)

No problem, feel free to do your own testing in case something is not working as expected.

@m3d
Copy link
Contributor

m3d commented Jun 18, 2020

OK, thanks. I added changes to the old config 1 (both are robots X2) as you suggested (#468)

@angelacmaio
Copy link
Contributor

As noted in this post, we did not provide a sensor configuration submission window for Cave Circuit in order to focus on new robot models. This PR is being declined because other competitors are also prohibited from submitting new sensor configurations for the Cave Circuit.

@zbynekwinkler
Copy link

This PR is being declined because other competitors are also prohibited from submitting new sensor configurations for the Cave Circuit.

The original announcement did not state that modifications are prohibited, only that new robots will have a priority over modifications of existing robots. It would really help if the communication was clearer since many people actually wasted their time on something that could have been cleared months ago.

@m3d
Copy link
Contributor

m3d commented Jul 31, 2020

Is there plan (yours) to add breadcrumbs also for "robotika X2" model from your side? I noticed that you extended Freyja and Kloubak K2 models as config2 (thanks for that), but this PR was rejected.
thanks
Martin/Robotika Team

@angelacmaio
Copy link
Contributor

We have added breadcrumbs to the new Systems robot platforms but do not plan to add more sensor configurations for X1-X4 platforms for the Cave Circuit.

@zbynekwinkler
Copy link

Can we request change of the plan? We would like to have breadcrumbs on ROBOTIKA_X2 and SSCI_X2.

@nkoenig nkoenig deleted the model_pr_430 branch December 10, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants