ksheumaker / homeassistant-apsystems_ecur

Home Assistant custom component for local querying of APSystems ECU-R Solar System
Apache License 2.0
166 stars 42 forks source link

Voltage assignment issue in process_yc1000() #30

Closed gaardiolor closed 3 months ago

gaardiolor commented 2 years ago

Hi,

Didn't test myself yet, but In APSystemsECUR.py, in the function process_yc1000(), the voltage variable is assigned 3 times, of which 2 times unused. Doesn't look logical, I think it should be voltages.append() . Also I think it should look for 4 voltages instead of 3.

Original:

    def process_yc1000(self, data, location):

        power = []
        voltages = []

        power.append(self.aps_int(data, location))
        location += 2

        voltage = self.aps_int(data, location)
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        voltage = self.aps_int(data, location)
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        voltage = self.aps_int(data, location)
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        voltages.append(voltage)

        output = {
            "model" : "YC1000",
            "channel_qty" : 4,
            "power" : power,
            "voltage" : voltages
        }

        return (output, location)

Didn't test yet, but it'd make more sense if it looked like this:

    def process_yc1000(self, data, location):

        power = []
        voltages = []

        for i in range(0, 4):
            power.append(self.aps_int(data, location))
            location += 2

            voltages.append(self.aps_int(data, location))
            location += 2

        output = {
            "model": "YC1000",
            "channel_qty": 4,
            "power": power,
            "voltage": voltages
        }

        return output, location
ksheumaker commented 2 years ago

You are correct that doesn't make much sense, and I should be appending those into the voltage array.

I agree it's odd there are only 3 voltages, but I think the YC100 is a 3 phase inverter, so you aren't getting voltage from each channel, but voltage from each AC power phase: https://usa.apsystems.com/wp-content/uploads/2017/07/APsystems-YC1000-Datasheet-7.20.17.pdf

On my system which all QS1s I only get 1 voltage, which is from the AC side, not the DC voltage of each panel.

ksheumaker commented 2 years ago

Currently the integration only exposes one voltage anyway the first one in that array.

        if self._field == "voltage":
            return self.coordinator.data.get("inverters", {}).get(self._uid, {}).get("voltage", [])[0]
gaardiolor commented 2 years ago

Aha, ok. I looked at process_yc600() for comparison. It appends 2 voltages, but it's a one phase inverter. So I figured those voltages must be the voltage per panel, since the YC600 supports 2 panels.. ?

ksheumaker commented 2 years ago

I think they've changed what the report over time, like I said my QS1 only shows one, and it's over 200V all the time, so I'm pretty sure it's not panel voltage.

gaardiolor commented 2 years ago

Ah yes, you're right, I've updated the code to only read 3 voltages.

It lists the first voltage as 0 though, on all 4 YC1000's. Any known issue reading this data, or should I check with my installer to verify that they've correctly connected all 3 phases ?

{'uid': '<snip>', 
'online': True, 
'unknown': '02', 
'frequency': 0.0, 
'temperature': 0, 
'signal': 0, 
'model': 'YC1000', 
'channel_qty': 4, 
'power': [0, 0, 0, 0], 
'voltage': [0, 220, 219]}   # <--- first voltage is 0
ksheumaker commented 2 years ago

I'm not sure about power where you are from. In the US, we don't use 3 phase power except at larger commercial/industrial buildings. So if I had a YC1000 on my home I'd see something similar where one phase would be 0. Do you know if you have 3 phase power where these panels are installed?

I can certainly make the code report multiple voltages for the inverters that support that, but I need to be careful not to break existing implementations (currently it's always just one). So there might need be sensors like this:

inverter_XXXXX_voltage
inverter_XXXXX_voltage_1
inverter_XXXXX_voltage_2

I wouldn't want to remove or rename inverter_XXXXX_voltage since it already exists for everyone else.

tv3 commented 2 years ago

I've got 3 phase connection at home with yc1000 inverters. I will check what I find this weekend.

gaardiolor commented 2 years ago

Thanks for your quick answers. I'm living in the Netherlands, I've got 3 phase power. All 3 phases are working normally, I've got my electrical equipment balanced across those 3 phases and everything is working. I'm also querying each phase from my smart electricity meter, all 3 phases are reporting back the voltage, also looks good. So I'm guessing either something is wrong with the solar installation, or it's a bug in the yc1000 reporting.

What you could do in theory, is to average out the 3 voltages in inverter_XXXXX_voltage , and list them separately in inverter_XXXXX_voltage_1, inverter_XXXXX_voltage_2, inverter_XXXXX_voltage_3. In my case the '0' would be averaged out as well though. Either my installation is faulty and it doesn't matter (good indicator that something is wrong), or there's a bug in the data in which case you could just get the max() instead of the average. I'll check and report back.

Can't find the 3 separate voltages in the ECUAPP unfortunately.. would be a good cross-check. In the ECUAPP > Data > Real Time Data, all 4 YC1000's are showing no data at all, while the script is working perfectly. The ECUAPP doesn't seem to be very reliable.

gaardiolor commented 2 years ago

Now that there's some daylight, I've got 3 voltages. And the ECUAPP is reporting data. Temperature, frequency and signal are reporting back now as well (all reported back 0 yesterday). Possibly the YC1000's go into standby at night, shutting down one phase and not reporting back properly to ECUAPP to save power. This looks good now.

As a consideration, it would be nice if you could add those 3 separate voltages. The existing field could perhaps be adjusted to the mean of the non-0 voltages, inverter_XXXXX_voltage = sum(voltage) / len([v for v in voltage if v > 0])

Back to the original issue, my process_yc1000() now looks like this:

    def process_yc1000(self, data, location):
        power = []
        voltages = []

        power.append(self.aps_int(data, location))
        location += 2

        voltages.append(self.aps_int(data, location))
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        voltages.append(self.aps_int(data, location))
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        voltages.append(self.aps_int(data, location))
        location += 2

        power.append(self.aps_int(data, location))
        location += 2

        output = {
            "model": "YC1000",
            "channel_qty": 4,
            "power": power,
            "voltage": voltages
        }

        return output, location
ksheumaker commented 2 years ago

Ok, I'll work on an update that handles the voltages better for the yc1000 - thanks for the info.

HAEdwin commented 5 months ago

@gaardiolor I think this issue was taken care of some releases ago, is it allright to close it? The Voltages indeed represent the AC voltages on the phases not the individual DC voltages on the panels.