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

telemetry: add local position subscription #433

Merged
merged 3 commits into from
Jul 2, 2018

Conversation

Stifael
Copy link
Contributor

@Stifael Stifael commented Jun 28, 2018

In progress:
ToDo:

  • integration / unit tests

@Stifael Stifael force-pushed the telemetry-local-position branch from fe250f7 to 00d1c83 Compare June 28, 2018 10:09
@Stifael Stifael changed the title WIP telemetry: add local position subscription telemetry: add local position subscription Jun 28, 2018
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Nice work! Including tests and all :). My comments are mostly about the API and the doxygen comments.


void print_position_velocity_local(Telemetry::PositionVelocityLocal position_velocity_local)
{
std::cout << "Got position north_m: " << position_velocity_local.north_m << " m, "
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is the unit m at the end so you don't need to write north_m as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

{
std::cout << "Got position north_m: " << position_velocity_local.north_m << " m, "
<< "east_m: " << position_velocity_local.east_m << " m, "
<< "donw_m: " << position_velocity_local.down_m << " m" << std::endl
Copy link
Collaborator

Choose a reason for hiding this comment

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

donw -> down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@@ -43,6 +43,21 @@ class Telemetry : public PluginBase {
float relative_altitude_m; /**< @brief Altitude relative to takeoff altitude in metres */
};

/**
* @brief Position and velocity local type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please describe what it is for someone that doesn't know or understand PX4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

* @brief Position and velocity local type.
*/
struct PositionVelocityLocal {
float north_m; /**< @brief Position along north-direction of local frame in meters. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's clear what is meant with north_m and north_m_s.

I think it would make more sense to have the structure like this:

struct LocalPositionNED {
    float north_m;
    float east_m;
    float down_m
};
struct VelocityNED {
    float north_m_s;
    float east_m_s;
    float down_m_s;
};
struct LocalPositionAndVelocity {
    LocalPositionNED position;
    VelocityNED velocity;
};

and maybe you can even share some of the types with other structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the structure you suggested, but decided against using one of the other structures, despite the available velocity structure (GroundSpeedNED). The name GroundSpeedNED is contradicting because Speed implies that there is no direction information and the values always have to be positive (in contrast to velocity). However, NED implies the opposite that the quantity in question is velocity and not speed.

@@ -250,6 +273,14 @@ class Telemetry : public PluginBase {
*/
Result set_rate_rc_status(double rate_hz);

/**
* @brief Set rate of position velocity local updates (asynchronous).
Copy link
Collaborator

Choose a reason for hiding this comment

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

"position velocity local updates" is not clear what it means. The comments need to be proper sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

- rename struct from PositionVelocityLocal to PositionVelocityNED
- PositionVelocityNED based on PositionNED and VelocityNED struct
- Improve comments
@Stifael Stifael force-pushed the telemetry-local-position branch from d21fd41 to 1f21f77 Compare June 29, 2018 07:15
@julianoes
Copy link
Collaborator

Thanks @Stifael, I think this is good to go in.

@julianoes julianoes merged commit 8885cf5 into mavlink:develop Jul 2, 2018
rt-2pm2 pushed a commit to rt-2pm2/DronecodeSDK that referenced this pull request Nov 27, 2018
telemetry: add local position subscription
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.

2 participants