AP_Mount: add CADDX gimbal support #28942

rmackay9 · 2024-12-24T05:30:59Z

This is a simplified version of @Hwurzburg's previous PR #28373 to add support for the CADDX GM3 gimbal

This simply adds support for the gimbal without trying to support the extra lock types that the gimbal provides. These extra lock types add the ability to control each axis (roll, pitch and yaw) in earth or body frame independently. For example roll control could be in body frame while pitch is in earth frame. This new ability is not supported in this PR.

This has been tested on real hardware and seems OK.

The corresponding wiki PR is here ArduPilot/ardupilot_wiki#6441

Hwurzburg · 2024-12-24T12:29:21Z

why drop the lock types and control?

rmackay9 · 2024-12-25T04:22:27Z

Hi @Hwurzburg,

I dropped the lock types and control because I couldn't get them to work and also they are really an enhancement so they naturally fall into a follow-up PR that could potentially affect other gimbals as well. The Siyi gimbal for example also supports FPV mode and users have been asking for that for a long time so we could do the two together

Lalbabu2001 · 2024-12-25T07:45:05Z

Hi please add support for this board
https://www.6sfull.cz/en/neutronrc-mini-f405-2-6s-20--20-30--30/

Don't forget to add Bmp 280 baro because new versions come with bmp280.
Also add support for PPM .
Thanks.

rmackay9 · 2024-12-25T11:32:56Z

Hi @Lalbabu2001,

I've created an issue here to capture this enhancement request #28945. No promises on when/if it will be done though. I'm not personally very experienced porting AP to new boards.

peterbarker

partial on-call review only :-)

libraries/AP_Mount/AP_Mount_CADDX.h

PeterB's requests addressed, thanks!

peterbarker

Looks pretty good.

libraries/AP_Mount/AP_Mount_CADDX.h

peterbarker · 2025-01-02T03:10:17Z

libraries/AP_Mount/AP_Mount_CADDX.h

+
+#include "AP_Mount_Backend_Serial.h"
+#include <AP_HAL/AP_HAL.h>
+#include <AP_Math/AP_Math.h>


Suggested change

#include <AP_Math/AP_Math.h>

removed include, thanks

Actually I've put this back because AP_Math.h is where "quaternion" is included

diff --git a/libraries/AP_Mount/AP_Mount_CADDX.h b/libraries/AP_Mount/AP_Mount_CADDX.h index fe423f55459..3d831209bf7 100644 --- a/libraries/AP_Mount/AP_Mount_CADDX.h +++ b/libraries/AP_Mount/AP_Mount_CADDX.h @@ -8,8 +8,7 @@ #if HAL_MOUNT_CADDX_ENABLED #include "AP_Mount_Backend_Serial.h" -#include <AP_Math/AP_Math.h> -#include <AP_Common/AP_Common.h> +#include <AP_Math/quaternion.h> class AP_Mount_CADDX : public AP_Mount_Backend_Serial {

ok thanks. Updated to remove AP_Common.h and change to <AP_Math/quaternion.h>. I'm surprised we don't need AP_Common.h in there. We include that in every other AP_Mount backend so I guess we could remove it from many places

ok thanks. Updated to remove AP_Common.h and change to <AP_Math/quaternion.h>. I'm surprised we don't need AP_Common.h in there. We include that in every other AP_Mount backend so I guess we could remove it from many places

Probably, yes - and that will save compile time, which everybody likes.

We're getting away with some shenanigans by having those all around the place - things like the stdint.h include come to mind :-)

peterbarker · 2025-01-02T03:10:59Z

libraries/AP_Mount/AP_Mount_CADDX.h

+protected:
+
+    // get attitude as a quaternion.  returns true on success
+    bool get_attitude_quaternion(Quaternion& att_quat) override;


Suggested change

bool get_attitude_quaternion(Quaternion& att_quat) override;

bool get_attitude_quaternion(class Quaternion& att_quat) override;

Not actually including the right header to get this - just getting it transitively somehow

I'm going to skip this change I think because it leads to a compile failure and would make it inconsistent with the virtual method that it is overriding and also all other drivers. Apologies, I'm probably misunderstanding something

peterbarker · 2025-01-02T03:12:14Z

libraries/AP_Mount/AP_Mount_CADDX.h

+
+    // lock mode enum
+    enum class LockMode {
+        FOLLOW = 0,


This doesn't make a lot of sense!

Suggested change

FOLLOW = 0,

Hi @peterbarker, thanks for the review. I've removed this line because we don't use it but I can imagine that we could use it as a default value so not sure why it doesn't makes sense?

Generally we don't use the same enumeration which is giving names to bit-offsets for combinations of those bits (AKA a "mask").

It would be nice to have some type-safety around this sort of thing, but I've never managed to propose a PR to add that....

peterbarker

LGTM

peterbarker · 2025-01-02T06:10:01Z

libraries/AP_Mount/AP_Mount_CADDX.h

+
+#include "AP_Mount_Backend_Serial.h"
+#include <AP_HAL/AP_HAL.h>
+#include <AP_Math/AP_Math.h>


diff --git a/libraries/AP_Mount/AP_Mount_CADDX.h b/libraries/AP_Mount/AP_Mount_CADDX.h index fe423f55459..3d831209bf7 100644 --- a/libraries/AP_Mount/AP_Mount_CADDX.h +++ b/libraries/AP_Mount/AP_Mount_CADDX.h @@ -8,8 +8,7 @@ #if HAL_MOUNT_CADDX_ENABLED #include "AP_Mount_Backend_Serial.h" -#include <AP_Math/AP_Math.h> -#include <AP_Common/AP_Common.h> +#include <AP_Math/quaternion.h> class AP_Mount_CADDX : public AP_Mount_Backend_Serial {

libraries/AP_Mount/AP_Mount_CADDX.cpp

Co-authored-by: Randy Mackay <rmackay9@yahoo.com>

rmackay9 · 2025-01-02T07:20:14Z

I've updated with @peterbarker's requests and marked as "MergeOnCIPass"

peterbarker · 2025-01-02T11:18:28Z

Merged it!

rmackay9 added Enhancement Driver labels Dec 24, 2024

rmackay9 force-pushed the caddx-mount-serial branch from df038ba to 358d493 Compare December 24, 2024 06:12

rmackay9 mentioned this pull request Dec 24, 2024

CADDX gimbal backend #28373

Closed

rmackay9 requested a review from Hwurzburg December 24, 2024 10:02

rmackay9 mentioned this pull request Dec 25, 2024

Add support for the Neutron RC Mini F405 #28945

Open

rmackay9 added the DevCallTopic label Dec 27, 2024

Hwurzburg added the WikiNeeded needs wiki update label Dec 30, 2024

CraigElder removed the DevCallTopic label Dec 30, 2024

peterbarker previously requested changes Dec 30, 2024

View reviewed changes

libraries/AP_Mount/AP_Mount_CADDX.h Outdated Show resolved Hide resolved

libraries/AP_Mount/AP_Mount_CADDX.h Outdated Show resolved Hide resolved

libraries/AP_Mount/AP_Mount_CADDX.h Outdated Show resolved Hide resolved

rmackay9 force-pushed the caddx-mount-serial branch from 358d493 to 317c8b3 Compare January 2, 2025 03:07

peterbarker requested changes Jan 2, 2025

View reviewed changes

rmackay9 force-pushed the caddx-mount-serial branch from 317c8b3 to 190f25e Compare January 2, 2025 03:28

peterbarker approved these changes Jan 2, 2025

View reviewed changes

rmackay9 force-pushed the caddx-mount-serial branch from 190f25e to 950b0f2 Compare January 2, 2025 07:15

Hwurzburg and others added 2 commits January 2, 2025 16:18

AP_Mount: add CADDX gimbal support

ebaf759

Co-authored-by: Randy Mackay <rmackay9@yahoo.com>

Tools: add CADDX gimbal to build server options

b943f94

rmackay9 force-pushed the caddx-mount-serial branch from 950b0f2 to b943f94 Compare January 2, 2025 07:18

rmackay9 added the MergeOnCIPass label Jan 2, 2025

peterbarker merged commit e2e7d1d into ArduPilot:master Jan 2, 2025
99 checks passed

rmackay9 deleted the caddx-mount-serial branch January 3, 2025 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_Mount: add CADDX gimbal support #28942

AP_Mount: add CADDX gimbal support #28942

rmackay9 commented Dec 24, 2024

Hwurzburg commented Dec 24, 2024

rmackay9 commented Dec 25, 2024

Lalbabu2001 commented Dec 25, 2024

rmackay9 commented Dec 25, 2024

peterbarker left a comment

peterbarker left a comment

peterbarker Jan 2, 2025

rmackay9 Jan 2, 2025

rmackay9 Jan 2, 2025

peterbarker Jan 2, 2025

rmackay9 Jan 2, 2025

peterbarker Jan 2, 2025

peterbarker Jan 2, 2025

rmackay9 Jan 2, 2025

peterbarker Jan 2, 2025

rmackay9 Jan 2, 2025

peterbarker Jan 2, 2025

peterbarker left a comment

peterbarker Jan 2, 2025

rmackay9 commented Jan 2, 2025

peterbarker commented Jan 2, 2025

	bool get_attitude_quaternion(Quaternion& att_quat) override;
	bool get_attitude_quaternion(class Quaternion& att_quat) override;

AP_Mount: add CADDX gimbal support #28942

AP_Mount: add CADDX gimbal support #28942

Conversation

rmackay9 commented Dec 24, 2024

Hwurzburg commented Dec 24, 2024

rmackay9 commented Dec 25, 2024

Lalbabu2001 commented Dec 25, 2024

rmackay9 commented Dec 25, 2024

peterbarker left a comment

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 commented Jan 2, 2025

peterbarker commented Jan 2, 2025