kellerza / pysma

Async library for SMA Solar's WebConnect interface
MIT License
59 stars 51 forks source link

Added support for new Tripower SE #107

Closed FindusK closed 1 year ago

FindusK commented 2 years ago

This Pull-Request adds support for the new Tripower SE by adding a detection for the additional battery

It also contains fixes for skipped tests because of missing async-decorators.

This closes #105.

@rklomp can you perform a few tests with your inverter and get sure that the library doesn't accidentally detect a additional battery. Thanks.

rklomp commented 2 years ago

Thanks! I will try to review and test this in the coming days.

FindusK commented 2 years ago

Oh (a few of the steps in the workflow failed), I will add another commit and fix the errors from the style-checks for the code

FindusK commented 1 year ago

I set up github-actions in my forked repository. I think all tests should work now: https://github.com/FindusK/pysma/actions/runs/2889528750

codecov[bot] commented 1 year ago

Codecov Report

Merging #107 (2e697c7) into master (50b26a9) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #107   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          428       436    +8     
=========================================
+ Hits           428       436    +8     
Impacted Files Coverage Δ
pysma/definitions.py 100.00% <ø> (ø)
pysma/__init__.py 100.00% <100.00%> (ø)
pysma/const.py 100.00% <100.00%> (ø)
pysma/sensor.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rklomp commented 1 year ago

@FindusK. Thanks for your work on this!

Instead of the

            elif devclass_keys[0] == "9":
                self._devclass = DEVCLASS_INVERTER

and

JMESPATH_VAL_TRISE = '"9"[0].val'
JMESPATH_VAL_TRISE_TAG = '"9"[0].val[0].tag'

Maybe we could restructure the sensor_map in definitions.py to be something like this:

SENSORS_INVERTER: [
    status,
    pv_power_a,
    pv_power_b,
    ...
]
SENSORS_BATTERY: [
    battery_voltage_a,
    battery_voltage_b,
    ...
]

sensor_map = {
     DEVCLASS_INVERTER: SENSORS_INVERTER,
     DEVCLASS_INVERTER_TRI: SENSORS_INVERTER,
     DEVCLASS_BATTERY: SENSORS_BATTERY,
}

In const.py

DEVCLASS_INVERTER_TRI = "9"

This way we don't need the extra jmespath queries.

FindusK commented 1 year ago

Ok, good idea.

I will restructure the inverter into a new device class. Instead of sharing the sensor-definitions, I could also write a new device class, that fits the new inverter perfectly. (The Tripower SE, doesn't have all sensors, like a third photovoltaic string) @rklomp What do you think about this?

How would the reading of the values work without the extra jmespath? Because the values are inside a new path in the Tripower SE. For example: "6180_104A9A00": { "9": [ { "val": "192.168.2.106" } ] },

rklomp commented 1 year ago

That would also be a good solution to me.

Regarding the jmespath. The first {} will be replaced by the devclass. So 9 in this case. The second {} will be replaced by the idx of the sensor. So 0 in most cases, and 1 for for example the second string of solar panels.

So bases on that the jmespath is exactly the same.

JMESPATH_VAL_IDX = '"{}"[{}].val'
JMESPATH_VAL_IDX_TAG = JMESPATH_VAL_IDX + "[0].tag"
JMESPATH_VAL_TRISE = '"9"[0].val'
JMESPATH_VAL_TRISE_TAG = '"9"[0].val[0].tag'

The _0 and _1 in the key is the idx. If not provided it will default to 0.

#: Current voltage generated by the solar panels (A side) 
 pv_voltage_a = Sensor( 
     "6380_40451F00_0", "pv_voltage_a", unit="V", factor=100, enabled=False 
 ) 
 #: Current voltage generated by the solar panels (B side) 
 pv_voltage_b = Sensor( 
     "6380_40451F00_1", "pv_voltage_b", unit="V", factor=100, enabled=False 
 )
rklomp commented 1 year ago

I have updated the config for pytest to have asyncio_mode=auto. See #108. So you can remove all @pytest.mark.asyncio statements from this pull request.

FindusK commented 1 year ago

Ok 👍 then I remove the test-decorators

FindusK commented 1 year ago

I have the new version nearly finished. Only these 6 sensor aren't working:

DEBUG:pysma.sensor:Sensor pv_power_a: No successful value decoded yet: {'1': [{'val': 1539}, {'val': 3423}]}
DEBUG:pysma.sensor:Sensor pv_power_b: No successful value decoded yet: {'1': [{'val': 1539}, {'val': 3423}]}
DEBUG:pysma.sensor:Sensor pv_voltage_a: No successful value decoded yet: {'1': [{'val': 34860}, {'val': 62790}]}
DEBUG:pysma.sensor:Sensor pv_voltage_b: No successful value decoded yet: {'1': [{'val': 34860}, {'val': 62790}]}
DEBUG:pysma.sensor:Sensor pv_current_a: No successful value decoded yet: {'1': [{'val': 4414}, {'val': 5451}]}
DEBUG:pysma.sensor:Sensor pv_current_b: No successful value decoded yet: {'1': [{'val': 4414}, {'val': 5451}]}

All other sensor are behind a 9 like daily_yield: Will be decoded with "9"[0].val from {'9': [{'val': 69168}]} and can be decoded.

rklomp commented 1 year ago

Interesting. So the device is using two different IDs? Can you share the output of a POST request to /dyn/getAllOnlValues.json?

Maybe we could change JMESPATH_VAL_IDX to be *[{}].val. So using a wildcard instead of the "{}" that will be replaced by devclass. Not sure if this gives any side effects though...

Raouzer commented 1 year ago

Hi,

I also have a SMA SUNNY TRIPOWER (8.0 SE) si if you need another tester, i can help you . :)

Do you have an idea when theses changes wil be apply on SMA Home assitant integration ?

regards

FindusK commented 1 year ago

Hi together,

I attached the output of a POST-request as a txt-file: getAllOnlValues.json.txt

@rklomp I will also commit my current state of the code.

@Raouzer normally the update of an integration can be done quite fast, but it normally takes 1-2 months till it's part of the next release. If you want to test a new version, you can also manually install a modified version of the library into your home-assistant (the library will be overwritten in the next update).

FindusK commented 1 year ago

the tests currently fail because there are overlaps between the sensor definitions in the sensor_map (the sensors are part of the normal INVERTER device-class as well as INVERTER_TRI)

the failing test-function is test_sensor_map in _testdefinitions.py

rklomp commented 1 year ago

I have made some minor changes and added a test to cover the new code. @FindusK, Can you test this?

We only need to fix the strange thing that your inverter is using both "9" and "1" as deviceclass. A possible fix is adding the following at line 69 in sensor.py as a kind of fallback to devclass "1":

                    JMESPATH_VAL_IDX.format(DEVCLASS_INVERTER, self.key_idx),

So would look like this:

            # Try different methods until we can decode...
            _paths = (
                [sens_path.format(devclass, self.key_idx) for sens_path in self.path]
                if isinstance(self.path, (list, tuple))
                else [
                    JMESPATH_VAL,
                    JMESPATH_VAL_STR.format(self.key_idx),
                    JMESPATH_VAL_IDX.format(devclass, self.key_idx),
                    JMESPATH_VAL_IDX.format(DEVCLASS_INVERTER, self.key_idx),
                ]
            )

Might not be the nicest solution, but I guess it will work.

rklomp commented 1 year ago

Ive implemented a wildcard jmespath query in 77a024b

This might be a better solution compared to adding the extra line of code, but not sure if this gives any side effects, so it should be tested.

rklomp commented 1 year ago

I have been working on a completely dynamic way of finding the sensors that are present on a device. Completely getting rid of the device class principle.

Created a separate pull request to keep it clear. Check out #109!

FindusK commented 1 year ago

Good idea tell me when I should test something on my inverter

Toooias commented 1 year ago

Hi together,

I attached the output of a POST-request as a txt-file: getAllOnlValues.json.txt

@rklomp I will also commit my current state of the code.

@Raouzer normally the update of an integration can be done quite fast, but it normally takes 1-2 months till it's part of the next release. If you want to test a new version, you can also manually install a modified version of the library into your home-assistant (the library will be overwritten in the next update).

@FindusK , I also have a Tripower 10 SE and I am still very new to github and Home Assistant. Is there a tutorial on how to install a modified library in advance?

rklomp commented 1 year ago

Good idea tell me when I should test something on my inverter

You can checkout my dynamic-sensors branch from https://github.com/rklomp/pysma/tree/dynamic-sensors Run the example.py and see if all required sensors are displayed.

FindusK commented 1 year ago

I removed the sensors that will be added later on (I will commit it on monday evening):

So the current solution mostly works, but there are many additional sensors that aren't present in the Inverter The only problem are the array-based sensors of the inverter like pv_voltage_b, for these it can't detect the second sensor:

              pv_power_a           1428 W
               pv_power_b              0 W
             pv_voltage_a          367.1 V
             pv_voltage_b
             pv_current_a          3.889 A
             pv_current_b
                 power_l1           1235 W
                 power_l2           1224 W
                 power_l3           1231 W
              total_yield       2651.563 kWh
              daily_yield           5417 Wh
             pv_gen_meter       2651.233 kWh
  metering_power_supplied           3225 W
  metering_power_absorbed              0 W
       metering_frequency           50.0 Hz
     metering_total_yield       1520.899 kWh
  metering_total_absorbed        238.847 kWh
      metering_current_l1          5.065 A
      metering_current_l2          3.891 A
      metering_current_l3          5.095 A
      metering_voltage_l1         236.13 V
      metering_voltage_l2         235.38 V
      metering_voltage_l3         236.46 V
metering_active_power_feed_l1           1186 W
metering_active_power_feed_l2            841 W
metering_active_power_feed_l3           1197 W
metering_active_power_draw_l1              0 W
metering_active_power_draw_l2              0 W
metering_active_power_draw_l3              0 W
metering_current_consumption
metering_total_consumption
        battery_voltage_a            0.0 V
        battery_voltage_b
        battery_voltage_c
battery_charging_voltage_a
battery_charging_voltage_b
battery_charging_voltage_c
        battery_current_a            0.0 A
        battery_current_b
        battery_current_c
     inverter_power_limit          10000 W
battery_power_charge_total              0 W
   battery_power_charge_a
   battery_power_charge_b
   battery_power_charge_c
battery_power_discharge_total              0 W
battery_power_discharge_a
battery_power_discharge_b
battery_power_discharge_c
      grid_reactive_power              0 var
   grid_reactive_power_l1             -3 var
   grid_reactive_power_l2             -3 var
   grid_reactive_power_l3             -3 var
      grid_apparent_power           3690 VA
   grid_apparent_power_l1           1230 VA
   grid_apparent_power_l2           1230 VA
   grid_apparent_power_l3           1230 VA
        grid_power_factor            0.0
grid_power_factor_excitation
     battery_charge_total            0.0 kWh
         battery_charge_a
         battery_charge_b
         battery_charge_c
  battery_discharge_total            0.0 kWh
      battery_discharge_a
      battery_discharge_b
      battery_discharge_c
        battery_soc_total              0 %
            battery_soc_a
            battery_soc_b
            battery_soc_c
   battery_capacity_total              0 %
       battery_capacity_a
       battery_capacity_b
       battery_capacity_c
           battery_temp_a            0.0 °C
           battery_temp_b
           battery_temp_c
insulation_residual_current
       inverter_condition             Ok
 operating_status_general      Activated
battery_status_operating_mode            Off
        grid_relay_status         Closed
   grid_connection_status
     inverter_system_init            Yes
               grid_power           3690 W
               voltage_l1          238.0 V
               voltage_l2          236.1 V
               voltage_l3          238.0 V
               current_l1            5.1 A
               current_l2            5.1 A
               current_l3            5.1 A
            current_total           15.3 A
                frequency          50.01 Hz
                   status             Ok
FindusK commented 1 year ago

@Toooias For simple testing you can just download my fork of the repository (Link) and run the example.py: python example.py user <password>

If you want to test it in your Home-Assistent installation, you can switch the library bundled with the installation for a custom one: Home-Assistent has a site-packages folder, in there are all libraries the exact location depends on the Home-Assistent variant you have installed, for example in some docker variants it's located in /usr/local/lib/python3.9/site-packages/ how to replace it in docker: https://community.home-assistant.io/t/location-of-usr-local-lib-python3-9-site-packages-for-ha-core-docker-version/379139

There you can replace the files in the pysma folder with ones you downloaded Then you need to restart Home-Assistent and the integration should work

for debugging you can add

logger:
  default: info
  logs:
    homeassistant.components.sma: debug
    pysma: debug

to your confiuration.yaml

rklomp commented 1 year ago

I removed the sensors that will be added later on (I will commit it on monday evening):

So the current solution mostly works, but there are many additional sensors that aren't present in the Inverter The only problem are the array-based sensors of the inverter like pv_voltage_b, for these it can't detect the second sensor:

If you use my for in #109 there is no need for any changes here. As it builds on top of your changes.

Can you test it and see how it compares to this output in which sensors you are getting?

FindusK commented 1 year ago

ok sure

Toooias commented 1 year ago

@FindusK

Thank you for the tutorial. I wanted to test it directly in Home Assistant, but I just can't find the directory.

I use the Home Assistant Operating System under Synolgy VMM (VMware ESXi/vSphere (.ova)).

The Find command does not give any results

rklomp commented 1 year ago

Superseded by #109