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

Fine grained ADR NbTrans controls #6962

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Feb 29, 2024

Summary

This PR introduces the capability to limit the NbTrans of an end device on a per data rate basis. This allows lower data rates to have a limited NbTrans while higher data rates can benefit from additional transmissions when needed.

Refs #6966

Changes

  • Add per data rate index overrides to the dynamic ADR API.
  • Allow these overrides to override the top level ADR settings.

Testing

  1. Parameter sanity checking - in short we need to check that the minimum NbTrans is not above the maximum NbTrans, or vice versa. Given an application app1 and a device eui-1231231231231231 we start by setting the minimum NbTrans override to 2.
ttn-lw-cli dev set app1 eui-1231231231231231 --mac-settings.adr.mode.dynamic.overrides.data-rate-0.min-nb-trans 2
INFO    Telemetry is enabled. Check the documentation for more information on what is collected and how to disable it   {"documentation_url": "https://www.thethingsindustries.com/docs/reference/telemetry/cli"}
{
  "ids": {
    "device_id": "eui-1231231231231231",
    "application_ids": {
      "application_id": "app1"
    },
    "dev_eui": "1231231231231231",
    "join_eui": "0000000000000000"
  },
  "created_at": "2024-02-29T19:22:43.994254Z",
  "updated_at": "2024-02-29T19:29:10.563060Z",
  "network_server_address": "localhost",
  "mac_settings": {
    "adr": {
      "dynamic": {
        "overrides": {
          "data_rate_0": {
            "min_nb_trans": 2
          }
        }
      }
    }
  }
}

We then attempt to set the max NbTrans to 1, expecting an error.

ttn-lw-cli dev set app1 eui-1231231231231231 --mac-settings.adr.mode.dynamic.overrides.data-rate-0.max-nb-trans 1
INFO    Telemetry is enabled. Check the documentation for more information on what is collected and how to disable it   {"documentation_url": "https://www.thethingsindustries.com/docs/reference/telemetry/cli"}
error:pkg/networkserver:field_value (invalid value of field `mac_settings.adr.mode.dynamic.overrides.data_rate_0.max_nb_trans`)
    field=mac_settings.adr.mode.dynamic.overrides.data_rate_0.max_nb_trans
    correlation_id=edb13b6506894274b17ba4b5a6e7e749
exit status 255

We then set the max NbTrans to 3, expecting this to succeed.

ttn-lw-cli dev set app1 eui-1231231231231231 --mac-settings.adr.mode.dynamic.overrides.data-rate-0.max-nb-trans 3
INFO    Telemetry is enabled. Check the documentation for more information on what is collected and how to disable it   {"documentation_url": "https://www.thethingsindustries.com/docs/reference/telemetry/cli"}
{
  "ids": {
    "device_id": "eui-1231231231231231",
    "application_ids": {
      "application_id": "app1"
    },
    "dev_eui": "1231231231231231",
    "join_eui": "0000000000000000"
  },
  "created_at": "2024-02-29T19:22:43.994254Z",
  "updated_at": "2024-02-29T19:29:29.322193Z",
  "network_server_address": "localhost",
  "mac_settings": {
    "adr": {
      "dynamic": {
        "overrides": {
          "data_rate_0": {
            "max_nb_trans": 3
          }
        }
      }
    }
  }
}

Finally, we check that the values have been stored.

ttn-lw-cli dev get app1 eui-1231231231231231 --mac-settings
INFO    Telemetry is enabled. Check the documentation for more information on what is collected and how to disable it   {"documentation_url": "https://www.thethingsindustries.com/docs/reference/telemetry/cli"}
{
  "ids": {
    "device_id": "eui-1231231231231231",
    "application_ids": {
      "application_id": "app1"
    },
    "dev_eui": "1231231231231231",
    "join_eui": "0000000000000000"
  },
  "created_at": "2024-02-29T19:22:43.994254Z",
  "updated_at": "2024-02-29T19:29:29.322193Z",
  "network_server_address": "localhost",
  "mac_settings": {
    "rx2_data_rate_index": 0,
    "rx2_frequency": "869525000",
    "adr": {
      "dynamic": {
        "overrides": {
          "data_rate_0": {
            "min_nb_trans": 2,
            "max_nb_trans": 3
          }
        }
      }
    }
  }
}

The above test can be done on any data rate index.

  1. Test that the override works. Since most test devices are going to jump to SF7 quite fast, let's override the minimum NbTrans of data rate index 5 to 3 and expect that the device is set to NbTrans 3 via LinkADRReq events. This assumes that the device operates in a band where SF7 is data rate index 5. Adjust accordingly if that's not the case.
ttn-lw-cli dev set app1 eui-1231231231231231 --mac-settings.adr.mode.dynamic.overrides.data-rate-5.min-nb-trans 3
INFO    Telemetry is enabled. Check the documentation for more information on what is collected and how to disable it   {"documentation_url": "https://www.thethingsindustries.com/docs/reference/telemetry/cli"}
{
  "ids": {
    "device_id": "eui-1231231231231231",
    "application_ids": {
      "application_id": "app1"
    },
    "dev_eui": "1231231231231231",
    "join_eui": "0000000000000000"
  },
  "created_at": "2024-02-29T19:22:43.994254Z",
  "updated_at": "2024-02-29T19:57:36.896606Z",
  "network_server_address": "localhost",
  "mac_settings": {
    "adr": {
      "dynamic": {
        "overrides": {
          "data_rate_5": {
            "min_nb_trans": 3
          }
        }
      }
    }
  }
}
Regressions

The overrides have no effect on existing devices. As such, no specific regressions are expected.

Notes for Reviewers

@KrishnaIyer please take a look.

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.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • 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.

@adriansmares adriansmares added this to the v3.30.0 milestone Feb 29, 2024
@adriansmares adriansmares self-assigned this Feb 29, 2024
@github-actions github-actions bot added the c/network server This is related to the Network Server label Feb 29, 2024
@adriansmares adriansmares force-pushed the feature/adr-fine-grained branch from 8bba752 to b936589 Compare February 29, 2024 20:19
@adriansmares adriansmares force-pushed the feature/adr-fine-grained branch from b936589 to 9524f26 Compare March 1, 2024 10:27
@adriansmares adriansmares marked this pull request as ready for review March 1, 2024 10:37
@adriansmares adriansmares requested a review from a team as a code owner March 1, 2024 10:37
@KrishnaIyer
Copy link
Member

Would it be too complex/out of scope to be able to set this on all devices on an application?

@adriansmares
Copy link
Contributor Author

Would it be too complex/out of scope to be able to set this on all devices on an application?

Yes, because we don't have any mechanism at NS level for MAC settings grouping.

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.

API LGTM. For the feature review, I'll defer to @johanstokking.

@adriansmares adriansmares merged commit f372251 into v3.30 Mar 6, 2024
15 of 16 checks passed
@adriansmares adriansmares deleted the feature/adr-fine-grained branch March 6, 2024 16:33
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants