Added minimum/maximum to counter

[spacemanspiff2007]

Description:

Example entry for configuration.yaml (if applicable):

counter:
  my_counter:
    initial: 0
    step : 1
    minimum: 0
    maximum: 10

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

[houndci-bot]

line too long (80 > 79 characters)

[frenck]

Could not find a related documentation PR. Added docs-missing label.

[spacemanspiff2007]

Doc modification would be trivial. Where do I have to add it?

[tjorim]

In the documentation repo, here's a direct link to the page in question: https://github.com/home-assistant/home-assistant.io/blob/next/source/_components/counter.markdown

[spacemanspiff2007]

Thanks a lot tjorim! Sorry - I am quite new to this.

[fabaff]

At first glance it looks like that this PR wants to integrate functionality which the min_max sensor is providing and can be done easily with automation.

What's the use case for this?

Edit: It's the threshold sensor and not the min_max sensor.

[spacemanspiff2007]

No quite. This will make the counter never go over maximum or below minimum (if specified).
Useful for limiting the counter to the appropriate boundaries. In my instance I count up and down, but never want the counter to go below zero even though I have more "decrease" events than "increase" events.

[fabaff]

The counter should count and not interpret values. Your issue can be solved with two conditions (or maybe even with one) in an automation rule.

[spacemanspiff2007]

Yes, but then I have to check from every instance I send the increase/decrease commands instead of doing it in central place. Also with initial value, restore and step the counter already interprets values.
Also, if I already know during creation of the counter that it never should go below/over a certain value why not specify it during creation.

[fabaff]

Yes, but then I have to check from every instance I send the increase/decrease commands instead of doing it in central place.

You have to check it in every automation rule which is interacting with the counter, that's correct. The proposed solution would do it the other way around. Instead of using a condition, one would need to use multiple counters because they check if the value hits the threshold.

Also with initial value, restore and step the counter already interprets values.

Those are the information to setup the counter.

Also, if I already know during creation of the counter that it never should go below/over a certain value why not specify it during creation.

Because if you know it then the condition is the right place. Automation rules can be changed during run time and their reloading is faster than reloading the core.

[spacemanspiff2007]

You have to check it in every automation rule which is interacting with the counter, that's correct. The proposed solution would do it the other way around. Instead of using a condition, one would need to use multiple counters because they check if the value hits the threshold.

I'm afraid I don't understand why I have to use multiple counters? Can you please elaborate?

Those are the information to setup the counter.

Also, if I already know during creation of the counter that it never should go below/over a certain value why not specify it during creation.

Because if you know it then the condition is the right place. Automation rules can be changed during run time and their reloading is faster than reloading the core.

Yes, the information is used to setup a counter, just like its min/max value would be.
If I follow your argument the offset could be added in the automation rule and the step could just be a multiplication factor in the automation rule. So these features should not exist eather.

In my opinion it boils down to this:
I want to have a counter and per definition this counter should not go below a minimum / over a maximum value.
This can be achieved through:

1. check in every automation rule I create for the counter state and only if it is below the threshold send an increase command
2. make the counter itself never go over the boundaries

Pros/Cons:

Solution 1

• Pro:
  • Can be reloaded faster
• Con:
  • Adds lots of boiler plate
  • If the boundaries should ever change there are lots of places it needs to be changed

Solution 2

• Pro:
  • logical: minimum/maximum is a counter property like step and initial value
  • Central place where the boundaries are defined
  • No boilerplate in automation rules
• Con:
  • Restart is required for new boundaries

Solution 2 is for me the way to go because every time I modify some config files I have to restart hass anyway. Also if one would like to change the boundaries dynamically one can still do this in automation.
So this con can be dismissed. But being able to prevent all this boiler plate really is a huge benefit. Also counter properties like step, initial value and min/max are very static and are almost never changed.
So this is the way to go.

[fabaff]

I'm afraid I don't understand why I have to use multiple counters? Can you please elaborate?

Because the counters would have different boundaries for different use cases. The use case for you is that you only want to have one counter "to rule them all".

If I follow your argument the offset could be added in the automation rule and the step could just be a multiplication factor in the automation rule. So these features should not exist eather.

The counter is an automation helper like the Threshold Binary Sensor or the Min/max Sensor. But this doesn't mean that those helpers or building blocks should cover every possible use case. Automation rules are well tested and the default way to handle state changes. Everything a helper can do, can be done in plain automation too.

The comparison is more like this:

• Solution 1
• Pro:
  • Can be reloaded faster
  • Is well tested
  • Is the standard and preferred way to work in Home Assistant
  • Can be edited through the frontend
• Con:
  • Adds lots of boiler plate (-> Automations rules are always boiler plates)
  • If the boundaries should ever change there are lots of places it needs to be changed (-> not if you use a Threshold sensor)

Solution 2

• Pro:
  • logical: minimum/maximum is a counter property like step and initial value (-> This really depends on the definition of a counter and somehow you have start the counter)
  • Central place where the boundaries are defined (-> For one is Input Number a "central" place too)
  • No boilerplate in automation rules (-> Automation rules is still needed, only condition can be avoided)
• Con:
  • Restart is required for new boundaries
  • Requires a lot of new unit tests for the counter and some more check in the code
  • Is code we need to maintain forever
  • Can't be modified through the frontend (at least not for now)

Solution 2 is for me the way to go because every time I modify some config files I have to restart hass anyway.

That may work for you but a lot of people don't want to restart or can't restart for every single configuration change.

Also if one would like to change the boundaries dynamically one can still do this in automation.

Not possible, the counter is initialized with the boundaries and those can't be changed during run time.

But being able to prevent all this boiler plate really is a huge benefit.

In your case maybe but as we have the automation editor. There is not longer much need to edit the rules manually. If you do so then you are probably one that never going to use counter in the first place.

Also counter properties like step, initial value and min/max are very static and are almost never changed.

Again, may be true for your use case. In general only step is fixed and the initial value for the counter is restored.

[dgomes]

Can I propose that the boundaries can be changed through a service call ?

[spacemanspiff2007]

Sure - both boundaries through one call ( set_boundaries ) or two separate calls ( set_minimum and set_maximum)?

[spacemanspiff2007]

@fabaff : I think there is still some misunderstanding going on. I'll try again:
I have seven sources which do a counter.decrease, but only five who do a counter.increase. The counter can not go below 0, because then the counting would be wrong. Since there is more decrease than increase events there has to be some check. The check has to be done in every possible source for the counter.decrease or centrally in the counter itself. The amount of counters for both solutions is still the same. The amount of code to achieve the same behavior is seven times more if it's not done in the counter.

Thanks to @dgomes idea, I've implemented a service call which can change the counter properties without having to restart. I've also create the unit-tests and the documentation. Imho this pretty much clears the cons-list in this PR.

[balloob]

I think that the way the current boundaries are implemented, as configuration options on a per counter basis is good.

This PR needs rebasing and I think that it's ok to merge.

[rohankapoorcom]

@spacemanspiff2007 I wanted to check in and see if you were going to have time to rebase this so it could be merged? I am willing to help with finishing it up if you don't have time.

[spacemanspiff2007]

Oh, sorry - I did not realize I had something todo.
How do I do a rebase?

[rohankapoorcom]

To rebase, you'll need to pull down the latest changes from the dev branch and then replay your changes on top of them. You can start this process by running the following:

git fetch upstream
git rebase upstream/dev

This is going to take each of your commits one at a time and put them on top of the current dev branch. If there are conflicts (which there are some), it will pause and have you resolve them before continuing.

Here's a good tutorial on rebasing: https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase

[spacemanspiff2007]

I hope I've done it correct :-)

[rohankapoorcom]

@spacemanspiff2007 - looks like something nasty happened with the rebase and merge. There was a bunch of code from dev that appeared on your branch. I tried to clean it up but it did not work in this PR. I have a clean branch with your code on it, but don't have the permissions to push it up to your fork. A fixed branch is available here: https://github.com/home-assistant/home-assistant/compare/dev...rohankapoorcom:counter-min-max?expand=1 You are welcome to push that branch over to your fork and make a new pull request, or I can open a new one on your behalf.

[spacemanspiff2007]

I have created a fresh repo and tried again:
#20319