use deterministic units #70
Conversation
solax/units.py
Outdated
| C = "°C" | ||
| HZ = "Hz" | ||
| PERCENT = "%" | ||
| UNKNOWN = "" |
There was a problem hiding this comment.
We probably want to call this some variant of "none", since it's not undefined- we just know it doesn't have a unit.
solax/inverters/qvolt_hyb_g3_3p.py
Outdated
| 'Today\'s Feed-in Energy': (90, 'kWh', div100), | ||
| 'Total Feed-in Energy': (86, Total(Units.KWH), | ||
| feedin_energy), | ||
| 'Total Feed-in Energy Resets': (87), |
There was a problem hiding this comment.
did you miss some 'none' / 'unknown' units here?
There was a problem hiding this comment.
The mapping is able to handle a variation of fallbacks/defaults
'Total Feed-in Energy Resets': (87), is equivalent to
'Total Feed-in Energy Resets': 87, and
'Total Feed-in Energy Resets': (87,Units.UNKNOWN), or
'Total Feed-in Energy Resets': (87,Measurment(Units.UNKNOWN)),
I did not remove the brackets, so that is somewhat visually consistent
VadimKraus
force-pushed
the
feature/units
branch
from
f189a2b
to
a3498ef
Compare
|
Also, thank you for adding a commend about backwards compatibility. |
|
I should be able to get this merged on the weekend |
|
I might try and follow up this PR with one containing a test to ensure these units are used for new inverters and not constants |
tests/test_smoke.py contains a test, which should ensure the use of units in all inverters. |
|
Going to rebase this with the black/mypy changes |
| C = "°C" | ||
| HZ = "Hz" | ||
| PERCENT = "%" | ||
| NONE = "" |
There was a problem hiding this comment.
As a physicist, I would say that the correct name for units of a pure number quantity is "dimensionless"
|
|
||
| if isinstance(mapping, tuple): | ||
| (idx, unit_or_measurement, *_) = mapping | ||
| else: |
There was a problem hiding this comment.
This else clause is never hit - coverage is complaining at me in #77 about this line never executing. A grep finds no inverter subclasses that don't give units for values.
There was a problem hiding this comment.
OK - this code does use this for the "resets" values, which I've removed in 77. Sorry for false alarm
| if processor: | ||
| sensors[name] = processor[0] | ||
| for name, mapping in cls.response_decoder().items(): | ||
| if isinstance(mapping, tuple): |
There was a problem hiding this comment.
Same as above - cov complains about not all branches taken
| @@ -18,8 +20,9 @@ class Inverter: | |||
| """Base wrapper around Inverter HTTP API""" | |||
|
|
|||
| ResponseDecoderType = Union[ | |||
| Dict[str, Tuple[int, str]], | |||
| Dict[str, Tuple[int, str, Callable[[Any, Any, Any], Any]]], | |||
| Dict[str, int], | |||
There was a problem hiding this comment.
Should this not have the union in the value part? i.e.
ResponseDecoderType = Dict[str, Union[int, Tuple[int, SensorUnit]. Tuple[int, SensorUnit, Callable]]]
possible solution for #62
After this PR:
Units need to be defined using the enum
Units.The classes
MeasurementandTotalspecify if the values are measurements with arbitrary content or if the continuously summing up (Total).A sensor_mapping tuple can have multiple variations:
If only an index is provided, will assume that this is a measurement with an unknown unit.
If only an unit enum value is provided, will assume that that this is a measurement with the provided unit.
Using the
MeasurementorTotalclasses it is possible to define the behavior explicitly.Consequences for home assistant
this change is NOT backwards compatible!
See VadimKraus/core@1ba0304 for how the changes would need to be reflected in the integration.
Basically, by type checking for the
Totaltype, it is clear with sensor is of typetotal_increasing