Closed bernardpe closed 3 months ago
Yeah sure...the issue is in automated testing not handling the case right now...but I'll wait until implementation is complete
Hello, I'm proceeding with reviewing the code and I have a few points:
MLModeStateSensor
and MLOutputPowerState
have been extracted from sensor.py
module and moved directly to the mts960.py
implementation since I mostly disagree with the design choice. All of the introduced dependencies have been extracted too from 'core' modules and ported to mts960.py (mainly SensorModeStateEnum
) since this entities are strictly related features of the mts960. I suggest you to not introduce general design concepts in your work unless we agree before since that would be a waste of time for both of us. I really like your contribution and enthusiasm but I've developed and still I'm maintaining meross_lan according to some design principles which I want to keep stricts. One for all is: put a feature where it belongs and try to 'generalize' it when it's general use is worth. Those 2 entities are just useful for mts960 since no other thermostat has these needs (only when it will happen the features could then be generalized).
This is particularly true for the MLOutputPowerState (more on this sensor later) since every other thermostat is 'just a thermostat' so it doesn't need an additional entity for output activity (the HVACAction already carries the state). MLModeStateSensor instead could be used for other thermostats but we have to discuss the details for this. At any rate, I've tried to leave these sensor working as they were, just moved to mts960.py module.MLOutputPowerState
is a binary_sensor and I don't see the need to build all of that custom state management. The default HA binary sensor already uses 'on'/'off' (which are also by default translated) to report the state so it is kind of a duplicate (this way you also risk loosing the native translations done by the frontend). Moreover, overriding the state
property in HA is highly discouraged (in fact it is marked as final
in the source code definition) since the HA core developers are trying to put some rules about what is 'custom code' and what not. Customizing the icon is useless (again, for the same reasons of uniformity) since users who don't like the default HA might always override these (the same would apply for entity names and the likes: everyone has a very different opinion in terms of aesthetics and naming so, setting this in code is almost always useless and just adds 'noise' to the code implementation). The icon state itself is usually automatically managed by HA so I'd just let the entity be a plain binary_sensor with no customization. We could just use the BinarySensorDeviceClass.PLUG
or BinarySensorDeviceClass.POWER
to give the sensor a reasonable meaning and let HA show an uniform representation.MtsTemperatureNumber
has been modified by overriding the value
property which is also 'discouraged' by HA dev guidelines (this property is also marked final
in Number
entity). Why the need for rounding ? I see the Number entity doesn't have the 'suggested precision' (the one used in Sensor
entity) property in HA core so I think you wanted to achieve the same result. I still haven't modified your code here but I'm planning to implement a general approach for MLConfigNumber in order to set the desired 'precision' in a uniform way. Could you provide same examples of why and where in the UI the rounding would be needed ? I'm asking since the other thermostats (no rounding) don't look bad in my opinion and there's no need for rounding but I know the mts960 is different since the temperatures are divided/multiplied by 100 while other thermostats use 10 as a device_scale
Hi,
My comments :
I'll add the management of the 'precision' directly to MLConfigNumber then so that the implementation is good for other cases too
Power return without change :
Plug return without change :
Both are not what we try to report !
Just for your information, even when we use standard entity the translation is not always done!
BTW, I'm insteresting to know what the 0 means ?
is there is a way to get 2, 3 or 5, is it a counter of warning ?
I got a other question about the entity schedulerb why schedulerb is reported as a entity with always off ? May be confusing isn't it ? when MTS MODE is set to shceduling but on the config schedulerb report off ?
This is not bad to be honest
Power return without change :
while this is more 'strange' I agree
Plug return without change :
Both are not what we try to report !
I suppose completely removing the device class would lead to 'on'/'off' in HA frontend
BTW, I'm insteresting to know what the 0 means ?
is there is a way to get 2, 3 or 5, is it a counter of warning ?
No idea what the number means. It is the plain value from the device payload value in the warning
field
I got a other question about the entity schedulerb why schedulerb is reported as a entity with always off ? May be confusing isn't it ? when MTS MODE is set to shceduling but on the config schedulerb report off ?
I don't know...technically speaking the HA calendar entity has state -> on when any event is active. In meross_lan the original implementation reported 'on' when the thermostat was in schedule mode (this is still correctly working for mts100-150-200) maybe there's a bug in how it is managed for mts960. I've never had a chance to test it on mts960. It should already be kept 'in sync' whenever the state of the climate gets flushed
Just for your information, even when we use standard entity the translation is not always done!
Yes, translation support for entities is being developed in recent months and so, some parts of the UI/state/labels are already natively supporting this, while other parts are not. It also depends on how the integration component has been developed and if it complies or not with HA core 'etiquette'. That's why it would be better to put the less amount of strings when coding since this could 'conflict' or hide the improvements made in HA core. For example, there is also support for translating entities names inside HA core but that's a long battle to carry on....
- the MLOutputPowerState is a binary_sensor ==> I make a special management of the state because POWER binary not not report ON/OFF but clear/unclear ! the core MtsTemperatureNumber : I round the temperature, because when set 150, MTS960 return 148 and having 1,48 it doesn't make sense !, my Davis Weather Station where the sensor cost 150$ report 1 digit only and this sensor that is always false report 2 digits !
I need to better understand what is going on here since this is really weird and it might be another problem. Number entities in HA have the native_step
property to manage the 'resolution' and precision of the number so there might be another issue.
First thing to check:
MtsRichTemperatureNumber.native_step
is set to fixed 0.1 (°C) or sometimes to the climate.target_temperature_step
because I have no way to check which one is good. The climate.target_temperature_step
too is fixed at 0.5 (°C) since that was the default for mts100 and now I'm not sure if the mts960 is different: maybe you can set the target temperature in 0.1° C steps with the app ?. If this is true you should just add the target_temperature_step: float = 0.1
to the Mts960Climate definition so to override the default. This way the climate and the frost temperature number would allow you to step by 0.1 if it is correct.Awnser by items :
you said that when you set 150 it returns 148 but I think 150 (1.5 °C) is not an allowed value since the device should accept values between 500 (5 °C) and 1500 (15 °C). The UI in the end should not allow you to enter 1.5 °C (if everything works fine). I got 3 devices and the device on the veranda get range from -10 °C to 75 C°, this allow 1,48°C isn't it ? there is the same issue with 37.44 °C and Calibration is +2.6 °C ! right now look trace ? FYI : the sensor temperature is 37.4°C
Am I wrong on the diag ?
Trace :
a2d4ce69548c796569336b4d226c1) {"header":{"messageId":"95fa2d4ce69548c796569336b4d226c1","namespace":"Appliance.Control.Multiple","method":"SETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719324566,"timestampMs":27,"sign":"71774e00b3ff779e6fbdd432f1266add"},"payload":{"multiple":[{"header":{"messageId":"ebec2281f50248edbdefa6891b985d9f","namespace":"Appliance.System.All","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719324566,"timestampMs":29,"sign":"2c33930e6c2340649084d4b0bfd9dfe2"},"payload":{"all":{"system":{"hardware":{"type":"mts960","subType":"eu","version":"4.0.0","chipType":"MT7686","uuid":"###############################0","macAddress":"################0"},"firmware":{"version":"4.2.6","homekitVersion":"2.0.1","compileTime":"Oct 10 2023 09:34:37","encrypt":1,"wifiMac":"################0","innerIp":"#############0","server":"###################0","port":"@0","userId":"@0"},"time":{"timestamp":1719324566,"timezone":"Europe/Paris","timeRule":[[1711846800,7200,1],[1729990800,3600,0],[1743296400,7200,1],[1761440400,3600,0],[1774746000,7200,1],[1792890000,3600,0],[1806195600,7200,1],[1824944400,3600,0],[1837645200,7200,1],[1856394000,3600,0],[1869094800,7200,1],[1887843600,3600,0],[1901149200,7200,1],[1919293200,3600,0],[1932598800,7200,1],[1950742800,3600,0],[1964048400,7200,1],[1982797200,3600,0],[1995498000,7200,1],[2014246800,3600,0]]},"online":{"status":1,"bindId":"Vmn8Y5EgNHiPzArt","who":1}},"digest":{"thermostat":{"modeB":[{"channel":0,"mode":1,"targetTemp":2600,"working":2,"currentTemp":3744,"state":1,"onoff":1,"sensorStatus":1}]}}}}},{"header":{"messageId":"52e33a26b0f043fdaa5833ce26eea7f6","namespace":"Appliance.Control.Thermostat.Calibration","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719324566,"timestampMs":75,"sign":"3b3abba9f5a404811679ef9fbaa19878"},"payload":{"calibration":[{"channel":0,"value":260,"min":-2000,"max":2000}]}},{"header":{"messageId":"90828c0e436542a1bf76d82ddd2e5f0b","namespace":"Appliance.Control.Thermostat.ScheduleB","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719324566,"timestampMs":77,"sign":"3958586db0b1a4292351bb16bdcf9583"},"payload":{"scheduleB":[{"channel":0,"section":12,"sun":[[1440,2600]],"mon":[[1440,2600]],"tue":[[1440,2600]],"wed":[[1440,2600]],"thu":[[1440,2600]],"fri":[[1440,2600]],"sat":[[1440,2600]]}]}}]}}
BTW, I'm insteresting to know what the 0 means ? image
is there is a way to get 2, 3 or 5, is it a counter of warning ?
No idea what the number means. It is the plain value from the device payload value in the warning field
warning looks reported as plain value, but this is not usual on homeassistant... on Miele integration warning is a binaryfield
and I made the test warning on MTS960 report 0 or 1 why reported are plain value for all MTS device ?
About schedulerb..
I got a other question about the entity schedulerb why schedulerb is reported as a entity with always off ? image May be confusing isn't it ? when MTS MODE is set to shceduling but on the config schedulerb report off ?
The management of the scheduler is attached to a entity (event if debut is off), but I think that it doesn't make sense on the philosophy of homeassistant to created an entiry with value always off for management of a calendar ?
Am I wrong ?
Why power return Detected/Clear
see on attach strings.json from components/binary_sensor strings.json
}, "power": { "name": "Power", "state": { "off": "[%key:component::binary_sensor::entity_component::gas::state::off%]", "on": "[%key:component::binary_sensor::entity_component::gas::state::on%]" } }, and gaz : }, "gas": { "name": "Gas", "state": { "off": "Clear", "on": "Detected" } },
on Dev blame :
I got 3 devices and the device on the veranda get range from -10 °C to 75 C°, this allow 1,48°C isn't it ? there is the same issue with 37.44 °C and Calibration is +2.6 °C ! right now look trace ?
I don't understand.
Previously you said:
the MLOutputPowerState is a binary_sensor ==> I make a special management of the state because POWER binary not not report ON/OFF but clear/unclear ! the core MtsTemperatureNumber : I round the temperature, because when set 150, MTS960 return 148 and having 1,48 it doesn't make sense !, my Davis Weather Station where the sensor cost 150$ report 1 digit only and this sensor that is always false report 2 digits !
So I understand you have modified the MtsTemperatureNumber in order to round the temperature...but I don't understand which temperature you're talking about, the 'frost temperature' ? the 'target temperature' ? the 'current temperature' ?
MtsTemperatureNumber is used to enter the frost temperature or the calibration...which one is not working ?
In meross devices every 'entity' has its own range. From the trace:
-10 °C to 75 C
which range is this ? target temperature ? current temperature ? does it appear in the traces ?
Switching topic: the warning question
and I made the test warning on MTS960 report 0 or 1 why reported are plain value for all MTS device ?
Because I don't know what is the meaning and sometimes the meross device could use different values for the "warning" field For example:
This happened in the past. Initially the warning was just:
EnumSensor
.The problem of the different values was fixed by implementing translations for the state. If you check the source code json in meross_lan for translations you'll find how it works. This is simple and worked.
Actually this doesn't work anymore but no one reported the problem. Back in January, the HA core entities had a big implementation change and because of that 'big change', the code in meross_lan doesn't work anymore as intended. I'll provide a fix for the issue
Because I don't know what is the meaning and sometimes the meross device could use different values for the "warning" field For example:
0: ok no warning 1: warning temperature too high 2: alarm: sensor disconnected
I just disconnect the sensor still warning = 0 !!
No impact on MTS960
Are you talking about frost warning ? If so then it has a different behavior than other mts200 similar features (I'm talking about overheat protection)
I think the difference is 'overheat protection' in mts200 works by using an external sensor so, if the wire is truncated it is able to detect the issue and report with "warning" == 2 Actually 'frost protection' might be different: it is just a threshold set on the main temperature sensor of the device:
If this is correct then there's no need for "disconnected" but that is not a big issue. The problem now is that the translation is not working
on MTS960 this is not complety true, because warning is 1 if
if actual temperature < frost temp then "warning" == 1 OR if actual temperature < temp lower alarm and temp lower alarm is activated then "warning" == 1 OR if actual temperature > temp high alarm and temp high alarm is activated then "warning" == 1
namespace:Appliance.Control.Thermostat.AlarmConfig payload:{'alarmConfig': [{'lowEnable': 2, 'lowTemp': -3500, 'min': -3500, 'max': 12000, 'channel': 0, 'highEnable': 2, 'highTemp': 6000}]}
About this : So I understand you have modified the MtsTemperatureNumber in order to round the temperature...but I don't understand which temperature you're talking about, the 'frost temperature' ? the 'target temperature' ? the 'current temperature' ?
I just send you a example that MTS960 is sending temperature with 2 digits.
I modified round on
class MtsTemperatureNumber(MLConfigNumber): """ Common number entity for representing MTS temperatures configuration """
__slots__ = (
"climate",
"device_scale",
)
@property
def value(self):
# Return the rounded value
return round(self.native_value,1) if self.native_value is not None else None
and MtsTemperatureNumber is used for all temperature on Config
When Frost alarm is off we can have frost warning = 1 !!!
2024-06-25 17:20:30.857 DEBUG (MainThread) [custom_components.merosslan.mts960###############################0] Handler undefined for method:PUSH namespace:Appliance.Control.Thermostat.AlarmConfig payload:{'alarmConfig': [{'lowEnable': 2, 'lowTemp': -3500, 'min': -3500, 'max': 12000, 'channel': 0, 'highEnable': 2, 'highTemp': 6000}]}
024-06-25 17:26:19.371 Level 5 (MainThread) [custom_components.merosslan.mts960###############################0] RX(mqtt) PUSH Appliance.Control.Thermostat.ModeB (messageId:d74172119ccda7c31cd8975d926d3a76) {"header":{"messageId":"d74172119ccda7c31cd8975d926d3a76","namespace":"Appliance.Control.Thermostat.ModeB","timestamp":1719336369,"timestampMs":953,"triggerSrc":"","payloadVersion":1,"method":"PUSH","sign":"af12920555c66f829fa45ed1a6c728ba","from":"/appliance/2308011347300060080148e1e9d3a12d/publish"},"payload":{"modeB":[{"working":2,"currentTemp":3485,"state":2,"onoff":1,"sensorStatus":1,"channel":0,"mode":1,"targetTemp":2600}]}} 2024-06-25 17:26:19.411 Level 5 (MainThread) [custom_components.merosslan.mts960###############################0] RX(http) SETACK Appliance.Control.Multiple (messageId:2c84b31762c9465cb57f3c52a09cbe44) {"header":{"messageId":"2c84b31762c9465cb57f3c52a09cbe44","namespace":"Appliance.Control.Multiple","method":"SETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719336379,"timestampMs":708,"sign":"34507f38916aa6a239c3a6ac977dac9c"},"payload":{"multiple":[{"header":{"messageId":"b626a2c748ac42198b1b9e71b3038026","namespace":"Appliance.Control.Thermostat.Calibration","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719336379,"timestampMs":709,"sign":"43d7cb77629b6b1c96e572e50a3ce2f7"},"payload":{"calibration":[{"channel":0,"value":150,"min":-2000,"max":2000}]}},{"header":{"messageId":"821a0013f6de4bb4aaffbfb6cea968d9","namespace":"Appliance.Control.Thermostat.DeadZone","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719336379,"timestampMs":710,"sign":"763f66ba3d0a140d6a1bcb73caac1001"},"payload":{"deadZone":[{"channel":0,"value":300,"min":50,"max":2000}]}},{"header":{"messageId":"ad660d9f0747436f87b94584527bde1a","namespace":"Appliance.Control.Thermostat.Frost","method":"GETACK","payloadVersion":1,"from":"/appliance/2308011347300060080148e1e9d3a12d/publish","timestamp":1719336379,"timestampMs":712,"sign":"9e9fe4ebdf339e5b408559a0143d8af6"},"payload":{"frost":[{"channel":0,"onoff":0,"value":700,"min":500,"max":1500,"warning":1}]}}]}}
2024-06-25 17:26:19.370 DEBUG (MainThread) [custom_components.merosslan.mts960###############################0] Handler undefined for method:PUSH namespace:Appliance.Control.Thermostat.Alarm payload:{'alarm': [{'channel': 0, 'temp': 3499, 'type': 1}]}
I see you set the alarm low temp == -35 , high temp == 60, why the mts960 reports an alarm? (the current temp is 34.99 apparently...
This is very strange..also the frost warning should not be == 1. I don't understand
P.S. the repo has been updated so now the translations for warning are working again...the problem is how to interpret the frost_warning == 1 now...since it doesn't look like the temperature is below frost temp
Frost warning on mts960 is device warning because : Namespace frost :
Onoff : 0 alarm frost is disable Warning : 1 ==> alarm is coming from alarm temp !
After reporting entity with values 0,1,2 is not user friendly and not philosophy of has !
Frost warning on mts960 is device warning because : Namespace frost :
Onoff : 0 alarm frost is disable Warning : 1 ==> alarm is coming from alarm temp !
This doesn't make sanse..there should be more about the funtioning which we don't understand
After reporting entity with values 0,1,2 is not user friendly and not philosophy of has !
I agree..
I'm finishing checking the calendar.py, I've simplified it while maintaining the code you've added for mts960. I have 2 questions:
if schedule_index==0:
raise Exception("Unable to delete first event of the day")
entry is now on diagnostic because this is not a SENSOR and not a CONFIG entities and the current implementation need a fake entity to work.
with MTS960 (and is looks that on other MTS too) you need to have at least 1 scheduling per day, if not update failed !
entry is now on diagnostic because this is not a SENSOR and not a CONFIG entities and the current implementation need a fake entity to work.
I totally disagree: this is an entity to configure the device it is a calendar but still an entity. Surely it's not a diagnostic tool. What do you mean 'current implementation need a fake entity' ?
with MTS960 (and is looks that on other MTS too) you need to have at least 1 scheduling per day, if not update failed !
Sure, in fact you cannot delete all the events (the check is some lines before) but if you delete the first, the second will just become the first...and so on. Unless you can explain why and where the previous implementation didn't work I'll restore that
Your comments : I totally disagree: this is an entity to configure the device it is a calendar but still an entity. Surely it's not a diagnostic tool. What do you mean 'current implementation need a fake entity' ?
==> the entity is not using to configure calendar at ALL, you use the entity as base (according your tool kit) to make the calendar working, and in any case a calendar is a SENSOR !
Your comments : Sure, in fact you cannot delete all the events (the check is some lines before) but if you delete the first, the second will just become the first...and so on. Unless you can explain why and where the previous implementation didn't work I'll restore that
Why : Calendar : 0h-5h 26°C / 5h-10h 27°C / 11h-0h 28°C
if you delete 0h-5h ==>W Calendar becomes Calendar : 0h-10h 27°C / 11h-0h 28°C
Because Device NEEDS starting 0h and this is not what we expect when we delete 0h-5h to extend 5h-10h 27h to 0h-10h 27°C if doesn't make sense !
explain what is the added value to have a entity scheduleb (btw what this names means for users !) as sensor AND on the dashboard on the device with this history ?
and value is always OFF and with attribute named scheduleB like this ?
This is a very personal opinion which I stronly disagree with. Having the entity either as CONFIG or DIAGNOSTIC removes it from the 'default' dashboard while you still have (whatever is the category of the entity) it in your device dashboard. Device dashboard is an helper UI/tool for configuration overall, it is just a panel inside HA -> Settings -> Devices and Services
: as the name (settings) implies it is used to configure the device and not as a 'final user friendly' dashboard. Whn you are in the 'Device dashboard' it doesn't make a big difference if the entity is under CONFIG or under DIAGNOSTIC and the entity itself is a 'configuration entity' sine it 'configures' the device
As for why the name 'scheduleB': it is the native name of the payload. SInce that attribute is there just for diagnostic purposes it doesn't matter how you name it - 99% of the people would not understand anyway what's in it - a raw list of numbers with no apparent meaning except for me, for you and few others). It should be better removed at all so no one could argue about what is it and why that name...it is just a simple info carried over with the calendar in case some 'geek' users or a developer want to know what's the real payload underlying the calendar...and there are many reasons why it is there which I'm sick to explain more.
As I've told you, the calendar is off when the climate is not scheduling while it is correctly on when the climate is scheduling. In my tests, if I set the device in 'schedule heating' or 'schedule cooling' it turns on..there should be some bug in the code you have if it doesn't work like this. I'm finishing off some tests and I will merge a working code that does exactly that: 'on' when climate is schedule mode - 'off' when climate is in timer or manual mode
At any rate I have enough of keeping arguing with your personal positions (you could say the same about my personal positions anyway) which are very different about the matter. We're not improving on anything if we keep debating of this and that.
meross_lan has some design choices which I want to try keep uniform. I'm open to suggestions and criticism too so I might change idea on some points. If you want to keep contributing I'm very happy with that: I'll check your code and add any useful function to the repo, but when something is not correct or I don't agree or I don't understand I'm asking you why you chose to implement that way in order to understand your point but in the end, I'm responsible on keeping the whole software working 'as smooth as possible' for users and according to my 'personal view' of how the software should work. GitHub repos are wonderful tools so that if you're not happy with this you can fork your own personal version (you've already done that anyway) and build on top of that.
Ok understood, I will keep by fork,
bye, bye !
Version of the custom_component
Configuration
Describe the bug
A clear and concise description of what the bug is.
MTS960 : firmware version : 4.2.6 hardware version : 4.0.0
it seems that API on MTS960 has been change, using modeB : I found that for MTS960 :
inferring the mts960 mode is now :
MTS960_MODE_HEAT_COOL = 1 MTS960_MODE_SCHEDULE = 2 MTS960_MODE_TIMER = 3
State to know if switch is ON/OFF
mapping the "state" key value to the socket/plug action
MTS960_STATE_UNKNOWN = 0 MTS960_STATE_ON = 1 MTS960_STATE_OFF = 2 # this appears when the plug is off (why not 0?)
To now of MTS960 is on HEAT or COOL mode we need to check/set working attribut
MTS960_WORKING_HEAT = 1 MTS960_WORKING_COOL = 2
To switch OFF : we can use onof with following value
mapping the "ONOFF" key value to the socket/plug action
MTS960_ONOFF_UNKNOWN = 0 MTS960_ONOFF_ON = 1 MTS960_ONOFF_OFF = 2 # this appears when the plug is off (why not 0?)
Debug log
By Activating the trace : 2024/06/05 - 10:56:53 RX http GETACK Appliance.Control.Thermostat.ModeB {"modeB":[{"channel":0,"mode":1,"targetTemp":2500,"working":2,"currentTemp":2435,"state":2,"onoff":1,"sensorStatus":1}]}
the Fix base on dev has been done on my github