TimeSyncHelper for GPS time synchronization

[liquidraver]

My shot at timesync, because if we have GPS, we shouldn't sync manually.
Tested on G2 repeater.
(enabled on repeaters & room servers with this commit)

[fdlamotte]

Time syncing can already be handled by MicroNMEALocationProvider ...

It only has to be correctly constructed (passing the rtc as a parameter)

ATM time is only synced when gps gets first fix, this could be made periodic

Why not adding a new mechanism for doing that, but I think it should at least be motivated ;)

[liquidraver]

I thought in the future there will be other methods for time sync, not just GPS, so a separate helper could be expanded.
Plus you don't need to edit variant files for this, if the GPS is defined, it will work.

[fdlamotte]

I have two major remarks:

• this seems a little complicated
• your implementation seems to take over the gps

It also blocks the node until a fix is obtained (which in some cases can take forever)

Maybe you could get a simpler solution by controlling the existing LocationProvider ? (while still achieving your goal of genericity) asking it to toggle gps until a fix is obtained ? and triggering a new sync after some time ? (enabling location for the time of the sync ...)

LocationProvider has a sync_time function you just have to call, and it will wait for a new valid fix to sync.

I don't say that the current implementation is perfect (it's been written quite some time ago, I think in february for my custom firmwares), and I'm ok to discuss them and accept changes ... ;)

[liquidraver]

It's complicated for some reasons:

• syncing only every 48 hours to save battery - it uses millis so a rollover guard was needed for uptime >49days
• Initial sync is done before the first advert after boot
• remembers GPS state before sync, so if any other stuff uses it, it won't interfere (switches it off if it was on etc)
• it waits for 3 fixes with 3 sats to be precise

What do you mean it takes over the GPS?
What do you mean it blocks the node? it has a 30 sec timeout for a fix and if no fix achieved it retries after 48 hours, how would it block the node?

[fdlamotte]

Please, be assured I just want to help you get the functionality merged ;)

At the present state (without getting more involved in your code and for that I'd need to be convinced, which I'm not yet), I only guess from what I overread, asking for some details ...

I'm sorry, but when I read:

  while (!TimeSyncHelper::isInitialSyncCompleted()) {
    TimeSyncHelper::process();
    delay(100);
  }
  the_mesh.sendSelfAdvertisement(16000);  

my first guess is that you block the repeater until the initial sync is completed ;)

Let's say it's ok, then, If you stop waiting gps after 30s at boot (but I prefer a repeater that boots directly and is not synced than waiting 30s). Maybe you could trigger an advert after sync (or delay the first advert)

I've seen you have your own instance of MicroNMEA and you directly read gps from serial port, if its not taking over gps, I don't know what it is ? And there you assume that gps is on Serial1, which might not be the case?

I really think you'd better rely on the MicroNMEALocationProvider, which already has a MicroNMEA instance and reads the gps port that has been configured for the board. It has also a simple timesync process that you could help improve

Once again, my wish is that you get your functionality in, proposing ways to better integrate with current codebase. In current state I doubt @ripplebiz would merge it but I might be wrong ...

Maybe I should have started by saying your code is interesting (which is true) before critisizing it ... but I feel 1/ the functionality is needed 2/ there are some issues to address before integrating it

[liquidraver]

I am very assured you want to help don't worry, even if my mediocre english is not reflecting it :)

That doesn't block the repeater until the first sync, the TimeSyncHelper::process() is non-blocking, at least I think, because I specifically tried to write it not to. I tested it (altough only on my G2 repeater).

I try to look into modifying micronmeaprovider but I wanted a separate instance so if we, say, try to sync from the mesh, or NTP in the future, everything will be in one place and location provider is providing location only as the name says :)

[fdlamotte]

That doesn't block the repeater until the first sync, the TimeSyncHelper::process() is non-blocking, at least I think, because I specifically tried to write it not to. I tested it (altough only on my G2 repeater).

was just trying to tell you I could not guess before going further in isInitialSyncComplet

I try to look into modifying micronmeaprovider but I wanted a separate instance so if we, say, try to sync from the mesh, or NTP in the future, everything will be in one place and location provider is providing location only as the name says :)

I find it nice to have a helper that handles synchronizing (and which can do it from several sources). But I think for the gps, it could delegate some part to the process that handles the gps. There is some balance to find here ...

[liquidraver]

I try to refactor, so some commands will be delegated, but I need to be careful, because location is a sensitive data and I really don't wanna touch anything that gives out location.

[fdlamotte]

this is definitely going the right way ...

Now you talk about giving location, I understand your point. But here we should be sure there are barriers in case we don't want to leak locations, and those should be between the location provider and the advertisements ;) (did not think about it yet)

I'm wondering why you chose to have all TimeSyncHelper members static ? It has some advantages, but that is not the way other helper classes are constructed. I would prefer to have objects used there for more consistency throughout the code (even if I agree that there this will probably end as a single object ;))

[fdlamotte]

_location is only present in EnvironmentSensorManager so you make an assumption on the sensor_manager that is here ... could be more generic by using this kind of func to get to the gps (I find it complicated but that's what we have for now):

MeshCore/examples/companion_radio/ui-new/UITask.cpp

Lines 750 to 770 in 95d1f05

	void UITask::toggleGPS() {
	if (_sensors != NULL) {
	// toggle GPS on/off
	int num = _sensors->getNumSettings();
	for (int i = 0; i < num; i++) {
	if (strcmp(_sensors->getSettingName(i), "gps") == 0) {
	if (strcmp(_sensors->getSettingValue(i), "1") == 0) {
	_sensors->setSettingValue("gps", "0");
	soundBuzzer(UIEventType::ack);
	showAlert("GPS: Disabled", 800);
	} else {
	_sensors->setSettingValue("gps", "1");
	soundBuzzer(UIEventType::ack);
	showAlert("GPS: Enabled", 800);
	}
	_next_refresh = 0;
	break;
	}
	}
	}
	}

I think we could add a more simple getter to access sensors from their name in the code ...

Once you have a link to the locationprovider, it can be a member of your class for later use ...

[fdlamotte]

Why not letting the repeater work even if the sync has not been done ?

And let TimeSyncHelper send the alert when ready ?

An idea (that has to be discussed) could be to make TimeSyncHelper manage the adverts ? (might be a bad idea, don't really know ...)

[liquidraver]

maybe let it manage only the initial boot-time advert... I'll think about some solution

[fdlamotte]

The target might not use EnvironmentSensorManager ...

the type is defined in target.h ...

[liquidraver]

yeah, it is getting way over my head sorry.
maybe I try it after the whole sensors and GPS is cleaned up and refactored to a much more uniform and simpler solution.
I never really coded and don't want to use LLM's to write code I can't understand after.

[liquidraver]

I did not want to commit it yet, sorry, it's just the first try.
I'll try to refine it and test it.

[fdlamotte]

Please take your time, there is no need to rush

And try to simplify as much as possible ;)

[liquidraver]

Closing this for now, as I can’t make it fully universal yet, and I don’t want to add an unnecessary sidetrack to the codebase.