pwm_out_sim: refactor to work queue and use the MixingOutput library #13205

bkueng · 2019-10-16T10:17:15Z

This refactors pwm_out_sim to use the MixingOutput library so that SITL uses the same code as on HW.
It also adds support for the test_motor uORB topic (motor_test CLI command) to MixingOutput (it works in SITL as well).
Next we can interface that to MAVLink, which possibly requires more changes to the uORB topic.

Tested with DShot + FMU PWM, SITL and HITL.

Checking the first output_limited for a disarmed value is fragile. For example a disarmed value might be within the range of min/max output values and could then be triggered while armed.

Tested with SITL + HITL

Does not compile on MacOS.

…outputs pub Required for pwm_out_sim: only publish actuator_outputs when we get actuator_controls. Otherwise lockstep startup does not work. The issue was there before but hidden, due to a long poll timeout. Works with HIL too.

julianoes

Very nice, lots of red!

julianoes · 2019-10-17T12:14:13Z

src/drivers/pwm_out_sim/PWMSim.cpp

 		break;

 	case PWM_SERVO_GET_DEFAULT_UPDATE_RATE:
-		*(uint32_t *)arg = 400;
+		*(uint32_t *)arg = 9999;


Couldn't this be the MAVLink update rate of 250 Hz?

It depends on the use-case and config. There is no good value, and this one makes that clear.

julianoes · 2019-10-17T12:15:12Z

src/drivers/pwm_out_sim/PWMSim.cpp

-			_groups_required = 0;
-		}
-
+		_mixing_output.resetMixerThreadSafe();
 		break;

 	case MIXERIOCLOADBUF: {
 			const char *buf = (const char *)arg;
 			unsigned buflen = strnlen(buf, 1024);


Where does this 1024 value come from?

Let's get rid of it then: 7bbcc15
It's not needed, the buffer is guaranteed to be null-terminated.

julianoes · 2019-10-17T12:18:19Z

src/lib/mixer_module/mixer_module.cpp

@@ -70,6 +73,12 @@ _control_latency_perf(perf_alloc(PC_ELAPSED, "control latency"))
 #endif

 	px4_sem_init(&_lock, 0, 1);
+
+	// Enforce the existence of the test_motor topic, so we won't miss initial publications


Why would we miss initial publications otherwise? I want to understand that.

If the topic does not exist, the subscriber checks for the existence in delayed intervals, which is typically not an issue. We however need to get the data immediately upon first publication here.

src/lib/mixer_module/mixer_module.cpp

src/lib/mixer_module/mixer_module.hpp

julianoes · 2019-10-17T12:26:21Z

src/lib/mixer_module/mixer_module.hpp

@@ -226,6 +252,7 @@ class MixingOutput : public ModuleParams

 	bool _safety_off{false}; ///< State of the safety button from the subscribed _safety_sub topic
 	bool _throttle_armed{false};
+	bool _ignore_lockdown{false}; ///< if true, ignore the _armed.lockdown flag (for HIL outputs)


Why do we do lockdown if when the ignore it again? That seems weird.

Yes, why can't pwm_out_sim be oblivious to HIL?

I initially thought so I well, when I tested HIL and it did not work.
Commander sets lockdown in HIL mode unconditionally. It is an additional safety-precaution, to avoid real actuators from responding. And given the importance of that I find this not a bad idea.

dagar · 2019-10-17T16:42:57Z

I'm now wondering if this should be a "PWM" output sim driver at all.

We do all this work to mix and create PWM outputs, then in both the simulator and mavlink modules we have this chunk of code that based on vehicle type (MAV_TYPE) and typical mixers undoes the PWM scaling, including guessing at servos vs ESCs.

The mixer already has this information, so why not just skip the PWM part entirely and publish the control outputs [-1, 1] ([0, 1] for motors)?

https://github.com/PX4/Firmware/blob/be1f966e5f793206cc418711d17d8b4431d7b6c2/src/modules/simulator/simulator_mavlink.cpp#L78-L171

https://github.com/PX4/Firmware/blob/be1f966e5f793206cc418711d17d8b4431d7b6c2/src/modules/mavlink/mavlink_messages.cpp#L3093-L3167

https://mavlink.io/en/messages/common.html#HIL_ACTUATOR_CONTROLS

mixer.cpp ensures the string is null-terminated (buffer length is 2048).

The assumption is that the system can only ever get into armed state if the safety button is already off.

bkueng · 2019-10-18T08:15:05Z

I'm now wondering if this should be a "PWM" output sim driver at all.

We can look into that, yes (outside the scope of this PR).

dagar · 2019-10-18T17:48:27Z

I'm now wondering if this should be a "PWM" output sim driver at all.

We can look into that, yes (outside the scope of this PR).

That's fair, although it actually came to mind from the comment above discussing PWM_SERVO_GET_DEFAULT_UPDATE_RATE.

bkueng added 2 commits October 16, 2019 12:02

mixer_module: more robust logic to set 'stop_motors' flag

1ee03f0

Checking the first output_limited for a disarmed value is fragile. For example a disarmed value might be within the range of min/max output values and could then be triggered while armed.

refactor mixer_module: move some code in update() into separate methods

64fa01d

bkueng force-pushed the pwm_out_sim_refactor branch from e3c0a9d to 0860638 Compare October 16, 2019 11:36

refactor pwm_out_sim: use mixer_module and run on work queue

94a0cee

Tested with SITL + HITL

bkueng force-pushed the pwm_out_sim_refactor branch from 0860638 to ffbd166 Compare October 16, 2019 12:07

bkueng added 3 commits October 17, 2019 08:01

mixer_module: add support for test_motor (motor_test CLI command)

9122dbf

mixer_module: avoid using an enum as px4::atomic argument

551d24b

Does not compile on MacOS.

bkueng force-pushed the pwm_out_sim_refactor branch from 010bb95 to c9524f5 Compare October 17, 2019 06:01

julianoes self-requested a review October 17, 2019 09:58

julianoes reviewed Oct 17, 2019

View reviewed changes

bkueng added 2 commits October 18, 2019 09:55

MIXERIOCLOADBUF ioctl: remove unnecessary string length restriction

7bbcc15

mixer.cpp ensures the string is null-terminated (buffer length is 2048).

MixingOutput: remove safety button check

a1d5503

The assumption is that the system can only ever get into armed state if the safety button is already off.

dagar approved these changes Oct 19, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #13236

Closed

bkueng merged commit f956baf into master Oct 21, 2019

bkueng deleted the pwm_out_sim_refactor branch October 21, 2019 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pwm_out_sim: refactor to work queue and use the MixingOutput library #13205

pwm_out_sim: refactor to work queue and use the MixingOutput library #13205

bkueng commented Oct 16, 2019

julianoes left a comment

julianoes Oct 17, 2019

bkueng Oct 18, 2019

julianoes Oct 17, 2019

bkueng Oct 18, 2019

julianoes Oct 17, 2019

bkueng Oct 18, 2019

julianoes Oct 17, 2019

dagar Oct 17, 2019

bkueng Oct 18, 2019

dagar commented Oct 17, 2019 •

edited

Loading

bkueng commented Oct 18, 2019

dagar commented Oct 18, 2019

pwm_out_sim: refactor to work queue and use the MixingOutput library #13205

pwm_out_sim: refactor to work queue and use the MixingOutput library #13205

Conversation

bkueng commented Oct 16, 2019

julianoes left a 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

dagar commented Oct 17, 2019 • edited Loading

bkueng commented Oct 18, 2019

dagar commented Oct 18, 2019

dagar commented Oct 17, 2019 •

edited

Loading