sys/phydat: New phydat_fit API by maribu · Pull Request #9702 · RIOT-OS/RIOT

maribu · 2018-08-03T15:03:21Z

The current phydat_fit implementation has two limitations: First, the API is way more complicated to use than needed. Second, it doesn't perform correct rounding. This PR addresses both.

Before this commit code using it looked like this:

long values[] = { 100000, 2000000, 30000000 };
phydat_t dat = { .scale = 42, .unit = UNIT_V };
phydat_fit(&dat, values[0], 0, phydat_fit(&dat, values[1], 1, phydat_fit(&dat, values[2], 2, 0)));

Now it can be used like this:

long values[] = { 100000, 2000000, 30000000 };
phydat_t dat = { .unit = UNIT_V, .scale = 42 };
phydat_fit(&dat, values, 3);

maribu · 2018-08-03T15:11:22Z

BTW: With k dimensions and n changes in scale, the old version required O(k*n) divisions. The new versions requires O(k+n) divisions and O(n) multiplications. Even though this won't matter in practice, it still reads nice :-)

maribu · 2018-08-04T06:19:32Z

OK, I was wrong that there is no obvious way of how to round negative numbers. This PR implements rounding away from zero. E.g. 0.5 ~= 1 and -0.5 ~= -1. This has the advantage that if |x| = |y| is true before rounding, it is true after rounding. But it seems that there are quite a lot of opinions how rounding is correctly performed...

maribu · 2018-08-06T15:53:04Z

I just updated the documentation to explicitly state the rounding strategy, so that people expecting other rounding strategies are at least warned.

Update: I fixed the whitespace error I just introduced and squashed

maribu · 2018-08-06T16:01:57Z

Just for reference the most popular rounding strategies with some examples:

Original Value	Always Down	Always Up	Towards Infinity	Towards Zero	Next Even	Next Odd
0.4	0	0	0	0	0	0
0.6	1	1	1	1	1	1
0.5	0	1	1	0	0	1
1.5	1	2	2	1	2	1
-0.5	-1	0	-1	0	0	-1
-1.5	-2	-1	-2	-1	-2	-1

So the strategies only differ when the result is exactly between two integer values.

maribu · 2018-08-06T17:21:13Z

Sorry, one more change: I change the input type from long to in32_t. I think in this API it is crucial to know the actual range the input type supports. Also it is more consistent with phydat_t, which uses int16_t instead of short for (I believe) the very same reason.

maribu · 2018-08-06T20:12:58Z

Fixed wrong variable name in assert
Squashed commits

Btw: Maybe assertions could be turned on by default in the unit tests?

maribu · 2018-08-15T16:19:59Z

@gebart: ping :-)

maribu · 2018-08-22T08:57:05Z

Ping?

maribu · 2018-09-19T09:58:06Z

@miri64: @gebart seems to be to busy to review this PR. Do you have someone else in mind who could review this? I want to use this API in another PR. Therefore that PR is put on hold until this PR is decided on.

miri64 · 2018-09-19T12:09:14Z

In general the change looks sensible to me. Maybe @haukepetersen can have a look. However, @gebart originally provided this function, so if possible I'd like to have his opinion on this change as well (that's why I originally asked for his review).

jnohlgard

There is an awful lot of division going on here, but I guess the original implementation wasn't very optimized either.
See my comments inline. The behaviour seems broken for -32768 and +32767, as they are supposed to fit inside an int16_t.

jnohlgard · 2018-09-19T13:16:44Z

+ * [@ref PHYDAT_MIN, @ref PHYDAT_MAX], and updates the stored scale factor.
+ * The value is rounded towards infinity, e.g. `0.5` and `0.6` are rounded to
+ * `1`, `0.4` and `-0.4` are rounded to `0`, `-0.5` and `-0.6` are rounded to
+ * `-1`.


Towards infinity (+infinity) means that 0.1 would be rounded to 1, -0.9 would be rounded to 0. This is rounding towards nearest integer (round half away from zero).

I see your point, this name is indeed misleading. Wikipedia refers to it as round half away from zero in addition to "towards infinity". Typing it into some internet search engine also shows that "rounding away from zero" is the more widely used term then "towards infinity".

jnohlgard · 2018-09-19T13:28:31Z

+        { .val = {      0,      0,  12346 }, .unit = UNIT_M,    .scale =  7 },
+        { .val = {   3277,   3277,   3277 }, .unit = UNIT_NONE, .scale =  1 },
+        { .val = {  32766,  32766,  32766 }, .unit = UNIT_NONE, .scale =  0 },
+        { .val = {  -3277,  -3277,  -3277 }, .unit = UNIT_NONE, .scale =  1 },


Wrong behaviour IMO.
-32767 fits inside phydat_t::dat

Also, please add tests for -32768 (expect -32768), -32769 (expect -3277, scale +1), 32767 (expect 32767), 32768 (expect 3277, scale 1)

This was actually a deliberate implementation decision for two reasons:

Both positive and negative numbers can be treated identical if treating -32768 as out of range

Special handling of the scale estimation can be skipped if treating 32767 (and thus also -32767) as out of range.

The special case for scale estimation is when the number is 32767 and has to be rounded up. By treating only [-32766, 32766] as target range no special treatment is required, which makes the code shorter, the ROM smaller and faster. Also, in 99.99542% of random numbers the result is the some, in the remaining 0.00458% of the cases some precision is lost.

But let me think about it. Maybe there is an elegant solution to use the full range.

Good arguments. It makes sense to introduce this limitation to save on code size and simplify the implementation if a rationale is clearly described in the module documentation.

jnohlgard · 2018-09-19T13:34:01Z

I agree the API usage was a bit hard to read before. This seem better in that regard.

jnohlgard

Forgot to set request changes.

maribu · 2018-09-21T19:18:10Z

@gebart: I basically rewrote the phydat_fit function now to use lookup tables. Apart from speeding things up quite a bit by significantly reducing the number of divisions required, this also allows to take rounding into account when detecting the correct scale. Thus, 32767 and -32767 can now be used. Still, by default, -32768 is considered out of range to safe ROM for a separate lookup table just for negative numbers. The probability of precision loss is reduced down to 0.00153% compared to the original implementation this way.

Additionally, by adding -DPHYDAT_FIT_TRADE_PRECISION_FOR_ROM=0 to the compiler flags, the separate lookup table is used for negative numbers to use the full range of phydat_t. This costs 20 bytes for the lookup table and two assignments (I guess about 28 bytes in total depending on the instruction set used).

maribu · 2018-10-02T07:38:02Z

@gebart: Could you please re-review? Thanks :-)

jnohlgard

The change looks good. Tested working on native and frdm-kw41z.

This small diff saves a few hundred bytes of ROM on armv6 by using unsigned division instead of signed.

diff --git a/sys/phydat/phydat.c b/sys/phydat/phydat.c
index 245d8f89d9..56b37d7d59 100644
--- a/sys/phydat/phydat.c
+++ b/sys/phydat/phydat.c
@@ -48,7 +48,7 @@ static const int32_t lookup_table_negative[] = {
 };
 #endif
 
-static const int32_t divisors[] = {
+static const uint32_t divisors[] = {
     100000,
     10000,
     1000,
@@ -61,7 +61,7 @@ static const int32_t divisors[] = {
 void phydat_fit(phydat_t *dat, const int32_t *values, unsigned int dim)
 {
     assert(dim <= (sizeof(dat->val) / sizeof(dat->val[0])));
-    int32_t divisor = 0;
+    uint32_t divisor = 0;
     int32_t max = 0;
     const int32_t *lookup = lookup_table_positive;
 
@@ -104,17 +104,17 @@ void phydat_fit(phydat_t *dat, const int32_t *values, unsigned int dim)
     }
 
     /* Applying scale and add half of the divisor for correct rounding */
-    long divisor_half = divisor >> 1;
+    uint32_t divisor_half = divisor >> 1;
     for (unsigned int i = 0; i < dim; i++) {
         if (values[i] >= 0) {
-            dat->val[i] = (values[i] + divisor_half) / divisor;
+            dat->val[i] = (uint32_t)(values[i] + divisor_half) / divisor;
         }
         else {
             /* For negative integers the C standards seems to lack information
              * on whether to round down or towards zero. So using positive
              * integer division as last resort here.
              */
-            dat->val[i] = -(((-values[i]) + divisor_half) / divisor);
+            dat->val[i] = -((uint32_t)((-values[i]) + divisor_half) / divisor);
         }
     }
 }

jnohlgard · 2018-10-02T08:20:42Z

- * @param[in]       value       value to rescale
- * @param[in]       index       place the value at this position in the phydat_t::val array
- * @param[in]       prescale    start by scaling the value by this exponent
+ * @warning The scale member in @p dat has to be initialized by the caller prior


you can use @pre for documenting preconditions and assumptions regarding parameters.
Optional extra: I think doxygen will generate a correct hyperlink if you write @ref phydat_t::scale instead of just scale

jnohlgard · 2018-10-02T08:23:54Z

+     * be used for both positive and negative values. As result, -32768 will be
+     * considered as out of range and scaled down. So statistically in 0.00153%
+     * of the cases an unneeded scaling is performed, when
+     * PHYDAT_FIT_TRADE_PRECISION_FOR_ROM is true.


Very helpful comment!

jnohlgard · 2018-10-02T08:27:32Z

+    10,
+};
+
+#define LOOKUP_LEN (sizeof(lookup_table_positive)/sizeof(int32_t))


add spaces around the division operator. uncrustify will complain otherwise.

jnohlgard · 2018-10-02T08:27:46Z

-            dat->val[k] /= 10;
+
+    for (unsigned int i = 0; i < LOOKUP_LEN; i++) {
+        if (max > lookup[i]){


space before {

jnohlgard · 2018-10-02T08:29:18Z

+    }
+
+    /* Applying scale and add half of the divisor for correct rounding */
+    long divisor_half = divisor >> 1;


~~int32_t~~ uint32_t

jnohlgard · 2018-10-02T08:46:07Z

+    long divisor_half = divisor >> 1;
+    for (unsigned int i = 0; i < dim; i++) {
+        if (values[i] >= 0) {
+            dat->val[i] = (values[i] + divisor_half) / divisor;


Do you know if this pulls in signed or unsigned division helpers on platforms without hardware division?
The preconditions before ensures that both operands are positive, but they are still signed types, so it is likely that this calls signed division (__divsi3, __aeabi_idiv). Unsigned division is cheaper in terms of both ROM usage and CPU cycles, at least on ARMv6, but I'm guessing it should be similar on smaller arches too e.g. AVR (for reference: #9738)

maribu · 2018-10-02T11:02:00Z

@gebart: Thanks for your review. I was completely unaware that signed division could be more expensive. Thanks for pointing that out :-)

maribu · 2018-10-08T18:39:52Z

The failed Murdock build seems to be unrelated. Let me try if it works after a rebase.

maribu · 2018-10-10T20:47:32Z

@gebart: The rebase solved indeed the Murdock issue :-). Could you check if I addressed your comments adequately?

maribu · 2018-10-24T08:38:34Z

@gebart: Sorry to annoy you again. Could you please check if I addressed your comments? If so, I would squash and rebase against the current master.

jnohlgard

I think all of my comments were addressed.
Please squash

The current phydat_fit implementation the following limitations: - The API is way more complicated to use than needed - It doesn't perform any rounding - It uses `long` in a place where actual width (or better range) of the type is pretty important. This commit addresses these limitations and uses lookup-tables to reduce the number of divisions required. Before this commit code using it looked like this: ``` C long values[] = { 100000, 2000000, 30000000 }; phydat_t dat = { .scale = 42, .unit = UNIT_V }; phydat_fit(&dat, values[0], 0, phydat_fit(&dat, values[1], 1, phydat_fit(&dat, values[2], 2, 0))); ``` Now it can be used like this: ``` C int32_t values[] = { 100000, 2000000, 30000000 }; phydat_t dat = { .unit = UNIT_V, .scale = 42 }; phydat_fit(&dat, values, 3); ```

maribu · 2018-10-25T06:53:12Z

Squashed and rebased :-)

@gebart: Thanks for your review and for pointing out the different ROM size of signed and unsigned divisions.

miri64 · 2018-10-25T07:44:13Z

Since this is an API change I don't want to merge this in hour zero of the feature freeze. Let's wait a bit, until the release branch is open.

miri64 · 2018-10-25T17:16:20Z

Let's wait a bit, until the release branch is open.

It is. Let's go!

miri64 · 2018-10-25T17:17:18Z

Thanks for your contribution and your patience @maribu!

maribu · 2018-10-25T18:12:41Z

No problem. I learned something really useful, so it was well spent time :-)

maribu mentioned this pull request Aug 3, 2018

drivers: Add driver for the LTC4150 coulomb counter #9653

Closed

miri64 requested a review from jnohlgard August 3, 2018 16:41

miri64 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Aug 3, 2018

maribu force-pushed the phydat_fit branch from d414ad7 to ece6dea Compare August 6, 2018 15:51

maribu force-pushed the phydat_fit branch 3 times, most recently from 33217d0 to c3a8ed2 Compare August 6, 2018 17:17

maribu force-pushed the phydat_fit branch from c3a8ed2 to d7f2b12 Compare August 6, 2018 20:11

maribu force-pushed the phydat_fit branch 2 times, most recently from 96205db to 7bd6278 Compare August 6, 2018 21:26

jnohlgard reviewed Sep 19, 2018

View reviewed changes

jnohlgard requested changes Sep 19, 2018

View reviewed changes

jnohlgard requested changes Oct 2, 2018

View reviewed changes

jnohlgard reviewed Oct 2, 2018

View reviewed changes

Comment thread tests/unittests/tests-phydat/tests-phydat.c

jnohlgard added this to the Release 2018.10 milestone Oct 2, 2018

jnohlgard self-assigned this Oct 2, 2018

maribu force-pushed the phydat_fit branch from 3beb129 to 2879a48 Compare October 8, 2018 18:40

miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: needs squashing and removed CI: needs squashing labels Oct 10, 2018

jnohlgard approved these changes Oct 24, 2018

View reviewed changes

maribu force-pushed the phydat_fit branch from 2879a48 to fe46cae Compare October 24, 2018 11:06

jia200x modified the milestones: Release 2018.10, Release 2019.01 Oct 25, 2018

miri64 merged commit 959e449 into RIOT-OS:master Oct 25, 2018

maribu deleted the phydat_fit branch October 25, 2018 18:11

maribu mentioned this pull request Aug 22, 2019

Driver for INA3221 current and power and bus voltage monitor #12055

Merged

Uh oh!

Conversation

maribu commented Aug 3, 2018

Uh oh!

maribu commented Aug 3, 2018

Uh oh!

maribu commented Aug 4, 2018

Uh oh!

maribu commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Aug 6, 2018

Uh oh!

maribu commented Aug 6, 2018

Uh oh!

maribu commented Aug 15, 2018

Uh oh!

maribu commented Aug 22, 2018

Uh oh!

maribu commented Sep 19, 2018

Uh oh!

miri64 commented Sep 19, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Sep 19, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Sep 21, 2018

Uh oh!

maribu commented Oct 2, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Oct 2, 2018

Uh oh!

maribu commented Oct 8, 2018

Uh oh!

maribu commented Oct 10, 2018

Uh oh!

maribu commented Oct 24, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

maribu commented Aug 6, 2018 •

edited

Loading

maribu commented Aug 6, 2018 •

edited

Loading

jnohlgard Oct 2, 2018 •

edited

Loading