dtex / j5e

Framework for embedded devices using ECMA-419, the ECMAScript® embedded systems API specification, based on Johnny-Five's API
https://www.j5e.dev/
MIT License
64 stars 6 forks source link

fmap function - really Float32? #60

Closed phoddie closed 4 years ago

phoddie commented 4 years ago

I bumped into the fmap function when experimenting more with preloading. It generates an exception writing to the Float32Array in f32A:

const f32A = new Float32Array(1);

https://github.com/dtex/j5e/blob/7c393c2f088bf111ef8f8945c03f71685f9f1443/lib/fn/index.js#L106-L109

To make this work, f32A would need to be allocated at runtime (not during preload). But, it seems to me that fmap really just returns a JavaScript Number, not a Float32 as documented. The Number happens to be truncated so that it can fit into a Float32. The calling code (in my test) is sensor.js > scaled, which should still work with a full resolution float.

Maybe the point of fmap isn't to return a value constrained to a 32-bit float as much as it is to not return an integer (which is what map does)? In that case, it could just be:

 export function fmap(value, fromLow, fromHigh, toLow, toHigh) {
    return (value - fromLow) * (toHigh - toLow) / (fromHigh - fromLow) + toLow;
 };
dtex commented 4 years ago

I ran into this as well and just made the declaration inside the function to get around it for now. Let me track down when that code was changed from being a plain old Number and see if I can figure out the author's intent.

dtex commented 4 years ago

I found the commit and the code was changed from exactly what you suggested as an alternative.

J5 apps are rarely subject to memory constraints, nor can I think of any place where we would ever store a large array of these values. I also think that they have to be converted back to 64-bit before they are operated on so I don't think it's a performance thing.

Let's ask @rwaldron. It's been a few years so it may be hard to remember, but I'd wager he had a good reason.

dtex commented 4 years ago

This code has been switched to just use a Number so I'm going to close this issue.

phoddie commented 4 years ago

Nice. One less mystery in the universe. And a few more cycles saved.

You might want to update the comment at some point, especially if it generates docs:

Like map, but returns a Float32