util: Remove hardware specific values from BS router_config #2131

KrishnaIyer · 2020-03-11T16:45:24Z

Summary

Closed #2130

Changes

Add omitempty to necessary fields and set type-default values to them.
Update tests.

Notes for Reviewers

Pending testing on metal.

Checklist

Scope: The referenced issue is addressed, there are no unrelated changes.
Compatibility: The changes are backwards compatible with existing API, database and configuration, according to the stability commitments in README.md.
Testing: The changes are covered with unit tests. The changes are tested manually as well.
Documentation: Relevant documentation is added or updated.
Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

CHANGELOG.md

htdvisser · 2020-03-12T08:13:07Z

pkg/pfconfig/shared/shared.go

@@ -33,7 +33,7 @@ import (
 // SX1301Config contains the configuration for the SX1301 concentrator.
 type SX1301Config struct {
 	LoRaWANPublic       bool
-	ClockSource         uint8
+	ClockSource         uint8 `json:"omitempty"`


Your json tag now renames this field to omitempty

This has no effect, because SX1301Config has a MarshalJSON implementation. That's where you need to change this.

You need to make this a *uint8 so that we can make a distinction between 0 and "omit"

pkg/pfconfig/shared/shared.go

johanstokking · 2020-03-12T17:16:35Z

pkg/util/pointers/pointers.go

+import "time"
+
+// Bool returns a boolean pointer for the given value.
+func Bool(v bool) *bool { return &v }


Let's please not do this

If this is a one-time thing you could just oneline this. E.g.

lorawan-stack/cmd/internal/shared/networkserver/config.go

Line 36 in 9be9736

ClassBTimeout: func(v time.Duration) *time.Duration { return &v }(time.Minute),

Otherwise just have a non-exported boolPtr next to the usage.

Why not? Then I need to define the same boolPtr and functions for each type in 4 different files.

Yes, that is inherent to Go

johanstokking · 2020-03-12T17:17:06Z

pkg/pfconfig/basicstationlns/basicstationlns.go

@@ -116,10 +116,13 @@ func GetRouterConfig(bandID string, fps map[string]*frequencyplans.FrequencyPlan
 		// These fields are not defined in the v1.5 ref design https://doc.sm.tc/station/gw_v1.5.html#rfconf-object and would cause a parsing error.
 		sx1301Conf.Radios[0].TxFreqMin = 0
 		sx1301Conf.Radios[0].TxFreqMax = 0
-		// Remove hardware specific values that are not necessary.
+		// Remove hardware specific values that are handled by the gateway itself.
+		sx1301Conf.ClockSource = nil


Isn't the underlying problem that we have radio specific fields in the frequency plan?

Well these fields are valid SX1301_conf fields. Issue is that basic station specifically already has a config file on each hardware and hardware specific fields are already present there.

But isn't that true for other gateways too?

Most gateways use what's provided in the global_conf.json right? Some have overrides in local_conf.json but not all of them afaik.

What I understand from #2130 is that we send information too specific about a gateway to a gateway. That makes me think that we have fields in our frequency plans that don't belong there. So removing those from BSLNS as you do here is a fix, but it's a fix for the symptoms. We may need to remove these fields from the frequency plans, pfconfig and GCS.

rvolosatovs · 2020-03-13T09:08:44Z

pkg/pfconfig/shared/shared.go

@@ -285,7 +286,7 @@ func BuildSX1301Config(frequencyPlan *frequencyplans.FrequencyPlan) (*SX1301Conf
 	conf := new(SX1301Config)

 	conf.LoRaWANPublic = true
-	conf.ClockSource = frequencyPlan.ClockSource
+	conf.ClockSource = pointers.Uint8(frequencyPlan.ClockSource)


Suggested change

conf.ClockSource = pointers.Uint8(frequencyPlan.ClockSource)

conf.ClockSource = &frequencyPlan.ClockSource

rvolosatovs · 2020-03-13T09:09:31Z

pkg/pfconfig/shared/shared_test.go

@@ -88,25 +89,24 @@ func TestSX1301Conf(t *testing.T) {
 			},
 			SX1301Config{
 				LoRaWANPublic: true,
-				ClockSource:   1,
+				ClockSource:   pointers.Uint8(1),


Suggested change

ClockSource: pointers.Uint8(1),

ClockSource: func(v uint8) *uint8 { return &v }(1),

rvolosatovs · 2020-03-13T09:10:06Z

pkg/pfconfig/shared/shared_test.go

 				AntennaGain:   0,
 				Radios: []RFConfig{
 					{
 						Enable:     true,
 						Type:       "SX1257",
 						Frequency:  867500000,
-						TxEnable:   true,
+						TxEnable:   pointers.Bool(true),


Suggested change

TxEnable: pointers.Bool(true),

TxEnable: func(v bool) *bool { return &v }(true),

rvolosatovs · 2020-03-13T09:11:22Z

pkg/pfconfig/shared/shared_test.go

 						TxFreqMin:  863000000,
 						TxFreqMax:  870000000,
-						RSSIOffset: -166,
+						RSSIOffset: pointers.Float32(-166),


This is the only place you actually may want to have a helper function, since there are 2 usages.
However, why not just define const defaultRSSIOffset = -166 and use &defaultRSSIOffset here and below?

KrishnaIyer · 2020-04-02T11:41:11Z

Closing this in favor of https://github.com/TheThingsIndustries/lorawan-stack/issues/2018

…ge/open-source

KrishnaIyer added bug Something isn't working c/gateway conf server This is related to the Gateway Configuration Server labels Mar 11, 2020

KrishnaIyer added this to the March 2020 milestone Mar 11, 2020

KrishnaIyer requested a review from htdvisser March 11, 2020 16:45

KrishnaIyer requested a review from rvolosatovs as a code owner March 11, 2020 16:45

KrishnaIyer self-assigned this Mar 11, 2020

htdvisser suggested changes Mar 12, 2020

View reviewed changes

Krishna added 5 commits March 12, 2020 18:08

util: Remove hardware specific values from BS router_config

e805952

util: Add value to pointer functions

3d17b1e

all: Update CHANGELOG

c7b44c3

gs, util: Convert omittable fields into pointers

eff6827

util: Add omitempty to Marshaler

db9c4a3

KrishnaIyer force-pushed the fix/2130-router-config branch from 2775bf8 to db9c4a3 Compare March 12, 2020 17:12

KrishnaIyer requested a review from johanstokking as a code owner March 12, 2020 17:12

johanstokking reviewed Mar 12, 2020

View reviewed changes

rvolosatovs suggested changes Mar 13, 2020

View reviewed changes

KrishnaIyer closed this Apr 2, 2020

johanstokking deleted the fix/2130-router-config branch April 2, 2020 11:44

johanstokking restored the fix/2130-router-config branch April 2, 2020 11:44

KrishnaIyer deleted the fix/2130-router-config branch July 22, 2020 14:35

rvolosatovs added a commit to rvolosatovs/lorawan-stack-fork that referenced this pull request Sep 15, 2020

Merge pull request TheThingsNetwork#2131 from TheThingsIndustries/mer…

d8a9f2f

…ge/open-source

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Remove hardware specific values from BS router_config #2131

util: Remove hardware specific values from BS router_config #2131

KrishnaIyer commented Mar 11, 2020

htdvisser Mar 12, 2020

johanstokking Mar 12, 2020

rvolosatovs Mar 12, 2020

KrishnaIyer Mar 13, 2020

johanstokking Mar 24, 2020

johanstokking Mar 12, 2020

KrishnaIyer Mar 13, 2020

johanstokking Mar 13, 2020

KrishnaIyer Mar 13, 2020

johanstokking Mar 13, 2020

rvolosatovs Mar 13, 2020

rvolosatovs Mar 13, 2020

rvolosatovs Mar 13, 2020

rvolosatovs Mar 13, 2020

KrishnaIyer commented Apr 2, 2020

	conf.ClockSource = pointers.Uint8(frequencyPlan.ClockSource)
	conf.ClockSource = &frequencyPlan.ClockSource

	ClockSource: pointers.Uint8(1),
	ClockSource: func(v uint8) *uint8 { return &v }(1),

	TxEnable: pointers.Bool(true),
	TxEnable: func(v bool) *bool { return &v }(true),

util: Remove hardware specific values from BS router_config #2131

util: Remove hardware specific values from BS router_config #2131

Conversation

KrishnaIyer commented Mar 11, 2020

Summary

Changes

Notes for Reviewers

Checklist

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

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

KrishnaIyer commented Apr 2, 2020