-
-
Notifications
You must be signed in to change notification settings - Fork 35k
Add NASweb integration #98118
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
Add NASweb integration #98118
Conversation
Hello @nasWebio, 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! ❤️ |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Still waiting for the review |
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.
Some initial comments.
(Close + Open to run CI against the tip of dev) |
I think the integration is in a good shape now @nasWebio 👍 One concern though: Integrations added to the Home Assistant core repo should be useful for residential users. Looking at your web page, I get the impression you're rather targeting commercial building automation? |
Thank you for your feedback! It's true that our current products are targeted mainly towards commercial building automation. However, we are actively developing a product for the residential market to expand our offering. Home Assistant integration is key to this, as we recognize its importance for homeowners and smart home enthusiasts. |
@nasWebio Thanks for confirming! Based on your response, I'd suggest to initially publish your integration as a custom integration installable via HACS. Once the product intended for residential use materializes, you're most welcome to open another PR to add your integration to Home Assistant Core. |
Hello, and thank you for taking the time to review our integration and share your insights. I truly appreciate your effort and attention to detail! |
@chomtech thanks for the additional context, with that in mind I think it's OK to accept this integration as a core integration 👍 |
The link to the product page in the documentation PR, https://autoidentyfikacja.pl/pl/p/NASweb/262, is not working. Is there another link? |
Link was outdated. Documentation is updated with correct link: |
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 must have missed the lack of checks for serial number. We should store that as the config entry's unique ID.
_LOGGER.exception("Unexpected exception") | ||
errors["base"] = "unknown" | ||
else: | ||
return self.async_create_entry(title=info["title"], data=user_input) |
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.
We should set the unique ID to the serial number of the device, and abort the flow if there's already a config entry for the same device. Documentation: https://developers.home-assistant.io/docs/config_entries_config_flow_handler#managing-unique-ids-in-config-flows
DATA_NASWEB: HassKey[NASwebData] = HassKey(DOMAIN) | ||
|
||
|
||
async def async_setup_entry(hass: HomeAssistant, entry: NASwebConfigEntry) -> bool: |
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.
During setup, we should check that the serial number of the device we're connecting to is matching the stored serial number (the config entry's unique ID), in case the device's IP address has changed.
if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS): | ||
nasweb_data = hass.data[DATA_NASWEB] | ||
coordinator = entry.runtime_data | ||
serial = coordinator.webio_api.get_serial_number() |
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.
Change to config entry's unique id
async_add_entities(entities_to_add) | ||
entity_registry = er.async_get(hass) | ||
for index in removed: | ||
unique_id = f"{DOMAIN}.{coordinator.webio_api.get_serial_number()}.relay_switch.{index}" |
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.
Change to config entry's unique 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.
Please update config entry tests:
- Assert the config entry's unique id is the serial number of the device
- Add a test where we try to add the same device twice, the flow should be aborted
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.
One more remark
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.
LGTM, thanks for the patience with the review @nasWebio 👍
Breaking change
Proposed change
Add integration for WEBIO controller.
WEBIO is a specialized microprocessor controller that integrates building automation, intrusion and access control systems.
Eventually device will include few different platforms: switch, sensor, alarm control panel, climate.
Initial PR is limited to implement only relay switches.
Type of change
Additional information
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
.To help with the load of incoming pull requests: