`txdelay` and `rxdelay` can be set higher than documented maximum.

[dreirund]

According to the repeater CLI documentation, maximum value of txdelay is 2.0 and maximum value of rxdelay is 20.0.

BUT I was able to do set txdelay 15, set rxdelay 22, and get txdelay shows 15 afterwards and get rxdelay shows 22 afterwards.

So something is wrong here.

What are the actual correct maximum values? Documentation should be corrected accordingly.

Regards!

[kizniche]

Certainly having no limits is the worst option, so I added the limits described in the docs in the above PR. I also added descriptions for your related issue #2431 in another PR.

[dreirund]

Certainly having no limits is the worst option, so I added the limits described in the docs in the above PR.

I did reply over there.

[kizniche]

There was actually a significant bug found while documenting this feature (#2431 (comment)). All feedback from the CLI indicates values set above the documented limits are put into effect immediately and remain active until the next reboot. However, the limits are silently clamped at the next reboot and out-of-range values are not used, leaving the user with no indication their set value is not being honored.

The fix made above prevents new out-of-range values from being accepted at set time, but does not remedy the issue of out-of-range values already saved. A complete fix would also need to detect out-of-range values on load and write back the clamped value with savePrefs().

There's already a precendent for doing this:

MeshCore/src/helpers/CommonCLI.cpp

Lines 33 to 34 in 1a7b361

	loadPrefsInt(fs, "/node_prefs");
	savePrefs(fs); // save to new filename

I can update the above PR to include that at the end of loadPrefsInt(), after file.close():

MeshCore/src/helpers/CommonCLI.cpp

Line 122 in 1a7b361

file.close();

However, I'm not sure if this should be avoided for any reason. Perhaps a comparison of the values before and after clamping can be used to determine if savePrefs() should be used or not.

[kizniche]

I'd recommend maybe changing the title to indicate this is actually a bug rather than merely a documentation issue. Here is where the values are clamped at boot:

MeshCore/src/helpers/CommonCLI.cpp

Lines 94 to 96 in 1a7b361

	_prefs->rx_delay_base = constrain(_prefs->rx_delay_base, 0, 20.0f);
	_prefs->tx_delay_factor = constrain(_prefs->tx_delay_factor, 0, 2.0f);
	_prefs->direct_tx_delay_factor = constrain(_prefs->direct_tx_delay_factor, 0, 2.0f);

Thinking on it more, it might be better to open another issue for that to be discussed separately from the docs and setting limits for the cli.

[dreirund]

I'd recommend maybe changing the title to indicate this is actually a bug rather than merely a documentation issue. Here is where the values are clamped at boot:

MeshCore/src/helpers/CommonCLI.cpp

Lines 94 to 96 in 1a7b361
_prefs->rx_delay_base = constrain(_prefs->rx_delay_base, 0, 20.0f);
_prefs->tx_delay_factor = constrain(_prefs->tx_delay_factor, 0, 2.0f);
_prefs->direct_tx_delay_factor = constrain(_prefs->direct_tx_delay_factor, 0, 2.0f);

Thinking on it more, it might be better to open another issue for that to be discussed separately from the docs and setting limits for the cli.

So values out of range are just clamped to the range which is documented, instead of beeing reverted to zero? Well, in that case I think it is not a huge issue, and indeed I think the issue then is that the CLI just accepts higher values at runtime and after reboot they are silently clamped to the boundaries, and a fix would really be what you did to also when using the CLI enforce values within bounds.

Or are they actually completely discarded and reverted to zero?

[kizniche]

I think it is not a huge issue

Agreed. It's not a huge issue, since a reboot will currently correct out-of-range values that have been set. The bigger issue is that without savePrefs() used to correct currently-saved values, users may continue to believe out-of-range values are actually in use.

Or are they actually completely discarded and reverted to zero?

If a value is set above the upper range, the next reboot clamps it at the upper range value, not zero.

[oltaco]

@kizniche I've had a look at your proposed fix #2443 and it looks good to me. Thanks for your work on this 👍

[oltaco]

Closed by #2443