pbix / HA-solark-PV

Home Assistant integration for the SolArk PV Inverter
Apache License 2.0
36 stars 9 forks source link

Add daily and total load energy #11

Closed poldim closed 2 years ago

poldim commented 2 years ago

Wondering if you can add daily and cumulative load energy to the polled data. I'm only guessing at what to do here, so it might be wrong/need more work. image

pbix commented 2 years ago

Thanks for the pull request. I have a couple of questions for you one related to the naming confusion previously discussed.

1) I have nothing connected to the critical load breaker in my setup so I cannot test this, I think you do. Can you confirm that these two values are the power/energy flowing through this "load" breaker and that you tested it? Please put a comment in the code in hub.py that this is the power through the breaker labeled "Load" on the inverter. I need to so comment all these values as step one in resolving the name confusion.

2) You have used two state classes STATE_TOTAL and STATE_TOTAL_INCREASING. The best reference I have seen for these is here https://github.com/home-assistant/developers.home-assistant/blob/master/docs/core/entity/sensor.md It does not look to me that you have these correct. I suspect that they should both be STATE_TOTAL_INCREASING because they seem to be "uints" which means there is no way they can go negative.

3) For the 32bit value should use the uint32 version of the decode function decode_32bit_uint.

What do you think?

poldim commented 2 years ago

Thanks for the pull request. I have a couple of questions for you one related to the naming confusion previously discussed.

  1. I have nothing connected to the critical load breaker in my setup so I cannot test this, I think you do. Can you confirm that these two values are the power/energy flowing through this "load" breaker and that you tested it? Please put a comment in the code in hub.py that this is the power through the breaker labeled "Load" on the inverter. I need to so comment all these values as step one in resolving the name confusion.

Yes, most of my home is on load breaker, so that's easily confirmed. I'll add the comment.

energy registers:

image

power registers:

image

frequency register:

image
  1. You have used two state classes STATE_TOTAL and STATE_TOTAL_INCREASING. The best reference I have seen for these is here https://github.com/home-assistant/developers.home-assistant/blob/master/docs/core/entity/sensor.md It does not look to me that you have these correct. I suspect that they should both be STATE_TOTAL_INCREASING because they seem to be "uints" which means there is no way they can go negative.

84 is daily load energy so it gets reset to 0 every day at midnight while 85 is cumulative energy usage and I imagine it wont reset to 0 unless you factory reset the inverter. I've seen HA send out errors if it sees a sensor labeled as STATE_TOTAL_INCREASING that resets so I thought keeping it as STATE_TOTAL will eliminate that error. Alternatively, it could be set as MEASUREMENT instead?

  1. For the 32bit value should use the uint32 version of the decode function decode_32bit_uint.

thanks fixed

pbix commented 2 years ago

After further review I think what you have now as STATE_TOTAL_INCREASING for your daily energy value and STATE_TOTAL for your lifetime load energy value is consistent with all the other such values in the integration today. I have not seen HA throw errors recently on these other sensors so am hopeful these new ones will work well.

If you use MEASUREMENT I am not thinking such a sensor could be used with the ENERGY function native to HA. Today I am using that with my other daily energy values with STATE_TOTAL_INCREASING and they seem to be working OK.

I am going to merge what you submitted. Thanks for your work on this. I fail to have enough time to devote so any help from others is appreciated.

poldim commented 2 years ago

After further review I think what you have now as STATE_TOTAL_INCREASING for your daily energy value and STATE_TOTAL for your lifetime load energy value is consistent with all the other such values in the integration today. I have not seen HA throw errors recently on these other sensors so am hopeful these new ones will work well.

Shit, you're right. I had these backward to what I wanted to do. I just updated to your latest release, so we'll see what kind of things I get in the logs.

image image

Icons might need some tweaking overall but looks good.

If you use MEASUREMENT I am not thinking such a sensor could be used with the ENERGY function native to HA. Today I am using that with my other daily energy values with STATE_TOTAL_INCREASING and they seem to be working OK.

I didn't realize this limitation but I'm also not the biggest fan of the dashboard they've made so I don't really use it. But no sense in making these sensors not compatible so I agree with not making them measurements.

I am going to merge what you submitted. Thanks for your work on this. I fail to have enough time to devote so any help from others is appreciated.

Woo hoo, my first real contribution to a gh project! :-P