-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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 Overkiz AtlanticPassAPCHeatingAndCoolingZone #78659
Add Overkiz AtlanticPassAPCHeatingAndCoolingZone #78659
Conversation
Hey there @iMicknl, @vlebourl, @tetienne, mind taking a look at this pull request as it has been labeled with an integration ( |
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some minor remarks.
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Show resolved
Hide resolved
…ing_and_cooling_zone
…ttps://github.com/nyroDev/home-assistant-core into overkiz/atlantic_pass_apc_heating_and_cooling_zone
…ing_and_cooling_zone
…ttps://github.com/nyroDev/home-assistant-core into overkiz/atlantic_pass_apc_heating_and_cooling_zone
…ing_and_cooling_zone
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @nyroDev! Good to have it simplified and the other code improvements in another PR.
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
self.base_device_url = split_device_url[0] | ||
if len(split_device_url) == 2: | ||
self.index_device_url = split_device_url[1] | ||
self.executor = OverkizExecutor(device_url, coordinator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this code do? Tbh, I don't find this code very clean / readable. And it seems that self.index_device_url
is not typed / having None as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other devices, like somfy_thermostat for instance, we use a constant:
TEMPERATURE_SENSOR_DEVICE_INDEX = 2 |
core/homeassistant/components/overkiz/climate_entities/somfy_thermostat.py
Lines 67 to 74 in 551fb44
def __init__( | |
self, device_url: str, coordinator: OverkizDataUpdateCoordinator | |
) -> None: | |
"""Init method.""" | |
super().__init__(device_url, coordinator) | |
self.temperature_device = self.executor.linked_device( | |
TEMPERATURE_SENSOR_DEVICE_INDEX | |
) |
I think it might be clearer an less prone to error than doing n+1
as you do here
# Temperature sensor use the same base_device_url and use the n+1 index
self.temperature_device = self.executor.linked_device(
int(self.index_device_url) + 1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type of device don't work like that.
We have to use the climate device index and search for n+1 index, which will be the temperature sensor.
The n+1 is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it resolves the conversation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this PR is merged and published, this code will be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so, for now we are keeping it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working like this, it could be merged as it is.
Up to @iMicknl to decide I guess.
…ing_and_cooling_zone
…ing_and_cooling_zone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments, otherwise goob job !
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure this PR contains the latest ha-tahoma feature/fix
Co-authored-by: Quentame <polletquentin74@me.com>
…s_apc_heating_and_cooling_zone.py Co-authored-by: Quentame <polletquentin74@me.com>
…s_apc_heating_and_cooling_zone.py Co-authored-by: Quentame <polletquentin74@me.com>
Co-authored-by: Quentame <polletquentin74@me.com>
@Quentame This PR include almost everyt features that are present in ha-tahoma. Also the coding styles and the way to retrieve temparatures devices have been updated to match how it's done in others integrations here. There is a pending PR in pyoverkiz to improve this a little bit. |
…ing_and_cooling_zone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, thanks ! 🎉
Waiting one conversation resolution before merging.
@nyroDev Can you explain what is a As it will be used for the release notes, it will be more understandable for everyone. |
Updated, let me know if that's ok for you. |
Nice ! 👌 |
Breaking change
Proposed change
Implement AtlanticPassAPCHeatingAndCoolingZone with minimal but functionnal features.
AtlanticPassAPCHeatingAndCoolingZone is an heating system circuit managed by an Atlantic heat pump.
A corresponding temperature sensor is also attached to the heat pump and is used here in order to retrieve the current temperature of the heating zone.
1 Atlantic heat pump could have multiple AtlanticPassAPCHeatingAndCoolingZone attached.
Type of change
Additional information
It tested it on my real devices at home, which doesn't support cooling.
The features I implmented seem to work so far
I also added the async_execute_commands in the Overkiz executor in order to be able to execute multiple commands at once.
I did it because the regular web interface of Somfy do that.
It works great, but the cancel feature is not implemented.
As fas I async_execute_commandsI checked and it seems it's not used in any place for the single command execution
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: