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

Underlying config changes #590

Merged
merged 6 commits into from
Nov 3, 2024

Conversation

hilburn
Copy link
Contributor

@hilburn hilburn commented Oct 29, 2024

All (modified) tests working
Allows entry of an arbitrary number of entities into a climate, switch, or valve VTherm controller

Possibility of mild breaking changes for existing template/triggers as the attributes of the resulting climate entity have changed, from: underlying_X_[1-4] to underlying_entities - however I don't see that being a commonly referenced attribute in those contexts

Should supercede #551

image

Updates previously defined 4x entries to new config style
Changes to thermostat_X to load underlying entities from list
Changes to thermostat X to display underlying entities as a list - COULD BREAK EXISTING TEMPLATES
@jmcollin78
Copy link
Owner

Hello @hilburn ,

This is really a great change ! Thank you so much for this.
I have a dev on the go but when I finish my dev (may be one or two days), I will review and integrate your changes. So be patient please. I hope this will not generate conflict (or small conflicts on config_flow only).

I will also close the #551 PR.

@hilburn
Copy link
Contributor Author

hilburn commented Oct 31, 2024

The underlying_entities attribute handling could be moved into Base Thermostat, as it's now common to all the types, but I left it in the child classes for now as it could be changed back to climates, valves, and switches.

jmcollin78
jmcollin78 previously approved these changes Nov 3, 2024
Copy link
Owner

@jmcollin78 jmcollin78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

This seems just perfect. I don't have any comments to do.

@jmcollin78
Copy link
Owner

jmcollin78 commented Nov 3, 2024

Hello @hilburn ,
You have one failed validation. data_description have been updated it seems.
You have also to change the strings.json (a copy of en.json)

@hilburn
Copy link
Contributor Author

hilburn commented Nov 3, 2024

Ok, I think that should be all matching now

@jmcollin78
Copy link
Owner

I hv resolved the conflicts with my last release. It was minor.

jmcollin78
jmcollin78 previously approved these changes Nov 3, 2024
@jmcollin78
Copy link
Owner

I will merge and debug it in local. I don't understand why the test fails.

@jmcollin78 jmcollin78 merged commit e6c330f into jmcollin78:main Nov 3, 2024
4 of 5 checks passed
@hilburn hilburn deleted the underlying_config_changes branch November 3, 2024 22:03
@hilburn
Copy link
Contributor Author

hilburn commented Nov 3, 2024

Awesome, I'm glad it all looks good on your end

@jmcollin78
Copy link
Owner

jmcollin78 commented Nov 3, 2024

I have this error on test:

if errors:
>           raise er.MultipleInvalid(errors)
E           voluptuous.error.MultipleInvalid: extra keys not allowed @ data['climate_entity_id']

on test_user_config_flow_over_climate_auto_start_stop (my last test). I try to find out why.

EDIT: ok. Have to use CONF_UNDERLYING_LIST instead CONF_CLIMATE.

jmcollin78 pushed a commit that referenced this pull request Nov 3, 2024
@jmcollin78
Copy link
Owner

jmcollin78 commented Nov 3, 2024

It is fine now. I will do a beta release. Can you please give it a try ?
Ping @pounard if you want you can try also this beta release when ready (should come rapidly).

Pre-release is here: https://github.com/jmcollin78/versatile_thermostat/releases/tag/6.6.0.beta1

Many thanks @hilburn and @pounard for this great enhancement.

@hilburn
Copy link
Contributor Author

hilburn commented Nov 3, 2024

I just installed it and reloaded on my HA - everything seemed to import correctly
Except - for some reason all my VTherm entities have "auto fan mode" set to High
image
Despite none of them having fans or being set to that when I originally set them up.
I don't know if this is a new bug or it's been around for a while and I just never noticed though

@jmcollin78
Copy link
Owner

jmcollin78 commented Nov 3, 2024

You never configure this fan-mode ? You don't change this and me too.

It is set to "turbo" on my side. That was my previous value.

Nothing weird for me.

@hilburn
Copy link
Contributor Author

hilburn commented Nov 3, 2024

Ahhh never mind, it just defaults to that during setup and I never noticed or turned it off

@jmcollin78
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants