-
-
Notifications
You must be signed in to change notification settings - Fork 36.5k
System Nexa 2 Core Integration #159140
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
base: dev
Are you sure you want to change the base?
System Nexa 2 Core Integration #159140
Conversation
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.
Hi @konsulten
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!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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.
d038e84 to
3e4b1df
Compare
|
Please address the bot comment. |
Done! Sensor and Light should have been removed now. |
| ) -> None: | ||
| """Handle settings updates.""" | ||
| for entity in entry.runtime_data.config_entries: | ||
| if isinstance(entity, ConfigurationSwitch): |
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 don't expose platform details directly outside the platform, like accessing the entity directly.
It's ok to register a callback from the platform that we call in a central place to let the entity know about a state update.
One way is to use the DataUpdateCoordinator in push mode.
https://developers.home-assistant.io/docs/integration_fetching_data#pushing-api-endpoints
Another way is to use our dispatch helper. A third way is to design some custom callback registration way.
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.
Thank you will look into it!
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.
Should be fixed now
| except DeviceInitializationError as e: | ||
| raise ConfigEntryNotReady from e | ||
| if device.info_data is None: | ||
| return False |
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.
Is there any way to resolve this state (ConfigEntryError) for the user?
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.
The only ones I can think of is that user would need to look over configuration over the actual device or that there is some device error that's making it unreachable on the network?
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.
Or that if user had used IP for configuration host for the device it could have changed IP and needs IP address to be updated if it changed by ending DHCP lease or such. User could also have changed hostname of the devices so user would need to update configuration
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.
When is device.info_data None?
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.
Should never happen, it should raise a DeviceInitializationError if it fails to set it in the library. Writing a comment about it, or should it be an assert to just avoid mypy?
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.
If it can't happen we can use an assert to tell mypy about it. Use an if TYPE_CHECKING: block.
Although the library only conditionally sets the attribute so technically it can happen.
I'd suggest to try and improve the library so the logic and type annotations of the library matches reality.
Eg get_info will never return a falsy object according to the current type annotations in the library but we still check for that case.
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.
Updated the library and removed the checks
| } | ||
|
|
||
|
|
||
| async def test_zeroconf_discovery( |
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 missing a test of already configured device.
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.
Added test
|
I hope I got the things mentioned in the comments. :) |
|
Should be ready for review now, not any planned changes to be done |
|
|
||
| from homeassistant.const import Platform | ||
|
|
||
| DOMAIN = "systemnexa2" |
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.
Is systemnexa2 a right domain name?
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 think so at least, this is the corresponding company web page for the product line:
https://nexa.se/smarta-hem/system-nexa-2
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.
What about the first nexa? What if they release a new one? Would nexa be a better domain?
(this is mostly about trying to understand as we can't change the domain once it's in, so I just want to make sure we disucssed it to make sure it's future proof)
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 think it is its own API for these devices, Nexa is the brand name. There is another product line but its hub based. And it should be a separate domain
| try: | ||
| info = await temp_dev.get_info() | ||
| device_id = info.information.unique_id | ||
| device_model = info.information.model | ||
| device_version = info.information.sw_version | ||
| if device_id is None or device_model is None or device_version is None: | ||
| return self.async_abort( | ||
| reason="unsupported_model", | ||
| description_placeholders={ | ||
| ATTR_MODEL: str(device_model), | ||
| ATTR_SW_VERSION: str(device_version), | ||
| }, | ||
| ) | ||
|
|
||
| self._discovered_device = _DiscoveryInfo( | ||
| name=info.information.name or "Unknown Name", | ||
| host=host_or_ip, | ||
| device_id=device_id, | ||
| model=device_model, | ||
| device_version=device_version, | ||
| ) |
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.
Can we only keep things in the try block that can raise?
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.
Good catch, it should only be the get_info. Updated.
| model=device_model, | ||
| device_version=device_version, | ||
| ) | ||
| except Exception: # noqa: BLE001 |
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 would assume we would get something more specific than a bare exception?
| action-setup: done # No service actions in integration | ||
| appropriate-polling: done # No polling used, push only | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done | ||
| config-flow: done | ||
| dependency-transparency: done | ||
| docs-actions: done # No service actions in integration | ||
| docs-high-level-description: done | ||
| docs-installation-instructions: done | ||
| docs-removal-instructions: done | ||
| entity-event-setup: done # No events handled in entities | ||
| entity-unique-id: done | ||
| has-entity-name: done | ||
| runtime-data: done | ||
| test-before-configure: done | ||
| test-before-setup: done | ||
| unique-config-entry: done | ||
|
|
||
| # Silver | ||
| action-exceptions: done # No service actions in integration | ||
| config-entry-unloading: done | ||
| docs-configuration-parameters: done # No configuration parameters implemented. |
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 add the comments to the comment: part of the rule
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.
should it be status: exempt or status: done in that case?
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.
These read like exempt
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.
Updated! And put the diagnostic and reconfigure flow in todo
| SystemNexa2SwitchEntityDescription( | ||
| key="433Mhz", | ||
| translation_key="433mhz", | ||
| entity_category=EntityCategory.CONFIG, | ||
| ), | ||
| SystemNexa2SwitchEntityDescription( | ||
| key="Cloud Access", | ||
| translation_key="cloud_access", | ||
| entity_category=EntityCategory.CONFIG, | ||
| ), | ||
| SystemNexa2SwitchEntityDescription( | ||
| key="Led", | ||
| translation_key="led", | ||
| entity_category=EntityCategory.CONFIG, | ||
| ), | ||
| SystemNexa2SwitchEntityDescription( | ||
| key="Physical Button", | ||
| translation_key="physical_button", | ||
| entity_category=EntityCategory.CONFIG, | ||
| ), |
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.
What does this show?
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.
Hmm do you mean what is shown in the UI or something else? If in UI it shows configuration switches enabling/disabling the above functions
|
Updated after review comments, hopefully all is answered, I resolved the comments which was just regarding code removal as there was so many unresolved comments it was hard to scroll in Github. |
|
@joostlek and @MartinHjelmare Is there any outstanding changes needed? |
Proposed change
Introduce System Nexa 2 from Nexa to Home Assistant
Type of change
Additional information
Checklist
ruff format 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.Dependency link: https://github.com/konsulten/python-sn2
To help with the load of incoming pull requests:
Update to Improv Wifi Config flow for 2.3 changes #159222
Assign unique IDs to IoTaWatt output sensors #158995