Add send_target_to_gimbal to AP_Mount_Backend #31732
Conversation
peterbarker
force-pushed
the
pr/mount-send_targets
branch
2 times, most recently
from
4698a03 to
cddf90b
Compare
| }; | ||
| class MountRateTarget { | ||
| public: | ||
| float roll; |
There was a problem hiding this comment.
txs for this. let's put units on these members. "rads" perhaps? We should do the same for the MountAngleTarget as well because roll is only an angle (in radians I think)
There was a problem hiding this comment.
txs for this. let's put units on these members. "rads" perhaps? We should do the same for the MountAngleTarget as well because roll is only an angle (in radians I think)
Can we do this as a follow-up PR?
It would be a lot of changed lines - if I do it as part of a follow-up PR then it will be a no-compiler-output change PR.
Changing this would also cause Henry significant issues!
| send_gimbal_device_set_rate(mnt_target.rate_rads.roll, mnt_target.rate_rads.pitch, mnt_target.rate_rads.yaw, mnt_target.rate_rads.yaw_is_ef); | ||
| break; | ||
| } | ||
| send_target_to_gimbal(); |
There was a problem hiding this comment.
nitpick, if possible I'd like to have more commenting. It's pretty obvious but I like it to be really obvious :-)
| // directly commanded to move to a retracted state | ||
| uint8_t natively_supported_mount_target_types() const override { | ||
| return ( | ||
| (1U<<unsigned(MountTargetType::ANGLE)) | |
There was a problem hiding this comment.
how about if we use defines or an enum to avoid the bit math here? It's going to be in every gimbal so I think just some ORing of defines or something might be easier. alternatively just three different methods, "bool supports_rate_targets() const { return true; }" might be clearer
There was a problem hiding this comment.
I've just pushed this up as four separate methods. Man, is that expensive in bytes, 'though!
Board,copter
CubeOrange,472
Yeah, the code is nicer, but I don't think it is 472 bytes nicer :-)
There was a problem hiding this comment.
Oops! The number is actually 240 bytes, I went against the wrong hash.
In any case I've pushed up a version which uses static const members to make the backends look nicer which doesn't cost any extra flash.
There was a problem hiding this comment.
I think we can replace this with the new pretty const exprs that you've added?
calls virtual methods which sends commands to the gimbal
peterbarker
force-pushed
the
pr/mount-send_targets
branch
from
cddf90b to
762a12b
Compare
peterbarker
force-pushed
the
pr/mount-send_targets
branch
from
c895d3e to
fb14250
Compare
| ANGLE = 0, | ||
| RATE = 1, | ||
| RETRACTED = 2, | ||
| NEUTRAL = 3, |
There was a problem hiding this comment.
do we need neutral? Isn't Neutral just ANGLE?
|
This is looking quite good to me so we could promote it from Draft to a regular PR? |
2 participants
This is a Draft Pull Request, not intended for review at this point.
It:
get_rc_targetwhich previously allowed all of the backends to share this code. It is no longer needed as they all now call update_mnt_target. It's being inlined because it has an awkward signature and does some strange dancing within it which is better done with the code in update_mnt_target.mount_targetobject to contain one of each. Rate doesn't need anywhere near as much stuff in it as Angle!send_target_to_gimbal. All backends call this method (may fold call into update_mnt_targets and rename it?). It looks atmnt_target. If a backend natively supports that type of target it calls a method to send that target to the gimbal. If it does not then it converts frommnt_target.target_type` to a target type that the gimbal does understand and sends that instead.