Add more registers for S-series, update extensions.json, update mappings for climate systems #206
Conversation
|
@elupus Can you please review this PR. |
| @@ -252,8 +252,9 @@ | |||
| } | |||
| }, | |||
| { | |||
| "description": "Corrections for compressor power units - S series", | |||
| "description": "Corrections for compressor power units - S735, S1155/S1156", | |||
There was a problem hiding this comment.
Don't to the models to the description. That will be hard to maintain
There was a problem hiding this comment.
Hi @elupus! The reason I updated this line is because this extension does not apply to all S-series, only these two models, because for whatever reason the "Instantaneous used power" register in their respective CSV files has Ws defined as the unit instead of W like it should be:
Line 1380 in eeed310
nibe/nibe/data/s1155_s1255.csv
Line 1766 in eeed310
For all other S-series models, the unit for this register is already correctly reported as W in the CSV exports.
There was a problem hiding this comment.
Yes, but it is likely we find more (or these are fixed). So we should not mention the model names in the description.
It should just be a correction for the series pumps with invalid values.
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "description": "M12676EN-1 - Common Parameters", |
There was a problem hiding this comment.
Please don't change these descriptions. It breaks the diff and this spec is the source for most of these values.
There was a problem hiding this comment.
Just to clarify: I submitted these changes only as a suggestion after first privately messaging @yozik04 to ask if it is permitted to change the existing objects (by merging several mappings into one object) and update some of the descriptions to make it more clear what some of the extensions do.
I haven't gotten a response to that question yet, so my intention with this PR (at this point) was only to show what the result would be if I went ahead with those changes if permission was granted.
I changed the description right here because it doesn't convey any meaningful information, like which models are affected. The string "M12676EN-1" refers to the filename/document name for the NIBE Modbus registers PDF, as can be seen in the bottom left of the first page here.
But of course, if merging objects and updating descriptions breaks something, I will certainly not make any such changes!
There was a problem hiding this comment.
The M12676EN-1 document have separate sections for different models. I want the extensions to reflect which section of that document the data is from.
I wasn that document to be the main "source" of truth, then we can add corrections on top of that.
There was a problem hiding this comment.
@elupus Here's an image that better illustrates what I mean by "merging objects and update descriptions":
There was a problem hiding this comment.
The problem is that our extension system currently can't handle NOT adding registers if they don't exist in the orignal pump setup. So with your example to the right, we will end up adding registers with only mapping data.
I'm not against the idea, but we need some system to make sure that does not end up creating partial register configuration for pumps without support for a given register.
There was a problem hiding this comment.
Understood! I will update this PR to remove the changes to existing objects in extensions.json and then add new objects for the registers that I found were missing mappings. Thanks for clarifying!
|
I just checked the average temperature on F series. It is a time based average there. Very likely the same on S series. But it is a quite a fast average so should be fine to use.
|
@yozik04 here are a bunch of more registers for S-series units plus my proposed changes to the extensions. If you agree with these changes I will run the CSV to JSON script and add the updated JSON files to this PR.