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

Details for two parameters related to hot water. #101

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

gerw
Copy link
Collaborator

@gerw gerw commented Jul 7, 2023

This patch adds details for:

  • Parameter 994. This parameter decides if the heating rod is enabled to heat the hot water if the heat pump does not reach the desired temperature (Parameter 105) or if the temperature reaches the value of parameter 2.
  • Parameter 1020. I think that this parameter controls, how long the heating rod is allowed to heat the water.

Both parameters can be accessed at a German heat pump via "Service" -> "Einstellungen" -> "System Einstellungen" -> "Warmw. Nachheizung" / "Warmw. Nachh. max"

Parameter 1020 uses a new conversion formula for hours. ["Fun" fact: if you register as "Kundendienst" / "after sales service", then the parameter value is displayed (with raw/10) incorrectly in the "Event Log".]

By the way: I think that there was some discussion, whether more information concerning the parameters could be collected in this module. My suggestion would be to move the parameter information to a separate (json formatted?) file, which is consequently read by parameters.py. Then, one could also add something like max and min values for the parameters. Would there be any downside?

@kbabioch kbabioch requested review from Bouni and kbabioch July 7, 2023 17:34
@kbabioch
Copy link
Collaborator

kbabioch commented Jul 7, 2023

Changes look fine to me. Personally I would also like to see unit tests, as they are already available for the other datatypes.

Regarding the json file for meta information: I like the idea, but would encourage you to discuss is within #35 rather than in this pull request.

@gerw
Copy link
Collaborator Author

gerw commented Jul 7, 2023

I added a simple unit test.

Copy link
Owner

@Bouni Bouni left a comment

Choose a reason for hiding this comment

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

Lgtm

@gerw
Copy link
Collaborator Author

gerw commented Jul 9, 2023

Since the PR was not merged yet, I took the opportunity to identify some more calculations. For some reason, the energy from the heating rod seems to be a parameter.

Calculations 232,233, 236-243 can be seen in the web interface if you are logged in as after-sales service. I also have taken the liberty to rename calculation 241, since "Circulation_Pump" sounds (at least for me) like Zirkulationspump/ZIP which is something different.

For me, calculations 234 and 235 are always "0".

By the way: My heating rod uses 6kW but is configured to 9kW. This seems to be parameter 1025 with--yet another--a scaling factor of 100W. Can anybody confirm this? However, I do not find a menu in which I could set my heating rod to 6kW.

Comment on lines 277 to 278
242: Kelvin("HUP_Temp_Spread_Soll"),
243: Kelvin("HUP_Temp_Spread_Ist"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering what this means physically? Don't remember to have seen this in the menu (will check later). But HUP means Heizungsumwälzpumpe. To my knowledge it is just circulating the water in the heating system. Not entirely really sure what Soll and Ist are supposed to mean in this context?

How exactly are those parameters called in the menu?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These values can be seen in the (new) webinterface of the heat pump if you are logged in as Kundendienst.

If I understand my heat pump correctly, it only has two pumps: One for pumping the water into the bore holes (this is VBO which seems to come from Ventilator, Brunnen- oder Soleumwälzpumpe) and another one for pumping the heated water either into the hot water deposit or into the underfloor heating pipes).

These values report the Ist and Soll values of the temperatures of inflow and outflow. I think they are used to control the operating level of the pump.

@kbabioch
Copy link
Collaborator

By the way: My heating rod uses 6kW but is configured to 9kW. This seems to be parameter 1025 with--yet another--a scaling factor of 100W. Can anybody confirm this? However, I do not find a menu in which I could set my heating rod to 6kW.

That's interesting. For me it says:

Number: 1025 Name: ID_Einst_Leistung_ZWE Type: Unknown Value: 90

As you've said that could mean 9000 with a scaling factor of 100. Physically it's only 3 kW in my case, and I never seen a menu to change this. Would be interesting to learn what this is used for. Maybe some internal calculations?

@kbabioch
Copy link
Collaborator

In general: Thank you for working on identifying more parameters. That's appreciated.

I think we should try to avoid to have too many of those parameters introduced / changed in one MR, though in the future. Ideally, for every set of datapoints that are related to each other, there should be a dedicated MR.

Otherwise the discussion will get more and more confusing and it's hard to follow along and finish something.

So for the future, I would suggest to create a new MR for new datatypes.

luxtronik/calculations.py Outdated Show resolved Hide resolved
@gerw
Copy link
Collaborator Author

gerw commented Jul 11, 2023

By the way: My heating rod uses 6kW but is configured to 9kW. This seems to be parameter 1025 with--yet another--a scaling factor of 100W. Can anybody confirm this? However, I do not find a menu in which I could set my heating rod to 6kW.

That's interesting. For me it says:

Number: 1025 Name: ID_Einst_Leistung_ZWE Type: Unknown Value: 90

As you've said that could mean 9000 with a scaling factor of 100. Physically it's only 3 kW in my case, and I never seen a menu to change this. Would be interesting to learn what this is used for. Maybe some internal calculations?

In the webinterface, one can see the amout of heat produced by the heating rod. This seems to be also Parameter 1059. That is, parameter 1059 increases when the heating rod is on, and for this, parameter 1025 is used.

It is not clear to me whether this parameter 1025 is also used (internally) for other things.

@gerw
Copy link
Collaborator Author

gerw commented Jul 11, 2023

Concerning multiple MR: Yes, I agree.

@kbabioch
Copy link
Collaborator

Concerning multiple MR: Yes, I agree.

To speed things up: Could you split your work, so that we stick to the first two commits (already approved) in this MR?

Something like this should do the trick:

git checkout main
git checkout -b new-parameters
git checkout main
git reset --hard f47c37b
git push --force

Afterwards your main branch (source for this MR) will be in the previous state and you'll have your other commit in a new-parameters branch.

@gerw
Copy link
Collaborator Author

gerw commented Jul 11, 2023

Done.

@Bouni
Copy link
Owner

Bouni commented Jul 11, 2023

@kbabioch Feel free to merge this one as you have looked into this a lot more than I did 😅

@kbabioch
Copy link
Collaborator

There is one more minor issue. The linting now complains about:

https://github.com/Bouni/python-luxtronik/actions/runs/5520635370/jobs/10068627802?pr=101#step:6:13

luxtronik/datatypes.py:281:4: W0221: Number of parameters was 2 in 'Base.from_heatpump' and is now 2 in overriding 'Hours2.from_heatpump' method (arguments-differ)
luxtronik/datatypes.py:284:4: W0221: Number of parameters was 2 in 'Base.to_heatpump' and is now 2 in overriding 'Hours2.to_heatpump' method (arguments-differ)

The reason is the refactoring of the classes via #96. Unfortunately I don't have permission to change your branch, so could you refactor it on top of current main? Essentially you'll have to add the @classmethod keyword.

@Bouni
Copy link
Owner

Bouni commented Jul 11, 2023

@kbabioch You mean rebase on main, right? not refactor!?

@kbabioch
Copy link
Collaborator

@kbabioch You mean rebase on main, right? not refactor!?

Yes, exactly. Sorry for confusion. It needs to be rebased on top of current main.

@gerw
Copy link
Collaborator Author

gerw commented Jul 11, 2023

Done. However, in the test cases, should Hours2("").to_heatpump also be replaced by Hours2.to_heatpump? And similarly for all the other data types? [Why did the unit test not complain?]

@kbabioch
Copy link
Collaborator

Changes look good now, merging it into main. Thank you for the contribution. Let's discuss the other parameters in follow-up pull requests.

@gerw
Copy link
Collaborator Author

gerw commented Jul 11, 2023

@kbabioch : What about my last comment of Hours2.to_heatpump vs Hours2("").to_heatpump?

@kbabioch
Copy link
Collaborator

@kbabioch : What about my last comment of Hours2.to_heatpump vs Hours2("").to_heatpump?

Hm, seem to have missed that.

Can it be removed, though? to_heatpump is a class method, but we are still dealing with an instance of an object, aren't we?

@kbabioch
Copy link
Collaborator

Addressed with #105. Also identified an issue identified by closely looking at the CI/CD output (see #104). Need to address the issues with unit tests with SelectionBase now.

@gerw gerw mentioned this pull request Jul 11, 2023
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.

3 participants