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

mavlink: include differential pressure in fields updated bit shift #9296

Merged
merged 1 commit into from
Apr 16, 2018

Conversation

tstastny
Copy link

@tstastny tstastny commented Apr 12, 2018

adds differential pressure to the bit shifts on the fields_updated integer. diff pres values are already shown on QGC, but the MAVROS imu plugin has a condition with the fields_updated int before publishing new values. See mavlink/mavros#1001

Only question would be if including the bit shift within the "air_data" timestamp condition (a separate topic) is actually the most proper thing to do, or should there be a separate check specifically for the differential_pressure topic. And if so, how best to do this when "air_data" lumps all four quantities into one group?

@tstastny tstastny added the bug label Apr 12, 2018
@tstastny tstastny self-assigned this Apr 12, 2018
@tstastny tstastny requested a review from dagar April 12, 2018 20:40
@TSC21
Copy link
Member

TSC21 commented Apr 12, 2018

@tstastny would this required any change on QGC side too?

@tstastny
Copy link
Author

@tstastny would this required any change on QGC side too?

I don't believe so. At least I already saw the differential pressure values published on QGC before making this change. I think QGC just publishes whatever values are there without checking the "fields_updated" int.

@TSC21
Copy link
Member

TSC21 commented Apr 12, 2018

Ok sounds good. Thanks for the change then.

TSC21
TSC21 previously approved these changes Apr 12, 2018
@@ -718,7 +718,7 @@ class MavlinkStreamHighresIMU : public MavlinkStream

if (_baro_timestamp != air_data.timestamp) {
/* mark last group dimensions as changed */
fields_updated |= (1 << 9) | (1 << 11) | (1 << 12);
fields_updated |= (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to be added below instead if differential_pressure updated?

Copy link
Author

Choose a reason for hiding this comment

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

right -- i asked about this in the initial comment. I was only uncertain exactly how you may wish implement this -- does it then need it's own timestamp corresponding to the diff pres updated? (instead of sharing with the baro timestamp and the other three members?)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, duplicate the _baro_timestamp usage for differential_pressure (see a few lines below).

Copy link
Member

@dagar dagar left a comment

Choose a reason for hiding this comment

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

Incorrectly marking diff_pressure field_updated.

@tstastny
Copy link
Author

@dagar ok - made specific timestamp / field update for dpres - tested with mavros - can see numbers.

@dagar
Copy link
Member

dagar commented Apr 16, 2018

That's weird, I'm not sure why Jenkins is unhappy. Can you rebase on current Firmware master?

Once it's passing this looks good to merge.

@dagar dagar merged commit 71170dc into PX4:master Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants