torqeedo: Add support multi torqeedo #24302

xianglunkai · 2023-07-14T07:32:30Z

This is a preliminary draft and I hope to receive guidance so that I can quickly improve it！
Txs!

xianglunkai · 2023-07-19T08:44:28Z

@rmackay9
Could you give some suggest?

rmackay9 · 2023-07-20T12:12:38Z

Hi @xianglunkai,

It's really great to see this. I hope to give feedback tomorrow..

rmackay9 · 2023-07-27T01:08:29Z

libraries/AP_Torqeedo/AP_Torqeedo.h

+#include "AP_Torqeedo_Params.h"
+
+// Maximum number of range finder instances available on this platform
+#define TORQEEDO_MAX_INSTANCES 3


I wonder if perhaps 2 is enough. 2 is easy to imagine, 3 less so..

rmackay9 · 2023-07-27T01:10:01Z

libraries/AP_Torqeedo/AP_Torqeedo.h

-    // report changes in error codes to user
-    void report_error_codes();
+    // parameters for each instance
+    static const struct AP_Param::GroupInfo var_info[];


nitpick: it's a small style thing but I normally put this at the very bottom of the public section. So just before "protected:" or "private:"

rmackay9 · 2023-07-27T01:13:21Z

libraries/AP_Torqeedo/AP_Torqeedo.h

-        DRIVE = 0x82
-    };
+
+    bool get_batt_capacity_Ah(uint16_t &amp_hours) const


nitpick: let's put these two lines together and add a comment above or remove the blank lines above.

rmackay9 · 2023-07-27T01:17:45Z

libraries/AP_Torqeedo/AP_Torqeedo_Backend.h

+
+    // we declare a virtual destructor so that Proximity drivers can
+    // override with a custom destructor if need be
+    virtual ~AP_Torqeedo_Backend(void) {}


normally we don't need a destructor.

rmackay9 · 2023-07-27T01:18:18Z

libraries/AP_Torqeedo/AP_Torqeedo_Backend.h

+    virtual ~AP_Torqeedo_Backend(void) {}
+
+    // initialize drivers
+    virtual void init(void) {};


nitpick: personally I like to skip the "void" in the argument list. I find it doesn't add clarity.

rmackay9 · 2023-07-27T01:18:52Z

libraries/AP_Torqeedo/AP_Torqeedo_Backend.h

+    // get latest battery status info.  returns true on success and populates arguments
+    virtual bool get_batt_info(float &voltage, float &current_amps, float &temp_C, uint8_t &pct_remaining) const WARN_IF_UNUSED { return false; }
+
+    virtual bool get_batt_capacity_Ah(uint16_t &amp_hours) const { return false; }


nitpick: let's add a comment above this line or remove the whitespace line completely.

rmackay9 · 2023-07-27T01:20:19Z

libraries/AP_Torqeedo/AP_Torqeedo.h


-    static const struct AP_Param::GroupInfo var_info[];
+    // return the number of propellers backends
+    uint8_t num_propellers() const { return num_instances; }


while "num_propellers" sound cool, let's just use "num_instances" because it'll be more clear what we mean and removes potential confusion about whether a single instance might have more than one propeller.

rmackay9 · 2023-07-27T01:22:29Z

libraries/AP_Torqeedo/AP_Torqeedo.h

-    };
+    // return sensor health
+    Status get_instance_status(uint8_t instance) const;
+    Status get_status() const;


Let's move the status to the backend and change get_status(instance) to ask the backend. This should also remove the need for the semaphore protecting the frontend state.

rmackay9 · 2023-07-27T01:25:36Z

libraries/AP_Torqeedo/AP_Torqeedo.h

-    // returns true if communicating with the motor
-    bool healthy();
+     // The TorqeedoState structure is filled in by the backend driver
+    struct Torqeedo_State { 


If possible, let's remove this Torqeedo_State structure. We can pass in the instance number to each backend's constructor. The status can be maintained by the backend itself. The var_info seems to be a duplilcate of backend_var_info isn't it?

rmackay9 · 2023-07-27T01:27:47Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+    // @Values: 0:Disabled, 1:Tiller, 2:Motor
+    // @User: Standard
+    // @RebootRequired: True
+    AP_GROUPINFO_FLAGS("TYPE", 1, AP_Torqeedo_TQBus, _type, (int8_t)ConnectionType::TYPE_DISABLED, AP_PARAM_FLAG_ENABLE),


Is it correct that we have a TYPE parameter defined both here and in AP_Torqeedo_Params.h/.cpp?

rmackay9 · 2023-07-27T01:32:51Z

libraries/AP_Torqeedo/AP_Torqeedo_Backend_Serial.cpp

@@ -0,0 +1,49 @@
+/*
+   This program is free software: you can redistribute it and/or modify


At this point it seems like unnecessary complexity to have this AP_Troqeedo_Backend_Serial although I guess it makes sense if we're going to add ePropulsion support (which also uses serial). The Torqeedo CAN protocol would not re-use this backend though... I guess I'm OK with this _serial backend staying although I'd probably lean towards removing it and keeping things a little simpler.

rmackay9 · 2023-07-27T01:37:53Z

libraries/AP_Torqeedo/AP_Torqeedo_Backend_Serial.cpp

+// static detection function
+// detect if a proximity sensor is connected by looking for a configured serial port
+// serial_instance affects which serial port is used.  Should be 0 or 1 depending on whether this is the 1st or 2nd proximity sensor with a serial interface
+bool AP_Torqeedo_Backend_Serial::detect(uint8_t serial_instance)


I think we could just remove this "detect()" method. Just have the front-end create the backend and tell it its expected instance and serial_instance. If the serial backend can't find a uart then it should just report unhealthy (and make sure that it never tries to use the uart).

.. I don't feel really strongly about this though. having the detect does mean that we can avoid all the, "if (uart==nullptr) return;" checks.

rmackay9 · 2023-07-27T01:39:28Z

libraries/AP_Torqeedo/AP_Torqeedo_Params .cpp

+    // @Values: 0:None,1:TQBus
+    // @RebootRequired: True
+    // @User: Standard
+    AP_GROUPINFO_FLAGS("_TYPE",   1, AP_Torqeedo_Params, type, 0, AP_PARAM_FLAG_ENABLE),


As mentioned before this seems like a duplicate..

rmackay9 · 2023-07-27T01:42:07Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+        // @Field: ErrCnt: Error Count
+        AP::logger().Write("TRQD", "TimeUS,Health,DesMotSpeed,MotSpeed,SuccCnt,ErrCnt", "QBhhII",
+                           AP_HAL::micros64(),
+                           health,


We will need to add an instance in here. So another good reason for each backend to know which instance it is.

rmackay9 · 2023-07-27T01:42:51Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+    }
+
+    if ((_options & options::DEBUG_TO_GCS) != 0) {
+        gcs().send_text(MAV_SEVERITY_INFO,"TRQD h:%u dspd:%d spd:%d succ:%ld err:%ld",


we might want to add the instance number in this debug output as well although it's less important because it's just debug.

rmackay9 · 2023-07-27T01:45:27Z

Rover/Parameters.h

@@ -13,7 +13,7 @@
 #include "AP_Rally.h"
 #include <AP_SmartRTL/AP_SmartRTL.h>
 #include <AP_Stats/AP_Stats.h>
-#include "AP_Torqeedo/AP_Torqeedo.h"


Great to see this. This commit should be prefixed with "Rover:" ideally.

rmackay9 · 2023-07-27T01:46:22Z

Rover/Parameters.cpp

@@ -652,7 +652,7 @@ const AP_Param::GroupInfo ParametersG2::var_info[] = {
 #if HAL_TORQEEDO_ENABLED
    // @Group: TRQD_
    // @Path: ../libraries/AP_Torqeedo/AP_Torqeedo.cpp
-    AP_SUBGROUPINFO(torqeedo, "TRQD_", 49, ParametersG2, AP_Torqeedo),


Hi, this commit should also be prefixed with "Rover:". I think we should shorten the parameter prefix to just "TRQ" because then the instance number will be added making it TRQ1 and TRQ2 which seems fine to me. I think we try to keep our parameter prefixes to less than 4 these days.

rmackay9 · 2023-07-27T01:49:30Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+    _uart->set_unbuffered_writes(true);
+
+    // if using tiller connection set on/off pin for 0.5 sec to turn on battery
+    if (_type == ConnectionType::TYPE_TILLER) {


Ah, I see now why there are duplicate Type parameters. I think we could use the same TRQBus backend but have two types. One for tiller and one for Motor. The frontend TYPE parameter could have two values (1:TRQ Tiller, 2:TQR Motor) and which one is selected could be passed into the constuctor.

rmackay9 · 2023-07-27T01:53:01Z

Hi @xianglunkai,

Thanks again for all this. This is really great. I've put a few comments in so maybe you could have a look at them.

Ideally we should have parameter conversions so that upgrades are seamless.

Also have you tested on real hardware? I assume you've got the hardware but if you don't I do and can test for you (although my boat is an hour drive away). Hopefully we can get at least one person on the forums to test for us too.

rmackay9 · 2023-07-27T01:54:44Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+
+    // @Param: SRV_FUNC
+    // @DisplayName: Torqeedo motor servo function
+    // @Description: Torqeedo motor servo function. Default is kthrottle


Let's add Values here for the three options, "70:Throttle, 73:ThrottleLeft, 74:ThrottleRight"

rmackay9 · 2023-07-27T01:55:48Z

libraries/AP_Torqeedo/AP_Torqeedo_TQBus.cpp

+
+    // @Param: SRV_FUNC
+    // @DisplayName: Torqeedo motor servo function
+    // @Description: Torqeedo motor servo function. Default is kthrottle


For the Description, let's remove the "k" in front of "throttle". Programmers know the difference but users will just think it's a typo.

xianglunkai · 2023-07-27T02:39:35Z

@rmackay9
Thank you very much.
I will make the necessary changes as soon as possible, and then please continue with the review.

rmackay9 · 2024-05-08T07:53:45Z

The alternative PR #26494 has gone in so I'll close this. Thanks!

xianglunkai added 3 commits July 14, 2023 15:28

AP_Torqeedo: add supoort multi-Torqeedos

0de0aec

Parameters: using <> instead

377e66b

Parameters: fix name for torqeedo

a9b6cb9

rmackay9 reviewed Jul 27, 2023

View reviewed changes

Hwurzburg added the WikiNeeded needs wiki update label Jul 27, 2023

rmackay9 mentioned this pull request Mar 13, 2024

AP_Torqeedo: add support for two motors #26494

Merged

rmackay9 closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

torqeedo: Add support multi torqeedo #24302

torqeedo: Add support multi torqeedo #24302

xianglunkai commented Jul 14, 2023

xianglunkai commented Jul 19, 2023

rmackay9 commented Jul 20, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023 •

edited

Loading

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023 •

edited

Loading

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 Jul 27, 2023

rmackay9 commented Jul 27, 2023

rmackay9 Jul 27, 2023 •

edited

Loading

rmackay9 Jul 27, 2023

xianglunkai commented Jul 27, 2023

rmackay9 commented May 8, 2024

		@@ -0,0 +1,49 @@
		/*
		This program is free software: you can redistribute it and/or modify

torqeedo: Add support multi torqeedo #24302

torqeedo: Add support multi torqeedo #24302

Conversation

xianglunkai commented Jul 14, 2023

xianglunkai commented Jul 19, 2023

rmackay9 commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 Jul 27, 2023 • edited Loading

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

rmackay9 Jul 27, 2023 • edited Loading

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

rmackay9 commented Jul 27, 2023

rmackay9 Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xianglunkai commented Jul 27, 2023

rmackay9 commented May 8, 2024

rmackay9 Jul 27, 2023 •

edited

Loading

rmackay9 Jul 27, 2023 •

edited

Loading

rmackay9 Jul 27, 2023 •

edited

Loading