Add Crownstone integration#37005
Conversation
|
Hi @RicArch97, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
probot-home-assistant
Bot
added
has-tests
new-integration
integration: crownstone
labels
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
Did a quick scan of the PR to point out some early things.
The main missing part is the tests for the configuration flow, which we do require.
Furthermore, please limit the PR to a single platform (e.g., light or sensor). Additional platforms can be added later.
|
Changes were made after review. To sum up the changes:
|
|
New commit includes:
|
|
@RicArch97 Black has recently updated. Do a |
|
Black seems to be fine, but now checks are failing (that passed before) in files that either aren't mine, or i haven't changed anything to. |
…s. Fixed brightness adjustment. Fixed bug where presence was not updated correctly.
…ilable, bumped crownstone-sse. Other minor cleanups/fixes.
…y changing supported_features. Use assumed_state because there's no polling for state update confirmation.
|
Rebased on the lastest dev branch, after some failed checks on my previous commit. Common keys have been added to the config flow strings. |
This comment has been minimized.
This comment has been minimized.
|
Not stale. Crownstone integration has been further developed and updated here (HACS custom) |
| homeassistant/components/counter/* @fabaff | ||
| homeassistant/components/cover/* @home-assistant/core | ||
| homeassistant/components/cpuspeed/* @fabaff | ||
| homeassistant/components/crownstone/* @Crownstone @RicArch97 |
There was a problem hiding this comment.
Are you affiliated with Crownstone?
Are they aware they are set as a code owner?
There was a problem hiding this comment.
I started developing this integration as part of an internship for Crownstone. After it ended I decided to keep developing and maintaining the integration, but not as official member. This means Crownstone is the official owner of it, and they will take over maintenance in case I am unavailable.
There was a problem hiding this comment.
We are fine with it. ~CEO Crownstone.
| def __init__(self) -> None: | ||
| """Init with new event loop and instance.""" | ||
| self.loop = asyncio.new_event_loop() | ||
| self.uart_instance = CrownstoneUart(self.loop) |
There was a problem hiding this comment.
If this is an async package, why are you running it in a new event loop?
There was a problem hiding this comment.
The Uart lib is created by the Crownstone core members, with the idea to have it block until a USB is connected, so it works in a recursive way instead of creating tasks in the event loop to allow for other coroutines to be executed. If no USB is connected, it just keeps on scanning for USB dongles until it finds one. This way it would mean the USB could be connected and removed at any time, and the async_turn_on function uses the the USB whenever it's available, and falls back to the cloud if it is not.
We've decided to have it run in a separate thread to stop it from blocking the event loop.
I can request changes, if you deem it necessary.
There was a problem hiding this comment.
This will actually try to connect to every serial port it can find, to see if do a successful Crowdstone handshake.
This is very dangerous and can interfere with other equipment in use. Since this type of communication is not uncommon, I would say this is not a good method to have in Home Assistant.
There was a problem hiding this comment.
Holy moly. Yeah that is 100% not allowed in HA. USB port need to be passed in.
There was a problem hiding this comment.
It is possible for the lib to take a port parameter and skip the handshake section. How do we ensure to provide the correct port though? Let's say I have the Crownstone dongle at /dev/ttyUSB0, but then i add an other USB dongle of some kind and restart HA, and the Crownstone dongle is now at /dev/ttyUSB1
There was a problem hiding this comment.
By not using those ports. We advise our users to use the linked by-id paths in general to avoid those issues. However, that is a configuration problem, not a thing that integration should solve.
The ZHA integration might be worth a look. That one is showing a selection of devices/serial ports to use in the UI when setting up the integration.
There was a problem hiding this comment.
Another approach is to use a udev rule. In my case, I have the following line in a file in the folder /etc/udev/rules.d/:
SUBSYSTEM=="tty", ATTRS{idVendor}=="10c4", ATTRS{idProduct}=="ea60", SYMLINK+="crownstone"
This makes the usb dongle available at port /dev/crownstone
There was a problem hiding this comment.
@hillstub That is a modification done on the system administration level. From a Python application perspective, that is not an option or requirement we can take into account. Let's keep this PR review on the topic of reviewing code at hand. Thanks 👍
| self.sse.start() | ||
|
|
||
| # subscribe to Crownstone ability updates | ||
| self.sse.add_event_listener(EVENT_ABILITY_CHANGE_DIMMING, self.update_ability) |
There was a problem hiding this comment.
Is SSE calling event listeners from the event loop or a thread?
There was a problem hiding this comment.
They are called from a thread.
There was a problem hiding this comment.
Then don't decorate with @callback and don't call any async function.
There was a problem hiding this comment.
is it safe to initialize the SSE thread with the mainthread loop like so
self.sse = CrownstoneSSE(
customer_email, customer_password, asyncio.get_running_loop()
)And then in the SSE thread eventbus, provide coroutines to the loop by calling
asyncio.run_coroutine_threadsafe(coroutine, loop)There was a problem hiding this comment.
If it is a thread, why do you have to pass in the loop?
There was a problem hiding this comment.
The SSE lib has it's own eventbus. Which means any callbacks or coroutines added to it are executed within that thread (and the event loop running there). Let's say i want to run
await hass.async_create_task(coroutine)in a listener in that thread. Do i need to pass it to the mainthread loop specifically or am I not understanding it correctly?
| """Update the ability information.""" | ||
| # make sure the sphere matches current. | ||
| update_sphere = self.cloud.spheres.find_by_id(ability_event.sphere_id) | ||
| if update_sphere.cloud_id == self.sphere.cloud_id: |
There was a problem hiding this comment.
use a guard clause to simplify the code.
if update_sphere.cloud_id != self.sphere.cloud_id:
return
| update_crownstone = self.sphere.crownstones.find_by_uid( | ||
| ability_event.unique_id | ||
| ) | ||
| if update_crownstone is not None: |
There was a problem hiding this comment.
if update_crownstone is None:
return
| ability_event.ability_type | ||
| ].is_enabled = ability_event.ability_enabled | ||
| # reload the config entry to process the change in supported features | ||
| self.hass.async_create_task( |
There was a problem hiding this comment.
async_create_task can only be called from a callback inside the Home Assistant event loop.
| entities = [] | ||
| for crownstone in crownstone_hub.sphere.crownstones: | ||
| # some don't support light features | ||
| if crownstone.type not in CROWNSTONE_EXCLUDE: |
There was a problem hiding this comment.
It's more future proof to use an INCLUDE list.
| """Turn off this device via dongle or cloud.""" | ||
| if self.uart.is_ready(): | ||
| # switch using crownstone usb dongle | ||
| self.uart.switch_crownstone(self.unique_id, on=False) |
There was a problem hiding this comment.
This is doing I/O inside the event loop
| "Dimming is not enabled for this crownstone. Go to the crownstone app to enable it" | ||
| ) | ||
| elif self.uart.is_ready(): | ||
| self.uart.switch_crownstone(self.unique_id, on=True) |
| }, | ||
| "description": "Enter your email and password for Crownstone.", | ||
| "title": "Crownstone" |
There was a problem hiding this comment.
Title is filled in automatically based on name in manifest. This description is not needed.
| }, | |
| "description": "Enter your email and password for Crownstone.", | |
| "title": "Crownstone" | |
| } |
| "title": "Crownstone", | ||
| "config": { | ||
| "abort": { | ||
| "already_configured": "[%key:common::config_flow::abort::single_instance_allowed%]" |
There was a problem hiding this comment.
Why is only a single instance allowed? I didn't see any code that required this.
There was a problem hiding this comment.
This has to do with the Crownstone architecture. A config entry represents a "Sphere". A Sphere could be seen a house, or office. Since a single HA instance represents a house as well, it would not be logical to allow for multiple Spheres per HA instance.
There was a problem hiding this comment.
I think it's totally reasonable to be able to control my office space before I arrive at work or vice versa. We should not make that decision as developers. We should only limit to a single instance if a second instance is not necessary, like with Cast, which scans the network and find all.
There was a problem hiding this comment.
There are some difficulties when trying to allow for multiple spheres in a HA instance.
Crownstone communicates through a BLE mesh network and uses the smartphone of the user as gateway to provide the data to a cloud server; This means, if you want to switch a Crownstone that is in a different sphere, your smartphone would have to be physically there to be able to provide the data from to cloud to the BLE mesh, or in the other direction, send sensor data from the BLE mesh to the cloud server. A hub device is currently in development to help resolve that issue, but it is not ready yet.
There was a problem hiding this comment.
Sure, so those entities can become unavailable, just like what happens if the phone leaves the "sphere" now. That is unrelated to the number of spheres that can be configured.
| ) | ||
|
|
||
| async def async_step_sphere(self, user_input=None): | ||
| """Handle the step for selecting a sphere.""" |
There was a problem hiding this comment.
We should just add all spheres. Users can always disable devices later.
| CONF_EMAIL: self.login_info[CONF_EMAIL], | ||
| CONF_PASSWORD: self.login_info[CONF_PASSWORD], |
There was a problem hiding this comment.
Since you're involved with Crownstone, can you ask them to update their authentication to use something like OAuth2? It's highly prefered over storing email/password. (not for this PR)
|
This pull request is from some time ago, and is not completely in sync anymore with what is currently available on our HACS repository here. Would you recommend merging it first and then fixing according to review? (still limiting to 1 platform of course) |
|
Let's get this PR fixed and merged, and we can go from there. |
|
This PR seems to have gone stale. Closing it. |
|
Hi @balloob. Do you want us to open a new PR? We've been working on this steadily, following up your comments. It requires us to update our python code over the ecosystem in tandem though. My latest comment is from two days ago. :-) |
|
I did see your latest comment which actually made me realize this PR was stale. The developer hasn't been active on this in 1.5 months. Feel free to open a new PR once all comments have been addressed 👍 |
|
The changes are still a WIP indeed, but the process is going a bit slow right now, as i have decided to refactor some code in the Python libraries. This way we start off with a very stable PR we can build upon. |
I find it confusing to read only "If any of them do not return true, the automation will stop executing" (it's a double negation plus an "any" in the begining"). I've added an additional statement before
9 participants
Proposed change
New integration for Crownstone!
Crownstone creates smart crown stones to build into a power outlet, and plugs that plug into regular outlets/power strips.
A standout feature is the ability to detect a user's presence on room level by using BLE.
The integration exists of 3 core external libraries.
All of these libraries are available on PyPi.
Integration is configurable via the UI.
Type of change
Additional information
The full Crownstone integration (so far) is now available on HACS for testing.
See Crownstone HACS repository.
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale: