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

map() for float data type #71

Open
rch33 opened this issue May 21, 2016 · 18 comments
Open

map() for float data type #71

rch33 opened this issue May 21, 2016 · 18 comments

Comments

@rch33
Copy link

rch33 commented May 21, 2016

This is related to issue 288. The Arduino.app C++ compiler accepts float arguments (no warning) to the map() function and then truncates them to integer values. This provides erroneous output that may not be immediately noticed. I think that map() should be defined as all float -- args and return (below). Particle.io's development platform has the same problem. All C++ compilers?

My float version:
float mapf(float value, float istart, float istop, float ostart, float ostop) {
return ostart + (ostop - ostart) * ((value - istart) / (istop - istart));
}

See http://dicks-photon-arduino.blogspot.com/

@Chris--A
Copy link

Shouldn't need to rename to mapf. Overloading the map() function with all floats should suffice.

@rch33
Copy link
Author

rch33 commented May 23, 2016

This is just a dandy example of what's wrong with C++. I've been writing C since it was developed. Like Ken Thompson, I think C++ fills a much needed gap. The bit about "String" vs. "char" is another pain in the a**.

@Chris--A
Copy link

I do not quite understand the point you are making.

Overloading is a very useful feature. Also, Strings, and a single char are somewhat unrelated.

@q2dg
Copy link

q2dg commented May 24, 2016

Well, maybe it's unrelated, but it may also taken into account... https://github.com/arduino/Arduino/issues/2466

@cousteaulecommandant
Copy link

Shouldn't need to rename to mapf. Overloading the map() function with all floats should suffice.

Careful here. Actually what you're interested in is the RETURN value of the function, and as far as I know C++ doesn't allow overloading by return type.

Normally you want to deal with integers, so that the map() function receives an integer and yields an integer, and only integer arithmetic is used. Sometimes you may want to get a float, using the potentially slower float arithmetic. But if someone tries to do something like x = map(n, 0.0, 1.0, 0.0, 100.0); and n is an integer... then what? (I think the compiler will complain about stuff being ambiguous.)

So options are:

  • Implement map as an overloaded function, and tell users to be very careful with the types, casting when necessary.
  • Have an integer map and a float mapf.
  • Always use floats (map() is functionally equivalent to long(mapf()), but the latter probably uses more resources). (eww, no.)

I might be biased by C, but in this case I think the second option makes more sense, since the intent may not be clear from the arguments, and it is potentially dangerous for inexperienced programmers.

@rch33
Copy link
Author

rch33 commented Jul 20, 2016

To me, C++ (and other variants) fills a much-needed gap. And I was a Bell Labs when it was developed. I always treat it as plain C where possible and would avoid it all together if I weren't programming Arduinos and Particle Photons.

For Ken Thompson's view, see Ken Thompson on C++

|
|
|
|

|

|
|
| |
Ken Thompson on C++
In an interview for Coders at Work book: It certainly has its good points. But by and large I think it’s a bad l... | |

|

|

On Wednesday, July 20, 2016 11:33 AM, cousteau <notifications@github.com> wrote:

Shouldn't need to rename to mapf. Overloading the map() function with all floats should suffice.
Careful here. Actually what you're interested in is the RETURN value of the function, and as far as I know C++ doesn't allow overloading by return type.Normally you want to deal with integers, so that the map() function receives an integer and yields an integer, and only integer arithmetic is used. Sometimes you may want to get a float, using the potentially slower float arithmetic. But if someone tries to do something like x = map(n, 0.0, 1.0, 0.0, 100.0); and n is an integer... then what? (I think the compiler will complain about stuff being ambiguous.)So options are:

  • Implement map as an overloaded function, and tell users to be very careful with the types, casting when necessary.
  • Have an integer map and a float mapf.
  • (eww, no.)
    
    I might be biased by C, but in this case I think the second option makes more sense, since the intent may not be clear from the arguments, and it is potentially dangerous for inexperienced programmers.—
    You are receiving this because you authored the thread.
    Reply to this email directly, view it on GitHub, or mute the thread.

@Chris--A
Copy link

Careful here. Actually what you're interested in is the RETURN value of the function, and as far as I know C++ doesn't allow overloading by return type.

This is true only if the return type is changed (not the parameters). However what the op was requesting was the return type and parameters be a float, which provides a unique function signature and can be overloaded (A float return has no beneficial traits when all operands are integral types (not floats)).

Normally you want to deal with integers, so that the map() function receives an integer and yields an integer, and only integer arithmetic is used. Sometimes you may want to get a float, using the potentially slower float arithmetic. But if someone tries to do something like x = map(n, 0.0, 1.0, 0.0, 100.0); and n is an integer... then what? (I think the compiler will complain about stuff being ambiguous.)

If n is an integer the float version will be selected.

There is a problem when using all integers though as map originally implements the parameters as long int. This causes ambiguity as an int can implicitly cast to both a float and long int. So certainly there is a problem here, maybe not the one you pointed out, but a problem with my code none the less. This however, can be fixed using SFINAE to invalidate the float version if no parameters are a float.

This solution would be quite verbose (usage would be simple, however the code implementing it is complex). If there is interest for it, I'd certainly be keen to write it, or at least propose a change.

@cousteaulecommandant
Copy link

cousteaulecommandant commented Aug 17, 2016

But if someone tries to do something like x = map(n, 0.0, 1.0, 0.0, 100.0); and n is an integer... then what?

If n is an integer the float version will be selected.

Well, I got this behavior:

int foo(int a, int b, int c, int d, int e) { return 0; }
float foo(float a, float b, float c, float d, float e) { return 0; }

void setup() { }
void loop() {
  int n = 0;
  foo(n, 1.0f, 2.0f, 3.0f, 4.0f);
}
sketch_aug17a:7: error: call of overloaded 'foo(int&, float, float, float, float)' is ambiguous

If I remove the f at the end of each number it compiles. However, it seems that it is using the int version. Proof is that this code

int foo(int a, int b, int c, int d, int e) { volatile int x = 1; return 0; }
float foo(float a, float b, float c, float d, float e) { return 0; }

void setup() { }
void loop() {
  int n = 0;
  foo(n, 1.0, 2.0, 3.0, 4.0);
}

produces an output 26 bytes larger than if I move the volatile int x = 1; to the float version. (Unfortunately I don't have an Arduino board here to make a proper test, but feel free to check this.)

In short, I think just having two versions with different names would simplify things a lot. It will save headaches to both developers and users.

@peabo7
Copy link

peabo7 commented Aug 18, 2016

On August 17, 2016 at 2:17 PM cousteau notifications@github.com wrote:

But if someone tries to do something like x = map(n, 0.0, 1.0, 0.0, 100.0); and n is an integer... then what?

If n is an integer the float version will be selected.

Well, I got this behavior:

int foo(int a, int b, int c, int d, int e) { return 0; }
float foo(float a, float b, float c, float d, float e) { return 0; }

Shouldn't you write this as

float foo(int a, float b, float c, float d, float e) { return 0; }

Peter Olson

void setup() { }
void loop() {
int n = 0;
foo(n, 1.0f, 2.0f, 3.0f, 4.0f);
}


sketch_aug17a:7: error: call of overloaded 'foo(int&, float, float, float, float)' is ambiguous


If I remove the `f` at the end of each number it compiles.  However, **it seems that it is using the int version.**  Proof is that this code
```arduino
int foo(int a, int b, int c, int d, int e) { return 0; }
float foo(float a, float b, float c, float d, float e) { volatile int x = 1; return 0; }

void setup() { }
void loop() {
  int n = 0;
  foo(n, 1.0f, 2.0f, 3.0f, 4.0f);
}

produces an output 26 bytes larger than if I move the volatile int x = 1; to the float version. (Unfortunately I don't have an Arduino board here to make a proper test, but feel free to check this.)

In short, I think just having two versions with different names would simplify things a lot. It will save headaches to both developers and users.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/arduino/Arduino/issues/4975#issuecomment-240499774

@per1234 per1234 changed the title C++ bug/"feature" map() for float data type Jul 4, 2017
@liamkinne
Copy link

I think what you guys are looking for is C++ templates. This would ensure return types are the same as the output types and even convert types where needed.

Here's what I've whipped up in the past for another project:

template<typename T, typename T2>
inline T map(T2 val, T2 in_min, T2 in_max, T out_min, T out_max) {
		return (val - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

@cousteaulecommandant
Copy link

cousteaulecommandant commented Dec 30, 2017

Yep, maybe templates + indicating the return type as the out_min and out_max type is the way to go. My main concern was not being able to explicitly indicate the return type, only the argument types, but it is true that it could be made so that the type of out_min and out_max is used.
Also, templates would allow to state this type explicitly, like map<Type>(...) (although maybe using the correct argument type would be better).

EDIT: The issue with heterogeneous types still holds: map(1, 0.0, 2.0, 0.0, 10.0) complains; I would suggest making val its own type. Or even better; let each variable have its own type.
Apaprently in C++14 this is as easy as declaring auto map(auto val, auto in_min, auto in_max, auto out_min, auto out_max). For earlier C++ versions, the only way around seems to be the template.

@PaulStoffregen
Copy link
Sponsor

FWIW, here's the template-based map() from Teensy, where we're using C++14 dialect and type_traits.

#ifdef __cplusplus
#include <type_traits>
// when the input number is an integer type, do all math as 32 bit signed long
template <class T, class A, class B, class C, class D>
long map(T _x, A _in_min, B _in_max, C _out_min, D _out_max, typename std::enable_if<std::is_integral<T>::value >::type* = 0)
{
        long x = _x, in_min = _in_min, in_max = _in_max, out_min = _out_min, out_max = _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;
        }
}
// when the input is a float or double, do all math using the input's type
template <class T, class A, class B, class C, class D>
T map(T x, A in_min, B in_max, C out_min, D out_max, typename std::enable_if<std::is_floating_point<T>::value >::type* = 0)
{
        return (x - (T)in_min) * ((T)out_max - (T)out_min) / ((T)in_max - (T)in_min) + (T)out_min;
}

@cousteaulecommandant
Copy link

// when the input number is an integer type, do all math as 32 bit signed long

Oh yes; I forgot that. If you don't cast to long, an int->int mapping has high chances of overflowing.
(Example: map(150, 0,256, 0,360) would overflow int because 150*360>32767)

Honestly I think all this (rewriting all Arduino.h's function-like macros as templated functions or std:: ones) should be thoroughly discussed, maybe on a separate issue or a thread on the mailing list.

@liamkinne
Copy link

I agree, and moving things to templates and whatnot will inevitably break someones code somewhere.

I've been writing my own alternative Arduino core (@PhantomEmbedded) that addresses many of these issues, but is completely incompatible because everything is based on classes rather than functions.

If someone makes a separate thread on this topic I would be happy to help.

@MeAmAnUsername
Copy link

Question: is this something people still want?
I could implement it, but that's a bit wasted effort if it's not needed anymore.

@marmilicious
Copy link

I'm guessing that at this point, those that were bothered by this or wanted this have rolled our own. I wouldn't be opposed to seeing something official in the future though.

@dsyleixa
Copy link

map should be overloaded to work with DOUBLE,
NOT for float!

workaround:
which mapf works already for double?

@sandeepmistry sandeepmistry transferred this issue from arduino/Arduino Sep 16, 2019
@PaulStoffregen
Copy link
Sponsor

The template I showed above (about 3 years ago) automatically does map() using 64 bit double if your input variable is a 64 bit double. But if you give it 32 bit float, everything is done using faster 32 bit floats. And if you give it any integer, everything is computed using 32 bit long.

No need to add mapf() to the Arduino API. Templates can let you have your cake and eat it too, from one convenient map() that automatically adapts depending on the type of the user's input variable.

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

No branches or pull requests

10 participants