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

Sanity check configured durations #270

Open pdubois22 opened 5 years ago

pdubois22 commented 5 years ago

Summary: In the yml config file, we can put a dedup value like

ns: deduplication-window: 400

This value is interpreted as 400ns

in the command line, we have an error: invalid argument "400" for "--ns.deduplication-window" flag: time: missing unit in duration 400

Steps to Reproduce: put a value in a config file.

What do you see now? No error if no unit

It is important because I wanted 400ms and and I got 400ns without an error or a warning. And 400ns is close to zero. In other words, only the first received uplink (the fastest from a backhaul perspective) is taken in account. Subsequent uplinks to other much better gateways are dropped. The first one is almost never the best from a LoRa perspective (the best gateway is the closest from a distance perspective, rarely the fastest)

What do you want to see instead? An error: the stack should not start as this is the case with "--ns.deduplication-window"

How do you propose to implement this? consistent checking possible.

Environment: master as of today.

What can you do yourself and what do you need help with?

htdvisser commented 5 years ago

I'm afraid this is simply because of the way YAML works, and there isn't much we can do about that.

With the following config, 400 is parsed as a number:

ns.deduplication-window: 400

while with the configs below, it's parsed as a string:

ns.deduplication-window: "400"
ns.deduplication-window: 400ms

(note the difference in how Github colors it)

The number 400 is a perfectly valid duration, interpreted by Go as 400 nanoseconds. The string "400" would not be a valid duration, because it doesn't specify a unit. The string "400ms" is a valid duration, and is parsed correctly.

The CLI only works with strings, so a 400 argument is always a string.

The best thing we can do is document this behavior when we add documentation for how to write a config file.

pdubois22 commented 5 years ago

IMHO, anything under 1ms is not valid, except 0 is you really want that behaviour. There could be a warning at least if value >0 and <1ms.