Pr commander msg output cleanup #8175

Stifael · 2017-10-23T12:20:02Z

commander message cleanup
arm/disarm counter cleanup

Stifael · 2017-10-23T12:27:15Z

@MaEtUgR can you have another look at the arming/disarming logic?

MaEtUgR

It's very hard to review when messages and multiple arm logic changes are squashed together. I didn't see anything very obvious... But I would definitely not use static variables for the stick counter.

MaEtUgR · 2017-10-23T12:35:17Z

src/modules/commander/commander.cpp

@@ -937,7 +937,7 @@ bool handle_command(struct vehicle_status_s *status_local, const struct safety_s
 				} else {
 					// Refuse to arm if preflight checks have failed
 					if ((!status_local->hil_state) != vehicle_status_s::HIL_STATE_ON && !status_flags.condition_system_sensors_initialized) {
-						mavlink_log_critical(&mavlink_log_pub, "Arming DENIED. Preflight checks have failed.");
+						mavlink_log_info(&mavlink_log_pub, "Arming DENIED. Preflight checks have failed.");


why is that not critical? how much more critical can it be than not able to fly?

MaEtUgR · 2017-10-23T13:13:52Z

src/modules/commander/commander.cpp

@@ -2736,6 +2734,10 @@ int commander_thread_main(int argc, char *argv[])
 				(status.is_rotary_wing || (!status.is_rotary_wing && land_detector.landed)) &&
 				(stick_in_lower_left || (arm_button_pressed && arming_button_switch_allowed) || arm_switch_to_disarm_transition) ) {

+				/* counter for having stick in disarming position */
+				static unsigned stick_off_counter = 0;


Why did you replace them with static variables? I would not do that, in my opinion this makes it a lot less clear with declaration, initialization and so on.

it limits the scope to this part of the code. the commander already has a lot of global (modular scope) variables.

Stifael · 2017-10-23T14:00:37Z

why would you not use a static variable for this? I am usually against static variables as well, but in this case I think it is appropriate because it restricts the scope to that part of the code that only uses it. otherwise i am quite confident someone else will use that variable for something else

dagar · 2017-10-24T00:12:17Z

I wouldn't worry too much about static variables as commander is currently full of them.
After the v1.7.0 release and this PR is merged I'll take another pass at getting the initial commander class in place and we can get rid of them. #7517

MaEtUgR

I let myself convince in person that the static makes sense for this special case. Could you do a quick sanity check with hardware that basic arming did not by chance catch a bug? This part is pretty safety critical.

bkueng · 2017-10-24T08:28:12Z

src/modules/commander/commander.cpp

@@ -937,7 +937,7 @@ bool handle_command(struct vehicle_status_s *status_local, const struct safety_s
 				} else {
 					// Refuse to arm if preflight checks have failed
 					if ((!status_local->hil_state) != vehicle_status_s::HIL_STATE_ON && !status_flags.condition_system_sensors_initialized) {
-						mavlink_log_critical(&mavlink_log_pub, "Arming DENIED. Preflight checks have failed.");
+						mavlink_log_info(&mavlink_log_pub, "Arming DENIED. Preflight checks have failed.");


bkueng · 2017-10-24T08:45:57Z

src/modules/commander/commander.cpp

@@ -1757,12 +1758,13 @@ int commander_thread_main(int argc, char *argv[])
 	int32_t ef_time_thres = 1000.0f;
 	uint64_t timestamp_engine_healthy = 0; /**< absolute time when engine was healty */

-	int32_t disarm_when_landed = 0;
+	float disarm_when_landed = 0;


Minor: should be = 0.f

In https://github.com/PX4/Firmware/blob/91b923a3db445611c3eeeab15551de9a32d31d87/src/modules/commander/commander.cpp#L1862
This will first cast to uint64_t, dropping any decimal places, and then do the multiplication. It should be:

(hrt_abstime)(disarm_when_landed * 1000000.f)

And in https://github.com/PX4/Firmware/blob/91b923a3db445611c3eeeab15551de9a32d31d87/src/modules/commander/commander.cpp#L2247
It should be made explicit that it's a float:

auto_disarm_hysteresis.set_hysteresis_time_from(false, disarm_when_landed * 1000000.f);

bkueng · 2017-10-24T08:46:34Z

src/modules/commander/commander.cpp

@@ -2033,7 +2035,7 @@ int commander_thread_main(int argc, char *argv[])
 					status_flags.barometer_failure = false;
 					status_changed = true;
 					if (status_flags.ever_had_barometer_data) {
-						mavlink_log_critical(&mavlink_log_pub, "baro healthy");
+						mavlink_log_critical(&mavlink_log_pub, "Barometer healthy.");


mavlink_log_info?

? should I use info?
I just changed the message

agreed. why should we send out message to the user when something is healthy

dagar · 2017-10-25T18:38:17Z

src/modules/commander/commander_params.c

 */
-PARAM_DEFINE_INT32(COM_DISARM_LAND, 0);
+PARAM_DEFINE_FLOAT(COM_DISARM_LAND, -1.0f);


FYI this will reset the parameter to default for anyone that's bothered to configured it. In this case I think we can live with it, but it's something to be aware of.

This lines does not come alone. In this pr there is a feature I implemented which allows to set auto disarm to less than one second (making it float).
see https://github.com/PX4/Firmware/pull/8175/files#diff-1d51a813dcb26f46616f71f28aacc70dR2247
This requires the parameter to be -1 by default because of float comparison so the change is needed. But thanks for pointing out the user perspective side-effects.

are these changes still valid? they seem out of scope of the title of the PR.

MaEtUgR · 2017-11-02T03:57:44Z

I rebased on master and added 61abb5b to make initialization consistent with the usage in the code later on: https://github.com/PX4/Firmware/pull/8175/files#diff-1d51a813dcb26f46616f71f28aacc70dR2234

@bkueng want to have a look again? 👼

MaEtUgR

static stick_counter accepted, float problems solved

bkueng

Good to merge, and thanks for the update @MaEtUgR. But I'm not sure if we want to take this in right before the release.

bkueng · 2017-11-02T08:13:38Z

CI failure is real:

../../src/modules/commander/state_machine_helper.cpp:75:19: fatal error: unused variable 'reason_no_gps_cmd' [-Wunused-const-variable]
static const char reason_no_gps_cmd[] = "no GPS cmd";

MaEtUgR · 2017-11-02T09:54:02Z

No, not before the release, let's make that clear. That's a good catch by CI. I need to go over it again...

dagar · 2018-01-17T12:42:04Z

Please rebase and I'll review.

dagar · 2018-06-13T05:01:06Z

State machine changes continued in #9657

stale · 2019-01-20T17:57:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Antiheavy · 2019-01-20T21:55:04Z