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 device AtlanticPassAPCHeatingZone #785

Closed

Conversation

jgarec
Copy link
Contributor

@jgarec jgarec commented Mar 2, 2022

Closes #731

@iMicknl iMicknl marked this pull request as draft March 2, 2022 09:24
Copy link
Owner

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Thanks @jgarec! Appreciated :).

Since you mention WIP in the title, I marked your PR as a draft for now, so it won't accidentally get merged before you are finished.

@jgarec jgarec changed the title WIP: add device AtlanticPassAPCHeatingZone add device AtlanticPassAPCHeatingZone Mar 2, 2022
@jgarec jgarec marked this pull request as ready for review March 2, 2022 19:53
@jgarec
Copy link
Contributor Author

jgarec commented Mar 2, 2022

This component is concerned by #167

CUSTOM_PRESET_AUTO = "Auto"
CUSTOM_PRESET_STOP = "Stop"

COMMAND_REFRESH_COMFORT_HEATING_TARGET_TEMPERATURE = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add all these states and commands to https://github.com/iMicknl/python-overkiz-api/tree/main/pyoverkiz/enums ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i also add commands / states not used in this class ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only add the ones you need

@jgarec jgarec force-pushed the feat/support_AtlanticPassAPCHeatingZone branch from da6fb8d to 0336ac9 Compare March 3, 2022 08:25
Comment on lines +89 to +90
_attr_max_temp = 30
_attr_min_temp = 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

These values are not exposed by Atlantic?

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 in this device (device number 7)

the navilink sensor (device number 8) have 2 attributes (from 5 to 30) :

2022-02-20 21:24:28 DEBUG (MainThread) [custom_components.tahoma] Unsupported device detected (Device(attributes=States(_states=[.... State(name='core:MaxSensedValue', type=<DataType.FLOAT: 2>, value=30.0), State(name='core:MinSensedValue', type=<DataType.FLOAT: 2>, value=5.0)]),available=True, enabled=True, label=** *(**#**)*, device_url=io://****-****-0589/12309249#8, controllable_name='io:AtlanticPassAPCZoneTemperatureSensor' ....

tahomalink allows to set a temperature between 7 and 35 ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤡 And what happens if you set 35? It's applied?
If the values are returned as you Show me, you can retrieve them into the init method using the linked device.

Comment on lines 218 to 227
if self.preset_mode == PRESET_COMFORT:
return self.executor.select_state(
CORE_COMFORT_HEATING_TARGET_TEMPERATURE_STATE
)
if self.preset_mode == PRESET_ECO:
return self.executor.select_state(CORE_ECO_HEATING_TARGET_TEMPERATURE_STATE)
if self.preset_mode == CUSTOM_PRESET_DEROGATION:
return self.executor.select_state(CORE_DEROGATED_TARGET_TEMPERATURE_STATE)

return self.executor.select_state(CORE_TARGET_TEMPERATURE_STATE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here also you can ease the code with a dict.

Copy link
Collaborator

@tetienne tetienne left a comment

Choose a reason for hiding this comment

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

Sorry for the number of comments.
As your code will be backport to the Core, I try to be as strict as they are (fairly).

@iMicknl
Copy link
Owner

iMicknl commented Mar 3, 2022

This component is concerned by #167

For what do you need this? There is a low chance that this will be implemented.

@jgarec
Copy link
Contributor Author

jgarec commented Mar 3, 2022

This component is concerned by #167

For what do you need this? There is a low chance that this will be implemented.

If i change a state in HA then i can handle the refresh by myself. But if it's changed outstide then some states are not always refreshed and HA uses old known states

@iMicknl
Copy link
Owner

iMicknl commented Mar 3, 2022

This component is concerned by #167

For what do you need this? There is a low chance that this will be implemented.

If i change a state in HA then i can handle the refresh by myself. But if it's changed outstide then some states are not always refreshed and HA uses old known states

Makes sense... I will dive into this.

Could you add your diagnostics log here? I would love to understand more about your device :). Sorry for the strict reviews, but we would like to have this one ready directly for core as well. Some of our (older) climate implementations still need to be rewritten.
Happy to help with some refactors, if you don't want to spend time on that.

Comment on lines +223 to +225
await self.executor.async_execute_command(
COMMAND_SET_DEROGATION_ON_OFF_STATE, "on"
)
Copy link
Owner

@iMicknl iMicknl Mar 3, 2022

Choose a reason for hiding this comment

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

Suggested change
await self.executor.async_execute_command(
COMMAND_SET_DEROGATION_ON_OFF_STATE, "on"
)
await self.executor.async_execute_command(
OverkizCommand.SET_DEROGATION_ON_OFF_STATE, OverkizCommandParam.ON
)

Would be great if you could use our enums here (and in all other executions). See https://github.com/home-assistant/core/blob/dev/homeassistant/components/overkiz/climate_entities/atlantic_electrical_heater.py.

However, many of these enums are probably missing and we need to add them to PyOverkiz.

@vlebourl vlebourl requested a review from tetienne March 8, 2022 09:14
@iMicknl
Copy link
Owner

iMicknl commented Nov 7, 2022

@jgarec would you still be interested of porting this one to core? We are now moving all climate entities.

@iMicknl iMicknl closed this Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Atlantic Naia 2
4 participants