-
Notifications
You must be signed in to change notification settings - Fork 986
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
mount_control.cpp: detect MOUNT_ORIENTATION stale messages #1744
mount_control.cpp: detect MOUNT_ORIENTATION stale messages #1744
Conversation
c4a5acf
to
444a1df
Compare
444a1df
to
cc072ab
Compare
correct MountConfigure response success correct constructor initialization order some gimbals send negated/inverted angle measurements, correct that to obey the MAVLink frame convention using run-time parameters
cc072ab
to
220de4b
Compare
if (negate_measured_yaw) { | ||
mo.yaw = -mo.yaw; | ||
mo.yaw_absolute = -mo.yaw_absolute; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the frames inconsistent (oscillating between left hand coordinate or right hand coordinate systems). Shouldn't this be something that is fixed on the driver level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gremsy probably behaves differently from storm32. I can not test, I only have gremsy. But what I can say ist that this let's the users fix issues on their Gimbals without changing the code.
And for gremsy the code is definitely broken right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a device specific fix that needs to happen, why not handle it on the user app level? I still think mavros is the wrong place to make a workaround for a device specific problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the diagnostics code needs to handle it at this level, take a look at the diagnostics class in the mount_control.cpp file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand what you mean... why does the diagnostic status influenced by the negation in the mount control handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagnostics compare the measured angle with the commanded (set-point) angle if both match the gimbal is working. If they do not, there is an issue with the gimbal.
Now due to the way some gimbals work, a set-point of 70° causes a measurement of -70°. and that triggers an error.
By configuring negate_measurement_xxx
it will transform the -70° into 70° and then the diagnostic will stop failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amilcarlucas But if the gremsy gimbal driver is implemented the opposite sign, shouldn't the user app command -70° in the first place? I am not comfortable for a device specific bug propagating into a common software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the gremsy bug, you command 70 and it measures -70 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @Jaeyoung-Lim your thoughts?
@vooon @amilcarlucas I still think this is the wrong place to fix the problem, However if it unblocks @amilcarlucas I think it doesn't hurt to have something like this merged in. I hope we don't forget about this and remove this as soon as the gremsy bug is fixed. |
fix pitch sign
correct MountConfigure response success