update broadlink.py to add support for MP1 switch#9222
Conversation
|
Hello @giangvo, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
| class BroadlinkMP1Switch(BroadlinkRMSwitch): | ||
| """Representation of an Broadlink switch.""" | ||
|
|
||
| def __init__(self, friendly_name, device, slot): |
There was a problem hiding this comment.
IndentationError: unexpected indent
indentation is not a multiple of four
unexpected indentation
| self._state = state | ||
|
|
||
|
|
||
| class BroadlinkMP1Switch(BroadlinkRMSwitch): |
There was a problem hiding this comment.
Add new BroadlinkMP1Switch class to control 4 slots
| class BroadlinkMP1Switch(BroadlinkRMSwitch): | ||
| """Representation of an Broadlink switch.""" | ||
|
|
||
| def __init__(self, friendly_name, device, slot): |
There was a problem hiding this comment.
slot param is used to define slot to control
| if switch_type == "mp1-3": | ||
| switches = [BroadlinkMP1Switch(friendly_name, broadlink_device, 3)] | ||
| if switch_type == "mp1-4": | ||
| switches = [BroadlinkMP1Switch(friendly_name, broadlink_device, 4)] |
There was a problem hiding this comment.
Does not all mp1 switches have 4 slots?
Then I think we always should add all 4 slots.
There was a problem hiding this comment.
yup. I updated the code to add all 4 slots and changed device type to mp1 only. New config should be like this
- platform: broadlink
host: 192.168.0.x
mac: xx:xx:xx:xx:xx:xx
type: mp1
friendly_name: 'MP 1'
slots:
# friendly name of slots - optional
# if not set, slot name will be switch's friendly_name + 'slot {slot_index}'. e.g 'MP 1 slot 1'
slot_1: 'TV slot'
slot_2: 'Xbox slot'
slot_3: 'Fan slot'
slot_4: 'Speaker slot'|
|
||
| def _update(self, retry=2): | ||
| try: | ||
| states = self._device.check_power() |
There was a problem hiding this comment.
I think we here will ask for the same data 4 times (once for each slot).
You should make a data object that you can pass to the init function instead of the device object. The data object should handle the check_power call.
Here is an example: https://github.com/home-assistant/home-assistant/blob/38071501b421627dadbceb4bbdced195ac4ec620/homeassistant/components/switch/digitalloggers.py#L137
| @Throttle(TIME_BETWEEN_UPDATES) | ||
| def update(self): | ||
| """Fetch new state data for this device.""" | ||
| self._update(); |
|
|
||
|
|
||
| class BroadlinkMP1Switch(object): | ||
| """Representation of a slot of Broadlink switch - Using for fetch states of all slots""" |
There was a problem hiding this comment.
line too long (92 > 79 characters)
| self._command_on = 1 | ||
| self._command_off = 0 | ||
| self._slot = slot | ||
| self._parent_device = parent_device; |
| broadlink_device = broadlink.mp1((ip_addr, 80), mac_addr) | ||
| parent_device = BroadlinkMP1Switch(broadlink_device) | ||
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 2), broadlink_device, 2, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 3), broadlink_device, 3, parent_device), BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 4), broadlink_device, 4, parent_device)] |
There was a problem hiding this comment.
continuation line under-indented for visual indent
line too long (195 > 79 characters)
| elif switch_type in MP1_TYPES: | ||
| broadlink_device = broadlink.mp1((ip_addr, 80), mac_addr) | ||
| parent_device = BroadlinkMP1Switch(broadlink_device) | ||
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 2), broadlink_device, 2, parent_device), |
There was a problem hiding this comment.
line too long (203 > 79 characters)
trailing whitespace
| from homeassistant.components.switch import (SwitchDevice, PLATFORM_SCHEMA) | ||
| from homeassistant.const import ( | ||
| CONF_FRIENDLY_NAME, CONF_SWITCHES, CONF_COMMAND_OFF, CONF_COMMAND_ON, | ||
| CONF_FRIENDLY_NAME, CONF_SWITCHES, CONF_SLOTS, CONF_COMMAND_OFF, CONF_COMMAND_ON, |
There was a problem hiding this comment.
line too long (85 > 79 characters)
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 2), broadlink_device, 2, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 3), broadlink_device, 3, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 4), broadlink_device, 4, parent_device)] |
There was a problem hiding this comment.
continuation line under-indented for visual indent
line too long (103 > 79 characters)
| parent_device = BroadlinkMP1Switch(broadlink_device) | ||
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 2), broadlink_device, 2, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 3), broadlink_device, 3, parent_device), |
There was a problem hiding this comment.
continuation line under-indented for visual indent
line too long (103 > 79 characters)
| broadlink_device = broadlink.mp1((ip_addr, 80), mac_addr) | ||
| parent_device = BroadlinkMP1Switch(broadlink_device) | ||
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), | ||
| BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 2), broadlink_device, 2, parent_device), |
There was a problem hiding this comment.
continuation line under-indented for visual indent
line too long (103 > 79 characters)
| elif switch_type in MP1_TYPES: | ||
| broadlink_device = broadlink.mp1((ip_addr, 80), mac_addr) | ||
| parent_device = BroadlinkMP1Switch(broadlink_device) | ||
| switches = [BroadlinkMP1Slot(_get_mp1_slot_name(friendly_name, 1), broadlink_device, 1, parent_device), |
There was a problem hiding this comment.
line too long (111 > 79 characters)
| vol.Optional(CONF_FRIENDLY_NAME): cv.string, | ||
| }) | ||
|
|
||
| MP1_SWITCH_SLOT_SCHEMA = vol.Schema({ |
There was a problem hiding this comment.
slot's schema to define slot's friendly names
|
@Danielhiversen , thanks for your review. Should I merge this PR now? Could u also advise me how to update document after merging this? |
pvizeli
left a comment
pvizeli
left a comment
There was a problem hiding this comment.
I know that was before but rename all friendly_name stuff to name only. Otherwise we compatition the real friendly name from core and it will be not very userfriendly.
| vol.Optional('slot_2', default=None): cv.string, | ||
| vol.Optional('slot_3', default=None): cv.string, | ||
| vol.Optional('slot_4', default=None): cv.string | ||
| }) |
There was a problem hiding this comment.
removed. btw, could u explain why we should not set default None here?
|
@pvizeli , thanks for your review. Could you please explain it a bit
Changing Update: Do you mean same change as in this one https://github.com/home-assistant/home-assistant/pull/4343/files ? If yes, I think it should be in different PR to refactor all broadlink switch types. |
|
@pvizeli could u please approve this PR? |
| CONF_STATE = 'state' | ||
| CONF_STRUCTURE = 'structure' | ||
| CONF_SWITCHES = 'switches' | ||
| CONF_SLOTS = 'slots' |
There was a problem hiding this comment.
Let's keep this in the local file instead of adding it to the general constants.
|
|
||
| def get_outlet_status(self, slot): | ||
| """Get status of outlet from cached status list.""" | ||
| return self._states['s' + str(slot)] |
There was a problem hiding this comment.
For string concatenation, please use the format function:
return self._states['s{}'.format(slot)]| if retry < 1: | ||
| _LOGGER.error(error) | ||
| return | ||
| if not self._auth(): |
There was a problem hiding this comment.
Did you try to get this kind of logic added to the broadlink lib that is being used? It would be more suitable to be added there.
There was a problem hiding this comment.
I would do it in other PR and let people who know more about broadlink lib do it. I am not familiar with it :(
| _LOGGER.error("Failed to send packet to device") | ||
|
|
||
| def _get_mp1_slot_name(switch_friendly_name, slot): | ||
| if not slots['slot_' + str(slot)]: |
| """Set up Broadlink switches.""" | ||
| import broadlink | ||
| devices = config.get(CONF_SWITCHES, {}) | ||
| slots = config.get('slots', {}) |
There was a problem hiding this comment.
No need to specify default, the platform schema provides a default.
|
@balloob , please review again. I updated code to use |
|
Awesome! Thanks! Congratulations on your first merged PR! 💃 🐬 👍 |
|
Thanks @balloob . Could u please advise how should I update the document? |
|
Yes. Make sure you branch off |
|
Thanks |
|
I created PR for updating document here: home-assistant/home-assistant.io#3367 . Please review @balloob |
ghost
added
7 participants
Description:
switch/broadlink.pyto add support for MP1 switchRelated issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed: