Closed gerw closed 1 year ago
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.
I added a simple unit test.
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.
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 of100W
. 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 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.
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 of100W
. 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 of100
. 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.
Concerning multiple MR: Yes, I agree.
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 f47c37b996cf6bae0fa3036a09ee1382484fc2b0 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.
Done.
@kbabioch Feel free to merge this one as you have looked into this a lot more than I did 😅
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.
@kbabioch You mean rebase on main, right? not refactor!?
@kbabioch You mean rebase on main, right? not refactor!?
Yes, exactly. Sorry for confusion. It needs to be rebased on top of current main
.
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?]
Changes look good now, merging it into main
. Thank you for the contribution. Let's discuss the other parameters in follow-up pull requests.
@kbabioch : What about my last comment of Hours2.to_heatpump
vs Hours2("").to_heatpump
?
@kbabioch : What about my last comment of
Hours2.to_heatpump
vsHours2("").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?
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.
This patch adds details for:
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?