commander: move more static variables to class #13613

dagar · 2019-11-27T18:22:48Z

No description provided.

dagar · 2019-11-27T19:23:21Z

The next little incremental step that would help is if we transition (or kill) the command line helpers to vehicle commands (calibrate, arm, disarm, mode, etc).

https://github.com/PX4/Firmware/blob/214e9c8244bfca72a319694ae222727f658ca6c1/src/modules/commander/Commander.cpp#L334-L491

dagar · 2019-11-27T19:35:18Z

MaEtUgR · 2019-11-27T20:48:27Z

transition (or kill) the command line helpers to vehicle commands

Makes sense, @julianoes also brought that up today. What about the acks? For the developer it's nice to have a response but e.g. for the calibration state command it's safety critical.

commander check

I wouldn't worry about this one too much. I want to run the checks periodically and the commander status will give read-only output about that. Credit to @bkueng , came up when dicussing with him.

VEHICLE_CMD_PREFLIGHT_REBOOT_SHUTDOWN move to commander?

I would say so. The check if it's possible has to be lightweight and when it's actually executed blockage if any is likely not a huge issue anymore.

VEHICLE_CMD_START_RX_PAIR move to commander?

The command executing RC driver should directly acknowledge instead of the commander.

Very good inputs, thanks!!

dagar · 2019-11-27T21:02:00Z

We can update the vehicle command helper to send the command, then poll vehicle_command_ack for a response.

https://github.com/PX4/Firmware/blob/494fe6ab388e5fa8b191347924cabcb8d2349d98/src/modules/commander/Commander.cpp#L169-L198

dagar · 2019-11-27T21:53:50Z

~~I removed the power_button_state callback handling from commander. If anyone still wants it we can find a more appropriate location for it.~~ Updated: I'll save this for a followup.

MaEtUgR

I went through all the changes in great detail. Overall it looks like a very nice improvement of readability, scopes, initialization and parameter handling.

I report the most significant comments that came to my mind the rest are just ideas for further improvement that I can follow up with.

src/modules/commander/Commander.hpp

src/modules/commander/Commander.cpp

src/modules/commander/Commander.hpp

src/modules/commander/Commander.cpp

…iable

dagar · 2019-11-29T18:59:40Z

Thanks for the thorough review. I have a few more commits coming that get everything except vehicle_status, vehicle_status_flags, and actuator_armed into the class.

To get the rest under control (and kill/move the commander low priority loop) what we're missing is a mechanism to specifically enter ARMING_STATE_INIT during calibration, and then back to ARMING_STATE_STANDBY once complete.

jorge789 · 2019-12-02T22:02:52Z

Tested on PixRacer V4:

Flight Card 1

Modes Tested:
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.

Procedure:
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode.
Notes:
No issues were noted, good flight in general.

Logs: https://review.px4.io/plot_app?log=eddc7cef-5af6-40af-b39e-eccaa4d5bed4

Flight Card 2

Modes Tested
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure

Armed form QGC.
Took-off on mission plan mode
. Note that commands were sent form QGC (Armed and takeoff)
Once all waypoint completed vehicle switched to RTL and landed as expected.
Good flight in general.
Note:
No issues were noted, good flight in general.

Logs: https://review.px4.io/plot_app?log=7c08c14f-a8b1-4a77-9996-c52f75517a0d

Flight Card 3
Modes Tested
Position Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint see landing behavior.
Good flight in general.
Note:
No issues were noted, good flight in general.

Logs: https://review.px4.io/plot_app?log=f1d090ec-c406-49ff-b0e8-14ba29b16f94

Flight Card 4
Modes Tested
Fail safe:The radio control turned off and the vehicle returned and landed correctly.
Tested good.

Logs: https://review.px4.io/plot_app?log=faf21f96-c658-4cd0-9bd0-3219d9d6346c

dannyfpv · 2019-12-02T22:33:08Z

Tested on Pixhawk 4 v5 f-450
Flight Card 1

Modes Tested:
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.

Procedure:
Arm and Takeoff in position mode, after flying for approximately one minute, switched to altitude then stabilized mode.
Notes:
No issues were noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=75494cfe-3c3d-42a0-9626-fba45b1f0de4

Flight Card 2

Modes Tested
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure

Armed form QGC.
Took-off on mission plan mode
Note that commands were sent form QGC (Armed and takeoff)
Once all waypoint completed vehicle switched to RTL and landed as expected.
Good flight in general.
Note:
No issues were noted, good flight in general.

Logs:
https://review.px4.io/plot_app?log=3907135f-036d-401e-8ae9-c472817bb2de

Flight Card 3
Modes Tested
Position Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Takeoff in position mode, after flying for approximately one minute, switched to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint see landing behavior.
Good flight in general.
Note:
No issues were noted, good flight in general.

Logs:

https://review.px4.io/plot_app?log=d26ed441-b2d9-4b59-aaa4-4c13b56c483a

Flight Card 4
Modes Tested
Fail-safe:The radio control turned off and the vehicle returned and landed correctly.
Tested good.

log:
https://review.px4.io/plot_app?log=d1655753-fb5c-474d-be17-59790869e2c8

julianoes

Better!

julianoes · 2019-12-06T14:50:03Z

src/modules/commander/Commander.cpp

-		if (TRANSITION_DENIED == arm_disarm(false, true, &mavlink_log_pub, "command line")) {
-			PX4_ERR("rejected disarm");
-		}
+		send_vehicle_command(vehicle_command_s::VEHICLE_CMD_COMPONENT_ARM_DISARM, 0.f, 0.f);


Nice, that's safer!

julianoes · 2019-12-06T14:50:43Z

src/modules/commander/Commander.cpp


 		return 0;
 	}

 	if (!strcmp(argv[1], "takeoff")) {
+		// switch to takeoff mode and arm
+		send_vehicle_command(vehicle_command_s::VEHICLE_CMD_NAV_TAKEOFF);
+		send_vehicle_command(vehicle_command_s::VEHICLE_CMD_COMPONENT_ARM_DISARM, 1.f, 0.f);


Hang on, first you need to arm and then send a takeoff, right?

That's indeed how it was done before: https://github.com/PX4/Firmware/blob/fbcc6a96bc89d14806126c14d70b9a0585dcdb6d/src/modules/commander/Commander.cpp#L412-L413
but I think QGC also switches to takeoff first and then arms.
If you'd ask me a takeoff command should do whatever is necassary to take off...

julianoes · 2019-12-06T14:52:41Z

src/modules/commander/Commander.cpp

+		if (shutdown_if_allowed()) {
+			bool shutdown_ret_val = PX4_ERROR;
+
+			if (((int)(cmd.param1)) == 1) {


No, I think we should be using roundf everywhere for these params.

julianoes · 2019-12-06T14:55:03Z

src/modules/commander/commander_tests/state_machine_helper_test.cpp

 bool StateMachineHelperTest::run_tests()
 {
 	ut_run_test(armingStateTransitionTest);
 	ut_run_test(mainStateTransitionTest);
-	ut_run_test(isSafeTest);


no more isSafe? 😁

sounds good, turns out to be useless 😄

Co-Authored-By: Matthias Grob <[email protected]>

dagar · 2019-12-14T16:36:01Z

@dagar Should I add a commit for my suggestions? Or do we merge since I don't see any blocking problem and I can follow up with my suggestions in a new pr?

In general you should feel free to commit the ones you're sure about. At this point let's try to get the current state merged with most, but not all suggestions applied and keep going with smaller followup PRs.

dagar · 2019-12-14T16:43:07Z

I'm hitting issues with the designated initializers, which don't seem to actually be a part of c++11.

Co-Authored-By: Matthias Grob <[email protected]>

dagar · 2019-12-14T16:58:07Z

@PX4/testflights could we please get another round of testing on this one?

@julianoes @MaEtUgR if you can, please give this PR a quick run through with anything you normally use (eg the command line helpers for takeoff, arm, etc). There's a lot more change in this PR than originally intended, including areas with no code coverage.

Junkim3DR · 2019-12-16T22:31:22Z

Tested on NXP FMUK66 v3

Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode.

Notes
No issues noted, good flight in general.

Logs
https://review.px4.io/plot_app?log=0034b2d7-43ff-4ec7-abab-fcdad5d0cbe7

Modes Tested

Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Take off in mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint see landing behavior.

Notes
Good flight in general.

Logs
https://review.px4.io/plot_app?log=8dc6e083-3bd7-48b3-80ac-3ace1ff36a45

Modes Tested

Position Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint see landing behavior.

Notes
Good flight in general.

Logs
https://review.px4.io/plot_app?log=3b1599f1-4bb7-4304-82ae-282502b02559

jorge789 · 2019-12-16T22:40:08Z

Tested on PixRacer V4:

Maximum wind 10.2 m/s

Flight Card 1

Modes Tested:
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.

Procedure:
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode.
Notes:
Maximum wind 10.2 m / s

Logs: https://review.px4.io/plot_app?log=17f640c2-9418-47bc-8d83-85edcd9f656c

Flight Card 2

Modes Tested
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure

Armed form QGC.
Took-off on mission plan mode
. Note that commands were sent form QGC (Armed and takeoff)
Once all waypoint completed vehicle switched to RTL and landed as expected.
Good flight in general.
Note: Maximum wind 10.2 m / s
Note:When the vehicle was landing, it did not descend vertically, drifted away with the wind and landed the vehicle manually

Logs: https://review.px4.io/plot_app?log=b0f1f872-fc9e-41ee-89e3-4e146862efc7

Flight Card 3
Modes Tested
Position Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint see landing behavior.
Good flight in general.
Note: Maximum wind 10.2 m / s
Note:When the vehicle was landing, it did not descend vertically, drifted away with the wind and landed the vehicle manually
Log: https://review.px4.io/plot_app?log=a8a1391c-06c0-475e-943a-48bea5da2e24

dannyfpv · 2019-12-16T22:44:15Z

Tested on Pixhawk 4 v5 f450
stabilize mode: no issues
position mode: no issues
mission mode: no issues

log:
https://review.px4.io/plot_app?log=239897f0-7312-4206-909a-cf2e38da969b

dagar · 2019-12-17T19:57:22Z

Thanks everyone! I'm going to merge this so we can keep going. Please pay particular attention to any potential commander regressions. We still have quite a lot to do before this module is under control.

dagar · 2019-12-17T20:15:11Z

This pushes the flash slightly over the v2 limit. Looks like I'll need to bring it something like #12601 first.

MaEtUgR · 2019-12-18T16:11:05Z

#13596 might give you some space

Regression from #13613. VEHICLE_CMD_COMPONENT_ARM_DISARM from CLI would enter the ARMING_STATE_IN_AIR_RESTORE logic. This was never intended for disarming (and leads to state machine transition failures), and IMO it is also not intended for commands from CLI.

dagar added the Admin: Cleanup (housekeeping) 🧹 label Nov 27, 2019

dagar requested a review from MaEtUgR November 27, 2019 18:22

dagar force-pushed the pr-commander_minor branch 2 times, most recently from 2e7562d to 494fe6a Compare November 27, 2019 20:58

dagar requested a review from julianoes November 27, 2019 20:59

dagar requested a review from a team November 27, 2019 21:21

dagar force-pushed the pr-commander_minor branch from b09ca4c to e43439b Compare November 27, 2019 21:53

dagar added 4 commits November 28, 2019 13:13

commander: move more static variables to class

57e6ad4

commander: move most params to Param<> and cleanup

7ab4ff2

commander: move more state to class

c12541d

commander: update cli mode and calibrate to use vehicle_command

57ee7f4

dagar force-pushed the pr-commander_minor branch from e43439b to 57ee7f4 Compare November 28, 2019 18:13

MaEtUgR mentioned this pull request Nov 28, 2019

Commander class structure instead of scattered static states #13612

Closed

MaEtUgR reviewed Nov 28, 2019

View reviewed changes

dagar added 7 commits November 29, 2019 13:19

respond to review comments

9e782bd

commander: use hrt_abstime for all timestamps

2c217d9

commander: update arming_requirements to struct and remove static var…

b8d1e86

…iable

Merge remote-tracking branch 'px4/master' into pr-commander_minor

920afe4

commander: move internal_state to class

6319150

commander: move boot timestamp to class

5b68278

WIP: move safety to class and shutdown to main loop

61cea0b

Merge branch 'master' into pr-commander_minor

d2057a2

julianoes reviewed Dec 6, 2019

View reviewed changes

dagar and others added 2 commits December 14, 2019 11:21

Merge branch 'master' into pr-commander_minor

cdf1fb4

Update src/modules/commander/Commander.hpp

82bdb24

Co-Authored-By: Matthias Grob <[email protected]>

Update src/modules/commander/Commander.cpp

abb7539

Co-Authored-By: Matthias Grob <[email protected]>

dagar force-pushed the pr-commander_minor branch from 9d82e9e to abb7539 Compare December 14, 2019 16:46

Merge branch 'master' into pr-commander_minor

34d37c9

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

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

Closed

dagar and others added 5 commits December 23, 2019 12:02

Merge remote-tracking branch 'px4/master' into pr-commander_minor

2f51022

Merge remote-tracking branch 'px4/master' into pr-commander_minor

a7dec80

Merge branch 'master' into pr-commander_minor

c536172

Merge branch 'master' into pr-commander_minor

994c33c

delete unused _main_state_before_rtl

6c957e9

dagar merged commit 0e70578 into master Dec 24, 2019

dagar deleted the pr-commander_minor branch December 24, 2019 04:38

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

Weekly Digest (22 December, 2019 - 29 December, 2019) #13804

Closed

Tony3dr mentioned this pull request Jan 15, 2020

Issue with landing PixRacer FMUV4_K66, FMUv5 #13956

Closed

bkueng mentioned this pull request Feb 20, 2020

commander: fix broken 'commander arm/disarm' CLI #14191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commander: move more static variables to class #13613

commander: move more static variables to class #13613

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019 •

edited

Loading

MaEtUgR commented Nov 27, 2019

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019 •

edited

Loading

MaEtUgR left a comment

dagar commented Nov 29, 2019

jorge789 commented Dec 2, 2019

dannyfpv commented Dec 2, 2019 •

edited by Tony3dr

Loading

julianoes left a comment

julianoes Dec 6, 2019

julianoes Dec 6, 2019

MaEtUgR Dec 6, 2019

julianoes Dec 6, 2019

julianoes Dec 6, 2019

MaEtUgR Dec 6, 2019

dagar commented Dec 14, 2019

dagar commented Dec 14, 2019

dagar commented Dec 14, 2019 •

edited

Loading

Junkim3DR commented Dec 16, 2019

jorge789 commented Dec 16, 2019

dannyfpv commented Dec 16, 2019 •

edited by Tony3dr

Loading

dagar commented Dec 17, 2019

dagar commented Dec 17, 2019

MaEtUgR commented Dec 18, 2019

commander: move more static variables to class #13613

commander: move more static variables to class #13613

Conversation

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019 • edited Loading

MaEtUgR commented Nov 27, 2019

dagar commented Nov 27, 2019

dagar commented Nov 27, 2019 • edited Loading

MaEtUgR left a comment

Choose a reason for hiding this comment

dagar commented Nov 29, 2019

jorge789 commented Dec 2, 2019

Tested on PixRacer V4:

dannyfpv commented Dec 2, 2019 • edited by Tony3dr Loading

julianoes left a comment

Choose a reason for hiding this comment

julianoes Dec 6, 2019

Choose a reason for hiding this comment

julianoes Dec 6, 2019

Choose a reason for hiding this comment

MaEtUgR Dec 6, 2019

Choose a reason for hiding this comment

julianoes Dec 6, 2019

Choose a reason for hiding this comment

julianoes Dec 6, 2019

Choose a reason for hiding this comment

MaEtUgR Dec 6, 2019

Choose a reason for hiding this comment

dagar commented Dec 14, 2019

dagar commented Dec 14, 2019

dagar commented Dec 14, 2019 • edited Loading

Junkim3DR commented Dec 16, 2019

Tested on NXP FMUK66 v3

jorge789 commented Dec 16, 2019

Tested on PixRacer V4:

Maximum wind 10.2 m/s

dannyfpv commented Dec 16, 2019 • edited by Tony3dr Loading

dagar commented Dec 17, 2019

dagar commented Dec 17, 2019

MaEtUgR commented Dec 18, 2019

dagar commented Nov 27, 2019 •

edited

Loading

dagar commented Nov 27, 2019 •

edited

Loading

dannyfpv commented Dec 2, 2019 •

edited by Tony3dr

Loading

dagar commented Dec 14, 2019 •

edited

Loading

dannyfpv commented Dec 16, 2019 •

edited by Tony3dr

Loading