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

fix derived rate, fixes #20097 #21620

Merged
merged 8 commits into from Mar 3, 2019
@@ -350,7 +350,8 @@ def state(self):
else:
# Recalculate the rate
diff = current_reading - self._previous_reading
self._state = diff
timediff = timestamp = self._previous_timestamp
This conversation was marked as resolved by wburgers

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 3, 2019

TabError: inconsistent use of tabs and spaces in indentation
indentation contains mixed spaces and tabs
indentation contains tabs
unexpected indentation

self._state = diff / timediff * 3600
This conversation was marked as resolved by wburgers

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 3, 2019

indentation contains mixed spaces and tabs


self._previous_reading = current_reading
self._previous_timestamp = timestamp
@@ -121,7 +121,7 @@ def test_derivative():
}
yield from entity.async_update()

assert entity.state == 1, \
assert entity.state == 3600, \
This conversation was marked as resolved by amelchio

This comment has been minimized.

Copy link
@amelchio

amelchio Mar 3, 2019

Contributor

Looking closer, this does not seem to test the time aspect at all?

This comment has been minimized.

Copy link
@wburgers

wburgers Mar 3, 2019

Author Contributor

Glad you are being critical, I think quality is very important.
I think it does test the time aspect, because the first telegram is imported with timestamp '1' (and value 1) and the second telegram with timestamp '2' (and value 2).
That is if I'm reading the MBusObjects correctly.
Please correct me if I'm wrong.

This comment has been minimized.

Copy link
@amelchio

amelchio Mar 3, 2019

Contributor

Ah, my bad ... I did not realize that 'value': 1 is in fact a timestamp (in seconds).

So, just to maybe understand this: is the issue that the data is in fact updated more frequently than the previous assumption of once per hour?

This comment has been minimized.

Copy link
@wburgers

wburgers Mar 3, 2019

Author Contributor

Yes, according to the dsmr standard, the data is sent from the meter every 10 seconds for all versions < 5.
For version 5 (latest) the meter readings are updated every second.

This comment has been minimized.

Copy link
@amelchio

amelchio Mar 3, 2019

Contributor

I see. Would you mind updating the test with realistic values so the next person to look at it does not get fooled like I did?

This comment has been minimized.

Copy link
@wburgers

wburgers Mar 3, 2019

Author Contributor

Sure!
Will do so later tonight, as I am not home right now.

This comment has been minimized.

Copy link
@wburgers

wburgers Mar 3, 2019

Author Contributor

I updated the test with values from my own smart meter.
My central heating was just in a state of low gas usage, so I had to wait a few seconds 😛.
In 330 seconds it used 0.003 m3 of gas, as you can see from the timestamps and m3 values.
The derived value came to the exact m3/h rate that I had calculated by calculator before running the test (0,03272727272727272727272727272727) apart from some rounding or maybe some float incorrectness.
I think it is close enough though.

This comment has been minimized.

Copy link
@amelchio

amelchio Mar 3, 2019

Contributor

Awesome! I updated the test to ignore this issue, hope you don't mind.

This comment has been minimized.

Copy link
@wburgers

wburgers Mar 3, 2019

Author Contributor

Awesome!

'state should be difference between first and second update'

assert entity.unit_of_measurement == 'm3/h'
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.