DC current value jumps to overflow value

Dear Tillaart,

I suspect there is an incorrect calculation.

in 0.3.1 the type of _midPoint has been changed from int to uint16_t:

ACS712/ACS712.h

Line 114 in e9d55d0

uint16_t _midPoint;

This in my opinion introduces a problem in ACS712::mA_DC() function here:

ACS712/ACS712.cpp

Line 214 in e9d55d0

sum += (value - _midPoint);

were _midPoint is used without an explicit cast. I'm a bit rusty on C++ after 20 years, but the reason might be that value - _midPoint can be negative but the compiler guesses to cast it to an unsigned. Then when it's added to sum that is float the damage is done and the converted value jumps to overflow value.

Other uses of _midpoint are to check.

The change has been introduced in 0.3.1. Version 0.3.0 does not present the issue.

The issue can be resolved either with an explicit cast or by changing the variable back to an int.

Let me know if I have to do a PR and in that case which solution you prefer.

Thanks @diniciacci for reporting this issue.
I will look into it later today.

I recall I changed _midPoint to prepare it for 16 bit ADC's so the int would not be sufficient.

Hey Tillaart,

thanks for your fast answer and triage. Just adds to your really great work.

I checked out the good old dusty book and it looks like that C++ casts signed to unsigned when adding the two.

In a mathematical operation in C++ (e.g. arithmetic or comparison), if one signed and one unsigned integer are used, the signed integer will be converted to unsigned. And because unsigned integers can not store negative numbers, this can result in loss of data. reference.

Naturally, IMHO, the suggestion to avoid them in the reference is out of context in this use case as you correctly changed the type already. My two cents is that an explicit cast gives readable code, in the end it's the compiler guessing the wrong type for this operation i.e. they both should be converted to float directly, even if that makes the operation costly.

A different solution from my numerical math background suggests me to subtract the _midpoint only after the calculation i.e. _midPoint * cycles. That exchanges several subtractions with one multiplication, might be advantageous compared to doing many casting or in case of a lot of cycles. In practice all the operations need to be reordered, unfortunately less readable.

I leave other comments (e.g. performance of the two solutions might be different, reverse the order of operations, remove midpoint only at the end of reading cycles). Hope my ramblings spark some of your plugs, but anyway take this as my humble way to try support open source ;)

Thanks again for your great work.

@diniciacci

I created a develop branch in which I reverted the type to int and looked at all the places it is referenced,
If you have time could you do some test runs if you have a hardware setup nearby? (cause I have not)

The proposal to optimize the math is interesting, although most time is spend in the analogRead() function.
On an UNO this takes 100++ us where a float multiply, addition or subtraction is ~12 us, float division is ~36 us.
I expect that the footprint and precision will not change much, .

In my todo list for the future is using a float midPoint so it could be set to e.g. 512.5 however that suggests a precision that is way better than the (default) noise levels. There might be a performance penalty, still what are your thoughts about that?

mmm, build failed, need to investigate what happened...

update: too eager to fix more than needed. Need more coffee :)

update: unit tests are OK again.

EHhehe I know the feeling ;)

Electronics team was off today and I was busy with the other team. I'll try to have a test on Monday when back on front of the hardware.

If the math operations are reordered I'm sure floats will still be faster than current implementation. If the operation is executed only once, then, as per the timings you suggest, it will not make much a difference.

I agree that giving a bad (float) suggestion on trimming might be "meh", but normally corrections are more accurate than the measurement to not introduce more uncertainty. I.e. measure +-1 + correction +-1 = value +-2, so well.. uncertainty on corrections is more "meh", than a bad suggestion. Moreover, averaging on many many results will actually make the user of float meaningful. Add to this that the .5 of midpoint is multiplied by some number grater than 10, so it will actually influence the next digit. float midpoint is IMHO tolerable.

I'll also have a deeper look on this last performance topic when we test the sensor again in the lab. Thanks for your work meanwhile.

I confirm the develop branch fixes the read for DC current, now It's bad as it used to be with this sensor ;)

Thanks for your hard work and fix. Appreciated.

Thanks for testing, I did a compare with the previous 0.3.0 version and found a bug in the incrMidpoint() that is also fixed.

Thanks for your hard work and fix. Appreciated.

You're welcome, spend quite some time on all my libraries and although usability varies the quality of most of them is coming to an acceptable level.

Will merge a.s.a.p.

@diniciacci
If there are other issues, let me know!

Nice, well done! I checked out the commit and looks good. And well :_D I totally forgot to report you the missing /2, my engineer had spotted it last week, but when we found the other thing it was overshadowed (not only in value ;) ). So, good catch to you!