Inconsistent config settings range usage

[aemileski]

When the UI was overhauled, and configuration settings changed to be handled more generically, an inconsistency in the usage of settings ranges became more obvious, and problematic.

The entire issue seems to stem from config settings having a range 'min <= value < max' which is programmatically sensible, instead of 'min <= value <= max' which is configuration sensible, yet both ranges are used interchangably throughout the code.

There is arguably reason to use either method, though I personally would support changing all ranges to 'min <= value <= max', as one shouldn't need to understand the subtleties of the code to tweak configuration values!

However, the following description assumes that 'min <= value < max' was the intended range for all config settings.

Reference: source/Core/Src/Settings.cpp

Config settings range 'min <= value < max' in these routines:

• setSettingValue() // if value >= constants.max then set it to constants.max - 1
• getSettingValue() // returns setting as-is
• nextSettingValue() // if value >= (constants.max - constants.increment) then wrap to constants.min

Config settings range from 'min <= value <= max' in these routines: INCONSISTENT

• sanitiseSettings() // if value > constants.max then use constants.defaultValue (instead of when value >= constants.max)
• prevSettingValue() // if value <= constants.min then wrap to constants.max (instead of wrapping to constants.max - 1)

Reference: source/Core/BSP/*/configuration.h

All the values defined in */configuration.h are used to populate 'settingsConstants' as-is.

• BAD: If the macros are used as a max value, that value can never be reached with range 'min <= value < max'.
• BAD: If the same macros are used as a setting value and as a max value, the value exceeds 'max - 1' but is NOT replaced by the default value by sanitiseSettings().
• BAD: If the value is used as a default value and as a max value, the value exceeds 'max - 1'
• OKAY: If the macros are used as a min value, it is used as expected.

Weirdness: nextSettingValue()

There are two different conditions to determine if 'max - 1' was exceeded, when it would be logical to use the same condition (the first one would be my preference):

if (systemSettings.settingsValues[(int)option] >= (constants.max - constants.increment)) {
...
return (constants.max - systemSettings.settingsValues[(int)option]) <= constants.increment;

[Ralim]

Alrighty; so the intended behaviour is min<=value < max.

sanitiseSettings and prevSettingValue are definitely a bugs.

The macros are generally not a large concern given that they change infrequently and are mostly there as a collection point for magic numbers than for edit-ability. It is expected their values are tested after any changes.

The nextSettingValue is written differently so that they read differently
systemSettings.settingsValues[(int)option] >= (constants.max - constants.increment)
Is meant to read as a constrained where convention is value>=limit
Where as (constants.max - systemSettings.settingsValues[(int)option]) <= constants.increment
is intended to read as "are we within one increment of the end"

[aemileski]

Thanks for your attention to this issue \o/

Using 'min <= value < max' suggests a review of the configuration settings may be warranted to be sure they have the intended range.

Adusting all 'max' settings by +1, or alternatively adding some macro function that does and use it throughout initializing 'settingsConstant' seems like a better solution to me.

The nextSettingValue is written differently so that they read differently

Rule that I recite to everyone at some point when doing code inspections:

"You can get away with coding almost anything, other than knowingly writing bad code, if you just comment your intentions!

Sometimes you need to code stuff oddly, but others looking at your code are not going to know your reasons.

Even a comment like this is useful:

// Yes, this is ugly code, but alternatives seem to break other stuff.  Be warned 'There Be Dragons Here'

Also, you are likely to forget the reasons yourself after a few months!"

[Ralim]

suggests a review of the configuration settings may be warranted to be sure they have the intended range.

You are more than welcome to do one if you have time 😀

Adusting all 'max' settings by +1, or alternatively adding some macro function that does and use it throughout initializing 'settingsConstant' seems like a better solution to me.

now we are into personal opinion territory.
I generally lean for the min <=value<max as that matches common notation for ranges/for loops etc.
A macro is not a good solution here since its more indirection people dont bother to figure out :P

"You can get away with coding almost anything, other than knowingly writing bad code, if you just comment your intentions!
I agree with this to an extent, I'm also not a huge fan of superfluous or redundant commenting, as that often leads to some said comments becoming wildly out of sync. (Thus why the descriptor for these is only in the header).

Now that said; by asking around this, makes me suspect that its not overly clear that the comparisons are written in grammatical order, so a comment might help here.

Also, you are likely to forget the reasons yourself after a few months!"

I wish I could 🙃

[aemileski]

suggests a review of the configuration settings may be warranted to be sure they have the intended range.

You are more than welcome to do one if you have time 😀

I would, but unfortunately I don't own all the supported irons, nor do I claim intimate knowledge of every setting, and the consequences of them.

The reason I originally posted this bug was because I changed how the temperature controls worked, and that patch no longer worked on the latest codebase. When I adapted my patch, it was obvious I had unintentionally broken other things. That's when I realized there were different ranges in use.

Then I ran into solderingTemp not being adjusted through the same methods, with its own range checking hardcoded with constants in gui_solderingTempAdjust() [that also failed if the 'min' is set to 0 because then the comparison 'newTemp < MIN_TEMP_C' becomes 'newTemp < 0' which is always false for unsigned numbers], which made me consider the possibility that other things I'm unfamiliar with might also have their own special handling.

After having cooked a thermistor in a 3D printer because of a failed PID, I decided I didn't want to cook my TS100, and thought I'd let the expert(s) confirm or deny what I thought I might have discovered.

Now that said; by asking around this, makes me suspect that its not overly clear that the comparisons are written in grammatical order, so a comment might help here.

Actually, I really appreciated your original explanation, and thought "Gee, that's clever!"

I stared at the code in question a long time, thinking it was just multiplied by -1 so the exact same thing, and then doubted myself thinking I must be missing something, making a silly math mistake (I actually looked-up solving inequalites to confirm) because why would somebody purposely do that?

I really wish you'd added your comment of:

// Intended to read as "are we within one increment of the end"
return (constants.max - systemSettings.settingsValues[(int)option]) <= constants.increment;

I would have just gone, "Gee, that's clever", and moved on.

Also, you are likely to forget the reasons yourself after a few months!

I wish I could 🙃

A co-worker recently strongly disagreed with my rule recital, insisting that he'd forget the reasons by the end of the week rather than in a few months. Another chimed in that he be lucky to make it to the end of day.

Memory aside, the previous point remains that most developers lack the ability to read minds, hence sharing the wisdom in comments.

now we are into personal opinion territory.

True. I apologize if I may have overstepped or offended. The intention was to be helpful, not insulting.

After thinking things over some more, I agree with you that it is better to use 'min <= value < max' everywhere for simplicity. Most developers wouldn't expect 'min <= value <= max' which makes it a bad choice.

However, I just don't see having say a max temperature define of say 451F when that results in an actual max temperature of 450F. That could also require adjusting many values in the configuration files, which could have be sourced from other developers.

Hence adding +1 when used in 'settingConstants', changing a much smaller amount of code. BUT that could lead to forgetting to add +1, hence the idea of using an ugly macro like say

// Ugly, yes, but less likely to be overlooked since value range is:  min <= value < max
#define MAX_VALUE(X) ((X) + 1)

would make it stand out like a sore thumb when used, making it impossible to overlook:

static const SettingConstants settingsConstants[(int)SettingsOptions::SettingsOptionsLength] = {
//{min,max,increment,default}
{MIN_TEMP_F, MAX_VALUE(MAX_TEMP_F), 0, SOLDERING_TEMP},   // SolderingTemp

I'm not claiming it is the best solution, just one that is hard to go unnoticed.

Note that in that example I also removed the hardcoded constants, and changed the increment to 0 as it really doesn't apply to solderingTemp.

As I've already admitted above, I know I'm not always right, but I am always open to learn new tricks, like coding "within one increment".

[Ralim]

So I've been thinking on this too; and I also see the gain of min<=value<=max in that it avoids having to keep track of the +1 that is required in some ranges.

So my original thinking was as I stated above, with keeping to match other conventions; and this works really well for things where its iterating over options, since for three options you can use 0,1,5 to map to [0,1,2,3,4]; since that feels nicer to me. But I can also see good intentions in 0,1,4 for that as well.

Since you raise the point around the temperature, its the main cause of the "within 1 increment of" as well, since it often doesnt land nicely on a more simple bounds check.

Thus, I'm not not certain which is the nicer range setup to use.
I feel that min<=value<=max is nicer than a macro that has to somehow add the increment (since its not always 1).
And thus even though that range isnt my first-to-mind preference, its neater than macros would be.

🤷🏼

I would, but unfortunately I don't own all the supported irons, nor do I claim intimate knowledge of every setting, and the consequences of them.

Dont ever let this stop you really; anyone is qualified to give this code a skim over and raise issues :)

The reason I originally posted this bug was because I changed how the temperature controls worked, and that patch no longer worked on the latest codebase. When I adapted my patch, it was obvious I had unintentionally broken other things. That's when I realized there were different ranges in use.

Ahh, I would love to hear about what/how you changed things :)

Then I ran into solderingTemp not being adjusted through the same methods, with its own range checking haardcoded with constants in gui_solderingTempAdjust() [that also failed if the 'min' is set to 0 because then the comparison 'newTemp < MIN_TEMP_C' becomes 'newTemp < 0' which is always false for unsigned numbers], which made me consider the possibility that other things I'm unfamiliar with might also have their own special handling.

The settings change was a non-trivial change, but was also long overdue. Apologies it was a mess for you :(

After having cooked a thermistor in a 3D printer because of a failed PID, I decided I didn't want to cook my TS100, and thought I'd let the expert(s) confirm or deny what I thought I might have discovered.

There is a hardware sanity mask to stop you cooking the tip beyound around 430C or so so, even if your temp settings go wildly wrong.

Now that said; by asking around this, makes me suspect that its not overly clear that the comparisons are written in grammatical order, so a comment might help here.

Actually, I really appreciated your original explanation, and thought "Gee, that's clever!"

I stared at the code in question a long time, thinking it was just multiplied by -1 so the exact same thing, and then doubted myself thinking I must be missing something, making a silly math mistake (I actually looked-up solving inequalites to confirm) because why would somebody purposely do that?

I really wish you'd added your comment of:

// Intended to read as "are we within one increment of the end"
return (constants.max - systemSettings.settingsValues[(int)option]) <= constants.increment;

I would have just gone, "Gee, that's clever", and moved on.

Aye; I do agree to this, will definitely add one 🤗

Also, you are likely to forget the reasons yourself after a few months!

I wish I could upside_down_face

A co-worker recently strongly disagreed with my rule recital, insisting that he'd forget the reasons by the end of the week rather than in a few months. Another chimed in that he be lucky to make it to the end of day.

Memory aside, the previous point remains that most developers lack the ability to read minds, hence sharing the wisdom in comments.

now we are into personal opinion territory.

True. I apologize if I may have overstepped or offended. The intention was to be helpful, not insulting.

Naah not all; possibly didnt come across but I meant that more in a " this could easily be one of those things that everyone differs on". Definitely not offended :)

After thinking things over some more, I agree with you that it is better to use 'min <= value < max' everywhere for simplicity. Most developers wouldn't expect 'min <= value <= max' which makes it a bad choice.

However, I just don't see having say a max temperature define of say 451F when that results in an actual max temperature of 450F. That could also require adjusting many values in the configuration files, which could have be sourced from other developers.

Hence adding +1 when used in 'settingConstants', changing a much smaller amount of code. BUT that could lead to forgetting to add +1, hence the idea of using an ugly macro like say

// Ugly, yes, but less likely to be overlooked since value range is:  min <= value < max
#define MAX_VALUE(X) ((X) + 1)

would make it stand out like a sore thumb when used, making it impossible to overlook:

static const SettingConstants settingsConstants[(int)SettingsOptions::SettingsOptionsLength] = {
//{min,max,increment,default}
{MIN_TEMP_F, MAX_VALUE(MAX_TEMP_F), 0, SOLDERING_TEMP},   // SolderingTemp

I'm not claiming it is the best solution, just one that is hard to go unnoticed.

As i mentioned earlier, now I'm conflicted on this.

Note that in that example I also removed the hardcoded constants, and changed the increment to 0 as it really doesn't apply to solderingTemp.

Yeah they are sorta there as placeholders, so there is at least something non-zero there

As I've already admitted above, I know not always right, but I am always open to learn new tricks, like coding "within one increment".

I do wonder how much that is less of a trick and a stylistic choice I've picked up somewhere and long forgotten the source.

[aemileski]

The reason I originally posted this bug was because I changed how the temperature controls worked, and that patch no longer worked on the latest codebase. When I adapted my patch, it was obvious I had unintentionally broken other things. That's when I realized there were different ranges in use.

Ahh, I would love to hear about what/how you changed things :)

Summary

When quickly adjusting temperature with a long-press, going through say 271, 276, 281, 286, 291 is visually a lot for my shotglass brain to follow, especially when the numbers are changing quickly. Maybe not for others, but for me it is hard to follow.

It is much easier for me to follow going rapidly from 271 to 275, 280, 285, 290 then fine-tuning to 291 with a short-press.

I admit to being in awe of market vendors that can add a mixed basket of item prices in their heads before I have even reached for my wallet. However, I did set grade school records for speed-reciting any multiplication table of any length.

Configuration changes

• shortStep: 1 to 50, default 1
• longStep: 1 to 50, default 5
• enforced: longStep >= shortStep
• Enforcement means that changing longStep may force a change to shortStep, and vice versa.

I was able to get the current codebase to do this by using custom increment functions, though I did note a lack of a way to replace the decrement function, but didn't need to as these values only increment.

I suspect solderingTemp is the only setting that decrements (maybe voltage calibration uses it?), but it is currently the only setting that doesn't use the standard increment / decrement functions in any manner.

Temperature adjustment changes

• BUTTON_*_LONG causes solderingTemp to change up / down to the nearest integer multiple of longStep
• BUTTON_*_SHORT causes solderingTemp to change up / down by shortStep
• solderingTemp can be adjusted down to 0, though pedantically that's current room temperature.

I'd really prefer to have done this using the standard setting adjustment routines, and that is on my "To Do" list.

This is just a small excerpt sample from my personal patch:

--- a/source/Core/Threads/GUIThread.cpp
+++ b/source/Core/Threads/GUIThread.cpp
@@ -190,7 +190,8 @@ static void gui_solderingTempAdjust() {
     } else {
       waitForRelease = false;
     }
-    int16_t delta = 0;
+    uint16_t newTemp = getSettingValue(SettingsOptions::SolderingTemp);
+    uint16_t delta = 0;
     switch (buttons) {
     case BUTTON_NONE:
       // stay
@@ -202,36 +203,59 @@ static void gui_solderingTempAdjust() {
       break;
     case BUTTON_B_LONG:
       if (xTaskGetTickCount() - autoRepeatTimer + autoRepeatAcceleration > PRESS_ACCEL_INTERVAL_MAX) {
+        delta = getSettingValue(SettingsOptions::TempChangeLongStep);
         if (getSettingValue(SettingsOptions::ReverseButtonTempChangeEnabled)) {
-          delta = getSettingValue(SettingsOptions::TempChangeLongStep);
-        } else
-          delta = -getSettingValue(SettingsOptions::TempChangeLongStep);
-
+          newTemp = (newTemp / delta + 1) * delta;
+       } else {
+         if (newTemp >= delta) {
+            newTemp = ((newTemp + delta - 1) / delta - 1) * delta;
+         } else {
+            newTemp = 0;
+          }
+        }
         autoRepeatTimer = xTaskGetTickCount();
         autoRepeatAcceleration += PRESS_ACCEL_STEP;
       }
       break;
     case BUTTON_B_SHORT:
+      delta = getSettingValue(SettingsOptions::TempChangeShortStep);
       if (getSettingValue(SettingsOptions::ReverseButtonTempChangeEnabled)) {
-        delta = getSettingValue(SettingsOptions::TempChangeShortStep);
-      } else
-        delta = -getSettingValue(SettingsOptions::TempChangeShortStep);
+        newTemp += delta;
+      } else {
+        if (newTemp >= delta) {
+          newTemp -= delta;
+       } else {
+          newTemp = 0;
+        }
+      }
       break;
     case BUTTON_F_LONG:
       if (xTaskGetTickCount() - autoRepeatTimer + autoRepeatAcceleration > PRESS_ACCEL_INTERVAL_MAX) {

Unrelated change

• The SleepingSimpleString four character "Zzzz" crowds the display with the temperature present, so changed to three character "Zzz " for same length, but untested if just a three character string would suffice. I noticed that some other language translations have also done this. I'd change them all myself, but I don't know the other languages, and wouldn't want to accidentally create some sort of international incident.

I also noticed that the EN translation file (and perhaps others) has been DOS edited so CRLF.

Not yet implemented

• Add a "Pb" setting to minimum voltage, or just simplify it to an numeric voltage setting, eliminating any cell count basis. Tenth of a volt resolution should be sufficient to keep everyone happy.

[Ralim]

Temperature adjustment changes

In regards to the rounding values to the increment, this makes a fair bit of sense to me.
Would be open to a PR or to put this on the backlog for future work.
I think this could be done inside the generate up/down functions since I can't see it harming any others

Unrelated change

Happy for this edit to be put in a PR; maybe just change languages you know to keep it easy :)
Endings are probably not consistent as different contributors have edited them over time and I dont care enough to filter them all 😁

[discip]

Good afternoon @aemileski,
would be nice, if you would check the latest build to determine, if your issue is solved as @Ralim stated here: #1164 (comment).

If this solves your issue, please do not forget to close this. 😊

thanks in advance
regards

[aemileski]

1. Thanks for all the work!

2. Wow! Beautiful comments! Thanks! I really appreciate the effort.

3. Please don't hate me for not closing this bug yet.

I've been away from the code long enough (too long) to remember what triggered my 'spidey-sense' with ranges. Looking over the current code ... and nothing. No tingling at all. I guess that's good. Will stare at it some more, fiddle with my iron some more, and rework my customization patch.

Looks like you went with min<=value<=max after all, if that's indeed the case, the max is unreachable in this function (easy to overlook, given it is only used in voltage calibration) unless:

diff --git a/source/Core/Src/Settings.cpp b/source/Core/Src/Settings.cpp
index e09501c..5715b48 100644
--- a/source/Core/Src/Settings.cpp
+++ b/source/Core/Src/Settings.cpp
@@ -175,8 +175,9 @@ bool prevSettingValue(const enum SettingsOptions option) {
   int        value     = systemSettings.settingsValues[(int)option];
   if (value <= constants.min) {
     value = constants.max;
+  } else {
+    value -= constants.increment;
   }
-  value -= constants.increment;
   systemSettings.settingsValues[(int)option] = value;
   return systemSettings.settingsValues[(int)option] == constants.min;
 }

[Firebie]

May be better like this (opposite of nextSettingValue):

// Step backwards on the settings item
// Return true if we are at the end (min)
bool prevSettingValue(const enum SettingsOptions option) {
  const auto constants = settingsConstants[(int)option];
  if (systemSettings.settingsValues[(int)option] == (constants.min)) {
    // Already at min, wrap to the max
    systemSettings.settingsValues[(int)option] = constants.max;
  } else if (systemSettings.settingsValues[(int)option] <= (constants.min + constants.increment)) {
    // If within one increment of the start, constrain to the start
    systemSettings.settingsValues[(int)option] = constants.min;
  } else {
    // Otherwise decrement
    systemSettings.settingsValues[(int)option] -= constants.increment;
  }
  // Return if we are at the min
  return constants.min == systemSettings.settingsValues[(int)option];
}

[Ralim]

Ah yep good spot.
I'll put in the patch for that now.
The pr should show up below :)