Rover hold/loiter #13767

ealdaz-seesai · 2019-12-20T13:57:17Z

This is my first pull request, so unsure as to whether I should raise an Issue before creating a pull request, but I'm hoping that this explanation will be sufficiently clear, any feedback welcome.

Describe problem solved by this pull request
When using HOLD/LOITER the rover would not hold position but instead circle/oscillate around the hold position. This was caused by the fact that the current algorithm sets velocity to zero when distance to waypoint is under a certain threshold, the vehicle would decelerate, but not enough to stop before it once again exceeded the threshold. At this point it would then try to return to the hold position and the process would repeat.

Describe your solution
In the proposed solution once the distance to the waypoint is under a threshold the velocity is set to zero until a new position waypoint is requested that is more than the threshold distance away from the previous waypoint. This way the vehicle always stops.

Test data / coverage
This was tested with both px4_sitl and a pixhawk4 rover running the Aion RObotics R1 UGV (Tank type/Differential drive)

ealdaz-seesai · 2019-12-20T14:29:51Z

This is an example of what happens when selecting HOLD in QGroundControl

ealdaz-seesai · 2019-12-20T14:35:43Z

In the proposed implementation the rover stops when HOLD is requested:

julianoes · 2020-02-14T16:06:14Z

@ItsTimmy would be great if you could review this one, thanks!

AmeliaEScott

Overall, this looks like a good solution to the problem. I just think there might be a few artifacts leftover from a merge or something.

src/modules/rover_pos_control/RoverPositionControl.hpp

src/modules/rover_pos_control/RoverPositionControl.cpp

julianoes · 2020-02-20T14:13:50Z

@ealdaz-seesai it would be great if you could follow up on the things @ItsTimmy mentioned and make sure the CI tests pass.

ealdaz-seesai · 2020-02-20T15:10:34Z

Will do Julian, currently travelling with very bad coverage. Back Monday. Thanks for pushing this along. Eduardo

…

--- Eduardo Aldaz Carroll CTO sees.ai E: ealdaz@sees.ai M: +44 (0) 7543 519191 Skype: e.aldaz-carroll

On 20 Feb 2020, at 15:13, Julian Oes ***@***.***> wrote: @ealdaz-seesai <https://github.com/ealdaz-seesai> it would be great if you could follow up on the things @ItsTimmy <https://github.com/ItsTimmy> mentioned and make sure the CI tests pass. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13767>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIQJYAQV3SY2SFFPRYCODM3RD2F2BANCNFSM4J56HLFQ>.

ealdaz-seesai

I've tested it in Gazebo, works as intended

src/modules/rover_pos_control/RoverPositionControl.cpp

src/modules/rover_pos_control/RoverPositionControl.hpp

Jaeyoung-Lim · 2020-04-13T18:36:10Z

@ealdaz-seesai Could you resolve the conflicts of this PR?

ealdaz-seesai · 2020-04-13T20:40:05Z

Of course, hadn't realized there were conflicts, might be due to some new changes to the file. Let me check.

ealdaz-seesai · 2020-04-13T22:25:12Z

All tests passing except some MacOS, complaining about implicit conversion from float to double in calls to function get_distance_to_next_waypoint using Vector2f variables
I've made some changes to try to pass those tests

ealdaz-seesai · 2020-04-27T11:07:15Z

Is there anything else required to merge this?

ealdaz-seesai · 2020-04-29T21:03:56Z

bump
@ItsTimmy @dagar @julianoes @Jaeyoung-Lim anything else necessary to merge this?
It seems to me it's ready to go?
Thanks!

julianoes

This is changing some submodules. Can you try to rebase on master, or at least merge the latest master into it, fix all conflicts, and make sure the diff is only what you changed?

ealdaz-seesai · 2020-05-04T20:37:37Z

Hello Julian
I merged my branch with master 3 weeks ago as some of the files that I had originally modified 4 months ago had been changed since. All tests passed at the time, and couldn't see any conflicts.
Regardless I've merged again without any conflicts.
All checks passed except for the Jenkins Compile MacOS one.
I don't understand what the error is, doesn't seem to be related to my code for what I can tell.
The error is:

ERROR: Timeout after 10 minutes
Error cloning remote repo 'origin'

Could you check please? I have no idea what this test does...
Thx

ealdaz-seesai · 2020-05-04T20:40:47Z

Also not sure I understand the submodule point you were making, I haven't changed any submodule.

julianoes · 2020-05-05T07:42:55Z

I'm really confused about the state of this PR, I can't get it merged properly either. It's odd.

Some submodules are wrong, see here:

Let me try to apply the diff and create one new clean commit.

julianoes · 2020-05-05T07:47:57Z

I have the diff applied cleanly but I can't push it to your fork.

Could you make the checkmark as described here?
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

ealdaz-seesai · 2020-05-05T10:18:48Z

I’m really sorry Julian, but I can’t find that check box anywhere on the Pull request page…. Any tips on how to find it, it’s like it’s not been enabled ... see screenshot below ![image](https://user-images.githubusercontent.com/35691522/81057604-a8814600-8ec4-11ea-8d9b-7a2aa6267da3.png) Is it because the repository is not my personal one but part of an organisation? Any other way to give you permission? Eduardo

julianoes · 2020-05-06T07:26:31Z

Is it because the repository is not my personal one but part of an organisation?

That's probably why. Do you want to create a new PR with the clean diff or should I do it?

ealdaz-seesai · 2020-05-06T07:30:35Z

I’ll give it a go Eduardo

…

--- Eduardo Aldaz Carroll CTO sees.ai E: ealdaz@sees.ai M: +44 (0) 7543 519191 Skype: e.aldaz-carroll

On 6 May 2020, at 08:26, Julian Oes ***@***.***> wrote: Is it because the repository is not my personal one but part of an organisation? That's probably why. Do you want to create a new PR with the clean diff or should I do it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13767 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIQJYAQVHUB6PIRTKDDUDOTRQEGLLANCNFSM4J56HLFQ>.

ealdaz-seesai · 2020-05-06T21:49:21Z

OK, I've figured out what the issue was. Those submodules were added to my git branch 3 weeks ago and I didn't realize. I've removed them now. I'm hoping it should all be OK now.

julianoes · 2020-05-07T06:13:10Z

Great, thanks @ealdaz-seesai.

@Jaeyoung-Lim could you please give this a review?

Jaeyoung-Lim

Thanks for the contribution and sorry for the late review.

Small comment regarding indentation changes

Would it be possible to squash the commits? Most of the commits seems to be merge commits or minor changes.

Jaeyoung-Lim · 2020-05-07T06:49:13Z

src/modules/rover_pos_control/RoverPositionControl.hpp

-	vehicle_attitude_s			_vehicle_att{};
-	sensor_combined_s			_sensor_combined{};
+	actuator_controls_s			    _act_controls{};		/**< direct control of actuators */
+	vehicle_attitude_s              _vehicle_att{};


Please fix the indentation errors

No problem for late review, everyone is very busy.
I'm just happy to be able to contribute.
Ha, pushing my very average git skills with this...never tried squashing commits before, I'll google and give it a try...

Hadn't noticed the indent changes, I had run the fix_code_style.sh and thought that took care of that sort of thing. But just realized that CLion was using tabs as spaces, will sort it out.

Jaeyoung-Lim · 2020-05-07T06:49:29Z

src/modules/rover_pos_control/RoverPositionControl.hpp

@@ -113,15 +114,15 @@ class RoverPositionControl : public ModuleBase<RoverPositionControl>, public Mod

 	uORB::Subscription	_parameter_update_sub{ORB_ID(parameter_update)};

-	manual_control_setpoint_s		_manual{};			/**< r/c channel data */
+	manual_control_setpoint_s		_manual{};			    /**< r/c channel data */


unnecessary indentation changes

White spaces are still not addressed

Jaeyoung-Lim · 2020-05-07T06:49:36Z

src/modules/rover_pos_control/RoverPositionControl.hpp

-	sensor_combined_s			_sensor_combined{};
+	actuator_controls_s			    _act_controls{};		/**< direct control of actuators */
+	vehicle_attitude_s              _vehicle_att{};
+	sensor_combined_s				_sensor_combined{};


White spaces are still not addressed

Jaeyoung-Lim · 2020-05-07T06:53:32Z

src/modules/rover_pos_control/RoverPositionControl.cpp

@@ -406,7 +407,7 @@ RoverPositionControl::run()
 		bool manual_mode = _control_mode.flag_control_manual_enabled;

 		/* only run controller if position changed */
-		if (fds[0].revents & POLLIN) {
+		if (fds[0].revents & POLLIN || fds[4].revents & POLLIN) {


I think we can use just local_position (fds[4]) and remove fds[0]

Is local position also available if only using GPS?

ealdaz-seesai · 2020-05-08T00:30:48Z

Hi @Jaeyoung-Lim , Ok so I've really tried (for 3 hours) I've rebased and squashed, but still all the commits appear. I tried rebasing again and then all sorts of conflicts happen, I think it's got to do probably with the fact that I've merged twice already in the last months. I'm not comfortable enough with git to understand what is really going on, and I'm afraid I can't really spend anymore time with this.
If it is really a big issue I could do a new PR...

Added local position as a valid source of position

ealdaz-seesai · 2020-05-08T00:59:14Z

OK, so it was annoying me to leave it half done. So decided to give it one last go and rebase again and skip some commits and it has worked.
I've tested in SITL and all works as intended.

I did notice a new "feature" in this latest master and it's that SITL rover complains that no power is unavailable and doesn't let you arm. I had to disable power check flag. (CBRK_SUPPLY_CHECK).

Note that I couldn't reboot either from QGroundcontrol... i'm pretty sure I could before...
So a couple of things to add as a future PR...

pxh> WARN  [PreFlightCheck] system power unavailable

ealdaz-seesai · 2020-05-11T11:31:56Z

Jenkins SITL Tests fail, but not sure why... would any of you know ?
Doesn't seem to be in an area related to the files changed... as these SITL tests are done for the iris not rover ...

julianoes · 2020-05-13T09:48:59Z

@Jaeyoung-Lim would be great if you could follow up.

Jaeyoung-Lim · 2020-05-13T09:58:39Z

@julianoes Jenkins failure is not related to this PR. There are a few comments that are not addressed from @ealdaz-seesai Otherwise looks good to me

julianoes · 2020-05-13T10:06:03Z

@Jaeyoung-Lim you're call if you want those addressed or merge anyway.

Jaeyoung-Lim

@julianoes Since this has been here for a while, I think it makes sense to merge. i will address the comments myself

julianoes · 2020-05-13T11:09:00Z

Thanks @ealdaz-seesai !

ealdaz-seesai · 2020-05-13T14:01:01Z

Thank you both ! A pleasure! Eduardo

…

--- Eduardo Aldaz Carroll CTO sees.ai E: ealdaz@sees.ai M: +44 (0) 7543 519191 Skype: e.aldaz-carroll

On 13 May 2020, at 12:09, Julian Oes ***@***.***> wrote: Thanks @ealdaz-seesai <https://github.com/ealdaz-seesai> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13767 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIQJYAXPC6D2OWLWDDBHAYTRRJ5VXANCNFSM4J56HLFQ>.

weekly-digest bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #13770

Closed

dagar requested review from AmeliaEScott and dagar January 6, 2020 17:24

dagar added Admin: Enhancement (improvement) 💡 Rover 🚙 Rovers and other UGV labels Jan 6, 2020

weekly-digest bot mentioned this pull request Feb 16, 2020

Weekly Digest (9 February, 2020 - 16 February, 2020) #14167

Closed

AmeliaEScott reviewed Feb 20, 2020

View reviewed changes

src/modules/rover_pos_control/RoverPositionControl.hpp Outdated Show resolved Hide resolved

src/modules/rover_pos_control/RoverPositionControl.cpp Outdated Show resolved Hide resolved

src/modules/rover_pos_control/RoverPositionControl.cpp Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Feb 23, 2020

Weekly Digest (16 February, 2020 - 23 February, 2020) #14213

Closed

weekly-digest bot mentioned this pull request Mar 1, 2020

Weekly Digest (23 February, 2020 - 1 March, 2020) #14268

Closed

ealdaz-seesai commented Mar 6, 2020

View reviewed changes

src/modules/rover_pos_control/RoverPositionControl.cpp Outdated Show resolved Hide resolved

src/modules/rover_pos_control/RoverPositionControl.cpp Outdated Show resolved Hide resolved

src/modules/rover_pos_control/RoverPositionControl.hpp Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Apr 19, 2020

Weekly Digest (12 April, 2020 - 19 April, 2020) #14700

Closed

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #14814

Closed

julianoes reviewed May 4, 2020

View reviewed changes

Jaeyoung-Lim requested changes May 7, 2020

View reviewed changes

Improved Hold behaviour

9008f4c

Added local position as a valid source of position

ealdaz-seesai force-pushed the rover-hold branch from 0848c36 to 9008f4c Compare May 8, 2020 00:45

Jaeyoung-Lim approved these changes May 13, 2020

View reviewed changes

julianoes merged commit c96b524 into PX4:master May 13, 2020

RichardHopkirk deleted the rover-hold branch September 3, 2020 13:01

Rover hold/loiter #13767

Rover hold/loiter #13767

Conversation

ealdaz-seesai commented Dec 20, 2019

ealdaz-seesai commented Dec 20, 2019

ealdaz-seesai commented Dec 20, 2019

julianoes commented Feb 14, 2020

AmeliaEScott left a comment

Choose a reason for hiding this comment

julianoes commented Feb 20, 2020

ealdaz-seesai commented Feb 20, 2020 via email

ealdaz-seesai left a comment

Choose a reason for hiding this comment

Jaeyoung-Lim commented Apr 13, 2020

ealdaz-seesai commented Apr 13, 2020

ealdaz-seesai commented Apr 13, 2020 • edited Loading

ealdaz-seesai commented Apr 27, 2020

ealdaz-seesai commented Apr 29, 2020

julianoes left a comment

Choose a reason for hiding this comment

ealdaz-seesai commented May 4, 2020 • edited Loading

ealdaz-seesai commented May 4, 2020

julianoes commented May 5, 2020

julianoes commented May 5, 2020

ealdaz-seesai commented May 5, 2020 via email • edited Loading

julianoes commented May 6, 2020

ealdaz-seesai commented May 6, 2020 via email

ealdaz-seesai commented May 6, 2020

julianoes commented May 7, 2020

Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ealdaz-seesai May 7, 2020 • 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

ealdaz-seesai commented May 8, 2020 • edited Loading

ealdaz-seesai commented May 8, 2020

ealdaz-seesai commented May 11, 2020 • edited Loading

julianoes commented May 13, 2020

Jaeyoung-Lim commented May 13, 2020

julianoes commented May 13, 2020

Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

julianoes commented May 13, 2020

ealdaz-seesai commented May 13, 2020 via email

ealdaz-seesai commented Apr 13, 2020 •

edited

Loading

ealdaz-seesai commented May 4, 2020 •

edited

Loading

ealdaz-seesai commented May 5, 2020 via email •

edited

Loading

ealdaz-seesai May 7, 2020 •

edited

Loading

ealdaz-seesai commented May 8, 2020 •

edited

Loading

ealdaz-seesai commented May 11, 2020 •

edited

Loading