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

Add set_profile service for Vallox integration #120225

Merged
merged 7 commits into from
Sep 8, 2024

Conversation

treetip
Copy link
Contributor

@treetip treetip commented Jun 23, 2024

Proposed change

Add service for activating a profile with optional duration for profiles that support it.
Add sensor for reporting duration left on activated profile.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • Tested on my Vallox 125MV running 2.0.24 firmware

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @treetip

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft June 23, 2024 08:01
@home-assistant
Copy link

Hey there @andre-richter, @slovdahl, @viiru-, @yozik04, mind taking a look at this pull request as it has been labeled with an integration (vallox) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of vallox can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign vallox Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@treetip treetip force-pushed the vallox-timed-profiles-service branch from 7152e2a to 34cbb9d Compare June 23, 2024 08:40
@treetip treetip marked this pull request as ready for review June 23, 2024 08:41
@home-assistant home-assistant bot dismissed their stale review June 23, 2024 08:41

Stale

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Let's split this PR up into 2, one for the service, one for the sensor

@home-assistant home-assistant bot marked this pull request as draft June 23, 2024 09:17
@treetip treetip force-pushed the vallox-timed-profiles-service branch from 34cbb9d to 4d1b6f9 Compare June 23, 2024 13:18
Comment on lines 31 to 47
set_profile:
fields:
profile:
required: true
selector:
select:
options:
- label: "Home"
value: "1"
- label: "Away"
value: "2"
- label: "Boost"
value: "3"
- label: "Fireplace"
value: "4"
- label: "Extra"
value: "5"
Copy link
Member

Choose a reason for hiding this comment

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

So I am wondering, would this be able to be split up to a select entity and a number entity? I think the select entity would work, but I won't be sure if the logic around it could be reflected correctly, wdyt?

Copy link
Contributor

@yozik04 yozik04 Jun 23, 2024

Choose a reason for hiding this comment

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

set_profile method in the library supports setting duration. It works only for fireplace, boost and extra profiles.

Copy link
Contributor

@yozik04 yozik04 Jun 23, 2024

Choose a reason for hiding this comment

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

So I think a single set_profile service would be fine accepting profile name string and optional duration. Creating int mappings feels to be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I mean its not about int mapping, its more about moving to a service or to entities, as entities are more user friendly and accesible, the only question is, is this a good idea for these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not that familiar with python, is there a simple way of having the parameter be coerced into the matching int enum of Profile from the lib?

or how would the mapping be done? e.g. from "Boost" to Profile(3)

Copy link
Member

Choose a reason for hiding this comment

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

Oh but that's not the thing I am worried about, you can indeed do Profile(3). I am more wondering if it fits and if it can be done directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

two parallel conversations happening I think :)

I'm not sure about either, my end goal is just to have a service to call from nodered to enable the programmable "extra" profile when my range hood turns on.

But the vallox_websocket_api makes interactions a bit easier by hiding some of the lower level stuff. Separating this into two entities might mean splitting some of the existing stuff to use the lower level modbus registers.

Like all the modes that allow timing, have separate registers for timing, and also a register for enabling or disabling the timers. And when setting a profile, it clears the existing timers for other profiles. Out of the box there is a priority order the device runs down.

I would keep everything close to the api which is less likely to change.

@yozik04
Copy link
Contributor

yozik04 commented Jun 23, 2024

I'd better add optional duration parameter to existing set profile services.

Scratch that. Misread the code.

schema=vol.Schema(
{
vol.Required("profile"): vol.All(
vol.Coerce(int), vol.Clamp(min=1, max=5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use ints as profile. Pass in a string that you would map to a profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any ideas why originally the Extra profile was not 'settable'? It's not included in VALLOX_PROFILE_TO_PRESET_MODE_SETTABLE, but I could add it there and cut down on the definitions in const.py and use it to map the param.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is some firmware limitation. Vallox's Web UI does not allow to set Extra profile in 2.0.16 (which I currently use).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does the HA side support some sort of conditionals based on the firmware version, it works fine on my .24 version

if someone can verify how it works on .20 i could branch it to either >=.20 or >=.24

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need that Extra profile? Isn’t fireplace and boost sufficient for all the use cases?

Copy link
Contributor Author

@treetip treetip Jun 24, 2024

Choose a reason for hiding this comment

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

It's a separate configurable profile that can be used via digital inputs on the board or via modbus or the websocket api.

I'd use Fireplace for the fireplace, and a differently configured extra profile to compensate for my kitchen air extraction.

image

This way i can avoid pulling a low voltage cable from the kitchen to the air handler, and just trigger the extra profile via a sensor on the kitchen range hood open/close relay.

edit: my kitchen air extraction is not via the vallox unit, so it needs to be tunable separately

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Got it. I have that in Web UI as well. I just did not know where to find it. If you make the parameter settable are you able to turn extra profile on? Would be sad if it would turn off automatically, because we do not apply any voltage on digital input.

Copy link
Contributor Author

@treetip treetip Jun 24, 2024

Choose a reason for hiding this comment

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

yes, if i apply it for 5 minutes, the ui says

image

then if you set for example home profile again, it just cancels the extra profile before the timer runs out

Copy link
Contributor Author

@treetip treetip Jun 24, 2024

Choose a reason for hiding this comment

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

and the same works from the fan entity as well

image

just like the fireplace mode it just doesn't have a fan speed as it's separately configured

@treetip
Copy link
Contributor Author

treetip commented Jun 24, 2024

Merged the settable/reportable constants, settable already supported 'fireplace' mode which works the same as extra mode.

@yozik04
Copy link
Contributor

yozik04 commented Jun 24, 2024

The code looks OK to me. Test coverage is missing.

@treetip treetip marked this pull request as ready for review June 24, 2024 18:41
@home-assistant home-assistant bot requested a review from joostlek June 24, 2024 18:41
@treetip treetip force-pushed the vallox-timed-profiles-service branch from b2272d7 to 32305d5 Compare June 25, 2024 14:15
@treetip
Copy link
Contributor Author

treetip commented Jun 25, 2024

Let me know if there's still something pending

the requested changes seem to concern something that was discussed and changed already

Copy link
Contributor

@yozik04 yozik04 left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@treetip treetip force-pushed the vallox-timed-profiles-service branch from 32305d5 to 22702e7 Compare June 26, 2024 18:46
@treetip
Copy link
Contributor Author

treetip commented Jul 2, 2024

Added documentation in home-assistant/home-assistant.io#33522

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jul 6, 2024
@treetip treetip force-pushed the vallox-timed-profiles-service branch from 8d5de30 to 0ff6c0d Compare July 8, 2024 10:29
@treetip
Copy link
Contributor Author

treetip commented Jul 16, 2024

What's the status with this? Has been done for a while and not clear to me at least what's preventing merging.

homeassistant/components/vallox/services.yaml Outdated Show resolved Hide resolved

with patch_set_profile() as set_profile:
service_data = {ATTR_PROFILE: profile} | (
{ATTR_DURATION: duration} if duration is not None else {}
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid if statements in test code

Comment on lines +216 to +217
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

I assume the true false is something that is for vallox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just imitated the other code in the vallox module, maybe someone else can answer

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not 100% sure but might just be a leftover from #56966.

@home-assistant home-assistant bot marked this pull request as draft July 21, 2024 10:42
@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Jul 22, 2024
@treetip
Copy link
Contributor Author

treetip commented Sep 7, 2024

Finally got back to this, rebased to continue working, but i can't commit anything locally due to:

Unknown rule selector: 'ASYNC210' in the hassfest commit hook.

Any tips on this? I couldn't find anything on google related to this in general or directly related to hass.

edit: run scripts/setup to update deps, for some reason pycharm didn't suggest this

@treetip treetip force-pushed the vallox-timed-profiles-service branch 2 times, most recently from f5b68b6 to df9a9f8 Compare September 7, 2024 11:11
@treetip treetip marked this pull request as ready for review September 7, 2024 11:12
@home-assistant home-assistant bot requested a review from joostlek September 7, 2024 11:12
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Tests are failing but the code starts to look good :)

@home-assistant home-assistant bot marked this pull request as draft September 8, 2024 09:52
@treetip treetip force-pushed the vallox-timed-profiles-service branch from df9a9f8 to aa178fc Compare September 8, 2024 12:49
@treetip treetip marked this pull request as ready for review September 8, 2024 12:52
@home-assistant home-assistant bot requested a review from joostlek September 8, 2024 12:52
@joostlek joostlek merged commit af62e82 into home-assistant:dev Sep 8, 2024
29 checks passed
@joostlek
Copy link
Member

joostlek commented Sep 8, 2024

Oh shit, forgot to check for the documentation. Can you please open a docs PR from and to the next branch with the docs for this service?

@treetip
Copy link
Contributor Author

treetip commented Sep 9, 2024

Oh shit, forgot to check for the documentation. Can you please open a docs PR from and to the next branch with the docs for this service?

has been there for a while: home-assistant/home-assistant.io#33522
i'll update the keys to the lower case ones tomorrow if someone else doesn't do it first

@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2024
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.

5 participants