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

Clean up KPL Dimmer MQTT Class Set Level; Condense KPL with Dimmer Set_Level #420

Open
krkeegan opened this issue May 22, 2021 · 0 comments

Comments

@krkeegan
Copy link
Collaborator

The KeypadLinc MQTT class contains the following code for handling set commands:

if level_str is None or level_str == "":
# Dimmer and command topic can be the same
# If this lacks a level command it is meant for on/off
self._input_set(client, data, message)
else:
try:
is_on, mode, transition = util.parse_on_off(data)
level = '0' if not is_on else data.get('level', None)
if level is not None:
level = int(level)
reason = data.get("reason", "")
self.device.set(is_on=is_on, level=level, mode=mode,
reason=reason, transition=transition)

I think this is a holdover from when the KPL MQTT class had to be compatible with both switch and dimmer KPLs. Today, this code sends commands to the set_topic if there is no level specified in the incoming data and otherwise processes the data if level is present. Which can make for odd results since the payload of an incoming mqtt message affects the route that the command takes, see for example #418.

The KPL code is complicated, but I don't think this is necessary anymore. If there is both an on_off_topic and a level_topic for group 1 on the device, these messages will be routed correctly based on the mqtt topic used. If the topics are the same, then only a level_topic exists. As a result, I think everything sent here can be processed in the level_topic code.

Speaking of level_topic, this code is very similar to what is present in the Dimmer MQTT class. (Fanlinc uses the dimmer code by inheriting dimmer). The KPL and Dimmer level_topic code could be merged into a single topic class. The only issue is that the test for transition support would have to be moved out of the MQTT class an into the device classes since the KPLs support transition and regular Dimmers do not. This is probably more logical anyways.

I am going to wait until after 1.0.0 to change all this, as we already have enough significant changes prepared, plus I think the proposed edit in #418 to the on_off_topic_payload is a sufficient patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant