-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Telnet switch #8913
Telnet switch #8913
Conversation
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.
Looks great overall! Just a few small questions/requests.
vol.Required(CONF_COMMAND_ON): cv.string, | ||
vol.Optional(CONF_COMMAND_STATE): cv.string, | ||
vol.Optional(CONF_FRIENDLY_NAME): cv.string, | ||
vol.Optional(CONF_OPTIMISTIC): cv.boolean, |
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.
Can we drop the optimistic configuration and just assume it's optimistic if there's no command state defined, or is there something I'm missing? For example, assumed_state can just be return self._command_state is None
.
"""Update device state.""" | ||
response = self.__telnet_command(self._command_state) | ||
if response: | ||
_LOGGER.error( |
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.
This should be debug level, correct?
self._optimistic = optimistic | ||
self._value_template = value_template | ||
|
||
def __telnet_command(self, command): |
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.
Small nitpick, but single underscore prefix is preferred for normal internal methods.
Updated PR. |
Good to go once the docs PR is up-to-date. |
Should the encoding be hardcoded to ASCII? Doesn't it depend on what the host is configured to accept? |
Here it states that Telnet uses ASCII encoding: http://www.pcmicro.com/netfoss/telnet.html |
"""Update device state.""" | ||
response = self._telnet_command(self._command_state) | ||
if response: | ||
_LOGGER.info( |
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.
Don't log this. Maybe a debug but not more.
I think if someone have a no ASCII Telnet device, he can make a PR for set encoding as options. |
vol.Required(CONF_COMMAND_OFF): cv.string, | ||
vol.Required(CONF_COMMAND_ON): cv.string, | ||
vol.Optional(CONF_COMMAND_STATE): cv.string, | ||
vol.Optional(CONF_FRIENDLY_NAME): cv.string, |
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.
Please use CONF_NAME
instead of CONF_FRIENDLY_NAME
.
|
||
DEFAULT_PORT = 23 | ||
|
||
SWITCH_SCHEMA = vol.Schema({ |
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.
Would be nice to keep it ordered.
|
||
# pylint: disable=unused-argument | ||
def setup_platform(hass, config, add_devices, discovery_info=None): | ||
"""Find and return switches controlled by shell commands.""" |
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.
Stale docstring.
Ping @multiholle |
I'm quite busy at the moment. I'm trying to get back to the PR next week. |
How can I use this addon when authentication is required in order to perform the on/off commands
|
Please use the forum or chat for support questions. |
Description:
New platform telnet switch. Allows to control a device with telnet commands for on and off. Can fetch the state of the device or work as optimistic switch.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3165
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
.coveragerc
.If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests pass