core icon indicating copy to clipboard operation
core copied to clipboard

Add support for AEH with adjustable temperature in Overkiz integration

Open tetienne opened this issue 3 years ago • 11 comments
trafficstars

Breaking change

Proposed change

As explained in #67045, we continue to add one by one all the climate devices we support. Here, AEH with adjustable temperature. Sauter Ipala Electrical Heater is an example of such device.

Type of change

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

Additional information

  • This PR fixes or closes issue: fixes #65427
  • This PR is related to issue:
  • Link to documentation pull request:

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
  • [ ] The code has been formatted using Black (black --fast 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.

The integration reached or maintains the following Integration Quality Scale:

  • [ ] No score or internal
  • [ ] 🥈 Silver
  • [ ] 🥇 Gold
  • [ ] 🏆 Platinum

To help with the load of incoming pull requests:

tetienne avatar May 31 '22 20:05 tetienne

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

Marked as draft because there seem to be two PRs implementing the same functionality; #72389 and #72790

@iMicknl, @tetienne Please align on which PR you want to go ahead with or comment if the two PRs are not the same.

emontnemery avatar Jun 14 '22 11:06 emontnemery

@tetienne #72389 is closed, please fix the merge conflict and set to "ready for review" if you're happy with the PR 👍

emontnemery avatar Jun 20 '22 10:06 emontnemery

@tetienne can you add fixes #65427 to the PR? To close that PR automatically.

iMicknl avatar Jun 21 '22 15:06 iMicknl

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

github-actions[bot] avatar Sep 27 '22 09:09 github-actions[bot]

fixes #65427

tetienne avatar Sep 27 '22 09:09 tetienne

fixes #65427

This should be added in the description of the PR, into the Additional information section. I added it for you.

Quentame avatar Nov 04 '22 08:11 Quentame

@tetienne Can you explain what is a AtlanticElectricalHeaterWithAdjustableTemperatureSetpoint device IRL in the "Proposed change" section of the description of the PR ?

As it will be used for the release notes, it will be more understandable for everyone.

Quentame avatar Nov 04 '22 10:11 Quentame

@tetienne Can you explain what is a AtlanticElectricalHeaterWithAdjustableTemperatureSetpoint device IRL in the "Proposed change" section of the description of the PR ?

As it will be used for the release notes, it will be more understandable for everyone.

I have added an example of device in the description. I took it from this issue. But there are probably a lot more devices that will be supported by this one.

tetienne avatar Nov 05 '22 21:11 tetienne

an humble review of your code, not sure if all my remarks should be applied.

Your code review is very welcomed ! Thanks 😌

Quentame avatar Nov 07 '22 09:11 Quentame

Testing ...

Quentame avatar Nov 07 '22 17:11 Quentame

  • the "Eco room temperature" configuration number entity seem not to work

Interesting. Do you have an eco mode as well? Does the set temperature work for you (through climate entity) when the device is in eco mode?

We added this extra number entity since some users wanted to change the eco temperature, without enabling the eco mode (yet).

  • the classic identify button is infinite, why there is a start and end identify anyway ?

This is mainly for lights and covers. With identify the device will 'wink' for a few times, so you can understand which device it is. Start identify will start this behaviour and stop identify will stop this behaviour. The reason why we have it as separate (but disabled by default) buttons, is that we don't know the state and thus cannot make it a switch.

@Quentame can you perhaps share you diagnostics file? Perhaps we can find the enabled preset modes in your attributes.

iMicknl avatar Nov 07 '22 18:11 iMicknl

Here is a diagnostic file of one of my Altantic electrical heater : overkiz-916b3dffc14d36deac64fa0d379bd16c-Radiateur-2652416d6c70cf752b5bfad094327b5a.json.txt

Quentame avatar Nov 07 '22 19:11 Quentame

  • the "Eco room temperature" configuration number entity seem not to work

Interesting. Do you have an eco mode as well? Does the set temperature work for you (through climate entity) when the device is in eco mode?

We added this extra number entity since some users wanted to change the eco temperature, without enabling the eco mode (yet).

  • the classic identify button is infinite, why there is a start and end identify anyway ?

This is mainly for lights and covers. With identify the device will 'wink' for a few times, so you can understand which device it is. Start identify will start this behaviour and stop identify will stop this behaviour. The reason why we have it as separate (but disabled by default) buttons, is that we don't know the state and thus cannot make it a switch.

@Quentame can you perhaps share you diagnostics file? Perhaps we can find the enabled preset modes in your attributes.

I should create issues for those points

Quentame avatar Nov 07 '22 23:11 Quentame