Add flic smart button component #4681
Conversation
|
@soldag, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @robbiet480 to be potential reviewers. |
|
|
||
| def new_button_callback(address): | ||
| """Setup newly verified button as device in home assistant.""" | ||
| hass.loop.create_task(async_setup_button(hass, config, |
| import pyflic | ||
|
|
||
| # Get event loop | ||
| loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
Preferred to use
loop = hass.loop
| if not was_queued: | ||
| # Set state | ||
| self._is_down = click_type == pyflic.ClickType.ButtonDown | ||
| self.update_ha_state() |
There was a problem hiding this comment.
Please use self.schedule_update_ha_state()
|
|
||
| if self._is_down: | ||
| self._last_down = now | ||
| else: |
There was a problem hiding this comment.
Instead of this else, include a return in the if statement above and you don't have to indent the other code that much
| """Fire click event depending on click type.""" | ||
| import pyflic | ||
|
|
||
| if not was_queued: |
There was a problem hiding this comment.
Prefer to use a guard clause
if was_queued:
return
# Set state
…| if result == pyflic.ScanWizardResult.WizardSuccess: | ||
| _LOGGER.info("Found new button (%s)", address) | ||
| elif result != pyflic.ScanWizardResult.WizardFailedTimeout: | ||
| _LOGGER.info("Failed to connect to button (%s). Reason: %s", |
There was a problem hiding this comment.
Should this be _LOGGER.warning ?
| if auto_scan: | ||
| scan_client = pyflic.FlicClient(host, port) | ||
| start_scanning(hass, config, async_add_entities, scan_client) | ||
| loop.run_in_executor(None, scan_client.handle_events) |
There was a problem hiding this comment.
Will this call ever finish ? Or will it occupy an executor thread forever?
|
|
||
| main_client.on_new_verified_button = new_button_callback | ||
| hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, main_client.close) | ||
| loop.run_in_executor(None, main_client.handle_events) |
There was a problem hiding this comment.
Will this call ever finish ? Or will it occupy an executor thread forever ?
There was a problem hiding this comment.
No, FlicClient.handle_events() has to run the entire time in background. It is a main loop of the pyflic lib that never exits, unless the socket to the flic service is closed.
|
@balloob Thanks :) Yes, the library is callback based. The only method that is doing I/O is |
|
Related question here, mostly for @balloob or other maintainers. Does it make sense for hass to have a stateless button component instead of using the binary_sensor component? In this case we're setting the state based on when the button is down, but it's only a momentary change. The useful feature exposed by this platform is the click events. Looking forward to other platforms like Amazon dash, we may not get this state information, and only get a single trigger. |
Use library method to determine click type. Merge three click events into single one with click_type parameter.
|
@armills Thanks for your work, even if I was a little bit faster 😜 |
|
@soldag Looks great! Yeah, they don't show it in their examples. I just stumbled across it when looking through the library. At least it wasn't for nothing! Can you take a look at my other two notes? They're just some smaller questions I had. |
|
@armills Where did you take this notes? I couldn't find them. |
|
They should show up when you scroll through here: https://github.com/home-assistant/home-assistant/pull/4681/files/8adb707e3262635f5b77530defea5d6cbbe26afa |
|
Yeah, I thought so, but there is nothing but the code. |
|
OK, thanks Github. I'll just put them here.
EDIT: Also, just noting here that this PR works on my system. |
|
These suggestions make totally sense. I updated the code 😉 |
| PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({ | ||
| vol.Optional(CONF_HOST, default='localhost'): cv.string, | ||
| vol.Optional(CONF_PORT, default=5551): cv.port, | ||
| vol.Optional(CONF_AUTO_SCAN, default=True): cv.boolean, |
There was a problem hiding this comment.
Does it make sense to use CONF_DISCOVERY from homeassistant.const here?
| auto_scan = config.get(CONF_AUTO_SCAN) | ||
| if auto_scan: | ||
| scan_client = pyflic.FlicClient(host, port) | ||
| start_scanning(hass, config, async_add_entities, scan_client) |
There was a problem hiding this comment.
Why are you creating a new FlicClient here instead of passing the main_client?
|
|
||
| # Initialize connection channel | ||
| self._channel = pyflic.ButtonConnectionChannel(self._address) | ||
| self._channel.on_button_up_or_down = self._button_up_down |
There was a problem hiding this comment.
A possible alternative here is to make use of on_button_single_or_double_click_or_hold to fire button click events, and only use on_button_up_or_down for state. That way we're making better use of the upstream logic rather than re-implementing it.
|
@armills I think that a button component would be overkill here as there is no generic interface to abstract out, we're just firing events. Things like this should probably just be their own component (we can still categorize them as button on the HA site to help people discover them). Then again, I don't mind it being a binary sensor either. For now I will merge this PR as it all works. |
|
@balloob OK, makes sense. Thanks for the reply! |
|
You should also use the other click callbacks, i.e. on_button_click_or_hold and on_button_single_or_double_click. Also, queued events might be of interest to some users so I suggest not discarding them and instead include the time_diff in the event data. |
|
@fabianbergmark What's the use of the other click callbacks? I thought the others are used to only listen to specific events and as I want to listen to all types of click events, The point about queued events makes sense and I will update the component code. |
|
The logic behind the click callbacks is pretty smart, but not very well explained. Say you are not interested in double clicks but only click and hold. If you click the button twice fast, the on_button_click_or_hold would fire twice with a click event, while the on_button_single_or_double_click_or_hold would only fire once with a double click event. This is very useful for applications where you want to click fast but still have a secondary hold trigger. |
|
I opened a PR to support queued events here: #4822 |
|
@fabianbergmark Okay, now I understand 👍 |
|
One last note on the click topic is that if you discard the double click event then you will get lower latency on the click event since the "logic" does not have to wait for a possible second click in a double click sequence. |
|
Hi. I don't think it should be default to constantly scan in the background. |
|
You can disable the scan by setting As far as security concerns go, barring an unknown vulnerability in flicd, the worst an attacker could do is a DOS, which they can probably do anyway by saturating the Bluetooth frequency. The scanning for new buttons is passive. The daemon is listening for advertisement packets from the buttons. It isn't actively polling the network, so there shouldn't be an impact on radio usage. The only change when turning off discovery is that the daemon ignores advertisements from unknown buttons. |
|
Yes it can be disabled, but I don't want it to be default true since most users will just leave with the default settings. I would rather have some default mode which scans only while there is currently no button paired with the daemon. I don't know if you got my point with the security problem. If you always run the scan wizard that means that anyone can stand outside the window and pair Flic buttons so they are now trusted by the daemon without the user noticing anything. If you open the Bluetooth settings on your smartphone and you spontaneously get a popup asking if it's OK to pair with some random device you've never seen before, you usually press NO, right? If you run a Raspberry Pi 3, then the WiFi chip and Bluetooth chip is the same thus it also shares the same radio, which can only do one thing at a time. When you run a Bluetooth scan, you reserve radio usage in certain time frames which blocks out all other traffic during that time window. Probably most users won't notice anything but I did that test once on a smartphone and the network speed indeed became slower when a Bluetooth scan was active. |
|
You must also check the status code of when the wizard completes. If the bluetooth dongle is currently unplugged, it will complete immediately with status WizardBluetoothUnavailable. With this current implementation that leads to an infinite busy loop. Instead, if you get WizardBluetoothUnavailable you should not try again but rather upon receiving EvtBluetoothControllerStateChange with state = Attached, then it is ok to try again. |
|
That's a good point. Can you open two issues on GitHub for these comments so they can be discussed in a clean thread? You could also just open a PR for the bluetooth unavailable if you want to write it up and test it. |
Description:
This component let users use flic buttons to trigger automations.
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#1527
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass