-
Notifications
You must be signed in to change notification settings - Fork 69
Update x1_lite_lv schema validation #196
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
Conversation
Introduced the `X1LiteLV` class to handle X1 Lite LV inverter data and decoding logic. Updated the `__init__.py` and `setup.py` files to register the new inverter for system integration.
Adjusted the PV1 and PV2 power values in the test expected values to reflect accurate scaling. This ensures alignment with expected data and improves test reliability.
|
If possible, do you have a sample response you can add as a test please? |
# Conflicts: # solax/inverters/x1_lite_lv.py
Expanded the X1 Lite LV inverter support to include handling for the v002 and v005 variants. Updated test fixtures, expected values, and response data while refining parsing logic to accommodate additional features and variations.
Streamlined parsing methods and formatting in the `X1LiteLV` class. Updated test fixtures and sample responses for better clarity and structure.
Standardized energy unit handling in the `X1LiteLV` inverter class by removing `.value` for `Total(Units.KWH)` across all entries. Adjusted test expected values for consistency in key naming.
…Power Standardized Total PV Power unit to Watts in `X1LiteLV` inverter. Updated test expected values and sample responses to reflect corrected scaling and consistency.
…rder for consistency
…verter class for clarity.
But I had an isue with run locally UT with the e.t.c. that is why so many commits Python 3.12.2 |
|
No worries about the number of commits. It all gets squashed anyway. I’ll review this later today. From a glance it looks good though! |
squishykid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally very PR, thank you for your attention to detail!
Two main questions, and these also have comments in the code:
- Why have you chosen to rename some of the sensors?
- Why have you chosen to edit one of the samples? For some context, they are there because they represent output from user inverters, so I need a really good reason for why we should change them. For example, I can imagine that your sensor renaming requires changes. Please share your thoughts on this.
Thanks again for your patience with all this, i'm sure we should be able to merge this soon!
| # "Grid power 1": (33, Units.W, twoway_div10), | ||
| # "Grid power 2": (34, Units.W, twoway_div10), | ||
| "Hourly Energy": (51, DailyTotal(Units.KWH), div100), | ||
| "Hourly Energy": (51, Total(Units.KWH), div100), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this changed from daily total to total?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this changes because had a warning from a HA.
So when I'll use the DailyTotal there I will get this warning: 2025-09-12 10:39:09.320 WARNING (MainThread) [homeassistant.components.sensor] Entity sensor.solax_xxxxxxx_hourly_energy (<class 'custom_components.custom_solax.sensor.InverterSensorEntity'>) is using state class 'measurement' which is impossible considering device class ('energy') it is using; expected None or one of 'total', 'total_increasing'; Please update your configuration if your entity is manually configured, otherwise report it to the author of the 'solax' integration
according the async_setup_entry
for the tupple lookup
SENSOR_DESCRIPTIONS[(measurement.unit, measurement.is_monotonic)]
(Units.KWH, False): SensorEntityDescription(
key=f"{Units.KWH}_{False}",
device_class=SensorDeviceClass.ENERGY,
native_unit_of_measurement=UnitOfEnergy.KILO_WATT_HOUR,
state_class=SensorStateClass.MEASUREMENT,
),
(Units.KWH, True): SensorEntityDescription(
key=f"{Units.KWH}_{True}",
device_class=SensorDeviceClass.ENERGY,
native_unit_of_measurement=UnitOfEnergy.KILO_WATT_HOUR,
state_class=SensorStateClass.TOTAL_INCREASING,
),better to use the Total instead of DailyTotal there or need to change
from
class DailyTotal(Measurement):to
class DailyTotal(Total):But this is unsafe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, as a side note it looks like we never use resets_daily? Wonder why I implemented that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the TOTAL_INCREASE also support the reset_daily state value + sum
solax/response_parser.py
Outdated
| """ | ||
|
|
||
| raw_json = resp.decode("utf-8").replace(",,", ",0.0,").replace(",,", ",0.0,") | ||
| _LOGGER.info("Received response: %s", raw_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important: please remove this, it looks like a debugging artifact
almost-online
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review
Two main questions, and these also have comments in the code:
Why have you chosen to rename some of the sensors?
I had rename some sensor in several reason:
- a sensor naming convension (capitalize first letters, e.g. "AC current Out" -> "AC Current Out")
- general right naming (e.g. 'Today's ...' -> 'Daily ...')
- Naming related to the solax's data naming (e.g. "Today's Generated Energy" -> "Daily Inverter Output". see the screen below)
Why have you chosen to edit one of the samples? For some context, they are there because they represent output from user inverters, so I need a really good reason for why we should change them. For example, I can imagine that your sensor renaming requires changes. Please share your thoughts on this.
I'd just rename the X1_LITE_LV_80_RESPONSE to the X1_LITE_LV_80_v002_RESPONSE without any changes (for the old the 002 firmware version) + aded a new firmware version response (X1_LITE_LV_80_v005_RESPONSE for the 005 firmware version).
Probably misunderstanding with the expected value variable X1_LITE_LV_80_v002_VALUES
Thanks again for your patience with all this, i'm sure we should be able to merge this soon!
I hope so, thank you too!
| # "Grid power 1": (33, Units.W, twoway_div10), | ||
| # "Grid power 2": (34, Units.W, twoway_div10), | ||
| "Hourly Energy": (51, DailyTotal(Units.KWH), div100), | ||
| "Hourly Energy": (51, Total(Units.KWH), div100), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this changes because had a warning from a HA.
So when I'll use the DailyTotal there I will get this warning: 2025-09-12 10:39:09.320 WARNING (MainThread) [homeassistant.components.sensor] Entity sensor.solax_xxxxxxx_hourly_energy (<class 'custom_components.custom_solax.sensor.InverterSensorEntity'>) is using state class 'measurement' which is impossible considering device class ('energy') it is using; expected None or one of 'total', 'total_increasing'; Please update your configuration if your entity is manually configured, otherwise report it to the author of the 'solax' integration
according the async_setup_entry
for the tupple lookup
SENSOR_DESCRIPTIONS[(measurement.unit, measurement.is_monotonic)]
(Units.KWH, False): SensorEntityDescription(
key=f"{Units.KWH}_{False}",
device_class=SensorDeviceClass.ENERGY,
native_unit_of_measurement=UnitOfEnergy.KILO_WATT_HOUR,
state_class=SensorStateClass.MEASUREMENT,
),
(Units.KWH, True): SensorEntityDescription(
key=f"{Units.KWH}_{True}",
device_class=SensorDeviceClass.ENERGY,
native_unit_of_measurement=UnitOfEnergy.KILO_WATT_HOUR,
state_class=SensorStateClass.TOTAL_INCREASING,
),better to use the Total instead of DailyTotal there or need to change
from
class DailyTotal(Measurement):to
class DailyTotal(Total):But this is unsafe
|
Thank you for taking the time to respond to my questions. It feels clear to me now. I think you only need to remove the debug statement and then we can merge: #196 (comment) Thank you for taking the time to work on this contribution! |
almost-online
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd already remove the _LOGGER artifact
Please, when you have time, merge the PR when it's ready
Thanks for reviewing!
| # "Grid power 1": (33, Units.W, twoway_div10), | ||
| # "Grid power 2": (34, Units.W, twoway_div10), | ||
| "Hourly Energy": (51, DailyTotal(Units.KWH), div100), | ||
| "Hourly Energy": (51, Total(Units.KWH), div100), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the TOTAL_INCREASE also support the reset_daily state value + sum
|
My bad on the debugger comment. I must have been looking at an old diff. Merge time! |

Fix schema validation error after a firmware update