sys: add common imath module mv isin() form test/driver_dac_dds #19378

benpicco · 2023-03-10T12:31:09Z

Contribution description

An integer-only sin() function is generally useful, move it to a common place imath where more integer-only math functions can be added when needed.

Testing procedure

This only moves code around.

Issues/PRs references

sys/imath/imath.c

riot-ci · 2023-03-10T14:30:12Z

Murdock results

✔️ PASSED

34d0027 imath: add powi() function

Success	Failures	Total	Runtime
6933	0	6933	13m:11s

Artifacts

Documentation preview

kfessel

I stared at this for too long:
bike-shed:

some pre- or postfix for the function names that indicate that it is not only special for its type but also less accurate i think fast_isin() would be nice (like fast_sqrt (just realised we don't have that quake algorithm :) ).
Q Notation needs to be explained https://en.wikipedia.org/wiki/Q_%28number_format%29
I think there should no be two ways for writing fractional binary number with N-Bit fraction in one piece of documentation (Q14 vs 2^^15)
define PI
ISIN_PERIOD is wrong

https://godbolt.org/z/edqv4T6YM

this get us both fast_isin and fast_icos (icos being smaller)
iPI = 0x8000>>1

static inline  __attribute__((always_inline))
int32_t ihelp(int32_t x){
    int32_t y;
    static const int32_t qN = 13,
                         qA = 12,
                          B = 19900,
                          C = 3516;

    x = x << (31 - qN);         /* Mask with PI                 */
    x = x >> (31 - qN);         /* Note: SIGNED shift! (to qN)  */
    x = x * x >> (2 * qN - 14); /* x=x^2 To Q14                 */

    y = B - (x * C >> 14);      /* B - x^2*C                    */
    y = (1 << qA)               /* A - x^2*(B-x^2*C)            */
      - (x * y >> 16);

    return y;
}

//static inline
int32_t isin(int32_t x){
    static const int32_t qN = 13;
    int32_t c = (x << (30 - qN));
    int32_t y = ihelp(x - (1 << qN));
    return c >= 0 ? y :-y ;
}

//static inline
int32_t icos(int32_t x){
    static const int32_t qN = 13;
    // vvv this is most probalbly not the optimal way to write that
    //     but the compiler gets it
    int32_t c = ((x + iPI/2) << (30 - qN));  
    int32_t y = ihelp(x);
    return c >= 0 ? y :-y ;
}

sys/include/imath.h

benpicco · 2023-03-14T19:03:35Z

iPI = 0x8000>>1

so iPI = ISIN_PERIOD >> 1?

benpicco · 2023-03-22T12:52:53Z

it is not only special for its type but also less accurate

but is it really less accurate if we only have fixed point? The accuracy is pretty much given by the number format.

Plot of the values looks good.

I did drop the ambiguous Q notation entirely.

kfessel · 2023-03-22T14:58:35Z

i checked with a scaled libm sin() and got a difference of upto 13 (12 with some prescaling) (so ~3-4 Bits may be not accurate)

https://godbolt.org/z/8hdW7ra6M

maribu · 2023-03-23T11:31:04Z

@kfessel asked me to join the bike shedding to untie the knot. My two cents are:

there are already double sin(double x), float sinf(float x), long double sinl(long double x) in the C standard, so an int32_t sini(int32_t x) (or maybe even an int32_t sini32(int32_t x)) would be more consistent
if there is an accuracy vs speed trade of an int32_t fastsini32(int32_t x) would make this obvious. But so would also be a note in Doxygen. Maybe a compromise would be an static inline wrapper without the fast that calls the fast one. And if one explicitly wants a fast or a highly accurate implementation, one could use the long name.

But to be honest, I'm not much a fan of bike shedding. I'm usually happy with whatever name the person investing the time and effort in PR picked for naming. IMO time is better spent on the implementation than on the naming of functions or data types.

kfessel · 2023-03-23T13:22:53Z

I'd prefer fast_sini() keeps the sin visible.

benpicco · 2023-06-05T16:57:50Z

Ok, I think it looks more ugly now, but there is now consistency with math.h

kfessel · 2023-06-05T17:12:23Z

bors merge

bors · 2023-06-05T17:12:27Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

19378: sys: add common imath module mv isin() form test/driver_dac_dds r=kfessel a=benpicco 19693: sys/color: extend unittest and fix module r=kfessel a=kfessel ### Contribution description this extends the unittest for sys_color testing more colors ### Testing procedure ``` RIOT_tree/tests/unittests$ make tests-color test ``` will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting)) the new `hsv2rgb` test will succeed ### Issues/PRs references #19614 was the reason i had a look at this #1315 added the rgb2hsv and hsv2rgb function #9940 added the test for black special case https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float) Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: Karl Fessel <[email protected]>

bors · 2023-06-05T17:57:51Z

Build failed (retrying...):

Murdock

19378: sys: add common imath module mv isin() form test/driver_dac_dds r=kfessel a=benpicco Co-authored-by: Benjamin Valentin <[email protected]>

maribu · 2023-06-05T18:28:08Z

bors cancel

bors · 2023-06-05T18:28:09Z

Canceled.

maribu · 2023-06-05T18:28:13Z

bors cancel

maribu · 2023-06-05T18:29:07Z

bors merge

19378: sys: add common imath module mv isin() form test/driver_dac_dds r=maribu a=benpicco 19693: sys/color: extend unittest and fix module r=kfessel a=kfessel ### Contribution description this extends the unittest for sys_color testing more colors ### Testing procedure ``` RIOT_tree/tests/unittests$ make tests-color test ``` will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting)) the new `hsv2rgb` test will succeed ### Issues/PRs references #19614 was the reason i had a look at this #1315 added the rgb2hsv and hsv2rgb function #9940 added the test for black special case https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float) Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: Karl Fessel <[email protected]>

bors · 2023-06-05T20:31:55Z

Build failed (retrying...):

Murdock

bors · 2023-06-06T01:12:29Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

benpicco requested review from smlng, leandrolanzieri, aabadie, MichelRottleuthner and fjmolinas as code owners March 10, 2023 12:31

github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Mar 10, 2023

benpicco force-pushed the imath branch 2 times, most recently from bcd87e1 to 12488c2 Compare March 10, 2023 13:04

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 10, 2023

benpicco requested review from maribu and kfessel March 10, 2023 13:05

kfessel reviewed Mar 10, 2023

View reviewed changes

sys/imath/imath.c Outdated Show resolved Hide resolved

benpicco force-pushed the imath branch 2 times, most recently from 0f6d400 to a51e84c Compare March 13, 2023 20:29

kfessel reviewed Mar 14, 2023

View reviewed changes

sys/include/imath.h Outdated Show resolved Hide resolved

kfessel changed the title ~~tests/driver_dac_dds: move isin() function to common imath module~~ sys: add common imath module mv isin() form test/driver_dac_dds Mar 22, 2023

sys/imath: add integer math module

c23cec9

benpicco force-pushed the imath branch from 38a96c7 to 5d0b173 Compare June 5, 2023 16:57

kfessel approved these changes Jun 5, 2023

View reviewed changes

benpicco added 3 commits June 5, 2023 19:09

tests/driver_dac_dds: make use of imath

69a6b3c

imath: add sqrti() function

eefa1b8

imath: add powi() function

34d0027

benpicco force-pushed the imath branch from 5d0b173 to 34d0027 Compare June 5, 2023 17:09

bors bot added a commit that referenced this pull request Jun 5, 2023

Merge #19378

220c83c

19378: sys: add common imath module mv isin() form test/driver_dac_dds r=kfessel a=benpicco Co-authored-by: Benjamin Valentin <[email protected]>

bors bot merged commit 375eae5 into RIOT-OS:master Jun 6, 2023

benpicco deleted the imath branch June 6, 2023 06:00

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: add common imath module mv isin() form test/driver_dac_dds #19378

sys: add common imath module mv isin() form test/driver_dac_dds #19378

benpicco commented Mar 10, 2023

riot-ci commented Mar 10, 2023 •

edited

Loading

kfessel left a comment •

edited

Loading

benpicco commented Mar 14, 2023

benpicco commented Mar 22, 2023

kfessel commented Mar 22, 2023

maribu commented Mar 23, 2023

kfessel commented Mar 23, 2023

benpicco commented Jun 5, 2023

kfessel commented Jun 5, 2023

bors bot commented Jun 5, 2023

bors bot commented Jun 5, 2023

maribu commented Jun 5, 2023

bors bot commented Jun 5, 2023

maribu commented Jun 5, 2023

maribu commented Jun 5, 2023

bors bot commented Jun 5, 2023

bors bot commented Jun 6, 2023

sys: add common imath module mv isin() form test/driver_dac_dds #19378

sys: add common imath module mv isin() form test/driver_dac_dds #19378

Conversation

benpicco commented Mar 10, 2023

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Mar 10, 2023 • edited Loading

Murdock results

Artifacts

kfessel left a comment • edited Loading

Choose a reason for hiding this comment

benpicco commented Mar 14, 2023

benpicco commented Mar 22, 2023

kfessel commented Mar 22, 2023

maribu commented Mar 23, 2023

kfessel commented Mar 23, 2023

benpicco commented Jun 5, 2023

kfessel commented Jun 5, 2023

bors bot commented Jun 5, 2023

bors bot commented Jun 5, 2023

maribu commented Jun 5, 2023

bors bot commented Jun 5, 2023

maribu commented Jun 5, 2023

maribu commented Jun 5, 2023

bors bot commented Jun 5, 2023

bors bot commented Jun 6, 2023

riot-ci commented Mar 10, 2023 •

edited

Loading

kfessel left a comment •

edited

Loading