Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong offset of Battery temperature sensor in single_phase.py for Deye #353

Open
OwlBawl opened this issue Oct 13, 2024 · 19 comments · Fixed by maslyankov/sunsynk#15
Open

Comments

@OwlBawl
Copy link

OwlBawl commented Oct 13, 2024

Your environment

  • Home Assistant version: last
  • Addon:
    • Name: multi
    • Version: 0.7.4
    • Inverter:
    • Make: Deye
    • Model: SUN 6K SG03 LP1 EU
    • Firmware: 9.0.2.6
  • Adaptor details:
    • RS485

Describe the issue/bug and what you expect

Wrong offset of sensor I think

##########

Battery

##########
SENSORS += (
TempSensor(182, "Battery temperature", CELSIUS, 0.1),

in /single_phase.py
(https://github.com/kellerza/sunsynk/tree/main/src/sunsynk/definitions)

Add-on shows 75C
while Solarman shows 25
C (correct temp) using the same register [0x00B6] with offset: 1000
I checked the yaml file to be sure.

From Deye ModBus description file:
182 - battery temperature - R [0,3000] - 0.1 - ℃
Real value of offset + 1000
(example 1200 is 20.0 ℃)

@OwlBawl
Copy link
Author

OwlBawl commented Oct 14, 2024

This shows the correct temp for register (made custom sensor while)

from sunsynk import AMPS, CELSIUS, KWH, VOLT, WATT
from sunsynk.rwsensors import NumberRWSensor, SelectRWSensor, TimeRWSensor
from sunsynk.sensors import (
    MathSensor,
    Sensor,
    SensorDefinitions,
    TempSensor,
)

SENSORS = SensorDefinitions()

from sunsynk.sensors import Sensor

class BatteryTempSensor(Sensor):
    def __init__(self, register, name, unit):
        super().__init__(register, name, unit, 0.1)

    def decode(self, value):
        # Adjust the value by subtracting 1000 and apply the scaling
        return (value - 1000) * 0.1

SENSORS += BatteryTempSensor(182, "Battery Temp", CELSIUS)

@kellerza
Copy link
Owner

Do all the temperature sensors work like this?
There is no generic offset handler in the sensors today, so will have to use a class like this

@OwlBawl
Copy link
Author

OwlBawl commented Oct 14, 2024

No, DC and Radiator temperatures readings are correct, only battery has this offset...

@kellerza
Copy link
Owner

@OwlBawl
Copy link
Author

OwlBawl commented Oct 16, 2024

If I understood correctly - installed Sunsynk/Deye Inverter Add-on (edge/dev) for test
That's what i've got:
image
real temp was 23*C

@OwlBawl
Copy link
Author

OwlBawl commented Oct 17, 2024

seems the value offset doubled with sensors.py main file:

class TempSensor(Sensor):
    """Offset by 100 for temperature."""

    offset: int = 100

    def reg_to_value(self, regs: RegType) -> ValType:
        """Decode the temperature (offset)."""
        try:
            val = regs[0]
            return int_round((float(val) * abs(self.factor)) - self.offset)  # type: ignore
        except (TypeError, ValueError) as err:
            _LOGGER.error("Could not decode temperature: %s", err)
        return None

@kellerza
Copy link
Owner

That's what i've got:
image
real temp was 23*C

From this the offset must be 0

class BatteryTempSensor(Sensor):
def init(self, register, name, unit):
super().init(register, name, unit, 0.1)

def decode(self, value):
    # Adjust the value by subtracting 1000 and apply the scaling
    return (value - 1000) * 0.1

SENSORS += BatteryTempSensor(182, "Battery Temp", CELSIUS)

This also results in an offset of 0 - You inherit from Sensor and then create a new method decode. decode is never called (maybe you were trying to override reg_to_value?)

So this custom Sensor should give you the same result:

SENSORS += Sensor(182, "Battery Temp", CELSIUS, 0.1)

@OwlBawl
Copy link
Author

OwlBawl commented Oct 18, 2024

Didn't get what you mean... My custom sensor I wrote few messages earlier is working correct. My last message was about your "dev" add-on test with new temp sensor... that showed wrong temp... and code was from addon sensors.py
But I'm not an expert and do not code, so maybe I was wrong.
Anyway.. the battery temp sensor still incorrect (as on last screen) in main and dev addon

@OwlBawl
Copy link
Author

OwlBawl commented Oct 18, 2024

Battery temp sensor shows
Multi add-on -78,0 °C
Dev add-on - 978 °C
While 22 °C in real

@OwlBawl
Copy link
Author

OwlBawl commented Oct 18, 2024

made custom
SENSORS += Sensor(182, "Battery Test", CELSIUS, 0.1)
and.... it shows 22 °C
multi add-on

While integrated sensor
image

@kellerza
Copy link
Owner

SENSORS += Sensor(182, "Battery Test", CELSIUS, 0.1)
and.... it shows 22 °C

This shows the offset is 0 😉

@OwlBawl
Copy link
Author

OwlBawl commented Oct 19, 2024

So the integrated sensor has extra offset?
Could fix the add-on now?

@OwlBawl
Copy link
Author

OwlBawl commented Nov 7, 2024

any plans to fix Battery temp sensor for Deye? Showing wrong data
image

While custom sensor
SENSORS += Sensor(182, "Battery Temp", CELSIUS, 0.1)
shows correct.

@maslyankov
Copy link
Contributor

182

What he means is that by having this definition:

SENSORS += Sensor(182, "Battery Temp", CELSIUS, 0.1)

You are effectively not setting any offset to the value. In deye modbus docs, it states that the "battery temperature" register is offset by 1000, so therefore the definition of "TempSensor(182, "Battery temperature", CELSIUS, 0.1, offset=1000)" in single-phase definition file. Is the reading you get same on the inverter screen as in Home Assistant/Sunsynk? Also is your inverter recently updated?

@OwlBawl
Copy link
Author

OwlBawl commented Dec 28, 2024

182

What he means is that by having this definition:

SENSORS += Sensor(182, "Battery Temp", CELSIUS, 0.1)

You are effectively not setting any offset to the value. In deye modbus docs, it states that the "battery temperature" register is offset by 1000, so therefore the definition of "TempSensor(182, "Battery temperature", CELSIUS, 0.1, offset=1000)" in single-phase definition file. Is the reading you get same on the inverter screen as in Home Assistant/Sunsynk? Also is your inverter recently updated?

That behavior is just from the beginning of multi add-on usage - 3 firmwares of Deye SUN 6k-SG03/05 inverter incl. stock one.
The default Temp shows -75C
While custom made - correctly - 25C
as on screenshot i've provided.

@maslyankov
Copy link
Contributor

182

What he means is that by having this definition:
SENSORS += Sensor(182, "Battery Temp", CELSIUS, 0.1)
You are effectively not setting any offset to the value. In deye modbus docs, it states that the "battery temperature" register is offset by 1000, so therefore the definition of "TempSensor(182, "Battery temperature", CELSIUS, 0.1, offset=1000)" in single-phase definition file. Is the reading you get same on the inverter screen as in Home Assistant/Sunsynk? Also is your inverter recently updated?

That behavior is just from the beginning of multi add-on usage - 3 firmwares of Deye SUN 6k-SG03/05 inverter incl. stock one. The default Temp shows -75C While custom made - correctly - 25C as on screenshot i've provided.

I have fixed it. Please try the following docker image: ghcr.io/maslyankov/hass-addon-sunsynk-multi:edge
Please let me know if it works or not. Thanks!

@maslyankov
Copy link
Contributor

@OwlBawl did you manage to test the above version?

@OwlBawl
Copy link
Author

OwlBawl commented Jan 3, 2025

No, it's disconnected for service now, so can't check changes.

@kellerza
Copy link
Owner

kellerza commented Jan 6, 2025

@OwlBawl I realixed what I did wrong here, when you omit the offset it becomes 100, not 0.

This aligns with the docs (1000 before the 0.1 factor) and works ok for my Sunsynk

Can you try the edge version when you have things back up and running?

kellerza added a commit that referenced this issue Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants