search

juicejuice / homeassistant_redback

Home Assistant integration for inverter and battery systems from Redback Technologies
7 stars 2 forks source link

Use alternative method to find battery #19

Open Mark-Hetherington opened 8 months ago

Mark-Hetherington commented 8 months ago

This uses an alternative method to check if there is a battery by checking if there is a battery SoC data item in the dynamic energy data, in an attempt to work around #18

I have also contacted Redback in the hope that the API can be fixed instead, and this pull request would then be moot.

I'm not clear on how this would interact with the "private" API, or how many people this change might benefit.

juicejuice commented 8 months ago

Thanks @Mark-Hetherington. The problem here is that once "hasBattery" is true, then it will build out the entities for all the various battery sensors. This includes the "BatteryCapacitykWh" and "UsableBatteryCapacitykWh" values from StaticData -> SiteDetails. Does your system have usable data for those values in SiteDetails? If not, there may be HA errors due to it building sensors based on invalid or missing data.

Mark-Hetherington commented 8 months ago

No, I have null values for BatteryCapacitykWh and UsableBatteryCapacitykWh. 

{
  "Data": {
    "SiteId": "XXXXXXXXXXX",
    "Nmi": null,
    "SerialNumber": "RB171218XXXXXXXXX",
    "GenerationHardLimitVA": null,
    "GenerationSoftLimitVA": null,
    "ExportHardLimitkW": null,
    "ExportSoftLimitkW": null,
    "SiteExportLimitkW": null,
    "BatteryMaxChargePowerkW": 4.6,
    "BatteryMaxDischargePowerkW": 4.6,
    "BatteryCapacitykWh": null,
    "UsableBatteryCapacitykWh": null,
    "SystemType": "Hybrid",
    "InverterMaxExportPowerkW": 5,
    "InverterMaxImportPowerkW": 5,
    "MinSoC0to1": null,
    "MinOffgridSoC0to1": null
  }
}

However, HA seems to be working with the change, and with debug logging on I'm not seeing errors. There are some unknown values: image The Battery charge and discharge values are incorrect though. I'm new to Home Assistant, so I'm not sure where this information flows to - I don't know if the battery capacity sensor data is consumed anywhere.

Mark-Hetherington commented 8 months ago

I've had a bit more of a poke around in the API, and I can't find an alternative source for BatteryCapacitykWh/UsableBatteryCapacitykWh to use as a fallback. Hopefully Redback can resolve the incorrect data.

Another, potentially related, glitch I'm seeing is that the Battery Discharge Total (energy) doesn't seem to accumulate. It just follows the discharge power. image image

I'm hoping to dig into why this is the case, but I'm assuming it works correctly for people with a less exotic configuration?

GrantKeymer commented 8 months ago

@Mark-Hetherington -

Another, potentially related, glitch I'm seeing is that the Battery Discharge Total (energy) doesn't seem to accumulate. It just follows the discharge power.

I'm hoping to dig into why this is the case, but I'm assuming it works correctly for people with a less exotic configuration?

My Battery Discharge Total Sensor works the same as yours. It shows the incremental amount of kWh added per accumulation period (1 minute I think) and always records very small values as your graph shows.

When teamed up with the Energy Dashboard, it works correctly. I suggest you have a look at the charts in there, to fully understand how your system is behaving. Now I use the Energy Dashboard more than Redback's app, which is the opposite to what it was in the beginning.

Mark-Hetherington commented 8 months ago

In that case I may have set up my other battery incorrectly, as a cumulative total, although both seem to look plausible in the daily statistics in the energy module. I suppose the energy dashboard is smart enough to accept either a cumulative or interval based input. I guess I need to understand the energy manager better. Thanks for your help, and thanks again for the integration. image image

juicejuice commented 8 months ago

@Mark-Hetherington I think something has changed as the Battery Chage/Discharge Total sensors did used to work as you first expected, e.g., an accumulating total. The relevant code is at sensor.py, class RedbackEnergySensor. That is where it sets up the "TOTAL" sensor to accumulate battery charge/discharge readings. I simply followed the documentation here (last bullet point) which says to update the last_reset on every sensor reading from the Redback portal. That is meant to ensure the sensor accumulates correctly as we are converting an instantaneous power reading to an energy total amount over time. Thinking about it some more, perhaps the issue lies in this code: self._attr_last_reset = datetime.now() - timedelta(minutes=1) Should it really be subtracting a minute from the current datetime or just using the current datetime without adjustment? You could try remove subtraction and see if that fixes it? If you do find a fix please make the PR and I'll pull it straight in. Otherwise, I can look into it over the weekend. Like Grant suggested, it slipped my attention as it looks OK in the Energy Dashboard.

Mark-Hetherington commented 8 months ago

For comparison, the "Grid Export Total" presents as a cumulative value, However, it comes from the API already as a cumulative value. image

In the case of the Battery Discharge/Charge Energy, from what I can understand, it takes each instantaneous power value, assumes it applies to a whole minute (I don't know how regular the updates actually are). But it's state class is SensorStateClass.TOTAL. The way I read the documentation, the numbers from a SensorStateClass.TOTAL sensor shouldn't be representing the rate of change (power), but the total change (energy). Effectively this sensor is representing a scaled amount of an instantaneous value, and so it appears to me more an (entity not representing a total amount)[https://developers.home-assistant.io/docs/core/entity/sensor/#entities-not-representing-a-total-amount]? The thing that throws me off here is that the Energy Dashboard seems fine with it! If the total charge energy decreases, it is like we have turned back time! From my current understanding (and I'm new to HA so this could be quite mistaken), I think the sensor should be starting with an internal total = 0, and then adding the total for each sample, accounting for the time between samples, and never setting "last_reset". I think this effectively makes the RedbackEnergySensor give the same kind of output as a RedbackEnergyMeter, but derived from different input. I've drafted this up at https://github.com/Mark-Hetherington/homeassistant_redback/tree/cumulative-energy.

For my other system I'm using the Riemann sum integration to calculate kWh from Watts, based on the information here. It seems to work out from a sample of power and the time between them, how much energy is represented over that time, and then add the samples.

juicejuice commented 8 months ago

it appears to me more an (entity not representing a total amount)

In this case "entity" means what we want the sensor to represent, not what the data source represents. We want the sensor to represent a total amount, effectively mapping that instantaneous power reading source data to an accumulating energy total. It definitely used to work with last_reset, that's why the code is written that way. I know HA have been reworking statistics lately so it might be that we just need to fiddle a little to get it working again, hence my suggestion of removing the one minute subtraction. Regards the one minute thing: the Redback system itself contains a Ouija board, which is a Raspberry Pi running their own software. This is programmed to send MQTT packets to Redback every 60s with all relevant data points. Therefore, I wrote this integration to poll Redback API every 60s to gather the data. Gathering the instantaneous power data every 60s this way can be used to closely approximate the actual energy values through averaging over the 60s period. It's obviously not perfect, but it was working for those two sensors where the Redback API does not supply the energy total values. EDIT: anyway, do let us know how you get along. I'd like to sort this out.

Mark-Hetherington commented 8 months ago

Yes, agreed, I think the sensor should represent a total amount, so should accumulate. My quote above was how I understood what the current implementation was doing, rather than describing what it should be doing.

The one minute thing makes sense, with the knowledge that the ouija board updates once a minute. I did see a reference to that in the code. I suppose the part I am less sure of is how often the integration runs the update - I didn't spot that.

The way I have understood it, I think the last_reset is supposed to be used when the source itself resets to zero. However, working trumps what the docs (or my naïve interpretation of them) says!

I'm running my test branch so far with no ill effects, however deployed it when the battery was fully charged and it didn't show any energy flows. We've just started drawing from battery, and it's not quite behaving how I expected.

Having another glance, I've just seen a rather obvious bug....

Mark-Hetherington commented 8 months ago

I now have accumulating kWh. image

I don't know how close to reality the numbers are - I'll run it for a while to see.

Mark-Hetherington commented 8 months ago

Over the weekend the numbers seemed to roughly match what I'd expect from the changes in SoC and the nominal battery capacity. From the redback portal, we added around 79%: image Which took the cumulative charge from 0.6 kWh to 7.3 kWh. image That's a 6.7 kWh added for 79% of a nominal 10 kWh, 8.5 kWh usable battery, which should be 8.5 * 0.79 = 6.715 kWh. So it looks spot on. I'll create a pull request.

juicejuice commented 8 months ago

So it looks spot on. I'll create a pull request.

Nice one! I'll test out your PR here and merge it in.

juicejuice commented 8 months ago

So it looks spot on.

Yes it all checked out here too during my testing, so I've merged the PR. Thanks! Regarding the alternative way to detect a battery, I'm a bit concerned that we only do that if we can get access to all the required data points from the API, and it sounds like that isn't the case.

Mark-Hetherington commented 8 months ago

Yes, I can understand that. However, I think it depends on how "required" is defined. My battery capacity shows as unknown, but I don't know if that affects anything else? I can see how the battery capacity would be useful for a system (e.g. EMHASS) that is planning to orchestrate the charge and discharge of the battery in conjunction with other loads. Is it used by the energy dashboard itself?

I have not yet heard back from Redback on correcting this on their side.

I have just thought of an alternative. Perhaps these values could be added as configuration inputs. If no battery is detected, the user could be prompted to confirm. "No battery detected. Do you have a battery connected", and then enter the figures. However that adds some complexity to the onboarding process, and this could be an issue that only affects me!

I was thinking it was only the static data (BatteryCapacitykWh/UsableBatteryCapacitykWh) that was missing, but I've just scanned over the data in HA and it looks like site load is also unknown. That could be unrelated so I might take a look and try to figure it out. It will be wrong in my case anyway, since the Redback is unaware of the other solar and battery systems on site, but I'm curious.

juicejuice commented 8 months ago

it looks like site load is also unknown.

Right, so that's a good example. Site Load is defined in sensor.py as a calculation: PvPowerInstantaneouskW + BatteryPowerNegativeIsChargingkW - ActiveExportedPowerInstantaneouskW + ActiveImportedPowerInstantaneouskW.

I know your StaticData is bad, but what data points are actually available in your DynamicData? Can you share a (sanitised) example?

Mark-Hetherington commented 8 months ago

Sorry, it's the Site Load Total, not the Site Load that is unknown. image

But this is not a data point I expect to be unknown, based on the missing static data. All the dynamic data appears to there for Site Load, but the LoadAllTimeEnergykWh is not! Here is a current same of dynamic data:

  "Data": {
    "TimestampUtc": "2023-12-18T22:23:31.6527514Z",
    "SiteId": "X000x00x0x00NSW",
    "Phases": [
      {
        "Id": "A",
        "ActiveExportedPowerInstantaneouskW": 0,
        "ActiveImportedPowerInstantaneouskW": 3.548,
        "VoltageInstantaneousV": 240.2,
        "CurrentInstantaneousA": -4.3,
        "PowerFactorInstantaneousMinus1to1": -0.99
      }
    ],
    "FrequencyInstantaneousHz": 49.95,
    "BatterySoCInstantaneous0to1": 0.18,
    "PvPowerInstantaneouskW": 0.984,
    "InverterTemperatureC": 38.5,
    "BatteryPowerNegativeIsChargingkW": 0.025,
    "PvAllTimeEnergykWh": 2993.517,
    "ExportAllTimeEnergykWh": 1703.545,
    "ImportAllTimeEnergykWh": 2238.474,
    "LoadAllTimeEnergykWh": null,
    "Status": "OK",
    "Inverters": [
      {
        "SerialNumber": "RB00000000000000",
        "PowerMode": {
          "InverterMode": "Auto",
          "PowerW": 0
        }
      }
    ]
  },
  "Metadata": null
}

In theory the load total can be calculated from PvAllTimeEnergykWh + ImportAllTimeEnergykWh - ExportAllTimeEnergykWh + BatteryDischargeTotalkWh. But there is no BatteryDischargeTotal in the data.

I assume LoadAllTimeEnergykWh is null because of my atypical set up. So I think I'm going back to Redback for a remedy on this one as well.

juicejuice commented 8 months ago

Interesting! It looks good and you have all the same DynamicData as my system except for the missing LoadAllTimeEnergykWh value. It definitely feels like a configuration issue; I can confirm none of the other users have had this to date and have not had missing StaticData for their batteries either. Anyway, I guess the "unknown" state for Site Load Total is a true reflection as with a null value in the source data the measurement remains unknown!

juicejuice commented 8 months ago

OK I've had a quick look to compare your system against other sample data I've received:

If we switch to your alternative battery identification method, then I think we're cool except for the fact that your BatteryCapacitykWh / UsableBatteryCapacitykWh will be "unknown" values. Other systems with or without batteries should not be affected by this change.

Final question, is there a reason you picked BatteryPowerNegativeIsChargingkW instead of BatterySoCInstantaneous0to1 as the datapoint for battery availability?

Mark-Hetherington commented 8 months ago

Thanks for the prompt response. I'm not sure I actually want this merged, at least not until we see if Redback can resolve the data for my system. The original code is nicer and makes more sense.

But nonetheless, my original thinking was the kW reading was more important because, well it's the data we want to use.

I actually switched from BatteryPowerNegativeIsChargingkW to BatterySoCInstantaneous0to1 in 29612ae. My thinking was that the kW reading could be zero, which is falsy. I'm comparing to None so it shouldn't matter, but I just figured the SoC is more likely to not be zero. and might indicate there is a battery even if there is not currently power information.

I could be persuaded either way, and I'm not sure it matters what value is used.

One interesting thing I note in your data samples, is the other systems without a battery list the BatteryCapacitykWh as zero instead of null.