Keep the ADR data rate index value between min and max values

[halimi]

Summary

References: https://github.com/TheThingsIndustries/lorawan-stack-support/issues/1336

The ADR desired data rate index didn't respect the MAC settings min and max value if the current data rate index was below the min or above the max value. It has been fixed.

Changes

• Keep the ADR data rate index value between min and max values

Testing

Steps

1. Check the MAC state current and desired adr_data_rate_index
2. Set a value for mac-settings.adr.mode.dynamic.min-data-rate-index and mac-settings.adr.mode.dynamic.max-data-rate-index that the current data rate value will be outside of this range.
3. Wait for the next uplinks and check the MAC state desired adr_data_rate_index value is between the min and max value

Results

Regressions

1. Check the MAC state current and desired adr_data_rate_index

  "mac_state": {
    "current_parameters": {
      "max_eirp": 16,
      "adr_data_rate_index": 1,
      "adr_tx_power_index": 1,
      "adr_nb_trans": 1,
      "rx1_delay": 1,
      "rx2_data_rate_index": 5,
      "rx2_frequency": "869525000",
      "ping_slot_frequency": "869525000",
      "beacon_frequency": "869525000",
       },
    "desired_parameters": {
      "max_eirp": 16,
      "adr_data_rate_index": 1,
      "adr_tx_power_index": 1,
      "adr_nb_trans": 1,
      "rx1_delay": 1,
      "rx2_data_rate_index": 5,
      "rx2_frequency": "869525000",
      "ping_slot_frequency": "869525000",
      "beacon_frequency": "869525000",
      },
   }

2. Set a value for mac-settings.adr.mode.dynamic.min-data-rate-index and mac-settings.adr.mode.dynamic.max-data-rate-index that the current data rate value will be outside of this range.

go run ./cmd/ttn-lw-cli end-devices set --application-id myapp --device-id arduino-mkr --mac-settings.adr.mode.dynamic  --mac-settings.adr.mode.dynamic.max-data-rate-index 4 --mac-settings.adr.mode.dynamic.min-data-rate-index 2

MAC settings:

  "mac_settings": {
    "rx1_delay": 1,
    "rx2_data_rate_index": 0,
    "rx2_frequency": "869525000",
    "factory_preset_frequencies": [
      "868500000"
    ],
    "supports_32_bit_f_cnt": true,
    "status_time_periodicity": "86400s",
    "status_count_periodicity": 200,
    "desired_rx1_delay": 1,
    "desired_rx1_data_rate_offset": 0,
    "desired_rx2_data_rate_index": 5,
    "desired_rx2_frequency": "869525000",
    "desired_max_duty_cycle": "DUTY_CYCLE_1",
    "adr": {
      "dynamic": {
        "min_data_rate_index": 2,
        "max_data_rate_index": 4
      }
    }

3. Wait for the next uplinks and check the MAC state desired adr_data_rate_index value is between the min and max value

  "mac_state": {
    "current_parameters": {
      "max_eirp": 16,
      "adr_data_rate_index": 1,
      "adr_tx_power_index": 1,
      "adr_nb_trans": 1,
      "rx1_delay": 1,
      "rx2_data_rate_index": 5,
      "rx2_frequency": "869525000",
      "ping_slot_frequency": "869525000",
      "beacon_frequency": "869525000",
       },
    "desired_parameters": {
      "max_eirp": 16,
      "adr_data_rate_index": 2,
      "adr_tx_power_index": 1,
      "adr_nb_trans": 1,
      "rx1_delay": 1,
      "rx2_data_rate_index": 5,
      "rx2_frequency": "869525000",
      "ping_slot_frequency": "869525000",
      "beacon_frequency": "869525000",
      },
   }

Notes for Reviewers

...

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.
• Testing: The steps/process to test this feature are clearly explained including testing for regressions.
• Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
• 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.

[pomplesiegel]

Hello, thank you for working on this! I believe this was inspired by the TTI ticket I filed earlier last month.

@halimi, Flagging that before this update I observed the device fully disregarding the min/max SF values, even if it was within the min/max range at the time of the settings change. For example, when I set it to between SF7 and SF9, then performed a reset and rejoin on the device, if the gateway was removed and the device was looking for an ack due to linkcheckmode, it would float all the way up to SF12 in the case of the EU868 band. Basically from my testing I found that the the min/max SF values were totally disregarded, irrespective of the initial state.

Are we confident the fix above would address this case as well?

[halimi]

Hello, thank you for working on this! I believe this was inspired by the TTI ticket I filed earlier last month.

@halimi, Flagging that before this update I observed the device fully disregarding the min/max SF values, even if it was within the min/max range at the time of the settings change. For example, when I set it to between SF7 and SF9, then performed a reset and rejoin on the device, if the gateway was removed and the device was looking for an ack due to linkcheckmode, it would float all the way up to SF12 in the case of the EU868 band. Basically from my testing I found that the the min/max SF values were totally disregarded, irrespective of the initial state.

Are we confident the fix above would address this case as well?

@pomplesiegel, Before this fix the min and max value was not enforced by the ADR. Now it enforces the desired value to be between the min and max value. When the device rejoins the data rate index can be outside of the min and max values for the first couple of uplinks, after that the NS should start to adjust it by sending CID_LINK_ADR command with the desired data rate index to the device. If the device accepts it then after that the desired data rate index will be between the min and max value. So yes, I think it should fix your case too.

[pomplesiegel]

@halimi, perfect, thank you! Looking forward to seeing this in a release and we'll update on our side.