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

Benni's SH register.yaml V2 #37

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

benni336
Copy link
Contributor

Hi @bohdan-s ,
this is my first pull request according to registers.yaml. You were right, that 90% has been merged.
I don't get why github showed me so many changes.

I want to point out line 217, which I think is a typo. At least to my knowledge and also referring to the Modbus documentation it should be "3P3L"

I also updated file version. I hope you are happy with that. I personally think that versions should not be written into files but only marked in git tags, but I guess that is also some kind of personal taste.

Last but not least I wanted to point out that some of the write registers are not applicable for SH series.

Please comment. I will create a new pull request for some bugfixes within the webserver.

@benni336 benni336 changed the title Bohdan s main Benni's SH register.yaml V2 Mar 22, 2022
Copy link
Owner

@bohdan-s bohdan-s left a comment

Choose a reason for hiding this comment

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

Can we leave 4990 and 5000 as level 3?
5000 is read on startup, and only needs to be read once.
If you want I can add the serial to script start as well. No need to read every cycle.
Why have you added the model lines? If there is no model line, then it is valid for all models. So only need to add that if we want to exclude a model.

@benni336
Copy link
Contributor Author

benni336 commented Mar 23, 2022

Hi,

SN
reading serial number and other basic and unchangable inverter information on script start up is a good idea, as long as they are cached and presented on the webserver export afterwards. I think that is the case but I am not sure if there is another check for the levels within the export part.
That's why I had moved them to level 2.

I think it is good value to read and display the inverter serial number e.g. for the web service, as it helps to identify the devices when viewing the data in browser. The SN is a true unique identifier. That's why I think they should be level two information. And that's why I think that information should be broadcasted with all other measurements.

If you change it to level 3 because of coding logic as it is only read once on startup and it does not affect any other feature I am fine with that. But following that logic, we could also move other basic information to that part such as: protocol number and version, arm_software_version, dsp_software_version and so on. On the other hand it would mean that we need a restart if this data changes (upgrading inverter firmware).

Long story short: If we really think about splitting data in to "scraped everytime" and "scraped fewer" we need to think about a small and big scraping cycle. I am happy to help with that ;)

Model lines

I guess you are referring to l 582, l 1580 and so on. According to my Modbus Communication documentation they are not part of SH-Inverters. E.g. register address 5006 is "reserved" same for the others. For SH inverters those addresses are not supported, so I added the whole bundle of non hybrid inverters. I hoped I didn't miss any and I didn't meant to break an existing config.

Best

[Addition:] I am going to set SN scraping to level 3 under the above named assumption.

@benni336
Copy link
Contributor Author

level updated

@benni336 benni336 requested a review from bohdan-s March 23, 2022 21:59
@benni336
Copy link
Contributor Author

In 13222f6 I have additionally adapted the register names to the standard naming.

I have adapted this locally already long time ago.
As only one load_power or export_power register is used (either hybrid or non hybrid) there are no conflicts. But using a standard layout for referencing the registers with the same meaning (even though they might have different addresses) has a great of advantage for compatibility.

@bohdan-s
Copy link
Owner

bohdan-s commented Jul 7, 2022

@benni336 can you have a look at fixing the conflicts?

@@ -1140,14 +1141,14 @@ registers:
accuracy: 0.1
unit: "kWh"
models: ["SH5K-20","SH3K6","SH4K6","SH5K-V13","SH5K-30","SH3K6-30","SH4K6-30","SH5.0RS","SH3.6RS","SH4.6RS","SH6.0RS","SH10RT","SH8.0RT","SH6.0RT","SH5.0RT"]
- name: "load_power_hybrid"
- name: "load_power"
Copy link
Collaborator

Choose a reason for hiding this comment

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

changing the name will break any existing queries/dashboards etc. that rely on it

@bohdan-s
Copy link
Owner

bohdan-s commented Apr 5, 2024

@benni336 could you have a look at this and resolve the conflicts when you get a chance? Trying to get through the backlog of the last year sorry.

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