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.

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

Add MACSettings.DesiredMaxEIRP field #4128

Merged
merged 6 commits into from
May 19, 2021
Merged

Conversation

neoaggelos
Copy link
Contributor

@neoaggelos neoaggelos commented May 5, 2021

Summary

Closes #4090

Changes

  • Introduce DeviceEIRPValue struct (containing an enum value of type DeviceEIRP)
  • Introduce MACSettings.DesiredMaxEirp field.
  • Consider MaxEirp value for mac.DeviceDesiredMaxEIRP
  • Miscellaneous changes required for validations and setting the DesiredMaxEirp field from the CLI.

Testing

  • Unit tests
  • Setting desired_max_eirp field from the CLI and verify that the DesiredParameters.MaxEIRP value is set properly after device joins.
Regressions

There should not be any, the default value for the new setting is nil which preserves the old behaviour.

Notes for Reviewers

  • Not using gogoproto, so the field is DesiredMaxEirp instead of DesiredMaxEIRP.
  • The validation and FieldIsZero() parts probably need a review from @rvolosatovs to make sure that I did not miss anything.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • 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.

@neoaggelos neoaggelos added this to the v3.13.0 milestone May 5, 2021
@neoaggelos neoaggelos self-assigned this May 5, 2021
@github-actions github-actions bot added c/network server This is related to the Network Server compat/api This could affect API compatibility ui/cli This is related to ttn-lw-cli labels May 5, 2021
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

I really don't know much about how these custom wrappers work, so maybe good to wait for review from @rvolosatovs.

pkg/ttnpb/testdata/ttnpb_encoding_golden.md Show resolved Hide resolved
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

Nothing to add.

@neoaggelos neoaggelos force-pushed the feature/max-eirp-mac-settings branch from afd2dac to 61b2092 Compare May 18, 2021 12:35
@neoaggelos neoaggelos force-pushed the feature/max-eirp-mac-settings branch from 61b2092 to 2952751 Compare May 19, 2021 13:05
@neoaggelos neoaggelos merged commit bc960f0 into v3.13 May 19, 2021
@neoaggelos neoaggelos deleted the feature/max-eirp-mac-settings branch May 19, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/network server This is related to the Network Server compat/api This could affect API compatibility ui/cli This is related to ttn-lw-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuring the MaxEIRP as part of device MAC settings
5 participants