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

Fix compensation curve heating values, update HeatPumpType models #96

Closed
wants to merge 0 commits into from

Conversation

ekouters
Copy link
Contributor

The descriptions of the heating compensation curve for Z1 and Z2 have the "lowest" and "highest" swapped.
Initially I thought the installer configured it incorrectly, but it turns out the description was incorrect. :-)

My heatpump was not recognized (T-CAP J-series 12kW), so I have updated the models.py with the attached script.

@Bucky2k
Copy link
Contributor

Bucky2k commented Aug 31, 2023

Not so sure about that, it's ok on my side. Remark: It's saying e.g. "temperature at lowest point on heat curve". That means: Low point on heat curve, e.g. -10°C outside temp, shall yield xx°C outlet temperature". This value will of course be the high one, as the other will be low on high outside temperatures. So it is counter intuitive on the first read, but correct

@geduxas
Copy link
Contributor

geduxas commented Aug 31, 2023

Never noticed that, but it's seam's that namming is little misleading.. (for outside sensors.. )

Screenshot_2023-08-31-08-27-30-36_c3a231c25ed346e59462e84656a70e50

Also same is with Z2 too.

@Bucky2k
Copy link
Contributor

Bucky2k commented Aug 31, 2023

Ahh, I see. The naming in Heishamon is okay, but in HA not. Never noticed until today
Screenshot_20230831_075558_Home Assistant

@ekouters
Copy link
Contributor Author

ekouters commented Aug 31, 2023

Ahh, I see. The naming in Heishamon is okay, but in HA not.

This is exactly what this PR attempts to fix. :-)

@geduxas
Copy link
Contributor

geduxas commented Aug 31, 2023

Ahh, I see. The naming in Heishamon is okay, but in HA not.

This is exactly what this PR attempts to fix. :-)

Yes, but it's problem only with outside temperature naming.. for heat in lowest point i think it's correct :)

@ekouters
Copy link
Contributor Author

Ahh, I see. The naming in Heishamon is okay, but in HA not.

This is exactly what this PR attempts to fix. :-)

Yes, but it's problem only with outside temperature naming.. for heat in lowest point i think it's correct :)

I see what you mean.
The naming does seem very confusing to me.

Here's my train of thoughts.
The MQTT topics are:

  • Z1_Heat_Curve_Target_High_Temp
  • Z1_Heat_Curve_Target_Low_Temp
  • Z1_Heat_Curve_Outside_High_Temp
  • Z1_Heat_Curve_Outside_Low_Temp

This image comes from the service manual:
image

The confusion comes from the Low_Temp and High_Temp.
These Low and High temperatures refer to the outside temperature, not the water temperature.
With a low outside temperature (Z1_Heat_Curve_Outside_Low_Temp), it uses the Low_Temp target (Z1_Heat_Curve_Target_Low_Temp).

These are currently described as:

  • Highest outside temperature on the heating curve (Z1_Heat_Curve_Outside_Low_Temp)
  • Target temperature at highest point on heating curve (Z1_Heat_Curve_Target_Low_Temp)

My PR attempts to correct these to:

  • Lowest outside temperature on the heating curve (Z1_Heat_Curve_Outside_Low_Temp)
  • Target temperature at lowest point on heating curve (Z1_Heat_Curve_Target_Low_Temp)

I agree my PR is incorrect on updating the Target temperature description.
I'm also unhappy with the description in general (lowest/highest point on heating curve).
Instead, I would suggest "Target water temperature at lowest outside temperature on the heating curve".
What do you guys think?

@geduxas
Copy link
Contributor

geduxas commented Aug 31, 2023

@ekouters i think you're free to make changes :)

@kamaradclimber
Copy link
Owner

Hello, thanks both for the patch and comments.
I've merged it (I messed up in git so the PR appeared as closed) and it's part of https://github.com/kamaradclimber/heishamon-homeassistant/releases/tag/1.0.8

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.

4 participants