gs: Fix LNS-initiated time transfers for Basic Station#7858
Closed
reissjason wants to merge 1 commit intoTheThingsNetwork:v3.36from
Closed
gs: Fix LNS-initiated time transfers for Basic Station#7858reissjason wants to merge 1 commit intoTheThingsNetwork:v3.36from
reissjason wants to merge 1 commit intoTheThingsNetwork:v3.36from
Conversation
The timesync request handler incorrectly assumed that a gateway sending a timesync request has access to a PPS source, and disabled LNS-initiated time transfers for that gateway. All Basic Station gateways send timesync requests regardless of PPS/GPS availability, as it is the standard mechanism for correlating the concentrator counter with GPS time. This fix: - Removes the incorrect PPS assumption and keeps time transfers enabled after receiving a timesync request - Tracks xtime and gpstime from recent uplinks in session state - Uses uplink-derived xtime+gpstime in LNS-initiated time transfers per the Basic Station protocol specification For GPS-equipped gateways, this enables direct concentrator-to-GPS time mapping. For non-GPS gateways (which report gpstime=0 in uplinks), the round-trip timesync mechanism remains the primary time source, and LNS-initiated transfers continue providing MuxTime updates. References TheThingsNetwork#4852. Made-with: Cursor
Member
johanstokking
left a comment
There was a problem hiding this comment.
Thanks @reissjason, one small thing and mostly one question.
Comment on lines
+345
to
+346
| if time.Since(rxAt) < 10*time.Second { | ||
| elapsed := time.Since(rxAt) |
Member
There was a problem hiding this comment.
Suggested change
| if time.Since(rxAt) < 10*time.Second { | |
| elapsed := time.Since(rxAt) | |
| if elapsed := time.Since(rxAt); elapsed < maxTimeSyncAfterUplink { |
And declare maxTimeSyncAfterUplink as const.
The time sync happens by default every 200 seconds. There might not be an uplink message in the last 10 seconds every period. On busy gateways, it may take long for time sync to happen.
What's the reasoning for the 10 seconds time since the last uplink? Clock drift?
Author
There was a problem hiding this comment.
Looks like thia is only a partial fix. I'll close this one and make a new request once it ia complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
References #4852
All Basic Station gateways send
timesyncrequests to correlate the concentrator counter with GPS time, regardless of PPS/GPS availability. The current code incorrectly assumes that a gateway sending atimesyncrequest has PPS and disables LNS-initiated time transfers for that session. This prevents non-GPS gateways from receiving periodic time correlation updates.Additionally, the LNS-initiated time transfer ticker passes
nilfor bothgpsTimeandconcentratorTime(with a TODO referencing #4852), so even when transfers are enabled, onlyMuxTimeis sent — never thextime+gpstimepair needed for direct concentrator-to-GPS mapping.Changes
timesyncrequest — both round-trip and direct mapping modes are needed.xtimeandgpstimefrom recent uplinks (jreq/updf) in session state.xtime+gpstime(extrapolated to current time) in LNS-initiated time transfers, per the Basic Station protocol specification.For GPS-equipped gateways, this enables the full direct concentrator↔GPS time mapping that #4852 describes (for frames from the same gateway). For non-GPS gateways, the round-trip
timesyncmechanism remains the primary time source, and the fix ensures it stays active.Note: This is a partial fix for #4852. The full cross-gateway GPS time propagation (transferring GPS time from a PPS-equipped gateway to a non-GPS gateway via overlapping frames) requires frame deduplication at the Gateway Server layer, which is out of scope for this change.
Motivation
We are developing Class B Network-Assisted Synchronization (NAS) for LoRaWAN, which uses multicast ping slot downlinks on non-GPS gateways (MultiTech MTCAP3 with cellular backhaul). Accurate
timesyncis critical for ping slot TX scheduling. Without this fix, the LNS disables time transfers after the firsttimesyncexchange, degrading the concentrator↔GPS correlation over time.Testing
TestTransferTimepasses unchanged.TestTimeSyncDoesNotDisableTransferTime: Verifies thatTransferTimeproduces output after atimesyncrequest (previously it returnednil).TestTransferTimeWithUplinkXTime: Verifies the end-to-end flow of storing uplink timing and using it in time transfers, for both GPS and non-GPS gateways.TestLastUplink: Unit test forUpdateLastUplink/GetLastUplinksession state round-trip.go test ./pkg/gatewayserver/io/semtechws/...passes with zero regressions.Regressions
Not expected. The change is additive — time transfers that were previously disabled are now enabled, and previously nil arguments are now populated when uplink data is available.
Checklist
CHANGELOG.md.CONTRIBUTING.md, there are no fixup commits left.Made with Cursor