Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add initial CPF support to GCS #1747

Merged
merged 3 commits into from
Dec 19, 2019
Merged

Add initial CPF support to GCS #1747

merged 3 commits into from
Dec 19, 2019

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Dec 18, 2019 •
edited
Loading

Summary

References #1631 TheThingsNetwork/kerlink-wirnet-firmware#9

Changes

  • Add support for (some of) CPF configuration to GCS - enough to provision the gateway to forward traffic to the configured stack instance
  • Improve testing(e.g. actually test the output on endpoints)

Notes for Reviewers

I will file issue(s) for added support for CPF in GCS before "undrafting" this

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.

@rvolosatovs rvolosatovs added the c/gateway conf server This is related to the Gateway Configuration Server label Dec 18, 2019
@rvolosatovs rvolosatovs added this to the December 2019 milestone Dec 18, 2019
@rvolosatovs rvolosatovs self-assigned this Dec 18, 2019
@coveralls
Copy link

coveralls commented Dec 18, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 73.705% when pulling a71a35f on rvolosatovs:feature/cpf-config into dc12b32 on TheThingsNetwork:master.

@rvolosatovs rvolosatovs mentioned this pull request Dec 18, 2019
Open
4 tasks
@rvolosatovs rvolosatovs force-pushed the feature/cpf-config branch 2 times, most recently from bff8baa to 210b026 Compare December 18, 2019 21:01
@rvolosatovs rvolosatovs marked this pull request as ready for review December 18, 2019 21:01
rvolosatovs
rvolosatovs commented Dec 18, 2019
View reviewed changes
pkg/pfconfig/cpf/cpf.go
return nil, err
}
var gatewayConf LoradGatewayConf
if len(gtw.Antennas) > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this makes sense - it seems that SX1301 only allows for 1 antenna gain to be configured, what should happen when there are multiple?
Same question for location.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, with the v1 and v1.5 ref design, there's usually only one antenna per concentrator and each concentrator gets one SX1301_Conf object.
@htdvisser can confirm.

@rvolosatovs rvolosatovs added prio/medium blocking Another issue or pull request is waiting for this labels Dec 18, 2019
@rvolosatovs
Copy link
Contributor Author

Blocking #1631 #1716 (transitively)

@rvolosatovs rvolosatovs mentioned this pull request Dec 18, 2019
Merged
KrishnaIyer
KrishnaIyer requested changes Dec 19, 2019
View reviewed changes
Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one.

pkg/gatewayconfigurationserver/gatewayconfigurationserver_test.go Outdated Show resolved Hide resolved
pkg/gatewayconfigurationserver/gatewayconfigurationserver_test.go Outdated Show resolved Hide resolved
pkg/gatewayconfigurationserver/gatewayconfigurationserver_test.go Outdated Show resolved Hide resolved
pkg/gatewayconfigurationserver/gatewayconfigurationserver.go Outdated Show resolved Hide resolved
pkg/gatewayconfigurationserver/gatewayconfigurationserver.go Outdated
})
}

func (gcs *GatewayConfigurationServer) makeTextMarshalerGatewayConfigHandler(contentType string, f func(context.Context, *ttnpb.Gateway) (encoding.TextMarshaler, error)) func(echo.Context) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "TextMarshalling" is a golang term right? I think it would be nice to use the output file format in the funtion name. For example, makeJSON, makeTOML, makeYAML etc..

Copy link
Contributor Author

@rvolosatovs rvolosatovs Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's named this way, because this method uses the MarshalText method on an encoding.TextMarshaler and it takes the content-type as parameter.
E.g. makeJSONHandler only takes an interface{}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that it's better to name a function after what it creates rather than what method it calls internally. But again this is nitpick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does not know what it creates - that's the point.
The content-type is a parameter - it may create TOML, JSON, YAML or anything else.

pkg/pfconfig/cpf/cpf.go
return nil, err
}
var gatewayConf LoradGatewayConf
if len(gtw.Antennas) > 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, with the v1 and v1.5 ref design, there's usually only one antenna per concentrator and each concentrator gets one SX1301_Conf object.
@htdvisser can confirm.

pkg/pfconfig/shared/shared.go Outdated Show resolved Hide resolved
KrishnaIyer
KrishnaIyer approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase for your fixup commit and I think you need to run go:fmt to fix the lint errors.

Roman Volosatovs added 3 commits December 19, 2019 11:26
util: Refactor ParseGatewayServerAddress
65ddcac 
Existing implementation silently ignores errors, replace that by a cleaner approach
gcs,util: Add initial CPF config support
d6849bd
dev: Regenerate messages
a71a35f
@rvolosatovs rvolosatovs merged commit fc1335f into TheThingsNetwork:master Dec 19, 2019
@rvolosatovs rvolosatovs deleted the feature/cpf-config branch December 19, 2019 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KrishnaIyer KrishnaIyer KrishnaIyer approved these changes

Assignees

@rvolosatovs rvolosatovs

Labels
blocking Another issue or pull request is waiting for this c/gateway conf server This is related to the Gateway Configuration Server
Projects
None yet
Milestone
December 2019
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rvolosatovs @coveralls @KrishnaIyer