Smartir class #192
Smartir class #192
Conversation
added device data files checks for light class
|
Hello @make-all and @danielcaceresm would you be please so kind and look to proposed change? Please read PR description including my thoughts about this. As I do not use any IR light myself I wanted to also ask you for testing if possible. The easiest way would be to use content of the smartir_class branch Thank you. |
|
Here are my observations:
Config is: climate:
- platform: smartir
name: Living room aircon
unique_id: living_ac
device_code: 1090
controller_data:
controller_type: Broadlink
remote_entity: remote.living_room_ir
delay_secs: 0.5
num_repeats: 1
temperature_sensor: sensor.living_room_ir_temperature
humidity_sensor: sensor.living_room_ir_humidity
light:
- platform: smartir
name: Ceiling light
unique_id: ceiling_light
device_code: 1000
controller_data:
controller_type: Broadlink
remote_entity: remote.living_room_ir
delay_secs: 0.5
num_repeats: 1There are no log messages that appear connected to this, and I have not been able to figure out why this happened to the light, but not the climate device.
|
custom_components/smartir/light.py
Outdated
| self._color_temp = None | ||
|
|
||
| self._brightness_list = device_data.get("brightness") | ||
| self._color_temp_list = device_data.get("colorTemperatures") |
There was a problem hiding this comment.
It is "colorTemperature" in the configs, and in the device_data.py checking above.
There was a problem hiding this comment.
Thank you, good catch. fixed in the last commit!
| await self.send_remote_command(remote_cmd, count) | ||
|
|
||
| async def send_remote_command(self, remote_cmd, count=1): | ||
| async def send_command(self, state, brightness, color_temp): |
There was a problem hiding this comment.
_send_command (prefixed by underscore) to be consistent with other uses in this file.
brightness and color_temp should have default values so the call with one argument from async_turn_off works.
There was a problem hiding this comment.
I did unify send_command as it was all over the places. Also we can set brightness and colorTemperature to anything as in STATE_OFF it is not sent. Now fixed as well.
custom_components/smartir/light.py
Outdated
| "on", | ||
| ) | ||
| else: | ||
| # if on code is not present, the on bit can be still set later in the all operation/fan codes""" |
There was a problem hiding this comment.
This line ends with """.
The highlighting in github does not treat the rest of the file as a comment, so maybe this is OK because it is in a comment. But in my editor it was causing highlighting to display everything below as a comment, so I think it is better to remove it even if it is harmless.
There was a problem hiding this comment.
Agree, this was strange comment, it seems to be in all classes probably even from the original code - removed everywhere.
multiple fixes for light implementation
|
Hello, first I would like to thank you for the testing, I appreciate it a lot. I just pushed multiple fixes for your first report. It shall fix the invalid naming, send_command naming and off command. I will take a look to your second report ASAP. |
fixed colorTeperature typo in the light
|
The The off command issue seems to be new after the last changes (I previously got it working just by adding Also, I tried to collect debug logs, but it seems the log messages are not being formatted correctly. For debug logs, it is generally better to use arguments for the format parameters instead of inline formatting, as then the string will only get expanded if debug logging is enabled. 2025-01-20 13:28:07.514 ERROR (MainThread) [custom_components.smartir.light] Missing device IR code for 'off' mode. |
added optimistic_state configuration parameter added generic update_entity callback fixed light class logging messages
|
Thank you!
Both shall be fixed in the last commit. I tested generic changes on my AC and it seems to be working... |
|
I didn't find any obvious issue with the off command, let's hope that debug logs would tell us more. |
logging improvements
|
It doesn't seem to have changed much other than logging the values it is setting. In the test below, I was trying to set the color temperature to 2700K when the light seems to be on 6500K but HA thinks it is on 2700K. As you can see from the log, it is using "colder" commands, which is wrong - the previous version used enough "warmer" commands to ensure the color_temp would resync at the end of the range. I tried to lower the brightness - it might have worked correctly, at the time I thought I was on the color_temp control so it didn't seem to be working correctly but in hindsight I was actually changing the brightness. The color_temp slider seems to work sometimes, so it is not consistently mixing colder vs warmer. |
|
The reason for not turning off is that you are checking that "off" exists in commands and is a string. |
|
The excessive "on" commands is a major problem. I don't think this should be conflated with "optimistic". The lights get out of sync, so if turn_on is called with no arguments, I want it to send the "on" command regardless of whether the light is on or off, so I don't want optimistic mode. But if turn_on is called with brightness or color_temp arguments, I want it to just change those and not try to turn the light on. This light has only three buttons to turn it on. "ON" turns it on at full brightness and 6500K regardless of any previous setting. "MEM" turns it on at the last saved user settings (which is what I have set up to use as "on"), and NIGHT turns on a separate small warm LED nightlight (and turns off the main light if it is on). None of these is suitable to use at random when the light is already on and has been adjusted. |
|
Another problem to report is that the current code with common supported_features method outputs the following log message at startup: I think it can be "fixed" by adding |
|
These are the changes I needed to get it working: diff --git a/custom_components/smartir/light.py b/custom_components/smartir/light.py
index bb22103..a5aab97 100644
--- a/custom_components/smartir/light.py
+++ b/custom_components/smartir/light.py
@@ -8,6 +8,7 @@ from homeassistant.components.light import (
ATTR_COLOR_TEMP_KELVIN,
ColorMode,
LightEntity,
+ LightEntityFeature,
)
from homeassistant.const import CONF_NAME, STATE_OFF, STATE_ON
from homeassistant.core import HomeAssistant
@@ -57,6 +58,8 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
self._brightness_list = device_data.get("brightness")
self._color_temp_list = device_data.get("colorTemperature")
+ self._support_flags = LightEntityFeature(0)
+
if self._color_temp_list is not None:
# The light can be dimmed and its color temperature is present in the state.
self._attr_supported_color_modes = [ColorMode.COLOR_TEMP]
@@ -137,22 +140,18 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
}
async def async_turn_on(self, **kwargs):
- brightness = kwargs.get(ATTR_BRIGHTNESS, self._brightness)
- color_temp = kwargs.get(ATTR_COLOR_TEMP_KELVIN, self._color_temp)
-
- if self._brightness_list is not None and brightness is None:
- _LOGGER.debug(
- "No power on brightness argument found, setting last brightness '%s'",
- self._brightness,
- )
- brightness = self._brightness
-
- if self._color_temp_list is not None and color_temp is None:
- _LOGGER.debug(
- "No power on color temperature argument found, setting last color temperature '%s'",
- self._color_temp,
- )
- color_temp = self._color_temp
+ if "on" in self._commands.keys():
+ _LOGGER.debug("Supports on, so leave brightness and color_temp if not explicitly given")
+ default_brightness = None
+ default_color_temp = None
+ else:
+ # If there is no explicit "on" command, the light needs to be turned on by setting brightness and/or color_temp
+ _LOGGER.debug("No explicit on, so always adjust brightness and color temp")
+ default_brightness = self._brightness
+ default_color_temp = self._color_temp
+
+ brightness = kwargs.get(ATTR_BRIGHTNESS, default_brightness)
+ color_temp = kwargs.get(ATTR_COLOR_TEMP_KELVIN, default_color_temp)
await self.send_command(STATE_ON, brightness, color_temp)
@@ -171,7 +170,11 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
if state == STATE_OFF:
await self._async_power_off()
else:
- await self._async_power_on()
+ if (brightness is None and color_temp is None) or not self.is_on:
+ _LOGGER.debug("Explicit 'on'")
+ await self._async_power_on()
+ else:
+ _LOGGER.debug("Skipping 'on' due to args: brightness %d and color_temp %d K", brightness or 0, color_temp or 0)
if color_temp is not None:
if "colorTemperature" in self._commands and isinstance(
@@ -198,7 +201,7 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
)
color_temp = self._color_temp_list[new_color_temp_index]
steps = new_color_temp_index - old_color_temp_index
- if steps < 0:
+ if steps < 0 or new_color_temp_index == 0:
cmd = "warmer"
steps = abs(steps)
else:
@@ -214,12 +217,13 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
steps = len(self._color_temp_list)
_LOGGER.debug(
- "Changing color temp from '%s'K index '%s' to '%s'K index '%s' using command '%s'",
+ "Changing color temp from '%s'K index '%s' to '%s'K index '%s' using command '%s' %d times",
self._color_temp,
old_color_temp_index,
color_temp,
new_color_temp_index,
cmd,
+ steps,
)
while steps > 0:
steps -= 1
@@ -256,7 +260,7 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
)
brightness = self._brightness_list[new_brightness_index]
steps = new_brightness_index - old_brightness_index
- if steps < 0:
+ if steps < 0 or new_brightness_index == 0:
cmd = "dim"
steps = abs(steps)
else:
@@ -272,12 +276,13 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
steps = len(self._brightness_list)
_LOGGER.debug(
- "Changing brightness from '%s'K index '%s' to '%s'K index '%s' using command '%s'",
+ "Changing brightness from '%s'K index '%s' to '%s'K index '%s' using command '%s' %d times",
self._brightness,
old_brightness_index,
brightness,
new_brightness_index,
cmd,
+ steps,
)
while steps > 0:
steps -= 1
@@ -286,8 +291,10 @@ class SmartIRLight(SmartIR, LightEntity, RestoreEntity):
self._on_by_remote = False
self._state = state
- self._brightness = brightness
- self._color_temp = color_temp
+ if brightness or state == STATE_OFF:
+ self._brightness = brightness
+ if color_temp or state == STATE_OFF:
+ self._color_temp = color_temp
self.async_write_ha_state()
except Exception as e:
diff --git a/custom_components/smartir/smartir_entity.py b/custom_components/smartir/smartir_entity.py
index 480bde1..f6ba935 100644
--- a/custom_components/smartir/smartir_entity.py
+++ b/custom_components/smartir/smartir_entity.py
@@ -243,7 +243,7 @@ class SmartIR:
_LOGGER.debug("Scheduled power sensor check for '%s' state", state)
async def _async_power_on(self):
- if "on" in self._commands.keys() and isinstance(self._commands["on"], str):
+ if "on" in self._commands.keys():
if (
self._power_sensor is not None or self._optimistic_state
) and self._state == STATE_ON:
@@ -255,7 +255,6 @@ class SmartIR:
)
elif (
"off" in self._commands.keys()
- and isinstance(self._commands["off"], str)
and self._commands["off"] == self._commands["on"]
and self._state == STATE_ON
and self._optimistic_state
@@ -277,10 +276,9 @@ class SmartIR:
)
async def _async_power_off(self):
- if "off" in self._commands.keys() and isinstance(self._commands["off"], str):
+ if "off" in self._commands.keys():
if (
"on" in self._commands.keys()
- and isinstance(self._commands["on"], str)
and self._commands["on"] == self._commands["off"]
and self._state == STATE_OFF
): |
This is expected at least for now - there is feature request for fan entity to support list of strings as a multiple command in the JSON, which I am going to implement in the native way in the controller class later on. For now you have to stick with single command, sorry. ;-o |
|
But a list with multiple strings works now. It has done since I have been using SmartIR from upstream. |
The command for resync of the color temp and brightness should stayed the same - see the original: steps = new_color_temp - old_color_temp
if steps < 0:
cmd = CMD_COLOR_MODE_WARMER
steps = abs(steps)
else:
cmd = CMD_COLOR_MODE_COLDER
if steps > 0 and cmd:
# If we are heading for the highest or lowest value,
# take the opportunity to resync by issuing enough
# commands to go the full range.
if (
new_color_temp == len(self._colortemps) - 1
or new_color_temp == 0
):
steps = len(self._colortemps)
self._colortemp = self._colortemps[new_color_temp]
await self.send_command(cmd, steps)and new version: steps = new_color_temp_index - old_color_temp_index
if steps < 0:
cmd = "warmer"
steps = abs(steps)
else:
cmd = "colder"
if (
new_color_temp_index == len(self._color_temp_list) - 1
or new_color_temp_index == 0
):
# If we are heading for the highest or lowest value,
# take the opportunity to resync by issuing enough
# commands to go the full range.
steps = len(self._color_temp_list)
while steps > 0:
steps -= 1
await self._controller.send(self._commands[cmd])
await asyncio.sleep(self._delay)I simply kept the code same with only one difference - previously you explicitly excluded resync if So while I agree that when staying on the warmest settings of 2700K and goingt to the same 2700K resync shall use warmer command (I will correct this) than in your case change with steps == 0 would never be resynced. The corrected code shall look like: # If we are heading for the highest or lowest value,
# take the opportunity to resync by issuing enough
# commands to go the full range.
if ( new_color_temp_index == 0 ):
cmd = "warmer"
steps = len(self._color_temp_list)
elif new_color_temp_index == len(self._color_temp_list) - 1:
cmd = "colder"
steps = len(self._color_temp_list)
elif steps < 0:
cmd = "warmer"
steps = abs(steps)
elif steps > 0:
cmd = "colder"
_LOGGER.debug(
"Changing color temp from '%s'K index '%s' to '%s'K index '%s' sending '%s'x command '%s'",
self._color_temp,
old_color_temp_index,
color_temp,
new_color_temp_index,
steps,
cmd,
)
while steps > 0:
steps -= 1
await self._controller.send(self._commands[cmd])
await asyncio.sleep(self._delay) |
|
I may not have noticed before because it didn't do anything in that case. But now that it is trying to resync even when steps == 0, it matters that colder is not the right command when you want to make sure you are at the warmest setting. But I'm not sure I like this change. My light beeps for every remote command, and I use ambient lighting, which is updating the color temperature and brightness every few minutes. So with this, it is going to beep all night once the warmest setting is reached. |
HA is really not consistent on using _supported features. The root cause is I moved that attribute into SmartIR class where it is initiated with self._support_flags = LightEntityFeature(0) |
Yep, I agree that command used is/was wrong - it is corrected as mentioned above. Regarding beeping - again while I understand your use case the 'mileage may differ` between users/use cases. So probably something configurable would be optimal, let me think about that. |
This is IMHO not completely correct argument. If we assume that state can get out of sync, than we shall assume that brightness and color_temp will get out of sync. Than the expected behaviour would be to resync those as well? I.E. going up or down whole scale? It is simpler if there are dedicated codes for every brightness and color_temp and not step commands - as then you would like to have it for sure. Again might well work well for your light, but is it generic approach?
I understand this but it doesn't seems to be good generic approach based on the one given light implementation. Additional question, what will HA execute (i.e. arguments for async_turn_on will be, if you will try to change brightness on the light which is off? I have Yeelight implementation and changing color or brightness of the off light will switch it on. But that is exactly something you don't need/want/expect... |
I am afraid I do not get it completely and I am half sleeping now, so I will take a look at your changes tomorrow. Thank you. |
|
I read all of your evaluation (thank you once again!) once more time and I agree, you are right about excessive usage of the
In addition, I would like to implement your night light as HA light effect. Lets define new config parameters: effects_onoff, effects_brightnesss, effect_color_temp as lists and then specify effect IR command declared to proper list in the commands. So in your case: What do you think? |
|
What you are describing here is basically how I first implemented it - to always sync at power on as it seemed good in theory to do that way. But in practice it is annoying to have the light spend 20-30 seconds going through a range of color temperatures and brightnesses every time it is turned on. If you primarily use HA to control your light, it doesn't get out of sync often so resyncing is usually unnecessary. What I ended up deciding after trying it for a while is that it is good to have a way to resync, but it shouldn't be too eagerly used. That is why my implementation only resynced when reaching the end of the brightness or color sync scale after a change. There is a lot of extra beeping from the light in that case, but the brightness or color temperature is only changing if it was out of sync before, and it is going to where it should be. I think media player volume would be similar - you really don't want it going up to full volume every time you turn on, but also it would be annoying if it spent 20 seconds turning the volume right down and back up again every time. Effect for the night light might be OK, I don't really use the night light on this light. Making it a brightness of 1 kept it out of the way of the UI but it could be available from an automation and didn't involve any more implementation effort. I'm not sure if your proposal would require duplicating it for every brightness and color temperature value though, if it does then that could be a downside. |
|
Unfortunately it is hard to predict user behaviour. I have many reports of users mixing HA control with native IR remote control. So I can't take your use case as a generic one. For the re-sync using deterministic commands (i.e. dedicated IR commands for specific brightness or temperature) there shall be no problem to re-sync always. For the devices having only up/down IR commands for control of given attribute I would propose configuration switch. Again using optimistic mode (where we expect that device should not go out of sync and re-sync only when reaching max or min value) or pessimistic, where we expect it can happen. So any user can choose based on their typical use case. In regards to the effect, for effect not combinable with any other attributes (onoff effect), you do not need to provide anything. So if in your night effect you can only turn light off/on. If you try to change brightness or color temp it will cancel the effect and go into normal mode. At least this is my understanding of the HA documentation. If your effect would be effect_brightness or effect_color_temp and you would like to control brightness and/or color temperature, then you need to provide commands for those specifically. In such case we would probably need to expand commands structure. Or the default could be the same commands only to be overwritten if defined for the effect mode. |
As I refactored and de-duplicated some of the code in the other classes into SmartIR parent class and I have pending feature requests which I would like to implement across all classes I needed to refactor
lightclass to follow code structure in other classes. I addition I implemented device data file validation forlightclass as well. For now I removed constants keeping names for device data JSON keys. There is possibility to create device_data_const.py and declare all constants for all classes there and then import from this central location. I am not yet fully decided if having JSON data keys declared in constant provide any real benefits, as the only real usage would be in case on renaming the key - but that would bring breaking changes to all JSON files using that key, so probably something we would like to avoid. I am fully open to discussion over this change.