Move calibration from commander to event framework #8706

sugnanprabhu · 2018-01-16T15:55:09Z

This patch moves all calibration routines out of commander to events framework and removes commander_low_prio_loop.

Includes following set of changes.

On receiving calibration request
i. commander does some minimal handling and move the arming state to ARMING_STATE_INIT.
ii. event framework waits until the arming state is moved to ARMING_STATE_INIT before timeout of 2secs.
event framework spawns a px4 task to handle the calibration and make sure only one calibration is active.
event framework sends a calibration_status orb message after the completion of the calibration.
Commander will move the arming state back to ARMING_STATE_STANDBY once it receives calibration_status.
This is one of the part of t

Rebase of #8200 #8630

sugnanprabhu · 2018-01-16T16:50:16Z

src/modules/events/calibration/calibration_routines.cpp

@@ -59,7 +59,7 @@

 #include "calibration_routines.h"
 #include "calibration_messages.h"
-#include "commander_helper.h"
+#include <commander/commander_helper.h>


@dagar
airspeed_calibration.cpp and calibration_routines.cpp are using following routines
tune_neutral,
tune_negetive,
tune_positve,
set_tune,
rgbled_set_color_and_mode

As the routines are used by commander as well, we have three options.

Have same routines in calibration, which will lead to duplication of code (This might need some testing of all the cases).

include "commander/commander_helper.h" in airspeed_calibration.cpp and calibration_routines.cpp for now.

commander_helper.cpp mainly has led and tune related code, move this code to someother location and use it in both module (commander and callibration).

expose tunes via uORB - Tunes library #8679

r0gelion · 2018-01-17T20:06:26Z

This PR is the continuation of the PR #8630 which is closed and had @px4/testflights as a reviewer. Please add this reviewer when the PR is ready for flight testing.

sugnanprabhu · 2018-01-18T09:50:33Z

Changes made depends on #8679, build is expected to fail.
But tested the changes applying the patches on top #8679

dagar · 2018-01-29T14:47:50Z

@sugnanprabhu the tunes library is finally in! If you have time this week let's sync up and work on finally getting this merged.

sugnanprabhu · 2018-01-29T16:29:29Z

sure @dagar I will rebase the changes and run some basic tests.

dagar · 2018-01-29T17:29:20Z

Great! I'll test and review on my end as soon as this PR is updated.

sugnanprabhu · 2018-01-30T13:50:52Z

@dagar
I have rebased the changes. but I'm observing one issue related to led which I'm debugging.
Earlier if one of the sensor is not calibrated, the led used to blink red and fast but now it is blinking normal blue.

sugnanprabhu · 2018-01-31T10:41:43Z

@dagar

Found a bug in state_machine_helper.cpp, please correct me if this was intentional.

arming_state_transition is setting condition_system_sensors_initialized to 1 even if the preflight check fails,
status_flags->condition_system_sensors_initialized = (prearm_ret == OK);
https://github.com/PX4/Firmware/blob/master/src/modules/commander/state_machine_helper.cpp#L191

The reason why this bug goes hidden is that, condition_system_sensors_initialized is set to 0 in commander_low_prio_thread just before calling arming_state_transition
https://github.com/PX4/Firmware/blob/master/src/modules/commander/commander.cpp#L4266

arming_state_transition is also called from commander main thread, which is is running on higer priority, so even before the arming_state_transition is called in the commander_low_prio_thread, it is called in the following.
so condition_system_sensors_initialized is still 1 and the buggy code never executes as the second call to arming_state_transition sees that the state is already changed to ARMING_STATE_STANDBY.

https://github.com/PX4/Firmware/blob/master/src/modules/commander/commander.cpp#L2316

sugnanprabhu · 2018-01-31T10:47:15Z

src/modules/commander/state_machine_helper.cpp

@@ -330,7 +330,7 @@ transition_result_t arming_state_transition(vehicle_status_s *status,
 				}

 				feedback_provided = true;
-				valid_transition = false;
+				valid_transition = true;


Preflight check during transition to ARMING_STATE_STANDBY shouldn't cause the state transition to fail.

Why not, isn't that its purpose?

ARMING_STATE_STANDBY = preflight checks passed?

Hi @dagar, @mhkabir
Sorry for the late reply, I'm out of country and do not have access to mail.

When I added this change I was facing some timing issue related to arming_state_transition.
The timing issue stayed hidden when the arming_state_transition is called from the low_prio_thread. As the calibration was moved to the the main thread, it was retuning TRANSITION_DENIED for the following.
https://github.com/PX4/Firmware/pull/8706/files#diff-1d51a813dcb26f46616f71f28aacc70dR1079

I already explained about the timing issue that was observed earlier in the following comment, when I made this change. Please note that the line numbers are changed in the mainline code and the line numbers mentioned in the comment do not match any more.
#8706 (comment)

I see recently there has been many fixes in state_machine_helper.cpp, I think removing this condition might work now. I can check on the actual hardware only after I return in few days.

sugnanprabhu · 2018-02-08T04:40:35Z

@dagar did you get a chance to try the changes, can this changes be tested by the testflight team?

dagar · 2018-02-08T13:48:56Z

Overall it's very close to mergeable, but I need to sit down and go through the intended LED and buzzer behaviour.

LorenzMeier · 2018-02-09T21:42:18Z

@PX4/testflights Could you please test a fresh vehicle setup and flight and see if it checks out?

Avysuarez · 2018-02-09T23:21:34Z

flight with pixhawk Pro (V4pro) Good flight, no issues.
https://review.px4.io/plot_app?log=d498f55e-3312-418d-a4bc-c21a7f479ab9

flight with pixhawk1 (V2) Good flight, no issues
https://review.px4.io/plot_app?log=e71d8198-7e5b-4817-b9e3-e09c44e20512

dannyfpv · 2018-02-09T23:26:39Z

test fligth pix mini (v3) flight better
https://logs.px4.io/plot_app?log=690932bb-93af-4f14-a5f7-decd7dbb3d60

dagar · 2018-03-07T05:15:10Z

I rebased and made one small change. The main thing we need to do here is go through all the intended buzzer and led usage and ensure it's actually working correctly.

This patch moves all the calibration routines out of commander to events framework. Also removes low prio thread from commander. Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

arming_state_transition function in state_machine is setting condition_system_sensors_initialized to wrong value if condition_system_sensors_initialized is set to 0 before calling it. Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

dagar · 2018-03-29T16:30:46Z

I'm not quite sure about the vehicle_command interaction here. Both commander and events seem to be responding to the same command in some situations.

I think events need to completely own the calibration commands, and use some other mechanism to ask commander to enter ARMING_STATE_INIT, wait for verification, run calibration, then end. During calibration events should periodically tell commander what's happening to prevent it from running preflight checks again and trying to transition to ARMING_STATE_STANDBY.

mhkabir · 2018-04-16T05:14:05Z

@sugnanprabhu Any updates here based on @dagar's comments?

Tony3dr · 2018-07-04T17:27:07Z

@dagar do you need @PX4/testflights to test this pull request again?

stale · 2019-01-20T05:06:25Z

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.

stale · 2019-02-04T00:04:17Z

Closing as stale.

sugnanprabhu commented Jan 16, 2018

View reviewed changes

sugnanprabhu force-pushed the mv_calibration branch 2 times, most recently from 433cc5c to 62145a4 Compare January 18, 2018 09:45

dagar self-assigned this Jan 29, 2018

dagar added the Admin: Enhancement (improvement) 💡 label Jan 29, 2018

dagar added this to the Release v1.8.0 milestone Jan 29, 2018

sugnanprabhu force-pushed the mv_calibration branch from 62145a4 to 95e9f84 Compare January 30, 2018 13:37

sugnanprabhu commented Jan 31, 2018

View reviewed changes

LorenzMeier requested a review from a team February 9, 2018 21:42

dagar force-pushed the mv_calibration branch from acd9a36 to 9df3d6b Compare March 7, 2018 01:32

dagar force-pushed the mv_calibration branch from 6815fb7 to a960743 Compare March 29, 2018 01:31

sugnanprabhu added 3 commits March 29, 2018 12:17

Move all calibration code from commander to events

7d768f8

This patch moves all the calibration routines out of commander to events framework. Also removes low prio thread from commander. Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

Reformatting older calibration code with astyle

50055e3

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

Move other calibration code out of commander

25c1615

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

sugnanprabhu and others added 5 commits March 29, 2018 12:17

Reformatting older calibration code with astyle

20dd3d4

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

Adding calibration routines based on tunes and led uORB

469068b

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

Fixing compilation issue after rebase

4d1cddb

Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s@intel.com>

events CMD_PREFLIGHT_CALIBRATION sleep in loop

26804b6

dagar force-pushed the mv_calibration branch from a960743 to 26804b6 Compare March 29, 2018 16:24

dagar removed this from the Release v1.8.0 milestone May 4, 2018

dagar removed the request for review from a team September 19, 2018 23:00

stale bot added the Admin: Wont fix label Jan 20, 2019

weekly-digest bot mentioned this pull request Jan 27, 2019

Weekly Digest (20 January, 2019 - 27 January, 2019) #11307

Closed

stale bot closed this Feb 4, 2019

MaEtUgR mentioned this pull request Nov 27, 2019

Commander class structure instead of scattered static states #13612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move calibration from commander to event framework #8706

Move calibration from commander to event framework #8706

sugnanprabhu commented Jan 16, 2018 •

edited by AuterionWrikeBot

Loading

sugnanprabhu Jan 16, 2018

dagar Jan 17, 2018

r0gelion commented Jan 17, 2018

sugnanprabhu commented Jan 18, 2018 •

edited

Loading

dagar commented Jan 29, 2018

sugnanprabhu commented Jan 29, 2018

dagar commented Jan 29, 2018

sugnanprabhu commented Jan 30, 2018

sugnanprabhu commented Jan 31, 2018

sugnanprabhu Jan 31, 2018

dagar Mar 29, 2018

sugnanprabhu Apr 19, 2018 •

edited

Loading

sugnanprabhu commented Feb 8, 2018

dagar commented Feb 8, 2018

LorenzMeier commented Feb 9, 2018

Avysuarez commented Feb 9, 2018

dannyfpv commented Feb 9, 2018

dagar commented Mar 7, 2018

dagar commented Mar 29, 2018

mhkabir commented Apr 16, 2018

Tony3dr commented Jul 4, 2018

stale bot commented Jan 20, 2019

stale bot commented Feb 4, 2019

Move calibration from commander to event framework #8706

Move calibration from commander to event framework #8706

Conversation

sugnanprabhu commented Jan 16, 2018 • edited by AuterionWrikeBot Loading

sugnanprabhu Jan 16, 2018

Choose a reason for hiding this comment

dagar Jan 17, 2018

Choose a reason for hiding this comment

r0gelion commented Jan 17, 2018

sugnanprabhu commented Jan 18, 2018 • edited Loading

dagar commented Jan 29, 2018

sugnanprabhu commented Jan 29, 2018

dagar commented Jan 29, 2018

sugnanprabhu commented Jan 30, 2018

sugnanprabhu commented Jan 31, 2018

sugnanprabhu Jan 31, 2018

Choose a reason for hiding this comment

dagar Mar 29, 2018

Choose a reason for hiding this comment

sugnanprabhu Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

sugnanprabhu commented Feb 8, 2018

dagar commented Feb 8, 2018

LorenzMeier commented Feb 9, 2018

Avysuarez commented Feb 9, 2018

dannyfpv commented Feb 9, 2018

dagar commented Mar 7, 2018

dagar commented Mar 29, 2018

mhkabir commented Apr 16, 2018

Tony3dr commented Jul 4, 2018

stale bot commented Jan 20, 2019

stale bot commented Feb 4, 2019

sugnanprabhu commented Jan 16, 2018 •

edited by AuterionWrikeBot

Loading

sugnanprabhu commented Jan 18, 2018 •

edited

Loading

sugnanprabhu Apr 19, 2018 •

edited

Loading