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

Add support to DISTANCE_SENSOR msg coder/decoder #264

Closed
TSC21 opened this issue Apr 4, 2015 · 32 comments
Closed

Add support to DISTANCE_SENSOR msg coder/decoder #264

TSC21 opened this issue Apr 4, 2015 · 32 comments

Comments

@TSC21
Copy link
Member

TSC21 commented Apr 4, 2015

Related to PX4/PX4-Autopilot#1986.

@TSC21
Copy link
Member Author

TSC21 commented Apr 4, 2015

Ideally, this has the following features:

  • rosserial connection
  • DISTANCE_SENSOR coder (decoder may only make sense if sensor is connected on the FCU)

@mhkabir
Copy link
Member

mhkabir commented Apr 5, 2015

A decoder probably makes more sense since it is better to connect whatever low level peripherals you want (e.g Laser ranger) to your flight controller.
Not much point using the companion computer for this. It would use up a serial port too.
And serial isn't the best bus for external sensors. Use a more appropriate bus (SPI, UAVCAN) for interfacing external sensors.

For the decoder to ROS, see the flow plugin. Add a appropriate handler in a new plugin and send in a a PR :D

@mhkabir
Copy link
Member

mhkabir commented Apr 5, 2015

PX4 implementation would involve adding a mavlink message stream to read from the distance sensor uORB topic.

@TSC21
Copy link
Member Author

TSC21 commented Apr 5, 2015

@mhkabir thanks for your inputs. I wasn't really asking to this to be implemented, more like this is an ongoing implementation where ideas are welcomed :D
The decoder is certainly easy to get done, also as the coder. The PX4 implementation though can involve adding support to this message I think.

@vooon
Copy link
Member

vooon commented May 13, 2015

Plugin almost done, by #292 . Need test and fix todo.

@vooon
Copy link
Member

vooon commented May 13, 2015

(#292 merged, my fixes also uses that number. Perhaps better use this issue, but too late)

@vooon vooon added this to the Version 0.12 milestone May 13, 2015
@TSC21
Copy link
Member Author

TSC21 commented May 17, 2015

@vooon new sensor type was added: https://github.com/mavlink/mavlink/pull/381/files. So, ideally @tfoote could add LASER radiation_type to Range.msg enum - reason: as I explained in #292 (comment).
Then, it can be added here: https://github.com/TSC21/mavros/blob/external_sensors_connect/mavros_extras/src/plugins/distance_sensor.cpp#L213

@TSC21
Copy link
Member Author

TSC21 commented May 17, 2015

Another thing: mavlink/mavlink#383 - can this enum be added then to /mavros/mavros/include/mavros in sensor_orientation.h or similar, @vooon?

@vooon
Copy link
Member

vooon commented May 17, 2015

@TSC21 as soon as it merged i will do release of mavlink package.

@TSC21
Copy link
Member Author

TSC21 commented May 17, 2015

@TSC21
Copy link
Member Author

TSC21 commented May 17, 2015

Update: #296 (comment)

@vooon
Copy link
Member

vooon commented May 17, 2015

Make a std::array with orientations then choose one by orientation enum.
Also add param for setting parent frame_id, child frame_id same as in Range.header.

As i understand tf: you don't need actually rotate FCU frame_id, only publish sensor orientation value.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

OK I think I will do this in a next PR. You can merge this one in the mean time, if everything's ok.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

@vooon mavlink/mavlink#383 was merged. You can do mavlink release now.

@vooon
Copy link
Member

vooon commented May 18, 2015

Already done.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

Also add param for setting parent frame_id, child frame_id same as in Range.header.

Why do we need parent frame_id, if we only want tf between fcu and the sensor? Doesn't make sense to add other tfs, I think.

As i understand tf: you don't need actually rotate FCU frame_id, only publish sensor orientation value.

We don't rotate fcu frame. We rotate the sensor id accoriding to the enum and then the tf between the fcu and the sensor is published, with the proper orientation of the sensor being updated.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

And do you merge now or are you waiting for the orientation stuff?

@vooon
Copy link
Member

vooon commented May 18, 2015

Waiting orientation and test results.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

Results from which side? Cause ROS side working ok. I'm getting the PX4 side worked on now.

@vooon
Copy link
Member

vooon commented May 18, 2015

Ok, if you say that plugin works, it's enough for now. I can merge current PR if you need that. But probably better merge when all features done.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

No need to merge, unless some other PR is added. I'm advancing to create the std::array sensor_orientation. One question: isn't it better to add it to uas definition, so to be used by other plugins if needed?

@vooon
Copy link
Member

vooon commented May 18, 2015

Because it not depend on uas data it may be simple helper function in
libmavros. Please make new cpp.
18.05.2015 23:29 пользователь "TSC21" notifications@github.com написал:

No need to merge, unless some other PR is added. I'm advancing to create
the std::array sensor_orientation. One question: isn't it better to add
it to uas definition, so to be used by other plugins if needed?


Reply to this email directly or view it on GitHub
#264 (comment).

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

@vooon
Copy link
Member

vooon commented May 18, 2015

I mean src/lib, and add function definition to utils.h
19.05.2015 0:24 пользователь "TSC21" notifications@github.com написал:

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

so what do I put in /src/lib/sensor_orientation.cpp and what do I put in /include/mavros/utils.h?
And then, what do I include in distance_sensor.cpp? <mavros/utils.h>?

I really can't state the use of a cpp in /src/lib/. Isn't it enough to just put the helper function definition in utils.h and then call the function on distance_sensor.cpp, having put #include <mavros/utils.h> in file header?

@vooon
Copy link
Member

vooon commented May 18, 2015

C++ and C split definition and implementation in .h and .cpp (.c) files. So you should put function definition in utils.h and implementation in /src/lib/sensor_orientation.cpp.

Then just use <mavros/utils.h>.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

C++ and C split definition and implementation in .h and .cpp (.c) files. So you should put function definition in utils.h and implementation in /src/lib/sensor_orientation.cpp.

Then just use <mavros/utils.h>.

OK that was what I was already doing :) thanks

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

In utils.h, do I have to use inline, since the method implementation is in /src/lib/sensor_orientation.cpp?

@vooon
Copy link
Member

vooon commented May 18, 2015

No! Inline for .h files. And there regular function, so no static, no inline no constexpr.

@TSC21
Copy link
Member Author

TSC21 commented May 18, 2015

Willco 👍

@TSC21
Copy link
Member Author

TSC21 commented Jun 11, 2015

This is tested and working ;) so can be closed, since #292 has been merged!

@TSC21 TSC21 closed this as completed Jun 11, 2015
nmichael pushed a commit to rislab/mavros that referenced this issue Mar 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants