clamp() does not work for byte type as expected

[sphh]

When I change a variable of the byte type between 0 and 100, it wraps at 0 despite setting the style to noStyle.

byte brightness;

FIELD(brightness, "Brightness", "%", 0, 100, 10, 1, doNothing, noEvent, noStyle)

I guess the same would happen if I set the upper border to 255 (and for any other (unsigned) variable which can overroll).

If I change the following code:

   void doNav(navNode& nav,navCmd cmd) {
       [...]
       case upCmd:
         menuField<T>::target()+=(menuField<T>::tunning?menuField<T>::tune():menuField<T>::step())*(options->invertFieldKeys?-1:1);
         menuField<T>::dirty=true;
         menuField<T>::clamp();
         nav.event(options->useUpdateEvent?updateEvent:enterEvent);
         break;
       [... similar for downCmd]

to

   void doNav(navNode& nav,navCmd cmd) {
       [...]
       case upCmd:
//         menuField<T>::target()+=(menuField<T>::tunning?menuField<T>::tune():menuField<T>::step())*(options->invertFieldKeys?-1:1);
         menuField<T>::target()=constrain(menuField<T>::target()+(menuField<T>::tunning?menuField<T>::tune():menuField<T>::step())*(options->invertFieldKeys?-1:1),menuField<T>::low(),menuField<T>::high());
         menuField<T>::dirty=true;
//         menuField<T>::clamp();
         nav.event(options->useUpdateEvent?updateEvent:enterEvent);
         break;
       [... similar for downCmd]

[neu-rah]

can you make it signed char?

[sphh]

Yes, in this case I could use an unsigned char, but if I want to use the whole range from 0 to 255, that's not possible. Maybe a warning in the documentation will keep others from pulling their hairs because of this behaviour.

I've no idea why constrain() works. It does basically the same as clamp(). From hardware/arduino/avr/cores/arduino/Arduino.h

#define constrain(amt,low,high) ((amt)<(low)?(low):((amt)>(high)?(high):(amt)))

(but you know that already!).

[neu-rah]

no i did not knew, thanks

do you want to do a pull request with the replacement or should i add it?

[neu-rah]

but i know why mine is not working, its because i'm not making distintion between signed and unsigned... its a bug. but instead of seeking for it i would rather use the existing solution. thanks again.

[neu-rah]

1) a byte (0..255) does auto wrap even if you do not define wrap 2) unsigned values are never negative, so if your lower bound is 0 then, when decreasing bellow 0 you will get a very large number, if the obtained large number is higher than the top then. 2a) if wrap defined it will "wrap" it to 0 and this looks like a non-wrapping. 2b) if wrap is not defined, the value is clamped to the top (because it was bigger than top) and it looks like a wrapping.

so situations where it looks ok from one side will not be ok from the other.

that constrain thing wont do it either.

and yes, i will add it to doc that unsigned values can not wrap. (or they can if the defined bounds are more than a step away from the numeric bounds)

thanks for noticing it

[sphh]

Hi,

I managed to make this work with unsigned values with the following patch. It simply checks the ranges before applying the increment. It also removes the enterEvent when leaving the editing mode (see #85). I hope I did not introduce some other errors.

--- src/menu.h.orig
+++ src/menu.h
@@ -145,13 +145,23 @@
         void printHigh(menuOut& o) const override;
         void printLow(menuOut& o) const override;
         bool async(const char *uri,navRoot& root,idx_t lvl) override;
-        void clamp() {
-          if (target()<low()) {
-            if (style()&wrapStyle) target()=high();
-            else target()=low();
-          } else if (target()>high()) {
-            if (style()&wrapStyle) target()=low();
-            else target()=high();
+        void stepit(int increment) {
+          int thisstep = increment*(tunning?tune():step())*(options->invertFieldKeys?-1:1);
+          dirty=true;
+          if (thisstep < 0 && (target()-low()) < -thisstep) {
+            if (style()&wrapStyle) {
+              target() = high();
+            } else {
+              target() = low();
+            }
+          } else if (thisstep > 0 && (high()-target()) < thisstep) {
+            if (style()&wrapStyle) {
+              target() = low();
+            } else {
+              target() = high();
+            }
+          } else {
+            target() += thisstep;
           }
         }
     };
@@ -599,24 +609,24 @@
           if (tunning||options->nav2D||!tune()) {//then exit edition
             tunning=false;
             dirty=true;
+            target() = constrain(target(), low(), high());
+            nav.event(options->useUpdateEvent?updateEvent:enterEvent);
             nav.root->exit();
+            return;
           } else tunning=true;
           dirty=true;
           break;
         case upCmd:
-          target()+=(tunning?tune():step())*(options->invertFieldKeys?-1:1);
-          dirty=true;
+          stepit(1);
           break;
         case downCmd:
-          target()-=(tunning?tune():step())*(options->invertFieldKeys?-1:1);;
-          dirty=true;
+          stepit(-1);
           break;
         default:break;
       }
       /*if (ch==options->getCmdChar(enterCmd)&&!tunning) {
         nav.event(enterEvent);
       }*/
-      clamp();
       if (dirty)//sending enter or update event
         nav.event(options->useUpdateEvent?updateEvent:enterEvent);
     }

[neu-rah]

yes, preventive checks before step are the way of dealing with unsigned's... thanks for coding it :D

now, is there a way to submit this to the repo keeping your authorship? I've just been accepting pull request and this patch thing here on github is completely new to me.

P.S. I see that you have cloned the repo, so you can do a pull request, if you don't mind. I would be glad.

[sphh]

Did I clone the repo? I'm not aware of it. It was probably done by accident... I find this github pull request business too complicated for a tiny change.

Just use the changes if you want to. If you want to add a reference to my authorship somewhere, that is fine with me. If you don't want to do it, that's also fine with me. Do as you please!

Maybe you want to rename the stepit function. At that time I could not come up with a better name, since step is already used for a variable. Thinking about it, increment(int incr) would be a better candidate...

[neu-rah]

Ok, i will do so, thanks for coding this

[neu-rah]

patched

https://github.com/neu-rah/ArduinoMenu/commit/79b8184dec44313b2ce247d308c1d37ac1ecffd1