Add option to set system time via GCS #7716

patrickelectric · 2018-02-14T16:03:14Z

Fix bluerobotics/ardusub#131

amilcarlucas

Instead of handling the MAVLINK_MSG_ID_SYSTEM_TIME in ArduSub/GCS_Mavlink.cpp how about doing it fully in libraries/GCS_MAVLink/GCS_Common.cpp

That way all vehicles would support it.

patrickelectric · 2018-02-15T16:47:24Z

@amilcarlucas Done.

amilcarlucas

Great, thanks !!!

OXINARF

@peterbarker @WickedShell Do you see any problems with changing the SYSTEM_TIME message time origin?

OXINARF · 2018-02-16T00:21:03Z

libraries/GCS_MAVLink/GCS_Common.cpp

@@ -1743,6 +1743,25 @@ void GCS_MAVLINK::handle_common_gps_message(mavlink_message_t *msg)
    AP::gps().handle_msg(msg);
 }

+void GCS_MAVLINK::handle_system_time_message(mavlink_message_t *msg)
+{
+    static bool system_time_set = false;


I don't think this is good (besides of us trying to not have statics in methods). I think It should be checked at HAL level, I believe we don't want to ever override the system time once it's set.

patrickelectric · 2018-02-16T23:50:35Z

@OXINARF done.

peterbarker

Looks good generally.

peterbarker · 2018-02-18T07:21:32Z

libraries/AP_HAL/Util.cpp

@@ -89,6 +106,11 @@ void AP_HAL::Util::get_system_clock_utc(int32_t &hour, int32_t &min, int32_t &se
    hour = hour_ms / (60 * 60 * 1000);
 }

+bool AP_HAL::Util::system_time_set() const


Maybe system_time_was_set()

peterbarker · 2018-02-18T07:23:58Z

libraries/GCS_MAVLink/GCS_Common.cpp

+void GCS_MAVLINK::handle_system_time_message(mavlink_message_t *msg)
+{
+    // exit immediately if system time already set
+    if (hal.util->system_time_set()) {


This seems a little odd.

Perhaps the clocks are drifting and the GCS wants to bring the clocks back into sync?

The idea behind this PR is to set time once when connecting to the GCS, exactly like the older implementation with GPS.

peterbarker · 2018-02-18T07:24:37Z

libraries/GCS_MAVLink/GCS_Common.cpp

+    hal.util->set_system_clock(packet.time_unix_usec);
+
+    // update signing timestamp
+    GCS_MAVLINK::update_signing_timestamp(packet.time_unix_usec);


Pretty sure stuff will go to hell here if time goes backwards.

If a board have a RTC, or anything that gives the correct time, system_time_set need to be true, like Linux boards.

But the only other place that sets the system time in ardupilot is when the gps is updated, it has no relation to RTC at all.

And the fact that during initialization mavlink packets are being handled, chances are that if the GCS sends this packet it will set the time before the GPS set it.

What if the GCS wants to set the time again, shouldn't it be allowed?

This particular line here looks very wrong btw. If GCS tries to update the time multiple times, we would fail hal.util->set_system_clock() but change the signing timestamp.

The GCS can look at sensor status flags and send SYSTEM_TIME if gps flags indicate that gps isn't connected.
Other alternative is to look at SYSTEM_TIME epoch time coming from autopilot and set time only once if epoch time is zero (happens when gps isn't connected).

This patch only allows to set time only once, the variable system_time_was_set will lock set_system_clock functionality after the first call. Besides that, it appears that this function only needs to be performed once, microsecond accuracy is not critical for system_time, right ?

In the ROV scenario, there are nearing thousands of sub users whose all logs are in 1970 because they cannot access 10k+ gps systems, this patch allows they to have a correct log time

This particular line here looks very wrong btw. If GCS tries to update the time multiple times, we would fail hal.util->set_system_clock() but change the signing timestamp.

This line:

// exit immediately if system time already set if (hal.util->system_time_was_set()) { return; }

This'll allow to set time only once, so this line will not run again.

What if the GCS wants to set the time again, shouldn't it be allowed?

No, it shouldn't be allowed. @patrickelectric is correct that the GCS will only be allowed to set the time once with this patch, and the signing timestamp will only be set once as a result.

this is still wrong. If GCS sends multiple times you hal.util->set_system_clock(packet.time_unix_usec); will fail but a) you will return all went well and b) you will call GCS_MAVLINK::update_signing_timestamp(packet.time_unix_usec); that you shouldn't.

@lucasdemarchi can you say where in the code this will happen ? The logic behind this is to set time once.

If GCS sends multiple times you hal.util->set_system_clock(packet.time_unix_usec); will fail but a) you will return all went well and b) you will call GCS_MAVLINK::update_signing_timestamp(packet.time_unix_usec); that you shouldn't.

I'll rewrite this.
"If GCS sends multiple times you hal.util->set_system_clock(packet.time_unix_usec); will not occurs more than once" and update_signing_timestamp will not occurs.
That's why it have:

// exit immediately if system time already set if (hal.util->system_time_was_set()) { return; }

a) you will return all went well

It's void, there is no return or ACK. The gcs should not care of the result.

you will call GCS_MAVLINK::update_signing_timestamp(packet.time_unix_usec); that you shouldn't.

No, we are not. It happens one time only. And even if we did do it many times, why shouldn't we? Plane calls this method repeatedly, so obviously it is ok.

This still allows time to go backwards, right? Our system time isn't zero when the first mavlink packet comes in, and if it does say zero we'll go backwards (and mavlink2 signing will suffer)

What are the implications for allowing multiple sets of the system time, but only monotonically increasing? AFAICS there's no excuse for any GCS or GPS to give us a time in the future.

Giving a time in the past I have seen; the Sonix board in the SkyViper 2450GPS throws a time estimate at the GPS which is in the past; it may later update that estimate if it gets a better one (e.g. the SkyRocket app connecting and supplying one). That better estimate should (AFAIK) always be in the future. Note that it supplied the information directly to the GPS unit, not via mavlink system_time or similar, but I think the principle is the same - setting the time forward should be allowed.

Yes, that's true. But GPS can do the same thing as the GCS, moving the time backwards. But ardupilot doesn't know if a future time or a passed time is correct, the only way is to trust between the GPS or GCS.

jaxxzer · 2018-02-21T15:52:55Z

libraries/AP_HAL/Util.cpp

+    gettimeofday(&ts, nullptr);
+    return ((long long)((ts.tv_sec * 1000000) + ts.tv_usec));
+#elif CONFIG_HAL_BOARD == HAL_BOARD_CHIBIOS
+    return ST2US(chVTGetSystemTime());


No scaling? This is the same as _ms() above.

ST2MS is for ms, ST2US for us.

jaxxzer · 2018-02-21T16:27:53Z

libraries/AP_HAL_Linux/Util.cpp

@@ -28,6 +28,8 @@ void Util::init(int argc, char * const *argv) {
    saved_argc = argc;
    saved_argv = argv;

+    _system_time_was_set = CONFIG_HAL_BOARD_SUBTYPE == HAL_BOARD_SUBTYPE_LINUX_NONE;


Did you mean CONFIG_HAL_BOARD == HAL_BOARD_LINUX?

No, this is the definition of set_system_clock in linux.

void Util::set_system_clock(uint64_t time_utc_usec) { #if CONFIG_HAL_BOARD_SUBTYPE != HAL_BOARD_SUBTYPE_LINUX_NONE if (system_time_was_set()) { return; } _system_time_was_set = true; timespec ts; ts.tv_sec = time_utc_usec/1000000ULL; ts.tv_nsec = (time_utc_usec % 1000000ULL) * 1000ULL; clock_settime(CLOCK_REALTIME, &ts); #endif }

set_system_clock only works if CONFIG_HAL_BOARD_SUBTYPE != HAL_BOARD_SUBTYPE_LINUX_NONE, therefore _system_time_was_set should be CONFIG_HAL_BOARD_SUBTYPE == HAL_BOARD_SUBTYPE_LINUX_NONE;.

Why is this line necessary? We are saying system_time_is_set for linux_none, but that's not actually the case. If we initialize it to false, it won't change or hurt.

jaxxzer · 2018-02-21T17:49:40Z

libraries/AP_HAL_Linux/Util.cpp

@@ -28,6 +28,8 @@ void Util::init(int argc, char * const *argv) {
    saved_argc = argc;
    saved_argv = argv;

+    _system_time_was_set = CONFIG_HAL_BOARD == HAL_BOARD_LINUX;


Is this redundant? We are in Linux namespace.

Check my last comment.
#7716 (comment)

lucasdemarchi · 2018-02-22T07:23:46Z

Humn... I was pretty sure to have touched this code before. Item (1) from #5548 ... What do you think?

jaxxzer · 2018-02-22T16:14:10Z

Linux and PX4 could share the same code, but that is a nonfunctional change unrelated to this PR. Correct?

patrickelectric · 2018-02-26T19:39:48Z

@lucasdemarchi, true, that's exactly what are you talking about, I believe that more PR will appear in the future to expand functionality and compatibility between boards.

Besides that, I believe that the last updates turn this PR more likely to be accepted.

patrickelectric · 2018-03-08T18:48:11Z

Anything else necessary to merge this ?

jaxxzer

https://github.com/ArduPilot/ardupilot/blob/master/ArduSub/commands.cpp#L151

We should remove this flag and use the new hal.util function to check. Same in Copter and Rover.

patrickelectric · 2018-03-09T16:19:36Z

@jaxxzer Sub, Copter and Rover updated to use HAL.

lucasdemarchi · 2018-03-09T16:44:06Z

libraries/AP_HAL_Linux/Util.cpp

@@ -94,6 +96,11 @@ void Util::_toneAlarm_timer_tick() {
 void Util::set_system_clock(uint64_t time_utc_usec)
 {
 #if CONFIG_HAL_BOARD_SUBTYPE != HAL_BOARD_SUBTYPE_LINUX_NONE
+    if (system_time_was_set()) {
+        return;
+    }


ok, now that the checks are in other places you can remove this check. Let this decision be done by the caller and not by the hal implementation here.

Leaving it in the hal and taking the checks out of vehicle seems like the better choice to me, in order to guarantee that the necessary action is always executed and prevent developer mistakes. The checks were always in vehicle, they are redundant now, and could be removed, but I don't think that this is the desirable action. I don't see a problem with one extra 'if' to be sure, and to make it easier for a human to know what is going on with the code.

From my POV we are spreading it on different HALs in a not obvious way, so I don't agree. The checks are redundant now because each vehicle handles it. It doesn't need to be that way, though. It could be done my common vehicle code.

lucasdemarchi · 2018-03-09T16:49:40Z

libraries/AP_HAL_PX4/Util.cpp

@@ -27,6 +27,7 @@ extern bool _px4_thread_should_exit;
 */
 PX4Util::PX4Util(void) : Util()
 {
+    AP_HAL::Util::_system_time_was_set = false;


why using the upper class names here?

lucasdemarchi · 2018-03-09T16:50:16Z

libraries/AP_HAL_PX4/Util.cpp

@@ -98,6 +99,11 @@ enum PX4Util::safety_state PX4Util::safety_switch_state(void)

 void PX4Util::set_system_clock(uint64_t time_utc_usec)
 {
+    if (AP_HAL::Util::system_time_was_set()) {
+        return;
+    }


this should be removed as well as any check on util classes.

jaxxzer · 2018-04-04T03:07:30Z

Ok, I though this number was used only for timestamping logs, which should not require gps-clock precision. I was not aware of this use case to start missions at exactly the same time, that is a good point. It should be ok if they get their time from the same GCS.

Even if missions start at exactly the same time to the microseconds, everything else from that point is non-deterministic, is there any coordination or synchronization in timing of mission commands after the mission starts? It seems like even if the clocks are off by a few seconds, it won't really make a difference in practice.

Of course, a parameter won't hurt if we need it.

OXINARF

Besides inline comment, the only other thing I don't like is the different behavior in Plane and Tracker - I guess we need to re-think what are the reasons for only setting system time once; in any case, this is pre-existing and shouldn't hold the PR.

A better protocol for time sync would be nice, but as a first step this seems good enough to me.

OXINARF · 2018-04-04T11:32:39Z

ArduCopter/Copter.h

@@ -319,14 +319,14 @@ class Copter : public AP_HAL::HAL::Callbacks {
            uint8_t throttle_zero           : 1; // 15      // true if the throttle stick is at zero, debounced, determines if pilot intends shut-down when not using motor interlock
            uint8_t system_time_set         : 1; // 16      // true if the system time has been set from the GPS


We should remove this since it's not needed anymore. And while at it, just remove the numbers from the comments instead of keeping updating them.

+1 those diffs are annoying

peterbarker · 2018-04-04T21:24:15Z

On Wed, 4 Apr 2018, Jacob Walser wrote: +1 those diffs are annoying

Oh good, it wasn't just me.

rmackay9 · 2018-04-05T03:34:34Z

I think @OXINARF and @peterbarker are happy now so we can merge any time right?

peterbarker · 2018-04-05T05:00:37Z

libraries/GCS_MAVLink/GCS_Common.cpp

+    hal.util->set_system_clock(packet.time_unix_usec);
+
+    // update signing timestamp
+    GCS_MAVLINK::update_signing_timestamp(packet.time_unix_usec);


This still allows time to go backwards, right? Our system time isn't zero when the first mavlink packet comes in, and if it does say zero we'll go backwards (and mavlink2 signing will suffer)

What are the implications for allowing multiple sets of the system time, but only monotonically increasing? AFAICS there's no excuse for any GCS or GPS to give us a time in the future.

Giving a time in the past I have seen; the Sonix board in the SkyViper 2450GPS throws a time estimate at the GPS which is in the past; it may later update that estimate if it gets a better one (e.g. the SkyRocket app connecting and supplying one). That better estimate should (AFAIK) always be in the future. Note that it supplied the information directly to the GPS unit, not via mavlink system_time or similar, but I think the principle is the same - setting the time forward should be allowed.

tridge

as I've mentioned in my comments, I think we need a separate RTC handling library, perhaps called AP_RTClock. I don't think the hal should be the keeper of the time since 1970.

tridge · 2018-04-05T06:38:44Z

libraries/AP_HAL/Util.cpp

@@ -77,6 +77,23 @@ uint64_t AP_HAL::Util::get_system_clock_ms() const
 #endif
 }

+uint64_t AP_HAL::Util::get_system_clock_us() const
+{
+#if defined(__APPLE__) && defined(__MACH__)


this looks quite odd doing ifdefs for system time inside AP_HAL/Util.cpp. Normally we'd make it a virtual function and implement in each HAL.
I actually think this isn't really the job of the HAL though, I think we need a AP_RTClock library for this work

This is the exactly same code that exist in uint64_t AP_HAL::Util::get_system_clock_ms(). I disagree that is necessary an entire library to hold this simple PR.

tridge · 2018-04-05T06:42:40Z

libraries/AP_HAL/Util.cpp

+    gettimeofday(&ts, nullptr);
+    return ((long long)((ts.tv_sec * 1000000) + ts.tv_usec));
+#elif CONFIG_HAL_BOARD == HAL_BOARD_CHIBIOS
+    return ST2US(chVTGetSystemTime());


this is bad in several ways on ChibiOS
First off, its calling a ChibiOS API that is only allowed in normal thread context from a function that users may think is OK from within a lock context or ISR. That could cause a fault (and hard crash)
Secondly, system ticks wrap really fast. So the result will wrap every few seconds on some boards.
Again, I think this should be in a AP_RTClock library, with the HAL providing the minimum information needed. Maybe a hal.util->get_initial_system_time(uint64_t &system_time_us, uint64_t &time_since_boot_us);
that would give two 64 bit timestamps, one in time since boot, the other a real time (since the epoch). It should return a bool, and return false if the system can't provide that time.

Again, this code is exactly the same that exist in uint64_t AP_HAL::Util::get_system_clock_ms(), only converting to us. It's working with pixhawk and I believe that will work with other boards. Besides that, it's not necessary to hold this PR If modifications in chibios or the creation of a new library is necessary.

peterbarker · 2018-04-06T01:13:55Z

On Thu, 5 Apr 2018, Patrick José Pereira wrote: Yes, that's true. But GPS can do the same thing as the GCS, moving the time backwards. With the pixhawk usually it starts with 1970 or 2000, don't remember.

You're absolutely right, and I am worried about the mavlink and HIL GPS backends in that case. A real GPS should never attempt to set the time in the past.

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

patrickelectric · 2018-04-06T01:50:43Z

Removed ChibiOS support.
https://gitter.im/ArduPilot/ardupilot?at=5ac69b6b109bb04332913e3f

patrickelectric · 2018-04-09T00:21:00Z

mavlink/qgroundcontrol#6246 already merged.

rmackay9 · 2018-04-09T01:47:49Z

I think this is stuck because @tridge asked that it be reworked and added to a new AP_RTClock library instead of being in the HAL. It's possible that @peterbarker is willing to do that work, or perhaps @patrickelectric can do it after a discussion with Tridge and Peter?

This is an important feature so I'm personally keen that we get it resolved.

rmackay9 · 2018-04-10T04:39:56Z

Closing this in favour of @peterbarker's PR which includes this one. #8121

amilcarlucas suggested changes Feb 15, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch from c0c5749 to df6be41 Compare February 15, 2018 16:45

amilcarlucas approved these changes Feb 15, 2018

View reviewed changes

OXINARF added the Library label Feb 16, 2018

OXINARF requested review from WickedShell and peterbarker February 16, 2018 00:22

OXINARF reviewed Feb 16, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch from df6be41 to b6bbc48 Compare February 16, 2018 23:12

peterbarker reviewed Feb 18, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch from b6bbc48 to be975cf Compare February 18, 2018 14:21

jaxxzer reviewed Feb 21, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch from be975cf to efd3986 Compare February 21, 2018 17:38

jaxxzer reviewed Feb 21, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch 4 times, most recently from 44fd9f7 to 8a4b55a Compare February 21, 2018 22:13

jaxxzer requested changes Mar 9, 2018

View reviewed changes

lucasdemarchi reviewed Mar 9, 2018

View reviewed changes

lucasdemarchi mentioned this pull request Mar 9, 2018

ChibiOS support - part 6 #7851

Merged

11 tasks

OXINARF requested changes Apr 4, 2018

View reviewed changes

peterbarker requested changes Apr 5, 2018

View reviewed changes

tridge requested changes Apr 5, 2018

View reviewed changes

patrickelectric force-pushed the patrickelectric/fix_#131 branch from b4eaef6 to 5b3925b Compare April 6, 2018 01:49

patrickelectric added 11 commits April 5, 2018 22:49

AP_HAL: Add get_system_clock_us in utils

ce55739

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

GCS_MAVLink: Use get_system_clock_us in mavlink_msg_system_time_send

5047518

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

AP_HAL: Add system_time_was_set

ceac774

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

AP_HAL_Linux: Add system_time_was_set to set system_time once

aa661fe

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

AP_HAL_PX4: Add system_time_was_set to set system_time once

0da5d50

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

AP_HAL_VRBRAIN: Add system_time_was_set to set system_time once

e7493dd

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

Rover: Use HAL system_time_was_set

32c07b6

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

Copter: Use HAL system_time_was_set

5274b04

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

Sub: Use HAL system_time_was_set

9363bf8

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

GCS_MAVLINK: Add SYSTEM_TIME handle

a437f07

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

Copter: Remove ap_t bits counter

24f080f

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

patrickelectric force-pushed the patrickelectric/fix_#131 branch from 5b3925b to 24f080f Compare April 6, 2018 01:53

OXINARF added the DevCallTopic label Apr 9, 2018

peterbarker mentioned this pull request Apr 10, 2018

Set time #8121

Merged

rmackay9 closed this Apr 10, 2018

OXINARF removed the DevCallTopic label Apr 10, 2018

patrickelectric changed the base branch from master to Sub-3.5 April 17, 2018 23:07

patrickelectric changed the base branch from Sub-3.5 to master April 17, 2018 23:15

		@@ -319,14 +319,14 @@ class Copter : public AP_HAL::HAL::Callbacks {
		uint8_t throttle_zero : 1; // 15 // true if the throttle stick is at zero, debounced, determines if pilot intends shut-down when not using motor interlock
		uint8_t system_time_set : 1; // 16 // true if the system time has been set from the GPS

Add option to set system time via GCS #7716

Add option to set system time via GCS #7716

Conversation

patrickelectric commented Feb 14, 2018

amilcarlucas left a comment

Choose a reason for hiding this comment

patrickelectric commented Feb 15, 2018

amilcarlucas left a comment

Choose a reason for hiding this comment

OXINARF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric commented Feb 16, 2018

peterbarker 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

patrickelectric Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

jaxxzer Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric Apr 6, 2018 • 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

patrickelectric Feb 21, 2018 • 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

lucasdemarchi commented Feb 22, 2018

jaxxzer commented Feb 22, 2018

patrickelectric commented Feb 26, 2018

patrickelectric commented Mar 8, 2018

jaxxzer left a comment

Choose a reason for hiding this comment

patrickelectric commented Mar 9, 2018

Choose a reason for hiding this comment

jaxxzer Mar 20, 2018 • 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

jaxxzer commented Apr 4, 2018 • edited Loading

OXINARF 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

peterbarker commented Apr 4, 2018 via email

rmackay9 commented Apr 5, 2018

Choose a reason for hiding this comment

tridge 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

patrickelectric Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

peterbarker commented Apr 6, 2018 via email

patrickelectric commented Apr 6, 2018

patrickelectric commented Apr 9, 2018

rmackay9 commented Apr 9, 2018

rmackay9 commented Apr 10, 2018

patrickelectric Mar 9, 2018 •

edited

Loading

patrickelectric Mar 20, 2018 •

edited

Loading

jaxxzer Mar 20, 2018 •

edited

Loading

patrickelectric Apr 6, 2018 •

edited

Loading

patrickelectric Feb 21, 2018 •

edited

Loading

jaxxzer Mar 20, 2018 •

edited

Loading

jaxxzer commented Apr 4, 2018 •

edited

Loading

patrickelectric Apr 5, 2018 •

edited

Loading