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

Extras: add ardupilot rangefinder plugin #746

Merged
merged 1 commit into from
Jul 2, 2017

Conversation

khancyr
Copy link
Contributor

@khancyr khancyr commented Jul 2, 2017

This add ardupilot rangefinder message support. It will be deprecated with next version that support DISTANCE_SENSOR msg

Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

@khancyr I don't think that adding a new plugin would be the best choice. Adding support to this msg would be better on distance_sensor plugin (the plugin name doesn't have to be directly related to its message). Besides, as you said, this would be deprecated.

Copy link
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

Perhaps adding parameters for frame_id FoV etc may be good. But LGTM.

@vooon vooon merged commit 9494c26 into mavlink:master Jul 2, 2017
@vooon vooon added the plugin label Jul 2, 2017
@vooon vooon added this to the Version 0.20 milestone Jul 2, 2017
@TSC21
Copy link
Member

TSC21 commented Jul 2, 2017

I don't like the approach msg_type<->plugin. I think it's better purpose<->plugin, but anyway it is your call. Besides, this plugin will only allow receiving msg, no sending as distance_sensor allows. Anyways, this is a addition to a soon deprecated msg, so should be added to blacklist at that time.

@khancyr
Copy link
Contributor Author

khancyr commented Jul 2, 2017

@TSC21 I agree I could have put it in distance_sensor file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants