-
Notifications
You must be signed in to change notification settings - Fork 990
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
DISTANCE_SENSOR support #292
Conversation
dist_laser_in_pub = dist_nh.advertise<mavros_extras::LaserDistanceSensor>("laser_distance", 10); | ||
else if (rangefinder_type == 1) | ||
dist_sonar_in_pub = dist_nh.advertise<mavros_extras::SonarDistanceSensor>("sonar_distance", 10); | ||
else ROS_ERROR_NAMED("rangefinder", "Invalid rangefinder type! Valid values are 0 (laser) and 1 (ultrasound)!"); |
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.
Fix log name.
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.
Got it!
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.
Wait, what's the problem with the log name? :D
Did you used uncrustify? Also i don't understand why you need non-standard messages. Also i think that LASER type (LidarLite) is more like INFRARED range finders, rather than LaserScan sources. |
|
|
Hmm, also i think we may ask for adding LASER range finder to Range enum. |
Ok I will move to Range.msg then. Redefine map in params is what I want to do. Can you give an hand here? About the Range enum, how can we ask that? |
*/ | ||
void send_sonar_dist_sensor_data(const ros::Time &stamp, uint16_t min_distance, | ||
uint16_t max_distance, uint16_t current_distance) { | ||
distance_sensor(stamp.toNSec() / 1000000, |
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.
Should uas->synchronise_stamp()
also be used here and how?
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.
No, synchronize needed for data comes from FCU. To FCU send current time (in us or ms).
Asked at ros-users: http://lists.ros.org/pipermail/ros-users/2015-May/069403.html |
@vooon changes done. Using |
|
||
uint8 id # XXX FIXME enum ?? | ||
|
||
uint8 orientation # rad |
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.
Rad? I don't think so. Uint8 is more likely orientation enum used in params for ahrs.
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.
Probably. Will change it then
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.
This also should be captured in the frame_id using tf information.
I still think that new message not needed. Only one really needed data is variance. May be better do following param structure: distance_sensor:
sensor_topic_name:
id: 0 # sensor id
frame_id: some_frame
other_sensor_topic:
id: 1
frame_id: other_frame
subscribed_topic:
subscribe: true
id: 42
type: 0 # we may try to guess by Range.radiation_type
variance: ???
orientation: 0 Pro:
|
What do you think? 2.I was considering dynamically add/delete publishers, depending on the rangefinder type and its id of the data coming from the FCU. But I'm not really sure how to do it. Can you give a hand here? |
BTW, here's the RViz representation you were talking about: http://wiki.ros.org/rviz/DisplayTypes/Range |
@TSC21 You don't need to use orientation, because it already coded in frame_id + transform (i think from URDF). Maybe for now we may drop variance? Or show in diagnostics. At least until someone not need it. I think instead of dynamic add/del better specify topics in config file (px4_config.yaml) and provide mapping to id (plus warning messages about unmapped id's). |
|
||
sensor_msgs/Range range | ||
|
||
uint8 type # see MAV_DISTANCE_SENSOR enum |
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.
This is already in the Range message
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.
Yes you are right. But since it won't be used, I will delete this custom msg. Anyway, it's useful info for future implementation, so thanks :)
I agree with @vooon to not create a new message datatype. The variance is not currently captured in the Range message, but that should not be a problem to parameterize properly when processing. If we need it we can consider adding it to the Range message, however from experience it tends to be application specific more than device specific. Similarly the LaserScan does not have variance/covariance. |
@TSC21 i'm done. But it little incomplete, but not that not big deal. Example test config: mavros:
plugin_whitelist: ["sys_*", distance_sensor]
distance_sensor:
topic_pub_1:
id: 0
frame_id: sensor_1
topic_pub_2:
id: 1
frame_id: sensor_2
orientation: 2
topic_sub_1:
subscriber: true
id: 2
orientation: 2 But i don't tested recv/send (need FCU or emulator). |
@vooon why have you changed all the code structure? Am I supposed to pick up what you've done a add something to it? Because from what I can see you done a new PR all by yourself and mine almost disappeared... |
@TSC21 because i done param to map, which requires many instances of pub/sub |
OK... but then I think the colaborative part somewhat disappeared on this. I want to give some contribution but like this is like doing nothing. You can add it as your own PR then. |
Sorry, but i done what i mentioned yesterday (topic-id-frame_id mapping). |
Ok. Sent you a PM in GHangout regarding this. Anyway, I'll see what I can do about other stuff. Also I don't know why you deleted covariance publishing. We didn't agreed to drop it down. |
@vooon, regarding this new config:
If in the |
Subscriber not used frame_id, just use right topic ( |
OK, understood. Adding some enhancements to code now (variance calculation, etc.). Then I'll do PR. |
Trying to troubleshoot an error, which is not allowing me to load the
(I put the min/max range and fov as needed param for pub). I get this when launching the node:
Solutions? |
Small note : The PX4Flow sonar doesn't publish to the PX4 Remove the HRLV-EZ4 please, or follow up with a PR to Firmware which publishes the sonar to the distance sensor topic and also the required modifications to all the estimators to use the topic. |
I'm aware of it thanks ;)
It's my next step. Also I'm planning to create a .h with various sensor definitions to be used in the ROS config. Anyway, @mhkabir do you have any idea of what can be firing the errors above? |
@TSC21 try |
|
OK I installed |
Well, it is solved. Had to clone repo again and add changes. Will do a PR now. Update: nope! Apparently not. I did a clean and build again and it show this errors again. Really don't know the problem at all. Will it help send a PR so you can check the error, @vooon? |
Commit changes to some branch. |
Commented in diff. Main problem source is static class members you added. Why you ever need static? |
Code is done! Tested the readings with a
rosserial_python
node which was retrieving data from an Arduino into a Range type of message, which is read by the plugin usage. So now this only lacks implementation on the FCU side, which includes receive and send and publish/advertise it a DISTANCE_SENSOR uOrb topic.