TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
975 stars 306 forks source link

Basicstation does not implement reftime for dntxed message #4804

Closed KrishnaIyer closed 2 years ago

KrishnaIyer commented 2 years ago

Summary

Basicstation does not implement reftime for dntxed message. And it never did apparently https://github.com/lorabasics/basicstation/blob/f8b9dcb98ca5d3aabc1bb619b9648664d2ecdf47/src/s2e.c#L247-L256 🤦

reftime is a part of the RadioMetadata object which is a part of only JoinRequest, Uplink and Proprietary frames. However this is not documented; https://doc.sm.tc/station/tcproto.html#radio-metadata

Also the RTT section only indicates "uplink" which we assumed to mean upstream. This is ambiguous as not just "uplinks" send reftime; https://doc.sm.tc/station/tcproto.html#monitoring-round-trip-times

The result is that we just emit a bunch of warnings that refTime is unmarshed as 0 and is out of bounds.

Steps to Reproduce

  1. Connect an LBS gateway
  2. Check logs for the dntxed message.

What do you see now?

{
    "level": "warn",
    "msg": "Gateway reported reftime greater than the valid maximum. Skip RTT measurement",
    "delta": 1635512940.4123478,
    "endpoint": "/traffic/:id",
    "gateway_uid": "",
    "method": "GET",
    "namespace": "gatewayserver/io/ws",
    "received_at": 1635512940.4123478,
    "ref_time": 0,
    "remote_addr": "77.248.84.105:44390",
    "request_id": "01FK62NJWE9FT97YD14DAQK92T",
    "upstream_type": "dntxed",
    "url": ""
}

What do you want to see instead?

This message is removed.

Environment

TTS v3.16.0

How do you propose to implement this?

There are two separate things

  1. LBS documentation should make it clear that refTime is sent for all upstream messages except dntxed for some reason.
  2. We should revisit how we calculate RTT. We started using only dnmsg:dntxed pair for this since long times of no traffic can badly skew the reftime.

@johanstokking; My Proposal is to drop using refTime altogether and just calculate RTT directly on the server. Doing this with the gateway reference has only caused us issues. The spec and the implementation differ. And some vendors do whatever they want with these fields.

How do you propose to test this?

with some gateways

Can you do this yourself and submit a Pull Request?

Yes but let's first discuss.

adriansmares commented 2 years ago

The result is that we just emit a bunch of warnings that refTime is unmarshed as 0 and is out of bounds.

Specifically, every transmission acknowledgement causes that warning. No RTT is recorded since we're recording it for the only message in the whole protocol that doesn't use a RefTime.


I have revisited https://github.com/TheThingsIndustries/lorawan-stack-support/issues/203#issuecomment-732805329 and I have a couple of comments:

adriansmares commented 2 years ago

Closed in #4805