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

Publish the pulse_meter total when setting the total #5475

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

TrentHouliston
Copy link
Contributor

@TrentHouliston TrentHouliston commented Oct 3, 2023

What does this implement/fix?

When the pulse total count is manually set the new state should be published.
Since this sensor should be a total increasing sensor when analysing if the total drops it can be seen as a continuation point.

If the initial value you set to isn't published it's impossible to know how big the "total increase" was for the step after that.

This PR changes the set total to also publish the value.

Also added myself to the code owners since I've touched this component enough times now that at the very least I'm probably someone to be notified for future reviews.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/issues#4941

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040

Example entry for config.yaml:

# Reset at midnight
time:
  - platform: homeassistant
    on_time:
      seconds: 0
      minutes: 0
      hours: 0
      then:
        - pulse_meter.set_total_pulses:
            id: meter
            value: 0

sensor:
  - platform: pulse_meter
    pin: GPIO05    
    id: meter
    total:
      name: "Total Pulses"

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

@probot-esphome
Copy link

probot-esphome bot commented Oct 3, 2023

Hey there @cstaahl, @stevebaxter, mind taking a look at this pull request as it has been labeled with an integration (pulse_meter) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@TrentHouliston
Copy link
Contributor Author

By this same logic it may also makes sense to publish a 0 on boot.

However that one I can see potentially causing issues if someone is attempting to restore a saved value on boot as it would publish 0, then their restored value.

Also I'm not sure where the best place to publish a 0 on boot would be...

@ssieb
Copy link
Member

ssieb commented Oct 3, 2023

Maybe it would be worth considering adding support for restore value.

@TrentHouliston
Copy link
Contributor Author

I think for this PR it's complete as is, and there is a method that you can use to restore on boot anyway (an on boot lambda). It might be better to do it that way since then you could restore from other sources than flash.

@ssieb ssieb merged commit d824719 into esphome:dev Oct 19, 2023
@ssieb ssieb added this to the 2023.10.2 milestone Oct 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2023
@TrentHouliston TrentHouliston deleted the publish_on_set branch October 23, 2023 08:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulse Meter Sensor does not set Value
3 participants