-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: Remove hardware specific values from BS router_config #2131
Changes from all commits
e805952
3d17b1e
c7b44c3
eff6827
db9c4a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -28,12 +28,13 @@ import ( | |||||
"go.thethings.network/lorawan-stack/pkg/errors" | ||||||
"go.thethings.network/lorawan-stack/pkg/frequencyplans" | ||||||
"go.thethings.network/lorawan-stack/pkg/ttnpb" | ||||||
"go.thethings.network/lorawan-stack/pkg/util/pointers" | ||||||
) | ||||||
|
||||||
// SX1301Config contains the configuration for the SX1301 concentrator. | ||||||
type SX1301Config struct { | ||||||
LoRaWANPublic bool | ||||||
ClockSource uint8 | ||||||
ClockSource *uint8 | ||||||
AntennaGain float32 | ||||||
LBTConfig *LBTConfig | ||||||
Radios []RFConfig | ||||||
|
@@ -85,7 +86,7 @@ func (m orderedMap) MarshalJSON() ([]byte, error) { | |||||
func (c SX1301Config) MarshalJSON() ([]byte, error) { | ||||||
var m orderedMap | ||||||
m.add("lorawan_public", c.LoRaWANPublic) | ||||||
m.add("clksrc", c.ClockSource) | ||||||
m.add("clksrc,omitempty", c.ClockSource) | ||||||
m.add("antenna_gain", c.AntennaGain) | ||||||
if c.LBTConfig != nil { | ||||||
m.add("lbt_cfg", *c.LBTConfig) | ||||||
|
@@ -206,14 +207,14 @@ type LBTChannelConfig struct { | |||||
|
||||||
// RFConfig contains the configuration for one of the radios. | ||||||
type RFConfig struct { | ||||||
Enable bool `json:"enable"` | ||||||
Type string `json:"type,omitempty"` | ||||||
Frequency uint64 `json:"freq"` | ||||||
RSSIOffset float32 `json:"rssi_offset"` | ||||||
TxEnable bool `json:"tx_enable"` | ||||||
TxFreqMin uint64 `json:"tx_freq_min,omitempty"` | ||||||
TxFreqMax uint64 `json:"tx_freq_max,omitempty"` | ||||||
TxNotchFreq uint64 `json:"tx_notch_freq,omitempty"` | ||||||
Enable bool `json:"enable"` | ||||||
Type string `json:"type,omitempty"` | ||||||
Frequency uint64 `json:"freq"` | ||||||
RSSIOffset *float32 `json:"rssi_offset,omitempty"` | ||||||
TxEnable *bool `json:"tx_enable,omitempty"` | ||||||
TxFreqMin uint64 `json:"tx_freq_min,omitempty"` | ||||||
TxFreqMax uint64 `json:"tx_freq_max,omitempty"` | ||||||
TxNotchFreq uint64 `json:"tx_notch_freq,omitempty"` | ||||||
} | ||||||
|
||||||
// IFConfig contains the configuration for one of the channels. | ||||||
|
@@ -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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
if frequencyPlan.LBT != nil { | ||||||
lbtConfig := &LBTConfig{ | ||||||
|
@@ -311,10 +312,10 @@ func BuildSX1301Config(frequencyPlan *frequencyplans.FrequencyPlan) (*SX1301Conf | |||||
Enable: radio.Enable, | ||||||
Type: radio.ChipType, | ||||||
Frequency: radio.Frequency, | ||||||
RSSIOffset: radio.RSSIOffset, | ||||||
RSSIOffset: &radio.RSSIOffset, | ||||||
} | ||||||
if radio.TxConfiguration != nil { | ||||||
rfConfig.TxEnable = true | ||||||
rfConfig.TxEnable = pointers.Bool(true) | ||||||
rfConfig.TxFreqMin = radio.TxConfiguration.MinFrequency | ||||||
rfConfig.TxFreqMax = radio.TxConfiguration.MaxFrequency | ||||||
if radio.TxConfiguration.NotchFrequency != nil { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |||||
"go.thethings.network/lorawan-stack/pkg/errors" | ||||||
"go.thethings.network/lorawan-stack/pkg/frequencyplans" | ||||||
. "go.thethings.network/lorawan-stack/pkg/pfconfig/shared" | ||||||
"go.thethings.network/lorawan-stack/pkg/util/pointers" | ||||||
) | ||||||
|
||||||
func TestSX1301Conf(t *testing.T) { | ||||||
|
@@ -88,25 +89,24 @@ func TestSX1301Conf(t *testing.T) { | |||||
}, | ||||||
SX1301Config{ | ||||||
LoRaWANPublic: true, | ||||||
ClockSource: 1, | ||||||
ClockSource: pointers.Uint8(1), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
AntennaGain: 0, | ||||||
Radios: []RFConfig{ | ||||||
{ | ||||||
Enable: true, | ||||||
Type: "SX1257", | ||||||
Frequency: 867500000, | ||||||
TxEnable: true, | ||||||
TxEnable: pointers.Bool(true), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
TxFreqMin: 863000000, | ||||||
TxFreqMax: 870000000, | ||||||
RSSIOffset: -166, | ||||||
RSSIOffset: pointers.Float32(-166), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only place you actually may want to have a helper function, since there are 2 usages. |
||||||
}, | ||||||
{ | ||||||
Enable: true, Type: "SX1257", | ||||||
Frequency: 868500000, | ||||||
TxEnable: false, | ||||||
TxFreqMin: 0, | ||||||
TxFreqMax: 0, | ||||||
RSSIOffset: -166, | ||||||
RSSIOffset: pointers.Float32(-166), | ||||||
}, | ||||||
}, | ||||||
Channels: []IFConfig{ | ||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,32 @@ | ||||
// Copyright © 2020 The Things Network Foundation, The Things Industries B.V. | ||||
// | ||||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||||
// you may not use this file except in compliance with the License. | ||||
// You may obtain a copy of the License at | ||||
// | ||||
// http://www.apache.org/licenses/LICENSE-2.0 | ||||
// | ||||
// Unless required by applicable law or agreed to in writing, software | ||||
// distributed under the License is distributed on an "AS IS" BASIS, | ||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||
// See the License for the specific language governing permissions and | ||||
// limitations under the License. | ||||
|
||||
package pointers | ||||
|
||||
import "time" | ||||
|
||||
// Bool returns a boolean pointer for the given value. | ||||
func Bool(v bool) *bool { return &v } | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's please not do this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is a one-time thing you could just oneline this. E.g.
Otherwise just have a non-exported There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not? Then I need to define the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is inherent to Go |
||||
|
||||
// Duration returns a duration pointer for the given value. | ||||
func Duration(d time.Duration) *time.Duration { return &d } | ||||
|
||||
// Time returns a time.Time pointer for the given value. | ||||
func Time(t time.Time) *time.Time { return &t } | ||||
|
||||
// Float32 returns a float32 pointer for the given value. | ||||
func Float32(v float32) *float32 { return &v } | ||||
|
||||
// Uint8 returns a uint8 pointer for the given value. | ||||
func Uint8(v uint8) *uint8 { return &v } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the underlying problem that we have radio specific fields in the frequency plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't that true for other gateways too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most gateways use what's provided in the
global_conf.json
right? Some have overrides inlocal_conf.json
but not all of them afaik.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.