Add registers for Pool 40 accessory and some more missing registers to S1156/S1256 #204
Conversation
…ension for the change instead Note: I suspect that the changing of this value in the CSV file from my heat pump may come from the latest firmware version (3.4.11)
nibe/data/extensions.json
Outdated
| ], | ||
| "data": { | ||
| "40061": { | ||
| "default": 430.0 |
There was a problem hiding this comment.
Why you need this default? I do not think we use it anywhere?
There was a problem hiding this comment.
@yozik04 Hey! Apologies for submitting a so far undocumented PR, I am going to write a description but did not have time for it yesterday – will update it today!
Regarding this change in particular: the CSV export from my S1256 included this change in the default value of this register compared to the CSV file in the repository. I wrote in the commit description that I suspect this change might come from the latest firmware (3.4.11). But it also seems to me like it is a pretty insignificant change, and for that reason maybe it can be ignored? (If you agree, I can remove the addition to extensions.json from this PR.)
There was a problem hiding this comment.
Just ignore it. It is not possible to make it perfect. It changes every month for S series.
| @@ -1173,6 +1188,7 @@ Heating, auto MODBUS_HOLDING_REGISTER 3059 1 4 0 1 1 | |||
| Factor MODBUS_HOLDING_REGISTER 3033 1 4 1 10 5 | |||
| id:10881 MODBUS_HOLDING_REGISTER 3062 1 4 0 1 0 | |||
| id:10890 MODBUS_HOLDING_REGISTER 3063 1 4 0 1 0 | |||
| Hot water start (BT5) MODBUS_INPUT_REGISTER 2014 10 °C 2 0 0 0 | |||
There was a problem hiding this comment.
@yozik04 regarding this line and why it is "new": I see that you had a discussion recently about adding this register. It was then added to s1155_s1255.csv in #197 but then it seems the "Hot water start" was lost for the xx56 models when the new s1156.csv file was added in #198 and later renamed to s1156_s1256.csv in this commit.
There was a problem hiding this comment.
I think you can just drop extensions.json change, regenerate json file and it will be fine to merge. Of course tests need to pass =)
|
Hey @yozik04, I've updated the PR and removed the addition to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #204 +/- ##
=======================================
Coverage 70.17% 70.17%
=======================================
Files 14 14
Lines 1358 1358
=======================================
Hits 953 953
Misses 405 405 ☔ View full report in Codecov by Sentry. |
|
@vtamm Thank you for the effort! |
|
@yozik04 this is unrelated to this PR but for lack of a better place I'll notify you here; I've just started looking into the mapping of the S-series climate systems properties in I'm reviewing all of the climate systems now and will submit a new PR with any changes probably in the next couple of hours. Just wanted to let you know right away in case you're planning to tag a new version soon – you might want to hold it until I'm done with this review so that you can include the corrected mappings in the next release! |
|
@yozik04 if you don't mind, I have a couple of things I'd like to ask/discuss with you which are better suited for chat, so I sent you a friend request (Flux) on the Home Assistant Discord server! |
|
I've added you, but I think you want to talk to @elupus instead about S-series. |
No description provided.