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 an Integration sensor #19703

Merged
merged 20 commits into from Jan 29, 2019

Conversation

@dgomes
Copy link
Member

dgomes commented Jan 1, 2019

Description:

Sometimes a device only provides power information (measured in W or kW).
This sensor can be used to do an integration of Power and time in order to measure energy consumed in kWh (standard energy measurement unit)
But since it is generic, it can also be used to count liters from the information coming from a flow meter in L/s.

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8012

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: integration
    name: Energy Consumed
    source: sensor.measured_power

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

dgomes added some commits Dec 29, 2018

dgomes added some commits Jan 1, 2019

@dgomes dgomes referenced this pull request Jan 2, 2019

Merged

Adds Integration Sensor #8012

2 of 2 tasks complete
@andriej

This comment has been minimized.

Copy link
Contributor

andriej commented Jan 2, 2019

@dgomes any chance for this also as a water sensor? I have it with liter-counter without being able to get sum value. It would just do the job with liters instead of W/kWh :-)

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 2, 2019

@andriej liter is a volume though, what you suggest would make sense for a flowrate sensor. Or maybe I misunderstand?

@andriej

This comment has been minimized.

Copy link
Contributor

andriej commented Jan 2, 2019

@emontnemery I have flowmeter installed and based on it's reports I'd love to see this day/week/month water usage baded on sum - just like in kWh sensor :) I know it's not related to this PR, just an idea for improvement

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 2, 2019

Aha, I see. Maybe #19718 does what you want?

@andriej

This comment has been minimized.

Copy link
Contributor

andriej commented Jan 2, 2019

@emontnemery could be, yeah. Can't wait to test it

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 2, 2019

@andriej as @emontnemery pointed out, you want #19718 :)

These two PR are related, but separate. You will need to have energy (this PR) in order to use the Utility Meter (#19718). Since many devices already provide energy readings I've split the two and made the utility meter more general (supporting water, gas, heating, etc)

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Jan 3, 2019

We should create a generic sensor, measure_calc or so they have preset for volume or kw/h. I don't like to see too many different platforms they do the same at the end.

So you can use template sensor or this native adaption.

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 3, 2019

@pvizeli thanks for your feedback, I share your concerns for generalization.

Unfortunately I could not think of any other use case for integration of data as a function of time besides energy (kWh).

You mention volume, but volume is not a function of time, If someone explains me the use case I will gladly adapt this sensor into something more generic.

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 3, 2019

Volume could be a function of time if integrating a flow sensor (volume/time) as discussed yesterday. @Andiej, does your sensor report flow or volume, you mention flow but also that it measures litres..

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 3, 2019

@emontnemery ok I see the case for a flow meter providing L/h.

Are you aware of any flow meter sensor that provides L/h and not L? And I'm not referring to DIY stuff, because in that case both energy and volume would be better handled in the Arduino code.

I could rename the sensor "Integrator Sensor" and remove the energy specific stuff, but would like to avoid that if no practical use case exists as the name would be less obvious to users.

@dgomes dgomes added Ready for review and removed in progress labels Jan 3, 2019

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 4, 2019

Are you aware of any flow meter sensor that provides L/h and not L

No, I'm not.

Show resolved Hide resolved homeassistant/components/sensor/energy.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/energy.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/energy.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/energy.py Outdated
@Harriethijssen

This comment has been minimized.

Copy link

Harriethijssen commented Jan 7, 2019

Are you aware of any flow meter sensor that provides L/h and not L

Is this a sensor that you were asking for?
paddle wheel sensor

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 7, 2019

@Harriethijssen thank you for your reply!

I think these need to have a micro controller to process the PWM signal and therefore fall into the DIY bin.

@OttoWinter

This comment has been minimized.

Copy link
Member

OttoWinter commented Jan 9, 2019

@dgomes Context: I recently added a similar feature to my project: https://esphomelib.com/esphomeyaml/components/sensor/total_daily_energy.html - In that case the sensor itself doesn't check the unit of measurement - it just appends an h at the end. So if users want to use it in a different way they can just change the unit of measurement. Maybe that's also possible here? Then use cases like the one from @Harriethijssen woulf be easier too.

Also from the feedback there (it applies here too) a bunch of users apparently want to use it for energy cost measurement. So to have an option to auto-reset it every day/month/hour etc. Having a value increase indefinitely is not very useful IMO. Maybe document how to reset this value to 0 from an automation?

(another thing that was often requested was having different scales for different times of das as that's how energy is often billed; but that's probably out of scope here)

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 9, 2019

@OttoWinter
I guess I'll just rename the sensor to "integration sensor". I'm seeing support for this, so I think is the best way. Making it generic, I'll be removing the code that detects the source unit_of_measurement, and will provide instead a "unit_prefix" optional argument (similar to your multiply)

What relates to your 2nd and 3rd paragraphs, that is the other PR currently open: #19718
I decided to separate both functionalities because you find sensors/devices that already provide you with energy readings (no need for this PR functionality). That PR provides exactly what you mention and can be used together with this one (that's how I'm using it, as well as many folks in the Portuguese HA community)

I'll ask for your review after I refactor this PR 😉

@dgomes dgomes changed the title Energy sensor Adds an Integration sensor Jan 9, 2019

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 9, 2019

I've renamed the sensor and refactored the code to make it more generic 👍

@MartinHjelmare MartinHjelmare changed the title Adds an Integration sensor Add an Integration sensor Jan 13, 2019

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 13, 2019

@MartinHjelmare thank you for comments and review 😄

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 14, 2019

@pvizeli are you ok with this?

@OttoWinter
Copy link
Member

OttoWinter left a comment

👍 Thanks!

@dgomes dgomes requested a review from pvizeli Jan 16, 2019

@dgomes

This comment has been minimized.

Copy link
Member Author

dgomes commented Jan 22, 2019

@pvizeli sorry to ping, but can we move this forward ?

@pvizeli pvizeli merged commit b0ff51b into home-assistant:dev Jan 29, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 93.03%
Details

@wafflebot wafflebot bot removed the Ready for review label Jan 29, 2019

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment