Integrated Tusuav's gimbal driver and backend #28491
Conversation
| const char* AP_Mount_Tusuav::send_text_prefix = "Tusuav:"; | ||
|
|
||
| // init - performs any required initialisation for this instance | ||
| // void AP_Mount_Tusuav::init() |
There was a problem hiding this comment.
Thanks for the PR! We normally don't allow commented out code to be added so could you remove this?
There was a problem hiding this comment.
Thanks for the feedback! We've removed the commented-out code as requested. Let us know if there’s anything else that needs adjustment
| uint32_t now_ms = AP_HAL::millis(); | ||
|
|
||
|
|
||
| if(now_ms - _last_update_ms_parse >= AP_MOUNT_TUSUAV_PARSE_INTERVAL_MS) |
There was a problem hiding this comment.
small formatting issues here. There should be a space between the "if" and the bracket. Also the curly brace at the beginning of the next line should actually be on the end of this line
There was a problem hiding this comment.
Thanks for the feedback! We've fixed the spacing issue with "if" and moved the curly brace. We've also checked and adjusted similar formatting issues throughout.
| if(mnt_target.angle_rad.yaw_is_ef){ | ||
| gimbal_angle_mode.mode.bits.yaw = static_cast<uint8_t>(GimbalAngleMode::Earth_Frame_Angle); | ||
| debug("euler：%f %f", gimbal_angle_mode.ef_angle[0], gimbal_angle_mode.ef_angle[2]); | ||
| } |
There was a problem hiding this comment.
little white space issue here. The else should be on the same line as the previous curly bracket
There was a problem hiding this comment.
We've fixed this whitespace issue and reviewed similar formatting across the code to ensure consistency. Thank you!
| } | ||
|
|
||
| case GimbalSubCmd::GimbalSubCmd_LaserDistance:{ | ||
| // recieve laser rangefinder value in decimeters and save the value in meters |
There was a problem hiding this comment.
little white space issue here (too much indenting)
There was a problem hiding this comment.
Thanks for pointing that out! We've adjusted the indentation here and reviewed similar formatting throughout.
| } | ||
| break; | ||
| } | ||
| default:break; |
There was a problem hiding this comment.
we normally separate these into two lines
There was a problem hiding this comment.
Thank you for your reply. I will make modifications based on your suggestions. Another question, can I continue submitting the modified code? Do I need to create a new pull request?
There was a problem hiding this comment.
Hi @sunny-1987,
feel free to keep updating this PR. No need to create a new one. By the way, if you think we're going to slow in reviewing, the best thing to do is post a comment asking that the PR be added to the weekly dev call. You can even attend if you like it is 8am Japan time each tuesday. If that's too early then there's an "EU Dev Call" on Wed at 4pm (Japan Time).
There was a problem hiding this comment.
Hi! Could you let me know how to join the "EU Dev Call" on Wednesdays? Thanks!
There was a problem hiding this comment.
Hi @sunny-1987, just join ArduPilot's discord server and then go to the "weekly-devcall" voice channel at 4pm Japan time today.
|
Thanks very much for this. In general it looks like it's well written. It might be good to send me a gimbal so that I can properly test it and confirm that all the features are working. That's not completely necessary though, so no pressure. By the way, I see there is a "merge" commit in there which we normally don't merge. It would be good to get rid of this by rebasing on master. |
sunny-1987
force-pushed
the
master
branch
2 times, most recently
from
255b1d9
to
ba1a6df
Compare
| @@ -60,6 +60,10 @@ | |||
| #define HAL_MOUNT_TOPOTEK_ENABLED AP_MOUNT_BACKEND_DEFAULT_ENABLED | |||
| #endif | |||
|
|
|||
| #ifndef HAL_MOUNT_TUSUAV_ENABLED | |||
| #define HAL_MOUNT_TUSUAV_ENABLED AP_MOUNT_BACKEND_DEFAULT_ENABLED | |||
There was a problem hiding this comment.
This is a big chunk of code, enabled by default probably needs to be a bit more nuanced
There was a problem hiding this comment.
I agree, the flash cost will likely be the biggest issue in getting this merged. There are certainly things we can do to share more code amonst the various gimbals but whether we need to do that before this gets merged or not is a good question
There was a problem hiding this comment.
file.zip
We evaluated the flash storage conditions before and after implementing AP_Mount_Tusuav, and the results are attached. Additionally, we have included a video demonstrating our gimbal functionality, and we are using the CubeOrangePlus flight controller.
There was a problem hiding this comment.
fyi, we have several CI tests below that check sizes for several classes of autopilot
All reviews have been addressed.
Tusuav Gimbal
Tusuav is a company specializing in the research and production of VTOLs,including the gimbals. The drive for its GT30 series gimbals has been added in these commits. The pods featured in this submission offer continuous zoom capability, EO camera and IR camera switching with picture-In-picture (PIP) functionality, target tracking, pan and tilt angle & rate control , photo-taking, video recording, and rangefinder up to 3km.
The protocol description
/*Field Index Bytes Description
-------------------------------------------------------------------------------------------
Header 0 1 0x5A
Length 1 1 total length, n=all bytes from byte5 to checksum, min=6, max=128
Flag 2 1 bit0-2: board id of the gimbal 0=main controller 1=comm board 2~4=motor pitch/roll/yaw
bit3: 1= need reply 0 = no need
bit4: 1= is a reply message 0 = now a reply message
bit5: 1= is a reading message 0 = is a writing message
Cmd 3 1 see details at below
Sub Cmd 4 1 see details at below
Data 5-n+1 n data in little endian lowest byte first
Checksum n+2 1 calculate_crc from mavlink 2.0 of byte from 0 to n+1 (inclusive)
*/
The website of Tusuav
https://www.tusuav.com/pod
