Remainder when Incrementing Backwards

I'm having problems using a remainder to keep a value within a specified range. I'm using a rotary encoder to increment up and down, and this bit of code sets the offset to a clock value that comes into my code in GMT only. So I want to be able to offset that GMT time to get local time.

It works well when I increment up, but when clockOffset is an unsigned int and I increment down, i'll get to zero, and then it goes to 15, instead of 23 like I would expect. What am I missing here?

void incrementOffset() {
  unsigned char result = rotary.process();
  
if (result == DIR_CW) {
    clockOffset = (clockOffset + 1) % 24; // increment up one within range of 0-23
  } 
else if (result == DIR_CCW) {
    clockOffset = (clockOffset - 1) % 24; // increment down one within range of 0-23
  }
}

When you reach 0, the unsigned int wraps to its maximum value, which is not divisable by 24.

[edit]

To fix this, try to add 24 every time. E.g.,

clockOffset = (clockOffset + 25) % 24;
//...
clockOffset = (clockOffset + 23) % 24;
1 Like

nvm didn’t see it

More efficient:

void incrementOffset() {
  unsigned char result = rotary.process();
  
if (result == DIR_CW) {
    if (clockOffset == 23) clockOffset = 0; else clockOffset++; // increment up one within range of 0-23
  } 
else if (result == DIR_CCW) {
    if (clockOffset == 0) clockOffset = 23; else clockOffset--; // decrement one within range of 0-23
  }
}

The % operator is relatively slow/expensive.

1 Like

Very clever, thank you!

Interesting, makes perfect sense, thank you. I suppose this is probably the best solution since this function is an ISR, so I should keep it as light as possible.

or

void incrementOffset() {
  switch (rotary.process()) {
    case DIR_NONE:  return;
    case DIR_CW:    clockOffset = (clockOffset == 23) ? 0 : clockOffset + 1; break
    case DIR_CCW:   clockOffset = (clockOffset == 0) ? 23 : clockOffset - 1; break
  }
}

for readability and let the compiler optimize the switch.

Or using a if and testing first with DIR_NONE which is possibly going to be >99% of the time and returning will save time as you won't be doing the two other tests.

--
EDIT: if it's an ISR, is it called only when you move the rotary encoder or on a time base?
if it's the former you know DIR_NONE is not possible so you could write

void incrementOffset() {  
  if (rotary.process() == DIR_CW) {
    clockOffset = (clockOffset == 23) ? 0 : clockOffset + 1;
  } else {
    clockOffset = (clockOffset == 0) ? 23 : clockOffset - 1;
  }
}

and save one test because if it's not DIR_CW and it has moved, then it surely is DIR_CCW


side note I prefer to use the encoder library

This function is called on ly when the rotary coder is moved.

I wonder how fast the rotary encoder must be turned in order for this to become noticeable.

For some architectures (not AVR though) modulo a compile time constant is pretty fast. Even faster when the constant is a power of two.

When I said relatively expensive, I meant compared to the code I suggested which is only a comparison and an increment/decrement. Modulo is probably not not so slow that it would cause problems with rotary encoders at normal human speeds, but for an encoder attached to some high speed mechanism it might cause missed signals.

Indeed when the compiler cannot optimize the modulo it’s more costly than the test + addiction

That applies as well to test + addition, but indeed a bit later

Just out of interest I made some measurements and did a quick back of the envelope calculation.

On a Pro mini, this operation seems to take somewhere between 4µs (single measurement) and 6µs (1,000,000 measurements including loop overhead and an addition to avoid the compiler optimising everything away) so maybe 5µs would be a safe bet.

If the interrupt is triggered on every change (48 times per rotation) and we assume there is no other overhead, then we could reach 250,000rpm, enough to keep up with a dental drill.

It would be fun to see that in action. I do not think the rotary encoder would survive.

1 Like

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.