High Latency Telemetry Support #8582

acfloria · 2018-01-04T15:13:07Z

Improvements of the Iridiumsbd Driver and add a fallback to the high latency telemetry.

The changes in more details:

Iridiumsbd Driver
- Add parameter for session timeout, read interval, and stacking time
- Add VERBOSE_INFO macro
- Add sbd session timeout
- Update heartbeat after successful SBD session and fill it in the telemetry_status message
- Check test command to avoid being stuck in the test state
- Make class variables private
- Reorder function so that in the header and source file they have the same order
- Fix updating the rssi at a constant rate when no sbd session is scheduled
- Add variable timout to read_at function
Commander Module
- Add the logic for the high latency telemetry fallback
Mavlink Module
- Enable/disable transmitting from all mavlink instances of a certain type using a vehicle_command uorb message.

Switching Logic:

Only activate the high latency telemetry if the vehicle is armed.
If all low latency links are lost and a high latency link is present:
- Activate the high latency telemetry
If the high latency is active and a low latency link is regained:
- Deactivate the high latency telemetry
If all low latency links are lost and a high latency link is not present:
- Telemetry failsafe action
If all low and high latency links are lost:
- Telemetry failsafe action
If a low latency link is regained after all low and high latency links were lost:
- Deactivate the high latency telemetry

TSC21 · 2018-01-04T15:24:00Z

@acfloria this seems very interesting. Question: based on the latest iteration that's ongoing on Mavlink form HIGH_LATENCY msg, would this be ready to actually establish a two-way communication instead of just having the FCU sending the data?

dagar · 2018-01-04T16:08:12Z

Generally looks good. I'll review in detail and test on my side. What testing have you done with real hardware so far?

@TSC21 if you have everything it would be great to test this as well.

dagar · 2018-01-04T16:16:56Z

src/modules/commander/commander.cpp

+				mavlink_log_critical(&mavlink_log_pub, "ALL LOW LATENCY DATA LINKS LOST, ACTIVATING HIGH LATENCY LINK");
+				vehicle_command_s vehicle_cmd;
+				vehicle_cmd.command = vehicle_command_s::VEHICLE_CMD_MAVLINK_ENABLE_SENDING;
+				vehicle_cmd.param1 = 6;


magic numbers

dagar · 2018-01-04T16:17:50Z

src/modules/mavlink/mavlink_main.cpp

+			if (vehicle_cmd.command == vehicle_command_s::VEHICLE_CMD_MAVLINK_ENABLE_SENDING) {
+				if (_mode == (int)round(vehicle_cmd.param1)) {
+					if (_transmitting_enabled != (int)vehicle_cmd.param2) {
+						if ((int)vehicle_cmd.param2)


dagar · 2018-01-04T16:17:56Z

src/modules/mavlink/mavlink_main.cpp

+						if ((int)vehicle_cmd.param2)
+							PX4_INFO("Enable transmitting with mavlink instance of type %s on device %s", mavlink_mode_str(_mode), _device_name);
+						else
+							PX4_INFO("Disable transmitting with mavlink instance of type %s on device %s", mavlink_mode_str(_mode), _device_name);


dagar · 2018-01-04T16:23:37Z

I'm wondering if we need to generalize the link loss handling for all link types, and possibly push more of it into the mavlink module.
#7985

acfloria · 2018-01-11T09:57:38Z

I tested the switching logic and testing the signal strength with the real hardware. I haven't sent any message over the link as I do not have a line rented yet. I will test transmitting data when the overall system is ready for it (Mavlink and QGC)

TSC21 · 2018-01-27T16:16:25Z

@acfloria can you please rebase?

TSC21

It looks good to me. I can test this when I have my line rental activated. This should be synced also with the update on the HIGH_LATENCY Mavlink msg.

TSC21 · 2018-01-27T16:17:00Z

msg/vehicle_status.msg

@@ -62,6 +62,7 @@ bool rc_signal_lost				# true if RC reception lost
 uint8 rc_input_mode				# set to 1 to disable the RC input, 2 to enable manual control to RC in mapping.

 bool data_link_lost				# datalink to GCS lost
+bool high_latency_data_link_active # all low latency datalinks to GCS lost


indentation

TSC21 · 2018-01-27T16:17:16Z

src/drivers/iridiumsbd/IridiumSBD.h

@@ -85,54 +85,14 @@ const char *satcom_state_string[4] = {"STANDBY", "SIGNAL CHECK", "SBD SESSION",

 extern "C" __EXPORT int iridiumsbd_main(int argc, char *argv[]);

-#define SATCOM_TX_BUF_LEN				50		// TX buffer size - maximum for a SBD MO message is 340, but billed per 50
-#define SATCOM_RX_MSG_BUF_LEN			300		// RX buffer size for MT messages
+#define SATCOM_TX_BUF_LEN				340		// TX buffer size - maximum for a SBD MO message is 340, but billed per 50


indentation

TSC21 · 2018-01-27T16:20:21Z

src/drivers/iridiumsbd/iridiumsbd_params.c

+ * @max 300
+ * @group Iridium SBD
+ */
+PARAM_DEFINE_INT32(ISBD_SBD_TIMEOUT, 60);


Is this the time for "datalink" timeout on the high latency link?

TSC21 · 2018-01-27T16:23:30Z

src/modules/commander/commander_params.c

+ * @min 60
+ * @max 3600
+ */
+PARAM_DEFINE_INT32(COM_HLDL_LOSS_T, 120);


I got my answer here.

TSC21 · 2018-01-27T16:24:29Z

src/modules/mavlink/mavlink_main.h

@@ -476,6 +476,7 @@ class Mavlink

 private:
 	int			_instance_id;
+	bool		_transmitting_enabled;


indentation

TSC21 · 2018-01-27T16:29:31Z

@acfloria what's the deal with QRT that lead you to add the conditions?

acfloria · 2018-01-28T21:57:49Z

It looks good to me. I can test this when I have my line rental activated. This should be synced also with the update on the HIGH_LATENCY Mavlink msg.

I will also do some tests and rent a line once the HIGH_LATENCY message is merged on mavlink.

@acfloria what's the deal with QRT that lead you to add the conditions?

It is necessary, as for one build version mavlink is not used. I do not recall by heart which version.

TSC21 · 2018-01-30T12:56:19Z

@acfloria were you able to test this in the mean time with the new msg?

acfloria · 2018-01-30T17:07:32Z

@TSC21 I just added the support for it and tested it in SITL.

TSC21 · 2018-02-05T21:36:12Z

@TSC21 I just added the support for it and tested it in SITL.

I should be able to test this on my end with real HW this week.

vayu-julie · 2018-03-15T16:36:37Z

I tested this on a Pixhawk 3 Pro with a RockBlock Mk2 on a bench. The telemetry switching works as outlined in the switching logic.

acfloria · 2018-03-16T11:42:39Z

@dagar this PR has been extensively tested with real hardware. Can you review the code?

…time sending nothing

- Check if changing to a non standby state if the current state is the standy state and no change is already scheduled. - Add prefix _ to all class variables

…H_LATENCY

Due to the old heartbeat of the high latency telemetry when activating it after flying sufficiently long in normal telemetry it is first detected as lost until the first message is sent. By updating the heartbeat to the current time on switching this issue is avoided. Also includes a small style fix for the HIGH_LATENCY2 stream

… main loop delay

bkueng

I've had a look over the first part of the PR and remarked what caught my eye. Generally looks good.
I'll review the mavlink changes in a second pass.

bkueng · 2018-04-04T15:43:36Z

src/drivers/telemetry/iridiumsbd/IridiumSBD.cpp

+		if (*buffer == MAVLINK_PACKAGE_START) {
+			if (SATCOM_TX_BUF_LEN - _tx_buf_write_idx - (*(buffer + 1) + 8) < 0) {
+				_tx_buf_write_idx = 0;
+				mavlink_log_critical(&_mavlink_log_pub, "Deleting full TX buffer before writing new message");


Is this intended to be user-facing or just to debug?

This is just to debug as in high latency telemetry the status texts are not transmitted.

Ok, but it should not go into a release, as a user has no idea what this means.

Agreed. I am filling the tx_buf field of the telemetry_status correctly so it can be observed by that variable if a reset happens. However, I think for debugging it might be useful to have a status message of the driver where the status can be logged in more details. I will add that as a future PR after this one here is merged.

bkueng · 2018-04-04T15:45:15Z

src/drivers/telemetry/iridiumsbd/IridiumSBD.cpp

+
+	// check if there is enough space for the incoming mavlink message
+	if (buflen == 6) {
+		if (*buffer == MAVLINK_PACKAGE_START) {


This is fragile: it assumes mavlink 1 and a certain write pattern from mavlink (the buflen == 6 check)

I agree, that is why I added the check just after that one to catch anything which does not fit into that scheme.
However, in high latency mavlink 1 should be used because it assures that the HIGH_LATENCY2 message is 50 bytes. This is desired as for example rockblock bills in 50 byte increments.

I'd still rather have it done properly. You can look at how protocol_splitter does it for example: https://github.com/PX4/Firmware/blob/master/src/drivers/protocol_splitter/protocol_splitter.cpp#L358.
At the very least it should be documented that this is a hacky solution and nothing important must depend on this.

Does a user know that he/she should be using mavkink 1 with this device? Is it enforced somewhere?

I adopted it to the way the protocol_splitter is doing it.

Mavlink 1 is not enforced but encouraged as the design of the HIGH_LATENCY2 stream was to create a 50 byte status message. Using Mavlink 2 this goal cannot be met due to the larger header. I added a note in the header file. Once the full high latency telemetry together with QGC is working there should be a documentation about it for example in the dev guide where this information should also be written.

bkueng · 2018-04-04T16:00:26Z

src/modules/commander/Commander.hpp

+			int32_t datalink_loss_timeout, int32_t datalink_regain_timeout, bool *status_changed);
+
+	// telemetry variables
+	int _telemetry_subs[ORB_MULTI_MAX_INSTANCES] = {};


can you create a separate struct for these? Then you need only one array and can use a constructor or member initializer to initialize the fields

Good point. I added that struct.

bkueng · 2018-04-04T16:06:41Z

src/modules/mavlink/mavlink_main.cpp

@@ -988,6 +990,7 @@ Mavlink::send_bytes(const uint8_t *buf, unsigned packet_len)
 		return;
 	}

+


can you please avoid adding unnecessary newlines?

Sure, removed.

bkueng

I realize I have plenty of remarks, but the overall structure looks good.

bkueng · 2018-04-05T07:33:27Z

src/modules/mavlink/mavlink_main.cpp

+
+		if (cmd_sub->update(&cmd_time, &vehicle_cmd)) {
+			if ((vehicle_cmd.command == vehicle_command_s::VEHICLE_CMD_CONTROL_HIGH_LATENCY) && (_mode == MAVLINK_MODE_IRIDIUM)) {
+				if ((vehicle_cmd.param1 > 0.5f)) {


unnecessary brackets (more below)

bkueng · 2018-04-05T07:35:53Z

src/modules/mavlink/mavlink_main.cpp

+					_transmitting_enabled = true;
+					_transmitting_enabled_commanded = true;
+
+				} else if ((vehicle_cmd.param1 <= 0.5f)) {


Just } else { is enough

bkueng · 2018-04-05T07:58:05Z

src/modules/mavlink/mavlink_main.cpp

+		struct vehicle_command_s vehicle_cmd;
+
+		if (cmd_sub->update(&cmd_time, &vehicle_cmd)) {
+			if ((vehicle_cmd.command == vehicle_command_s::VEHICLE_CMD_CONTROL_HIGH_LATENCY) && (_mode == MAVLINK_MODE_IRIDIUM)) {


if this comes from external, someone needs to send an ack. It can be done from commander.

bkueng · 2018-04-05T08:02:41Z

src/modules/mavlink/mavlink_main.cpp

@@ -2201,7 +2236,11 @@ Mavlink::task_main(int argc, char *argv[])
 				msg.target_component = command_ack.target_component;
 				current_command_ack = command_ack.command;

+				// TODO: always transmit the acknowledge once it is only sent over the instance the command is received


Can you fix this?

I have an idea how to fix it, but as it potentially can break things and is not an trivial change I would prefer to do it in a separate PR.

Ok that's fine.

bkueng · 2018-04-06T07:03:48Z

src/drivers/telemetry/iridiumsbd/IridiumSBD.cpp

+		if (*buffer == MAVLINK_PACKAGE_START) {
+			if (SATCOM_TX_BUF_LEN - _tx_buf_write_idx - (*(buffer + 1) + 8) < 0) {
+				_tx_buf_write_idx = 0;
+				mavlink_log_critical(&_mavlink_log_pub, "Deleting full TX buffer before writing new message");


Ok, but it should not go into a release, as a user has no idea what this means.

bkueng · 2018-04-06T08:20:27Z

src/modules/mavlink/mavlink_messages.cpp

+
+		update_tecs_status(_update_rate_filtered);
+
+		update_battery_status(_update_rate_filtered);


Since you're passing a class member _update_rate_filtered, you can avoid adding an argument and use _update_rate_filtered directly in update_battery_status

bkueng · 2018-04-06T08:23:19Z

src/modules/mavlink/mavlink_stream.h

@@ -103,6 +103,11 @@ class MavlinkStream
 	virtual bool send(const hrt_abstime t) = 0;
 #endif

+	/**
+	 * Function to collect/update data for the streams when update() is called.


Can you document this a bit more? Like saying that it's called at a high rate, independent from the configured stream rate.

bkueng · 2018-04-06T08:24:52Z

src/modules/mavlink/mavlink_messages.cpp

@@ -3929,22 +3941,22 @@ class MavlinkStreamMountOrientation : public MavlinkStream
 	}
 };

-class MavlinkStreamHighLatency : public MavlinkStream
+class MavlinkStreamHighLatency2 : public MavlinkStream


since this is so large, it's almost worth splitting it into a separate file.

Sure, I moved it to a separate file

bkueng · 2018-04-06T08:38:57Z

src/modules/mavlink/mavlink_stream.cpp

 		if (sent) {
-			_last_sent = (interval > 0) ? _last_sent + interval : t;
+			_last_sent = ((interval > 0) && ((int64_t)(1.5 * interval) > dt)) ? _last_sent + interval : t;


Can you use 1.5f to avoid converting to double?

bkueng · 2018-04-06T08:44:28Z

src/modules/mavlink/mavlink_stream.cpp

+		// If the interval is non-zero and dt is smaller than 1.5 times the interval
+		// do not use the actual time but increment at a fixed rate, so that processing delays do not
+		// distort the average rate. The check of the maximum interval is done to ensure that after a long time
+		// not sending anything multiple messages in a short time are sent.


The check of the maximum interval is done to ensure that after a long time not sending anything multiple messages in a short time are sent.

Should this not be more like: The check of the maximum interval is done to ensure that after a long time not sending anything, sending multiple messages in a short time is avoided.

Sure, you are right. Changed it to your version

…status text

acfloria · 2018-04-10T15:17:16Z

@bkueng I implemented the changes you requested. Can you review the fixes again?

bkueng

Thanks for the update.

Yes, but this is also currently the default behavior. Any acknowledge will be sent over every instance.

I meant that multiple uorb acks are published. But since you moved this to commander, it's correct now.

Unfortunately not, as this function is used with fields of a packed struct and therefore won't compile using a reference.

Uh, but this is not good, and the compiler complains for good reasons. Passing the member of a packed struct as a pointer can lead to memory access alignment problems (and crashes).

bkueng · 2018-04-11T09:25:18Z

src/drivers/telemetry/iridiumsbd/IridiumSBD.h

+	char _test_command[32];
+	hrt_abstime _test_timer = 0;
+
+	uint8_t _rx_command_buf[SATCOM_RX_COMMAND_BUF_LEN] = {0};


I suggest you change it to = {}. {0} is confusing, as for example using {1} would lead to only the first element being 1 and all others 0.

bkueng · 2018-04-11T09:31:04Z

src/modules/mavlink/mavlink_high_latency2.h

+#include "mavlink_simple_analyzer.h"
+#include "mavlink_stream.h"
+
+#ifndef MAVLINK_HIGH_LATENCY2_H_


I suggest you use #pragma once instead

bkueng · 2018-04-11T09:31:54Z

src/modules/mavlink/mavlink_main.cpp

@@ -2201,7 +2236,11 @@ Mavlink::task_main(int argc, char *argv[])
 				msg.target_component = command_ack.target_component;
 				current_command_ack = command_ack.command;

+				// TODO: always transmit the acknowledge once it is only sent over the instance the command is received


Ok that's fine.

…e function - Addional small cleanup of the iridium driver and the include guards of mavlink module header files

bkueng

Good to go now!

bkueng · 2018-04-11T13:15:24Z

@dagar circleci seems to be broken

acfloria added the Admin: Enhancement (improvement) 💡 label Jan 4, 2018

acfloria requested a review from dagar January 4, 2018 15:13

dagar reviewed Jan 4, 2018

View reviewed changes

acfloria force-pushed the feature/telemetry_fallback branch from 71f290a to a15a1fa Compare January 11, 2018 10:01

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

acfloria changed the title ~~[WIP] High Latency Telemetry Logic~~ [WIP] High Latency Telemetry Support Jan 25, 2018

acfloria force-pushed the feature/telemetry_fallback branch from a15a1fa to 72b184e Compare January 25, 2018 15:44

TSC21 reviewed Jan 27, 2018

View reviewed changes

acfloria force-pushed the feature/telemetry_fallback branch from 72b184e to aabfbd7 Compare January 30, 2018 17:04

acfloria force-pushed the feature/telemetry_fallback branch from e0745ed to d8b7427 Compare January 31, 2018 14:23

acfloria force-pushed the feature/telemetry_fallback branch 2 times, most recently from a2c321b to f7f4276 Compare February 12, 2018 09:54

acfloria force-pushed the feature/telemetry_fallback branch from f7f4276 to 383e924 Compare February 26, 2018 14:54

acfloria force-pushed the feature/telemetry_fallback branch from 06697af to 7d8e591 Compare March 9, 2018 12:13

acfloria changed the title ~~[WIP] High Latency Telemetry Support~~ High Latency Telemetry Support Mar 15, 2018

acfloria force-pushed the feature/telemetry_fallback branch from 086b691 to c1cbaee Compare March 23, 2018 13:13

acfloria added 9 commits April 4, 2018 15:28

MavlinkStream: Ensure that not multiple messages are sent after some …

7b6f731

…time sending nothing

IridiumSBD: Robustify state machine and fix format

2a1b174

- Check if changing to a non standby state if the current state is the standy state and no change is already scheduled. - Add prefix _ to all class variables

Increase the MavLink module stack size

e2dece5

Commander: Improve handling and generating of VEHICLE_CMD_CONTROL_HIG…

9c2a30b

…H_LATENCY

Commander: Move telemetry logic to separate functions

6e4dc11

revert v2 cmake changes

5c65196

Commander: Remove Mavlink from CMakeFile

2015996

Mavlink: Use filtered update rate for HIGH_LATENCY2 stream instead of…

d4fd44d

… main loop delay

acfloria force-pushed the feature/telemetry_fallback branch from 02b248d to d4fd44d Compare April 4, 2018 13:39

bkueng reviewed Apr 4, 2018

View reviewed changes

acfloria added 3 commits April 5, 2018 13:29

Mavlink: Add acknowledge for VEHICLE_CMD_CONTROL_HIGH_LATENCY

b7de11a

Commander: Combine telemetry variables into struct

da63194

Remove unnecessary newlines

de9476f

bkueng reviewed Apr 6, 2018

View reviewed changes

acfloria added 6 commits April 6, 2018 16:04

IridiumSBD: Log tx buffer reset in the telemetry_status instead of a …

9d469b5

…status text

IridiumSBD: Clean up MavLink message parsing

9a8b7e0

Move VEHICLE_CMD_CONTROL_HIGH_LATENCY ack to commander

d2764ef

Move the SimpleAnalyzer to a separate file

e56e9ce

Move HIGH_LATENCY2 stream to separate file and update some comments"

e816741

MavlinkReceiver: Don't start MissionManager in IRIDIUM mode

2a7d5f1

bkueng requested changes Apr 11, 2018

View reviewed changes

Mavlink: Don't use a packed struct as input for the convert_limit_saf…

8097dd6

…e function - Addional small cleanup of the iridium driver and the include guards of mavlink module header files

bkueng approved these changes Apr 11, 2018

View reviewed changes

bkueng merged commit 0caa4dc into PX4:master Apr 11, 2018

okalachev mentioned this pull request Apr 11, 2018

Add new mode AUTO_PRECLAND #9261

Merged

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed

acfloria deleted the feature/telemetry_fallback branch July 10, 2018 13:12

		@@ -988,6 +990,7 @@ Mavlink::send_bytes(const uint8_t *buf, unsigned packet_len)
		return;
		}


		update_tecs_status(_update_rate_filtered);

		update_battery_status(_update_rate_filtered);

High Latency Telemetry Support #8582

High Latency Telemetry Support #8582

Conversation

acfloria commented Jan 4, 2018 • edited by AuterionWrikeBot Loading

TSC21 commented Jan 4, 2018

dagar commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Jan 4, 2018

acfloria commented Jan 11, 2018

TSC21 commented Jan 27, 2018

TSC21 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

TSC21 commented Jan 27, 2018

acfloria commented Jan 28, 2018

TSC21 commented Jan 30, 2018

acfloria commented Jan 30, 2018

TSC21 commented Feb 5, 2018

vayu-julie commented Mar 15, 2018

acfloria commented Mar 16, 2018

bkueng 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng 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

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

acfloria commented Apr 10, 2018

bkueng 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

bkueng left a comment

Choose a reason for hiding this comment

bkueng commented Apr 11, 2018

acfloria commented Jan 4, 2018 •

edited by AuterionWrikeBot

Loading