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

SENSORS: Lanbao PSK-CM8JL65-CC5 Orientation fix #23900

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

Claudio-Chies
Copy link
Contributor

@Claudio-Chies Claudio-Chies commented Nov 6, 2024

The parameter value for ROTATION_BACKWARD_FACING was wrong, it was 12 which corresponds with MAV_SENSOR_ROTATION_PITCH_180 which is the same orientation, but not used in the DistanceSensor.msg

Tests

  • Tested on Hardware

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 6, 2024

Seems like a problem in the definition because it's assumed the distance sensor's body yaw orientation doesn't matter which is probably the case.
It's still defined as pitch 180° in the MAVLink interface here:

case MAV_SENSOR_ORIENTATION::MAV_SENSOR_ROTATION_PITCH_180:
dist.orientation = distance_sensor_s::ROTATION_BACKWARD_FACING;

Shouldn't that also be changed or does that break compatibility because people use pitch 180 instead of yaw 180? 🤯

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 6, 2024

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 6, 2024

And then the definition changed here:
0fe271b#diff-471de0c68f584b7d80e91147614e1f96aedd01de7312675afa417f48ae8ced8cR36

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

I checked and with the latest change of the DistanceSensor message this makes sense. In the current implementation it's not used so there's no behavioral change as far as I can see.

@MaEtUgR MaEtUgR merged commit 04b6412 into main Nov 6, 2024
51 of 56 checks passed
@MaEtUgR MaEtUgR deleted the pr-lanbao_CM8_fix branch November 6, 2024 15:22
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