Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix qadd7() #295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix qadd7() #295

wants to merge 1 commit into from

Conversation

NicoHood
Copy link
Contributor

@NicoHood NicoHood commented Apr 29, 2016

No AVR implementation was added, defaulting to the c code!

Test Sketch:

#include "FastLED.h"
void setup() {}

void loop() {
  // put your main code here, to run repeatedly:
  Serial.println(qadd7(-100, -100));
  Serial.println(qadd7(100, 100));
  Serial.println(qadd7(-100, -10));
  Serial.println(qadd7(100, -10));
  Serial.println("*****");
  delay(1000);
}

/*
Output:
-128
127
-110
90
*****
*/

Feel free to close the PR and copy the code to get a better commit if you wish.

@NicoHood
Copy link
Contributor Author

Something like this would also make a lot of sense to me:

    uint8_t qmod8(uint8_t i, int8_t j)
    {
      int16_t t = i + j;
      if (t > 255) t = 255;
      if (t < 0) t = 0;
      return t;
    }

It is a mix of qadd8 and qsub8. I basically dont determine the plus or minus operation by the function name, but rather via a signed int8.

Maybe qadd7 can be renamed (or aliased) to qmod7 and qsub7, as it will all do the same. (qsub7 needs an inverted j though).

@focalintent
Copy link
Contributor

mod is the wrong name to use here - the modulus operation (usually abbreviated to mod) is the remainder of a division. https://en.wikipedia.org/wiki/Modulo_operation.

And your qmod8 function is defeating the whole purpose of the q* functions. qadd8 takes 2 unsigned 8-bit values and adds them together, clamping at 255. qsub8 takes 2 unsigned 8-bit values and subtracts them from each other, clamping at 0. By using an int8 for your qmod8 you cap the maximum amount of difference to 127 in either direction. So, for example, your qmod8 can't be used to do qadd8(5, 192) - because 192 wraps around to a negative value in int8. (Forget the fact that with math functions, most people new to programming do distinguish between addition and subtraction by the function names, not by the sign on the variable/number involved).

@NicoHood
Copy link
Contributor Author

NicoHood commented May 1, 2016

"mod" in the sense of "modify". I dont know any other name, suggest something.

The idea is to not make the inputs swapable. It will add/substract from the first value (i) depending on what the addition of value two (j) does. The result will be 0-255, but the 2nd parameter can be signed.

qadd8: uint8_t, uint8_t
qadd7: int8_t, int8_t
qxxx8: uint8_t, int8_t

So maybe just a more clever name has to be found.

@tullo-x86
Copy link
Contributor

The underlying operation is signed addition, so maybe qadd87(uint8_t, int8_t) makes sense?

I'm sure you've found a use for the function but whatever it is escapes me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants