-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Switchmate #15535
Switchmate #15535
Conversation
|
||
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({ | ||
vol.Required(CONF_MAC): cv.string, | ||
vol.Optional(CONF_FRIENDLY_NAME, default=DEFAULT_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.
Use CONF_NAME instead of CONF_FRIENDLY_NAME.
import bluepy | ||
self._state = False | ||
self._friendly_name = friendly_name | ||
self._handle = 0x2e |
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 extract the magic string into a constant.
self._device = bluepy.btle.Peripheral(self._mac, | ||
bluepy.btle.ADDR_TYPE_RANDOM) | ||
except bluepy.btle.BTLEException: | ||
_LOGGER.error("Failed to setup switchmate", exc_info=True) |
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.
I'm not sure we want to add exc_info
.
@property | ||
def unique_id(self) -> str: | ||
"""Return a unique, HASS-friendly identifier for this entity.""" | ||
return '{0}_{1}'.format(self._mac.replace(':', ''), self.entity_id) |
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.
Remove entity_id from unique_id.
"""Return the name of the switch.""" | ||
return self._friendly_name | ||
|
||
@Throttle(MIN_TIME_BETWEEN_UPDATES) |
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.
Why do we need to throttle this? If we want to adjust the scan interval, define the SCAN_INTERVAL
constant at the module level.
@Throttle(MIN_TIME_BETWEEN_UPDATES) | ||
def update(self) -> None: | ||
"""Synchronize state with switch.""" | ||
self._state = self._device.readCharacteristic(self._handle) == b'\x00' |
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 extract magic string b'\x00'
into constant.
def turn_on(self, **kwargs) -> None: | ||
"""Turn the switch on.""" | ||
self._device.writeCharacteristic(self._handle, b'\x00', True) | ||
self._state = True |
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.
Preferably we shouldn't modify state here but let update
do that, which will be called immediately after the call to this method. How is the responsiveness of the update call?
@property | ||
def unique_id(self) -> str: | ||
"""Return a unique, HASS-friendly identifier for this entity.""" | ||
return '{0}'.format(self._mac.replace(':', '')) |
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.
String formatting can be removed. Just do the replace.
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 good. Note my comment about exception info though. We usually don't include all that in error logs. We settle for the exception argument.
@MartinHjelmare : Can I merge this, or do you want someone else to take a look? |
Yes, go ahead and merge. I've approved it. |
Description:
Swichmate is a bluetooth switch https://www.mysimplysmarthome.com/products/switchmate-switches/
There are only 3 lines with device specific code, so I decided to not split it to an external library.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6031
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.