Flight tasks: param refactoring #2 #9255

bkueng · 2018-04-06T09:38:22Z

Same as #9159, but rebased & against master now

@Stifael I suggest we merge this before #9253

Stifael · 2018-04-06T14:40:19Z

looks again good, but I have not paid attention to the mc_pos_control changes yet.

Need to free some flash again though.

FYI #9257 (comment)

MaEtUgR

If the parameter library now works with that client code it's simply genius!!

I looked through pretty much indetail and hopefully didn't miss anything. It looks good except for the _z_vel_i copy pasta error.

MaEtUgR · 2018-04-09T15:06:16Z

src/lib/FlightTasks/tasks/Utility/ManualSmoothingZ.hpp

-	void overwriteAccelerationUp(float acc_max_up) {_acc_max_up = acc_max_up;};
-	void overwriteAccelerationDown(float acc_max_down) {_acc_max_down = acc_max_down;};
-	void overwriteJerkMax(float jerk_max) {_jerk_max = jerk_max;};
+	void overwriteAccelerationUp(float acc_max_up) { _acc_max_up.set(acc_max_up); }


I think these overwritten values are temporary and should not be saved to the parameters in the flash memory right?Is that still the case?

yes, it does not write back to the param store

MaEtUgR · 2018-04-09T15:08:14Z

src/lib/FlightTasks/tasks/FlightTask.hpp

+	/**
+	 * Call this whenever a parameter update notification is received (parameter_update uORB message)
+	 */
+	void handleParameterUpdate()


I'm assuming this method is added such that a task can customize its checks/reactions to parameter changes right?

The primary purpose to call the protected updateParams method. If needed, it can be used for additional checks as well, but it will need to be made virtual.

MaEtUgR · 2018-04-09T15:15:10Z

src/modules/mc_pos_control/mc_pos_control_main.cpp

-		/* since we use filter to detect manual direction change, take half the cutoff of the stick filtering */
-		param_get(_params_handles.rc_flt_cutoff, &(_params.rc_flt_cutoff));
+		ModuleParams::updateParams();
+		SuperBlock::updateParams();


That seems strange to have both... is that temporary because something is still legacy?

#9159 (comment)

MaEtUgR · 2018-04-09T15:17:26Z

src/modules/mc_pos_control/mc_pos_control_main.cpp

+		_vel_p(1) = _xy_vel_p.get();
+		_vel_p(2) = _z_vel_p.get();
+
+		_vel_i(0) = _z_vel_i.get();


xy velocity should have a different integral gain than z.

_vel_i(0) = _xy_vel_i.get(); _vel_i(1) = _xy_vel_i.get();

Thanks, fixed

MaEtUgR · 2018-04-09T15:25:07Z

src/modules/mc_pos_control/mc_pos_control_main.cpp

+		_vel_d(1) = _xy_vel_d.get();
+		_vel_d(2) = _z_vel_d.get();
+
+		_thr_hover.set(math::constrain(_thr_hover.get(), _thr_min.get(), _thr_max.get()));


.set() only sets the parameter locally temporarily right? Altought if it's out of bounds it could be saved back as feedback for the user that the velue he changed actually doesn't apply.

Correct. I thought about it, and we could write it back, but I did not want to change the behavior in this PR.

What does that mean locally? If I there are two modules that both consume the same parameter, and in one of the modules I use the setter method for that parameter, does the other module then have the new value or the default?
Or another scenario: If there is a class member that also uses the same parameter as the base class and the base class overwrites one parameter with the setter method, what is the value of the parameter that the member class will consume?

If I there are two modules that both consume the same parameter, and in one of the modules I use the setter method for that parameter, does the other module then have the new value or the default?

It keeps the previous value. You can use the commit() method to store it persistently and notify all modules about the param change.

If there is a class member that also uses the same parameter as the base class and the base class overwrites one parameter with the setter method, what is the value of the parameter that the member class will consume?

If it accesses the same class member, it will have the updated value. If you define the same param in the subclass as a different class member, it's separate with the same behavior as in the separate modules case.

If you define the same param in the subclass as a different class member, it's separate with the same behavior as in the separate modules case.

good to know. thanks

Plus some style fixes

…ARENT

…p equal strings reduces roughly 130 bytes. Almost enough to pass CI.

Since assertions lead to crashes, we need better failure handling. In all the cases in this patch, the assert is not required. All the ones with the task id should be replaced with the module base class. Ah yes, and this reduces flash space, since the ASSERT macro will expand to a printf that contains the source file name.

…ontrol due to param dependencies

MaEtUgR

Fixed

bkueng · 2018-04-11T05:47:01Z

Circleci failure unrelated (Action failed: brew install px4-dev)

Stifael · 2018-04-11T08:07:37Z

In general, how does a class member, that consumes parameters, knows when a new update of the parameter occurred (for instance the member class SmoothingZ.cpp)?

bkueng · 2018-04-11T08:57:30Z

Until now there was no need for that, but we can easily extend for that by making ModuleParams::updateParams() virtual, and override it in the smoothingZ class for instance.

bkueng mentioned this pull request Apr 6, 2018

Flight tasks: param refactoring #9159

Closed

Stifael mentioned this pull request Apr 6, 2018

[WIP] Flight task auto #9253

Closed

1 task

MaEtUgR requested changes Apr 9, 2018

View reviewed changes

bkueng added 4 commits April 10, 2018 09:41

mc_pos_control: refactor BlockParam -> Param

ae9d59c

flight tasks: refactor BlockParam -> Param and handle param updates

077c97e

flight tasks smoothing: fix style issues

19c6456

flight tasks smoothing: refactor param_find -> Param

f047316

Plus some style fixes

bkueng force-pushed the flight-task-param-refactoring branch from 93d3f2b to ee7d9d6 Compare April 10, 2018 07:42

px4_param.h: add override to _DEFINE_PARAMETER_UPDATE_METHOD_CUSTOM_P…

939984a

…ARENT

bkueng force-pushed the flight-task-param-refactoring branch from ee7d9d6 to 939984a Compare April 10, 2018 07:49

bkueng added 3 commits April 10, 2018 11:42

voted_sensors_update: reduce flash usage by allowing compiler to dedu…

27f5f25

…p equal strings reduces roughly 130 bytes. Almost enough to pass CI.

nuttx_px4fmu-v2_test.cmake: enable modules required to build mc_pos_c…

be13c20

…ontrol due to param dependencies

MaEtUgR approved these changes Apr 10, 2018

View reviewed changes

bkueng merged commit b52a8de into master Apr 11, 2018

bkueng deleted the flight-task-param-refactoring branch April 11, 2018 05:47

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flight tasks: param refactoring #2 #9255

Flight tasks: param refactoring #2 #9255

bkueng commented Apr 6, 2018 •

edited by AuterionWrikeBot

Loading

Stifael commented Apr 6, 2018 •

edited

Loading

MaEtUgR left a comment •

edited

Loading

MaEtUgR Apr 9, 2018

bkueng Apr 10, 2018

MaEtUgR Apr 9, 2018

bkueng Apr 10, 2018

MaEtUgR Apr 9, 2018

Stifael Apr 9, 2018

MaEtUgR Apr 9, 2018

bkueng Apr 10, 2018

MaEtUgR Apr 9, 2018

bkueng Apr 10, 2018

Stifael Apr 11, 2018 •

edited

Loading

bkueng Apr 11, 2018

Stifael Apr 11, 2018

MaEtUgR left a comment

bkueng commented Apr 11, 2018

Stifael commented Apr 11, 2018 •

edited

Loading

bkueng commented Apr 11, 2018

Flight tasks: param refactoring #2 #9255

Flight tasks: param refactoring #2 #9255

Conversation

bkueng commented Apr 6, 2018 • edited by AuterionWrikeBot Loading

Stifael commented Apr 6, 2018 • edited Loading

MaEtUgR left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stifael Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaEtUgR left a comment

Choose a reason for hiding this comment

bkueng commented Apr 11, 2018

Stifael commented Apr 11, 2018 • edited Loading

bkueng commented Apr 11, 2018

bkueng commented Apr 6, 2018 •

edited by AuterionWrikeBot

Loading

Stifael commented Apr 6, 2018 •

edited

Loading

MaEtUgR left a comment •

edited

Loading

Stifael Apr 11, 2018 •

edited

Loading

Stifael commented Apr 11, 2018 •

edited

Loading