Codecov Report

Merging #61 (8f789e5) into master (308d2fa) will increase coverage by 36.82%. The diff coverage is 99.62%.

@@             Coverage Diff             @@
##           master      #61       +/-   ##
===========================================
+ Coverage   56.49%   93.31%   +36.82%     
===========================================
  Files           3        4        +1     
  Lines         331      374       +43     
===========================================
+ Hits          187      349      +162     
+ Misses        144       25      -119

Impacted Files	Coverage Δ
pysma/sensor.py	`98.86% <98.86%> (ø)`
pysma/__init__.py	`85.00% <100.00%> (+34.28%)`	:arrow_up:
pysma/const.py	`100.00% <100.00%> (ø)`
pysma/definitions.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b85cbd...8f789e5. Read the comment docs.

rklomp commented 3 years ago

@fitool I made some changes to the battery sensor naming. Could you check if you agree with the current names and list of sensors? And verify all sensors work using example.py? See f913ecfc43fa867c7252dcb71dec94c2211f30d1

rklomp commented 3 years ago

@kellerza Could your check if you agree with the changes in pysma/__init__.py? When @fitool has double checked the battery sensors I will merge this pull request and bump the version to 0.5.0.

I've got a change ready for Home Assistant as well. You can check the changes required here: https://github.com/home-assistant/core/compare/dev...rklomp:sma-patch-1

fitool commented 3 years ago

@rklomp I can check more in detail on Friday but already a first feedback:

what does the name 'grid' refer to: grid as seen by the meter at grid connection point or grid as seen by the inverter AC side? because these are different keys, both reported by the inverter in my case (as I have Sunny Home Manager 2.0 which exposes its values via inverter).
I added 'DC' in the naming to make the difference between AC and DC side of the inverter; e.g. I was thinking to do a custom sensor to calculate DC power and inverter efficiency (AC power divided by DC power); might be nice to include in the library?
regarding number of battery(inverter) related sensors: Sunny Boy Storage inverter has option to hook up 3 different batteries via dedicated inputs (A,B and C) but I think that in practice, most people use only 1 battery; so might be an idea to leave out B and C ?
to reduce even further, I'm only planning to use the ones highlighted in yellow

rklomp commented 3 years ago

@rklomp I can check more in detail on Friday but already a first feedback:

what does the name 'grid' refer to: grid as seen by the meter at grid connection point or grid as seen by the inverter AC side? because these are different keys, both reported by the inverter in my case (as I have Sunny Home Manager 2.0 which exposes its values via inverter).

I tried to use names as close to the names used by SMA as shown in https://github.com/rklomp/pysma/wiki/Sensor-Overview and https://github.com/SBFspot/SBFspot/blob/master/SBFspot/TagListEN-US.txt

SMA uses grid for sensors related to the grind connection point of the device Metering is used for sensors provided by Energy Meter

I added 'DC' in the naming to make the difference between AC and DC side of the inverter; e.g. I was thinking to do a custom sensor to calculate DC power and inverter efficiency (AC power divided by DC power); might be nice to include in the library?

I think now all battery_* sensors are DC related and grid_* sensors are AC related?

The library is only used to retreive data from the device. If you want to do calculations you could use Home Assistant's template platform

regarding number of battery(inverter) related sensors: Sunny Boy Storage inverter has option to hook up 3 different batteries via dedicated inputs (A,B and C) but I think that in practice, most people use only 1 battery; so might be an idea to leave out B and C ?

Lets keep all 3 in. Unless you know a reliable way to retreive information about how many batteries are connected. Unused battery inputs can be disabled in HA.

to reduce even further, I'm only planning to use the ones highlighted in yellow

I think we can reduce the amount of sensors a bit then. I think there are a few that do not provide any added value to have in HA.

fitool commented 3 years ago

@rklomp thanks for the clarifications, ok to keep 3 inputs for battery inverter, in SMA datasheets the option is there to hook up more than one battery, so indeed best to keep it then not all battery_* sensors are dc related, for example BATTERY_POWER and BATTERY_CHARGE is AC, but I think it's sufficiently clear with the naming we have now

I reviewed the file and sent you a proposal for modification via pull request; I notice some errors popped up while doing this due to errors with in-line comments on device_class so I removed the comments via two additional commits, just wanted to clarify that inverter = pv inverter and battery = battery inverter in SMA device classes

rklomp commented 3 years ago

@kellerza Would you be able to give this one a review? Otherwise I will merge it.

kellerza / pysma

Implement device sensor map #61

Codecov Report