arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
208 stars 118 forks source link

map() function equation wrong #51

Open st42 opened 9 years ago

st42 commented 9 years ago

EDIT NOTE: I added a comment to the end of this from a later comment because I thought it should have been included in the first place.

The equation in the function stated at http://arduino.cc/en/Reference/Map

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

is wrong. The example given:

val = map(val, 0, 1023, 0, 255);

In this example 0-1023 is mapped to 0-255. The equation gets it wrong by mapping 1023 numbers to 255 numbers, when it should be mapping 1024 numbers to 256 numbers.

0 through 1023 is 1024 numbers not 1023 : in_max - in_min : 1023-0=1023 0 through 255 is 256 numbers not 255 : out_max - out_min : 255-0=255

The corrected equation in the function should be:

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
}

In the following simpler example it will be easier to see:

val = map(val, 0, 3, 0, 1);

In this example 0,1,2 and 3 is mapped to 0 and 1. Mapped correctly it should be:

0 maps to 0 1 maps to 0 2 maps to 1 3 maps to 1

Using the equation in the the function as it is now, it is mapped as such:

0 maps to 0 1 maps to 0 2 maps to 0 3 maps to 1

This is a common problem programmers have dealt with before. When dealing with arrays you can not determine array size by simply subtracting first position from last position you will always be off by 1. That is why you add 1 to get proper size. I know this is for the most part a minor problem, but if your project requires accurate results this could be a serious issue.

--- Added from later comment---

To give a real world scenario as to why this needs changed consider this. You are using the arduino with a sensor to sample data and based on this data set a servo into 1 of 4 positions. The arduino uses a 10 bit adc and outputs integers from 0 to 1023. You would use map(x, 0, 1023, 1, 4). With the map function as it is currently the only time position 4 would be active is when the arduino outputs the integer 1023. With my proposed change each position would get an equal share of the possible range. When they converted this function from a float function to an integer function they didn't consider that the output is no longer a value but a position. Value mapping maps one value to another and the function works perfectly for floats. Positional mapping distributes the input as evenly as possible to the output and the function as is doesn't do that.

This function needs to be split into two functions. One for floats as it is now and one for integers as I proposed.

sngl commented 9 years ago

Hello @st42 , The map method works with integer numbers. Therefore using too small ranges like 0 - 3 to 0 - 1 produces very large errors. If you try to map 3 from 0-10 to 0-3 you get 0,9 with the old expression and 1,09 with the new expression, that is the same from an integer point of view. The bigger is the range, the smaller is the error. It is not due to the map algorithm but to the machine precision and the lack of a FPU in the ATmega.

matthijskooijman commented 9 years ago

Not sure if the machine precision is really of influence here - the fact that this all uses (and rounds to) integers is. It's also a matter of intepretation. @st42 interprets map(x, 0, 3, 0, 1) as "distribute the integers {0, 1, 2, 3} over the integers {0, 1}. In this case, 0 and 1 should map to 0 and 2 and 3 should map to 1.

However, that really only works when the input range is more than the output range. E.g. with @st42's proposal, you get the following:

map(1, 0, 1, 0, 100) = 1 * 101 / 2 = 50

which seems weird. It seems that mapping the input max should at least give the output max (idem for min).

This hints to an alternative intepretation: map(x, 0, 3, 0, 1) means to map the range [0, 3> to the range [0, 1> (here I mean range of real numbers in the mathematical sense). This means 0 maps to 0, 1 maps to 0.33, 2 maps to 0.67, 3 maps to 1 (note that the length of the ranges is 3 and 1, not 4 and 2 here!). Because the result is truncated to an integer, it ens up mapping 0, 1, and 2 to 0 and only 3 to 1.

Perhaps the (implicit) truncated could be improved to do proper rounding, which would round 0.5 to 1 instead of 0, which is probably the proper solution for this problem?

Also, as @agdl notes, the error is really only significant for small values, on larger values it's lost in the rounding anyway.

cmaglie commented 9 years ago

Perhaps the (implicit) truncated could be improved to do proper rounding, which would round 0.5 to 1 instead of 0, which is probably the proper solution for this problem?

Yes it is.

This lead to another question: how to correctly round using only with integer arithmetic?

matthijskooijman commented 9 years ago

Normal approach to rounding (for non-negative numbers) is to do 0.5 and then truncate. In the above equation, you could do this addition before the divide, so:

return ((x - in_min) * (out_max - out_min) + (in_max - in_min)/2) / (in_max - in_min) + out_min;

which I think will get you as accurate as you can get. I think this even works for negative values, provided that the mapped values are inside the range given. Not sure how to handle negative values otherwise off-hand.

st42 commented 9 years ago

To give a real world scenario as to why this needs changed consider this. You are using the arduino with a sensor to sample data and based on this data set a servo into 1 of 4 positions. The arduino uses a 10 bit adc and outputs integers from 0 to 1023. You would use map(x, 0, 1023, 1, 4). With the map function as it is currently the only time position 4 would be active is when the arduino outputs the integer 1023. With my proposed change each position would get an equal share of the possible range. When they converted this function from a float function to an integer function they didn't consider that the output is no longer a value but a position. Value mapping maps one value to another and the function works perfectly for floats. Positional mapping distributes the input as evenly as possible to the output and the function as is doesn't do that.

This function needs to be split into two functions. One for floats as it is now and one for integers as I proposed.

matthijskooijman commented 9 years ago

I agree that there are really two different kinds of maps and having two functions would be useful. I don't agree that the distinction here is "integers" vs "floats" - I might very well map 0,1023 to 0,5000 to get an ADC reading in millivolts. I need the "value mapping" as you say, but I don't have any need for floats.

So I think think we should:

ffissore commented 9 years ago

I would leave the current map implementation unchanged (too widely used, changing it is not backwards compatible) and add a new function that uses floats for better precision

NicoHood commented 9 years ago

Please never use float. Thats totally inefficient.

Edit: I calculated the equation myself (the whole thing, not only use the values) and i dont see the real problem. Normally it should just work fine. 1023*255/1023+0 = 255??? The +1 seems wrong to me. Progra

You would use map(x, 0, 1023, 1, 4). With the map function as it is currently the only time position 4 would be active is when the arduino outputs the integer 1023. I'd use val >>=8; in this case ;)

agdl commented 9 years ago

Can we close this?

ffissore commented 9 years ago

I guess so. @cmaglie ?

cmaglie commented 9 years ago

No it can't be closed, there isn't still a good solution to this problem.

Just to make it more clear, you can try the following test on your PC:

#include <stdio.h>

// This is the original map() function in Arduino Core
long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

// This is the same but with the +1 "range extrension" as suggested by st42
long mapPlus1(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
}

// This is another version of map with rounding done only with integer calculations
// as suggested by M.Kooijman
long mapRound(long x, long in_min, long in_max, long out_min, long out_max)
{
  return ((x - in_min) * (out_max - out_min) + (in_max - in_min)/2) / (in_max - in_min) + out_min;
}

int main(void) {
        long x;
        printf("Range 0-20 to 0-4\n");
        printf("     x     map  map(+1) map(round)\n");
        for (x=-10; x<=30; x++) {
                printf("%6ld %6ld %6ld %6ld\n", x,
                        map(x, 0, 20, 0, 4),
                        mapPlus1(x, 0, 20, 0, 4),
                        mapRound(x, 0, 20, 0, 4));
        }

        printf("\n\n");
        printf("Range 0-5 to 0-1023\n");
        printf("     x     map  map(+1) map(round)\n");
        for (x=-5; x<=10; x++) {
                printf("%6ld %6ld %6ld %6ld\n", x,
                        map(x, 0, 5, 0, 1023),
                        mapPlus1(x, 0, 5, 0, 1023),
                        mapRound(x, 0, 5, 0, 1023));
        }

        return 0;
}

The output is:

Range 0-20 to 0-4
     x     map  map(+1) map(round)
   -10     -2     -2     -1
    -9     -1     -2     -1
    -8     -1     -1     -1
    -7     -1     -1      0
    -6     -1     -1      0
    -5     -1     -1      0
    -4      0      0      0
    -3      0      0      0
    -2      0      0      0
    -1      0      0      0
     0      0      0      0
     1      0      0      0
     2      0      0      0
     3      0      0      1
     4      0      0      1
     5      1      1      1
     6      1      1      1
     7      1      1      1
     8      1      1      2
     9      1      2      2
    10      2      2      2
    11      2      2      2
    12      2      2      2
    13      2      3      3
    14      2      3      3
    15      3      3      3
    16      3      3      3
    17      3      4      3
    18      3      4      4
    19      3      4      4
    20      4      4      4
    21      4      5      4
    22      4      5      4
    23      4      5      5
    24      4      5      5
    25      5      5      5
    26      5      6      5
    27      5      6      5
    28      5      6      6
    29      5      6      6
    30      6      7      6

Range 0-5 to 0-1023
     x     map  map(+1) map(round)
    -5  -1023   -853  -1022
    -4   -818   -682   -818
    -3   -613   -512   -613
    -2   -409   -341   -408
    -1   -204   -170   -204
     0      0      0      0
     1    204    170    205
     2    409    341    409
     3    613    512    614
     4    818    682    818
     5   1023    853   1023
     6   1227   1024   1228
     7   1432   1194   1432
     8   1636   1365   1637
     9   1841   1536   1841
    10   2046   1706   2046

The original map function gives good results on the extremes, but the distribution of numbers is not fair (for example -2 and 6 appears only at the extemes of the first table)

The map function suggested by @st42 gives a better distribution but it's terrible when going from smaller to bigger ranges (see the second table), and it maps 30 to 7 where it should really be 6.

The third version (by @matthijskooijman ) seems to handle better both extremes and distribution of number but has a strange behaviour with negative numbers (it seems to consider "0" and "-0" two different values).

st42 commented 9 years ago

This should solve the issue:

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  if ((in_max - in_min) > (out_max - out_min)) {
    return (x - in_min) * (out_max - out_min+1) / (in_max - in_min+1) + out_min;
  }
  else
  {
    return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
  }
}

It handles both going from bigger to smaller ranges and smaller to bigger ranges

NicoHood commented 9 years ago

The solution from @st42 works perfectly. Took me some time to understand the "why" and "how" but should be correct.

Another question: should we also add a 2nd version that prevents wrong input? eg you input -10 but the input range is only 0-255. that the output is then automatically out_min

Edit: This would make sense in the case of: I have input from 0-255. I want to reduce noise and only want to map the values from 20-255. But the input can still be 0-19 though. This would just be scaled down to zero.

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  // if input is smaller/bigger than expected return the min/max out ranges value
  if (x < in_min)
    return out_min;
  else if (x > in_max)
    return out_max;

  // map the input to the output range.
  // round up if mapping bigger ranges to smaller ranges
  else  if ((in_max - in_min) > (out_max - out_min))
    return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
  // round down if mapping smaller ranges to bigger ranges
  else
    return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
vvzen commented 6 years ago

I don't know if it's helpful, but this is how the ofMap() function works in openframeworks, a creative coding framework in c++:

//check for division by zero???
//--------------------------------------------------
float ofMap(float value, float inputMin, float inputMax, float outputMin, float outputMax, bool clamp) {

    if (fabs(inputMin - inputMax) < FLT_EPSILON){
        return outputMin;
    } else {
        float outVal = ((value - inputMin) / (inputMax - inputMin) * (outputMax - outputMin) + outputMin);

        if( clamp ){
            if(outputMax < outputMin){
                if( outVal < outputMax )outVal = outputMax;
                else if( outVal > outputMin )outVal = outputMin;
            }else{
                if( outVal > outputMax )outVal = outputMax;
                else if( outVal < outputMin )outVal = outputMin;
            }
        }
        return outVal;
    }

}

source code from here: https://github.com/openframeworks/libs/openFrameworks/math/ofMath.cpp

justin-romano commented 5 years ago

It annoyed me that the out max and min could be surpassed So i added some constraints

long map2(long x, long in_min, long in_max, long out_min, long out_max)
{
    auto res = (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
    res = min(out_max, res);
    res = max(out_min, res);
  return res;
}
PaulStoffregen commented 4 years ago

I recently created an integer-only map() function which (I believe) solves all the problems @cmaglie mentioned above (over 5 years ago), including the "strange behaviour with negative numbers".

It matches the results using floating point and with proper round off to the nearest integer.

When used "backwards" it gives identical results (mapping 20-0 to 4-0 should be identical to mapping 0-20 to 0-4) and when used in "reverse" (mapping 0-20 to 0-4 compared with 0-20 to 4-0) gives the same set of results in reverse order.

The only downside is the algorithm is quite complicated and slow....

long idealMap(long x, long in_min, long in_max, long out_min, long out_max) {
  long in_range = in_max - in_min;
  long out_range = out_max - out_min;
  if (in_range == 0) return out_min + out_range / 2;
  // compute the numerator
  long num = (x - in_min) * out_range;
  // before dividing, add extra for proper round off (towards zero)
  if (out_range >= 0) {
    num += in_range / 2;
  } else {
    num -= in_range / 2;
  }
  // divide by input range and add output offset to complete map() compute
  long result = num / in_range + out_min;
  // fix "a strange behaviour with negative numbers" (see ArduinoCore-API issue #51)
  //   this step can be deleted if you don't care about non-linear output
  //   behavior extrapolating slightly beyond the mapped input & output range
  if (out_range >= 0) {
    if (in_range * num < 0) return result - 1;
  } else {
    if (in_range * num >= 0) return result + 1;
  }
  return result;
}

I also wrote of this on the Arduino forum. https://forum.arduino.cc/index.php?topic=711016.0

If anyone wants to compare them all, here's a sketch you can run on Arduino Uno or other boards to see them all side-by-side.

const long in1 = 0;
const long in2 = 20;
const long out1 = 0;
const long out2 = 4;

void setup() {
  Serial.begin(9600);
  while (!Serial) ; // wait for Arduino Serial Monitor
  delay(100);
  Serial.println("map() function test");

  printHeader(in1, in2, out1, out2);
  for (long i = in1 - 10; i <= in2 + 10; i++) {
    printLong(i, 4);
    printLong(map(i, in1, in2, out1, out2), 7);
    printLong(map(i, in2, in1, out2, out1), 5);
    printLong(st42map(i, in1, in2, out1, out2), 7);
    printLong(st42map(i, in2, in1, out2, out1), 5);
    printLong(roundMap(i, in1, in2, out1, out2), 7);
    printLong(roundMap(i, in2, in1, out2, out1), 5);
    printLong(idealMap(i, in1, in2, out1, out2), 7);
    printLong(idealMap(i, in2, in1, out2, out1), 5);
    Serial.print("    ");
    Serial.println(floatMap(i, in1, in2, out1, out2));
  }
  printHeader(in1, in2, out2, out1);
  for (long i = in1 - 10; i <= in2 + 10; i++) {
    printLong(i, 4);
    printLong(map(i, in1, in2, out2, out1), 7);
    printLong(map(i, in2, in1, out1, out2), 5);
    printLong(st42map(i, in1, in2, out2, out1), 7);
    printLong(st42map(i, in2, in1, out1, out2), 5);
    printLong(roundMap(i, in1, in2, out2, out1), 7);
    printLong(roundMap(i, in2, in1, out1, out2), 5);
    printLong(idealMap(i, in1, in2, out2, out1), 7);
    printLong(idealMap(i, in2, in1, out1, out2), 5);
    Serial.print("    ");
    Serial.println(floatMap(i, in1, in2, out2, out1));
  }
}

float floatMap(float x, float in_min, float in_max, float out_min, float out_max) {
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

long idealMap(long x, long in_min, long in_max, long out_min, long out_max) {
  long in_range = in_max - in_min;
  long out_range = out_max - out_min;
  if (in_range == 0) return out_min + out_range / 2;
  // compute the numerator
  long num = (x - in_min) * out_range;
  // before dividing, add extra for proper round off (towards zero)
  if (out_range >= 0) {
    num += in_range / 2;
  } else {
    num -= in_range / 2;
  }
  // divide by input range and add output offset to complete map() compute
  long result = num / in_range + out_min;
  // fix "a strange behaviour with negative numbers" (see ArduinoCore-API issue #51)
  //   this step can be deleted if you don't care about non-linear output
  //   behavior extrapolating slightly beyond the mapped input & output range
  if (out_range >= 0) {
    if (in_range * num < 0) return result - 1;
  } else {
    if (in_range * num >= 0) return result + 1;
  }
  return result;
}

long st42map(long x, long in_min, long in_max, long out_min, long out_max) {
  if ((in_max - in_min) > (out_max - out_min)) {
    return (x - in_min) * (out_max - out_min + 1) / (in_max - in_min + 1) + out_min;
  } else {
    return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
  }
}

long roundMap(long x, long in_min, long in_max, long out_min, long out_max) {
  return ((x - in_min) * (out_max - out_min) + (in_max - in_min) / 2) / (in_max - in_min) + out_min;
}

void loop() {
}

void printLong(long n, int nchar) {
  char format[6], buf[12];
  sprintf(format, "%%%dd", nchar);
  sprintf(buf, format, n);
  Serial.print(buf);
}

void printHeader(long in_min, long in_max, long out_min, long out_max) {
  Serial.println();
  Serial.print("Testing map ");
  Serial.print(in_min);
  Serial.print(" - ");
  Serial.print(in_max);
  Serial.print("  ---->  ");
  Serial.print(out_min);
  Serial.print(" - ");
  Serial.println(out_max);
  Serial.println();
  Serial.println("Input   ArduinoMap   st42map     roundMap    idealMap   floatMap");
}
bperrybap commented 3 years ago

Here is one I did and proposed MANY years ago, when there was a long heated discussion on this topic.

long newmap(long x, long in_min, long in_max, long out_min, long out_max)
{
    if( x == in_max)
        return out_max;
    else if(out_min < out_max)
        return (x - in_min) * (out_max - out_min+1) / (in_max - in_min) + out_min;
    else
        return (x - in_min) * (out_max - out_min-1) / (in_max - in_min) + out_min;
}
bperrybap commented 3 years ago

Here is a test sketch that can be run on an Arduino to test range sets. The code includes 4 different map() functions for comparison.

To use, modify the variables: fromLow, fromHigh, toLow, toHigh

Then compile and upload to your Arduino and look at the serial port output. Here are some interesting ranges

here is the code:

#if 0
// this is the map() function that is currently in the Arduino.cc core library
long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
#endif

// a "rounding" version that some suggest
long maprnd(long x, long in_min, long in_max, long out_min, long out_max)
{
        return (x - in_min) * (out_max - out_min+1) / (in_max - in_min+1) + out_min;
}

// this is the map() function that is currently in the ESP866 platform core library
long mapESP(long x, long in_min, long in_max, long out_min, long out_max) {
    const long dividend = out_max - out_min;
    const long divisor = in_max - in_min;
    const long delta = x - in_min;

    return (delta * dividend + (divisor / 2)) / divisor + out_min;
}

// bperybap version
long newmap(long x, long in_min, long in_max, long out_min, long out_max)
{
    if( x == in_max)
        return out_max;
    else if(out_min < out_max)
        return (x - in_min) * (out_max - out_min+1) / (in_max - in_min) + out_min;
    else
        return (x - in_min) * (out_max - out_min-1) / (in_max - in_min) + out_min;
}

long fromLow = 0;
long fromHigh = 10;
long toLow = 10;
long toHigh = 0;

void setup(void)
{
long val, mval, mval2, mvalESP, mvalnew;
long dir = 0;
char buf[128];

    Serial.begin(115200);
    while(!Serial){}
    delay(500);
    Serial.print("\n\n");

    if(fromLow < fromHigh)
        dir = 1;
    else
        dir = -1;

    for( val = fromLow;; val += dir)
    {
        mval =       map(val,fromLow,fromHigh, toLow,toHigh);
        mval2 =     maprnd(val,fromLow,fromHigh, toLow,toHigh);
        mvalESP = mapESP(val,fromLow,fromHigh, toLow,toHigh);
        mvalnew = newmap(val,fromLow,fromHigh, toLow,toHigh);

        sprintf(buf, "map(%3ld,%3ld,%3ld,%3ld,%3ld)",
         val, fromLow, fromHigh, toLow, toHigh);

        Serial.print(buf);

        sprintf(buf, " map:%3ld, maprnd:%3ld, mapESP:%3ld, newmap:%3ld\n",
         mval, mval2, mvalESP, mvalnew);

        Serial.print(buf);

        if(val == fromHigh)
            break;
    }
}
void loop(void){}
per1234 commented 3 years ago

There is additional discussion on the subject at https://github.com/arduino/Arduino/issues/10382, which I am closing as a duplicate of this one.

bperrybap commented 3 years ago

So why can't we fix this with either my proposed map function or Paul's templates as either do the mappings correctly (the way people would expect) and work for some cases where others fall flat on their face ad screw up for certain ranges like the current Arduino.cc IDE supplied map() function, the map() function proposed in the first post in this issue, and the ESP8266 core map() function.

IMO, I think Paul's template is the way to go as it works properly and as expected for more than just ints. C++ templates are ideal for this kind of stuff as long as it is ok to no longer have a map() function for C code.

How big of deal is it that fixing the existing map() function with better code that works more correctly potentially breaks some small amount of existing code that has attempted to work around the issue by fudging up the parameters? And is support for C important or can it just exist and work with C++

It seems like we have a working solution for the calculation and it is now a question of deciding on the details of how to get it integrated. For example, does it replace the existing map() function, is a a new function with a different name, or should it be handled as method function off of some "math" or "map" object ? and that object might be some sort of math class library And is support for C important or can it just exist and work with C++

The reason I bring up a math or map object is that this math object could be used for all kinds of Arduino math functions that also have issues. i.e. all the functions / macros that overlaying and overriding standard C library functions. By supplying an Arduino "math" library with a default math object with templates it could fix several things at once like map(), min(), max() etc.... by using the new method functions like math.map() math.min(), math.floor(), etc.... It also avoids any backward compatibility issues.

Seems like once we decide these questions, a solution could be integrated into the IDE fairly easily / quickly.

PaulStoffregen commented 3 years ago

I'm pretty sure the answer to "why can't we" comes down to Arduino's priorities as a company. horse