Skip to content
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

AP_Follow: support for Mount following the lead vehicle in follow mode #23836

Merged
merged 2 commits into from May 26, 2023

Conversation

khanasif786
Copy link
Contributor

@khanasif786 khanasif786 commented May 19, 2023

While In follow mode, Mount also follows the same sysid the vehicle is following.
Tested in SITL.

#if HAL_MOUNT_ENABLED
Location target_loc;
Vector3f veh_vel;
AP_Follow *f = AP_Follow::get_singleton();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Txs for this. It's looking pretty good, We should add a nullptr check of f and s.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it looks like the rest of ModeFollow::run() uses "g2.follow" so let's use that instead.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. Mount can already follow a system ID - the only thing you might want to do is to add the option for follow mode to set the system ID for it to follow.

@rmackay9
Copy link
Contributor

Ah, PeterB is right of course. This makes it much simpler, because the code can probably be added only to the init() method. Nice catch.

Another small thing is when we get the singleton we normally use a variable name similar to the library name. So instead of "AP_Mount *s" we might use "AP_Mount *mount".

@rmackay9
Copy link
Contributor

rmackay9 commented May 19, 2023

.. and one last little thing is that PRs should always have a description even if the title makes it pretty obvious. .. normally that description should also include what testing has been done. If no testing has been done yet, then it's fine to say that and then we can edit the description later once it has been done.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer!

libraries/AP_Follow/AP_Follow.cpp Show resolved Hide resolved
libraries/AP_Follow/AP_Follow.cpp Outdated Show resolved Hide resolved
libraries/AP_Follow/AP_Follow.cpp Outdated Show resolved Hide resolved
libraries/AP_Follow/AP_Follow.h Outdated Show resolved Hide resolved
libraries/AP_Follow/AP_Follow.h Outdated Show resolved Hide resolved
libraries/AP_Follow/AP_Follow.h Outdated Show resolved Hide resolved
ArduCopter/mode_follow.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. This PR needs splitting as it crosses libraries and ArduCopter directories.

Please use ./Tools/gittools/git-subsystems-split to make this multiple commits

@magicrub magicrub merged commit edf9fbd into ArduPilot:master May 26, 2023
81 checks passed
@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label May 27, 2023
@rmackay9 rmackay9 mentioned this pull request May 28, 2023
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants