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

mavros_extras: Fix buggy check for lat/lon ignored #1805

Merged
merged 1 commit into from
Nov 27, 2022

Conversation

mortenfyhn
Copy link
Contributor

I stumbled upon what I think is a bug here, thanks to a -Wparentheses warning I got (clang 13, but I'm guessing this is on gcc too).

The warning text explains:

& has lower precedence than >; > will be evaluated first

which I don't think is what we want here. The if statement will evaluate to false in some cases where it shouldn't, such as when:

  • type_mask is 2 = 0b0010
  • IGNORE_LATITUDE is 1 = 0b0001
  • IGNORE_LONGITUDE is 2 = 0b0010

then the if statement evaluates to

  • 0b0010 & (0b0001 | 0b0010) > 0
  • 0b0010 & 0b0011 > 0
  • 0b0010 & true because > evaluates first
  • 0b0010 & 0b0001
  • 0b0000
  • false

which is wrong!

We can fix this by adding parentheses:

if ((position_target.type_mask & (mavros_msgs::GlobalPositionTarget::IGNORE_LATITUDE | mavros_msgs::GlobalPositionTarget::IGNORE_LONGITUDE)) > 0)

or just by removing the > 0 comparison and allowing the statement to implicitly convert to bool

if (position_target.type_mask & (mavros_msgs::GlobalPositionTarget::IGNORE_LATITUDE | mavros_msgs::GlobalPositionTarget::IGNORE_LONGITUDE))

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.

Good catch!

@vooon vooon merged commit f14d6cf into mavlink:master Nov 27, 2022
@vooon vooon added this to the Version 1.15 milestone Nov 27, 2022
@mortenfyhn mortenfyhn deleted the fix-latlon-check branch November 28, 2022 08:19
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

2 participants