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

ADD command uses wrong rounding (1.00 + 0.005 = 1.00) #4420

Closed
4 of 6 tasks
th-s-op opened this issue Nov 23, 2018 · 12 comments
Closed
4 of 6 tasks

ADD command uses wrong rounding (1.00 + 0.005 = 1.00) #4420

th-s-op opened this issue Nov 23, 2018 · 12 comments
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended

Comments

@th-s-op
Copy link

th-s-op commented Nov 23, 2018

Bug (minor): the numeric commands have a two digit precision after the decimal point (.00 to .99). At least the ADD command uses wrong rounding-up. It does NOT round up starting with "5" as the next digit, but with "6".

1.00 + 0.004 = 1.00 (correct)
1.00 + 0.005 = 1.00 (wrong)
1.00 + 0.006 = 1.01 (correct)

Also, make sure these boxes are checked [x] before submitting your issue - Thank you!

  • Searched the problem in issues and in the wiki
  • Hardware used : Sonoff S20
  • Development/Compiler/Upload tools used :atom:
  • If a pre-compiled release or development binary was used, which one? :
  • You have tried latest release or development binaries? : 6.3.0.5
  • Provide the output of commandstatus 0 :
21:35:32 RSL: STATUS = {"Status":{"Module":8,"FriendlyName":["Sonoff"],"Topic":"sonoff","ButtonTopic":"0","Power":0,"PowerOnState":3,"LedState":1,"SaveData":1,"SaveState":1,"SwitchTopic":"0","SwitchMode":[0,0,0,0,0,0,0,0],"ButtonRetain":0,"SwitchRetain":0,"SensorRetain":0,"PowerRetain":0}}
21:35:32 RSL: STATUS1 = {"StatusPRM":{"Baudrate":115200,"GroupTopic":"sonoffs","OtaUrl":"http://thehackbox.org/tasmota/release/sonoff.bin","RestartReason":"Power on","Uptime":"5T23:21:41","StartupUTC":"2018-11-17T21:13:51","Sleep":1,"BootCount":3,"SaveCount":234,"SaveAddress":"FA000"}}
21:35:32 RSL: STATUS2 = {"StatusFWR":{"Version":"6.3.0.5(basic)","BuildDateTime":"2018-11-09T00:42:37","Boot":6,"Core":"2_3_0","SDK":"1.5.3(aec24ac9)"}}
21:35:32 RSL: STATUS3 = {"StatusLOG":{"SerialLog":0,"WebLog":2,"SysLog":0,"LogHost":"","LogPort":514,"SSId":["XXX","XXX"],"TelePeriod":0,"SetOption":["00008001","55A18000","00000001"]}}
21:35:32 RSL: STATUS4 = {"StatusMEM":{"ProgramSize":420,"Free":580,"Heap":21,"ProgramFlashSize":1024,"FlashSize":1024,"FlashChipId":"14405E","FlashMode":3,"Features":["00000809","0F082780","24000000","00000006","000000C0"]}}
21:35:32 RSL: STATUS5 = {"StatusNET":{"Hostname":"tasmota-couchlicht","IPAddress":"XXX","Gateway":"XXX","Subnetmask":"255.255.255.0","DNSServer":"XXX","Mac":"EC:FA:BC:12:86:7B","Webserver":2,"WifiConfig":4}}
21:35:32 RSL: STATUS7 = {"StatusTIM":{"UTC":"Fri Nov 23 20:35:32 2018","Local":"Fri Nov 23 21:35:32 2018","StartDST":"Sun Mar 25 02:00:00 2018","EndDST":"Sun Oct 28 03:00:00 2018","Timezone":99,"Sunrise":"07:39","Sunset":"16:23"}}
21:35:32 RSL: STATUS10 = {"StatusSNS":{"Time":"2018-11-23T21:35:32"}}
21:35:32 RSL: STATUS11 = {"StatusSTS":{"Time":"2018-11-23T21:35:32","Uptime":"5T23:21:41","Vcc":3.182,"POWER":"OFF","Wifi":{"AP":1,"SSId":"XXX","BSSId":"00:E0:4B:9B:F8:94","Channel":1,"RSSI":100}}}

To Reproduce
commands:

21:33:38 CMD: var1 1
21:33:38 RSL: RESULT = {"Var1":"1"}
21:33:47 CMD: add1 0.005
21:33:47 RSL: RESULT = {"Add1":"1.00"}

21:41:42 CMD: var1 1
21:41:42 RSL: RESULT = {"Var1":"1"}
21:41:45 CMD: add1 0.005
21:41:45 RSL: RESULT = {"Add1":"1.00"}

I would have expected the Result 1.01 also when adding 0.005 to 1.00.

@ascillato
Copy link
Contributor

Sorry, Tasmota uses 2 decimals only

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

Yes, that's right. But why does the add command round up correct any .006 up to .009 and round down correcty any .004 to .001 but just the .005 is wrong - rounding down instead of up?

@ascillato
Copy link
Contributor

ascillato commented Nov 23, 2018

Sorry, the rounding is like that because it uses just 2 decimals, so it rounds to the closest value. It is correct like that.

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

No, as "rounding to the closest value" everywhere in the world ist defined as "round up if the next digit is >=5.

If you have a precision of two decimals only:
1.00 + 0.001 = 1.00
1.00 + 0.002 = 1.00
1.00 + 0.003 = 1.00
1.00 + 0.004 = 1.00
1.00 + 0.005 = 1.01 <- but Tasmota results in 1.00
1.00 + 0.006 = 1.01
1.00 + 0.007 = 1.01
1.00 + 0.008 = 1.01
1.00 + 0.009 = 1.01

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

Having looked at the code, I am quite sure that it's not a Tasmota bug but a really nasty bug in the dtostrf() function in the standard library.

@ascillato2
Copy link
Collaborator

When you enter anything in the console is stored as text, then when Tasmota see that is a command for numbers like ADD, it converts the text to number in the line:

https://github.com/arendst/Sonoff-Tasmota/blob/0daf26586d0a25cf2866aa9d2c590580c1565425/sonoff/xdrv_10_rules.ino#L598

So, the conversion and rounding is done by CHARTODOUBLE instruction.

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

More downwards, you call dtostrfd which itself calls dtostrf() and that one definitely has a rounding bug.

@ascillato
Copy link
Contributor

In which line you see that?

The code directly related to ADD command makes the conversion with chartodouble.

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

The CharToDouble function only converts the two values to the double data types. As you said, the variables are stored as text, the parameter to be added is coming as text, so CharToDouble converts both to double and does NOT reflect any precision restriction at that time. (see support.ino)

in xdrv_10_rules.ino, you see

else if ((CMND_ADD == command_code) && (index > 0) && (index <= MAX_RULE_VARS)) {
    if (XdrvMailbox.data_len > 0) {
      double tempvar = CharToDouble(vars[index -1]) + CharToDouble(XdrvMailbox.data);
      dtostrfd(tempvar, 2, vars[index -1]);
    }

So after the two converted double values are added and stored to the tempvar of still double type, the line
dtostrfd(tempvar, 2, vars[index -1]);
converts the double value to its string representation with precision of 2 digits.

in support.ino, dtostrfd is defined (around line 197 in my version) and it's just a secure wrapper around using dtostrf() from the standard library. That one really does the conversion of the double type value to the string, with precision of 2 digits (which you use as a parameter). That functions does rounding. But it does it wrong, see example. Possibly fixed in a newer ESP library, I can't check.

-> not a Tasmota bug, but a bug in dtostrf()

@th-s-op
Copy link
Author

th-s-op commented Nov 23, 2018

Fun fact: I recompiled Tasmota, replacing the target precision of "2" with "4" for the ADD, SUB, ... SCALE functions, and now rounding works correctly:

22:53:22 CMD: backlog var1 0; add1 0.00001
22:53:22 RSL: RESULT = {"Var1":"0"}
22:53:23 RSL: RESULT = {"Add1":"0.0000"}
22:53:32 CMD: backlog var1 0; add1 0.00002
22:53:32 RSL: RESULT = {"Var1":"0"}
22:53:32 RSL: RESULT = {"Add1":"0.0000"}
22:53:36 CMD: backlog var1 0; add1 0.00003
22:53:36 RSL: RESULT = {"Var1":"0"}
22:53:37 RSL: RESULT = {"Add1":"0.0000"}
22:53:42 CMD: backlog var1 0; add1 0.00004
22:53:43 RSL: RESULT = {"Var1":"0"}
22:53:43 RSL: RESULT = {"Add1":"0.0000"}
22:53:48 CMD: backlog var1 0; add1 0.00005
22:53:48 RSL: RESULT = {"Var1":"0"}
22:53:48 RSL: RESULT = {"Add1":"0.0001"}

Then, I changed the code back to use 2 digits precision, and - WONDER WHY - the bug is gone.

22:59:00 CMD: backlog var1 0; add1 0.001
22:59:00 RSL: RESULT = {"Var1":"0"}
22:59:00 RSL: RESULT = {"Add1":"0.00"}
22:59:04 CMD: backlog var1 0; add1 0.002
22:59:04 RSL: RESULT = {"Var1":"0"}
22:59:05 RSL: RESULT = {"Add1":"0.00"}
22:59:09 CMD: backlog var1 0; add1 0.003
22:59:10 RSL: RESULT = {"Var1":"0"}
22:59:10 RSL: RESULT = {"Add1":"0.00"}
22:59:14 CMD: backlog var1 0; add1 0.004
22:59:15 RSL: RESULT = {"Var1":"0"}
22:59:15 RSL: RESULT = {"Add1":"0.00"}
22:59:19 CMD: backlog var1 0; add1 0.005
22:59:19 RSL: RESULT = {"Var1":"0"}
22:59:20 RSL: RESULT = {"Add1":"0.01"}

The only difference is that I used Tasmota version 6.3.0.5 before and now it's 6.3.0.8, and my Atom did some updates.

For me, it's solved, but I would recommend to keep an eye on the dtostrf() function. Net net is full of statements that it's buggy.

Last question: was there any reason why the precision of the mathematic commands' result was set hard to "2"? It's a pity as it

  • brilliantly works with a higher precision (I tested 4)
  • some sensors like in the POW have default values with a precision of 3 and it's bad to lose a lot of precision when calculating with those
  • couldn't there be a command to set the calculation precision as that's just a static number used in 4 places in the xdrv_10_rules.ino code and it would be in perfect company to the AmpRes, EnergyRes etc. commands?

@ascillato
Copy link
Contributor

I like your idea of a configurable precision 👍

@ascillato2 ascillato2 added enhancement Type - Enhancement that will be worked on work in progress Action - Work in progress labels Nov 24, 2018
@ascillato
Copy link
Contributor

Proprosed PR #4466 for the addition of command CalcRes to set the number of decimals (0 - 7) to be used in commands ADD, SUB, MULT and SCALE

@ascillato2 ascillato2 added fixed Result - The work on the issue has ended and removed work in progress Action - Work in progress labels Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended
Projects
None yet
Development

No branches or pull requests

3 participants