search

jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
327 stars 34 forks source link

Fix #539 - allow up to 9 entities in over_climate, over_switch and over_valve config forms #551

Closed pounard closed 3 weeks ago

pounard commented 1 month ago

Following your advice in https://github.com/jmcollin78/versatile_thermostat/issues/539#issuecomment-2408473996, I tried!

OK so you have to know that Python is not the language I work with in a daily basis...

Some notes:

But, some caveat, it seems to alter some calculations:

Please note I did full TDD here, I didn't test it in real conditions, but I guess it should work.

I'm not accustomed enough with your test suite and scripts, and I have no idea how to run some test hassio instance with the dev code within. Any help would be greatly appreciated.

jmcollin78 commented 1 month ago

Heyy, thank you for this. I will have a look.

I have some major strategic questions before merging:

  1. I wonder when this will stop ... if someone wants 6 heaters or 7 heaters. My question what should be the limit for one VTherm. I though was enough. The other users have split into 2 heating zone, which one VTherm for each zone having less than 4 switch. Is that not preferable in fact ?
  2. the other question is why only for VTherm over_switch ? Why not for over_valve and over_climate ?
jmcollin78 commented 1 month ago

Please note I did full TDD here, I didn't test it in real conditions, but I guess it should work.

You can start a server (ctrl + P / start Home Assistant), connect to http://locahost:8084 and do some test. You have many fake entities to create your VTherm and test it. You can also deploy this in your real environment to test it in real life by just copy the code into config/custom_components/versatile_thermostast

pounard commented 1 month ago

Heyy, thank you for this. I will have a look.

I have some major strategic questions before merging:

  1. I wonder when this will stop ... if someone wants 6 heaters or 7 heaters. My question what should be the limit for one VTherm. I though was enough. The other users have split into 2 heating zone, which one VTherm for each zone having less than 4 switch. Is that not preferable in fact ?
  2. the other question is why only for VTherm over_switch ? Why not for over_valve and over_climate ?

Believe me or not, but I asked myself the exact same questions :sweat_smile:

Fact is I focused on my own use case, to see whether or not this was doable prior doing anything else.

I guess with some for loops at the right places, the max number of heaters could simply be generated, but it will make all the existing tests obsolete, and that's going to be really hard to fix all those.

pounard commented 1 month ago

I wanted to add that, my point of is, that all your tests are very thoroughly built but they are mostly functional tests and not unit tests enough.

I guess that, if I read your code correctly, whatever works for 4 will work for 5, and 6, etc... Simply put, I think coverage can omit building-up 6, 7... entities each time: on the other side we can focus on adding a single or two more "unity" tests for testing that configuration is correctly propagated.

pounard commented 1 month ago

I have some major strategic questions before merging:

Please don't merge as-is, I left a todo about a unit test I messed up but I don't understand what is its goal.

pounard commented 1 month ago

I will do a second pass in order to add more than 5 (maybe arbitrarily 10?) and also add them for over_valve and over_climate, is that OK for you ?

pounard commented 1 month ago

And did it:

jmcollin78 commented 1 month ago

I will do a second pass in order to add more than 5 (maybe arbitrarily 10?) and also add them for over_valve and over_climate, is that OK for you ?

Sorry but I don't think it is a good thing to multiply the number of underlying with hard coding the limit. In two years, you are the first that need more than 4 underlyings heaters, so there is not a real and massive need of so much entries. My point was not to say: let's have 10 entries but more, we need a limit and respect this limit. The fallback is very easy: just do another VTherm.

That makes awful code with repeated lines (10 times...). It was not very nice, but now it is awful. So my opinion as now is that I will not merge this PR. Sorry for your work and my time to make this response.

What could be fine is to find a way to add dynamically some underlying. But I think it is very complex

pounard commented 1 month ago

I understand your point, nevertheless, it doesn't really make any sense in my use case to have more than one VTherm for the same room and I'd really like to have only one. I'm quite happy to be able to contribute it and not let the burden all onto yourself, that's one of the great benefits I see in open source software!

To be honest, they are many software users but they do not always contribute or open issues. I think that having more than 4 heaters in a single zone is not that uncommon. In fact, in both houses I lived during the past 10 years, there was huge living rooms with more than 4 heaters. It's a common thing for people to open up walls and open spaces in houses. I think that, if I stumble on this problem, probably some more people will and experience frustration as I did, but not all will go as far as contributing. I see here contributing as a chance, not a burden, and it may help other people.

How about a compromise? Let's limit the number of added heaters, say 2 more for example, to avoid huge list propagation, and in exchange, I'll try to focus two things:

What do you think about this?

jmcollin78 commented 1 month ago

How about a compromise? Let's limit the number of added heaters, say 2 more for example, to avoid huge list propagation, and in exchange, I'll try to focus two things:

Yes exactly was I was thinking after a full night of rest. Let's limit to 5. It seems a good compromise and a "round" number. 5 for all VTherm type. Can you go in this direction please ? Then I will do a full review and do some response to your points. I think you miss translations (don't remember exactly if there is an impact).

And once againg, so many thanks for your contribution. That is so rare, I have to take care of contributors 👍

pounard commented 1 month ago

Yes exactly was I was thinking after a full night of rest. Let's limit to 5. It seems a good compromise and a "round" number. 5 for all VTherm type. Can you go in this direction please ?

I'm OK with this, mostly because it's enough for me :sweat_smile: but I hope there will not be people asking for more too soon.

I'll fix my PR and come back to you, thanks for showing interest in this.

Ho! And I'll buy you a beer if you find your way to Nantes, FR sometime :smile:

jmcollin78 commented 1 month ago

Ho! And I'll buy you a beer if you find your way to Nantes, FR sometime 😄

Be careful, je vais venir ! (Je n'y vais pas souvent mais je me le note)

pounard commented 1 month ago

I did lower the heater number to 6, is that OK for you? I can remove one more if you prefer, yet patch is less impressive already.

hilburn commented 3 weeks ago

Is it not possible to do this as an arbitrary list? this was one of the things Better Thermostat did quite well as you'd just have 

underlying entities:
  - climate.one
  - climate.two
  - climate.three
  etc

rather than 

underlying1: climate.one
underlying2: climate.two
jmcollin78 commented 3 weeks ago

Is it not possible to do this as an arbitrary list? this was one of the things Better Thermostat did quite well as you'd just have

underlying entities:
  - climate.one
  - climate.two
  - climate.three
  etc

rather than

underlying1: climate.one
underlying2: climate.two

Don't understand where your proposal is ? Are you in translations ?

hilburn commented 3 weeks ago

In better thermostat they use

vol.Required(CONF_HEATER): selector.EntitySelector(
    selector.EntitySelectorConfig(domain="climate", multiple=True)
)

And store the entities as a list In VTherm you have them stored as CONFCLIMATE([2,3,4]), but whenever you interact with the underlying entities you wrap them in a list and iterate over them anyway - so I think it makes more sense to store them as a list in the config in the first place and remove the arbitrary limit for the number of entities that can be controlled under one VTherm It also cleans up the config flow, as you only show currently selected entities + "add another one" rather than having 2/3 empty entries in most configurations

It would require some code to allow migration of the current structure of CONF_\<TYPE>_X -> CONF_\<TYPE>_LIST but imo it's worth it

jmcollin78 commented 3 weeks ago

It would require some code to allow migration of the current structure of CONF_X -> CONF_LIST but imo it's worth it

Oh yes. That is a very good way to handle this. The actual design is bad. Do you think you will be able to give it a try ?

hilburn commented 3 weeks ago

I should have some free time this week so will give it a go

hilburn commented 3 weeks ago

@jmcollin78 Running into some trouble with the vscode/devcontainers stuff - i have the devcontainers extension, docker installed (basically just followed the instructions in the .devcontainer README.md but I only get the attached as an error, what've I missed?

https://pastecode.io/s/h7i9woxq

jmcollin78 commented 3 weeks ago

@jmcollin78 Running into some trouble with the vscode/devcontainers stuff - i have the devcontainers extension, docker installed (basically just followed the instructions in the .devcontainer README.md but I only get the attached as an error, what've I missed?

https://pastecode.io/s/h7i9woxq

If you look carefully the logs you gave me, I think the error is there:

[2024-10-28T13:28:13.468Z] docker: Error response from daemon: invalid mount config for type "bind": bind s
ource path does not exist: /.ssh.
See 'docker run --help'.

You can comment this line about .ssh if you don't have one. This is to share the login and credentials with github.

hilburn commented 3 weeks ago

If you look carefully the logs you gave me, I think the error is there:

[2024-10-28T13:28:13.468Z] docker: Error response from daemon: invalid mount config for type "bind": bind s
ource path does not exist: /.ssh.
See 'docker run --help'.

You can comment this line about .ssh if you don't have one. This is to share the login and credentials with github.

There were a couple of things wrong - i apparently did WSL wrong so had to rebuild the containers, and then when i commented out the mounts it got into the devcontainer properly,

however it's not working - and none of the home assistant stuff is importing properly

image

image

jmcollin78 commented 3 weeks ago

@hilburn,

You should be able to run the container start command manually to see what happens.

hilburn commented 3 weeks ago

Yes, got it working eventually :)

jmcollin78 commented 3 weeks ago

Hello @pounard ,

Sorry for your PR, but @hilburn does something great by allowing to add arbitrary number of underlyings. This is in the PR https://github.com/jmcollin78/versatile_thermostat/pull/590.

Thank you anyway and don't hesitate to propose great changes !

pounard commented 2 weeks ago

@jmcollin78 and how about merging mine until the other one is completed?

Hint: winter is coming and I still have a non working heater...

jmcollin78 commented 2 weeks ago

Hello,

I will be fast (this week) to merge if all is alright. Just create 2 VTherms if you can't wait.

jmcollin78 commented 2 weeks ago

Relased with: https://github.com/jmcollin78/versatile_thermostat/releases/tag/6.6.0