Add LedSC integration #116957
Add LedSC integration #116957
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @PatrikKr010
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!
home-assistant
bot
added
cla-needed
new-integration
integration: ledsc
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
There was a problem hiding this comment.
An Integration may not communicate directly with devices or services as stated in the development checklist:
https://developers.home-assistant.io/docs/development_checklist/
|
Hi @tr4nt0r, I wrote the WebSC application with which the communication takes place in the interaction. This application is used to remove the hardware part and transfer it to the websocket API. I can separate the integration part into a separate library that will take care of the communication with WebSC, but I find it redundant. Is this separation necessary? |
requests is already part of Home Assistant, so it's quite redundant to add it to the manifest but anyway, the maintainers are pretty strict about this and requests is simply not considered a device specific abstraction layer, it is just an http client |
|
Hi @tr4nt0r |
| return not self._websc.is_lost | ||
|
|
||
| @property | ||
| def name(self) -> str: |
There was a problem hiding this comment.
Use the entity_description attribute instead and use strings.json for device names instead
There was a problem hiding this comment.
I can use entity_description for more detailed device information, but the entity name is dynamic based on the WebSC configuration.
There was a problem hiding this comment.
I think you don't even need a name for the entity, if you leave it empty, and set the name of the device and the manufacturer in the DeviceInfo it will use the generic name "Light", the entity will then be named light.ledsc_devicename_light
There was a problem hiding this comment.
Do is not a very nice default name.
If you don't mind, I would like to leave this property. This name is set in WebSC.
There was a problem hiding this comment.
It's tricky to get this right, haven't done this yet with the light platform. I would like to test it, is there some way to do this? The host demo.ledsc.eu did not work.
There was a problem hiding this comment.
The demo.ledsc.eu domain is still under development. I am currently creating a way to create a virtual instance of WebSC so that anyone who wants to try the demo can run their own instance of WebSC for a limited time. However, for testing purposes I can run one static instance on this domain. Otherwise you can run your own instances of course.
There was a problem hiding this comment.
You can try: http://demo.ledsc.eu:8080/
There was a problem hiding this comment.
Personally, I wouldn't add a comment here, but according to the checklist, the code must pass the check using the
python3 -m script.hassfestcommand, which requires docstring on methods.
In this state the test passes without any errors.
The first line describing the function is ok, what i meant was the second line.
Done |
There was a problem hiding this comment.
It is required to have tests at least for the config flow. Files that are not covered by tests must be added to
.coveragerc
Done
You NEED to add testing for the config flow, it is mandatory for new integrations. If you run scaffold in VS Code and select that you want to add config flow to your integration, it will create some example test files, that should be a good starting point
There was a problem hiding this comment.
I used this way of identification to make it unique for the same connection and it was not possible to connect more integrations to the same WebSC server.
Is this ID global or must it be unique only in my integrations? If is global, I suggest f"ledsc:{host}:{port}".
Please look at the requirements for unique ids https://developers.home-assistant.io/docs/entity_registry_index/#unique-id-requirements
Co-authored-by: Mr. Bubbles <[email protected]>
There was a problem hiding this comment.
I used this way of identification to make it unique for the same connection and it was not possible to connect more integrations to the same WebSC server.
Is this ID global or must it be unique only in my integrations? If is global, I suggest f"ledsc:{host}:{port}".
You have _async_abort_entries_match to make sure, there is only one config entry per ledsc server. That should do the job.
|
I fixed the tests. Now all tests are passing. I added a strings.json file, but it doesn't work. But I don't know if I understand its principle correctly. |
| load the configured devices from WebSC Server and add them to hass. | ||
| """ | ||
| client = WebSClient(host=config.data[CONF_HOST], port=config.data[CONF_PORT]) | ||
| await client.connect() | ||
| hass.async_create_background_task(client.observer(), name="ledsc-observer") | ||
|
|
There was a problem hiding this comment.
| load the configured devices from WebSC Server and add them to hass. | |
| """ | |
| client = WebSClient(host=config.data[CONF_HOST], port=config.data[CONF_PORT]) | |
| await client.connect() | |
| hass.async_create_background_task(client.observer(), name="ledsc-observer") | |
| load the configured devices from WebSC Server and add them to hass. | |
| """ | |
| client = config.runtime_data | |
Initialization of the client should be done in init.py and then stored in the config entry. See https://developers.home-assistant.io/blog/2024/04/30/store-runtime-data-inside-config-entry on how to do this.
|
|
||
| def __init__( | ||
| self, | ||
| client_id: str, | ||
| websc: WebSCAsync, | ||
| hass: HomeAssistant, | ||
| ) -> None: | ||
| """Initialize an LedSC Light.""" | ||
| self._hass: HomeAssistant = hass | ||
| self._websc: WebSCAsync = websc | ||
| self._attr_unique_id = f"{client_id}-{websc.name}" | ||
| _LOGGER.debug(f"LedSC '%s' initialized!", self.name) | ||
|
|
There was a problem hiding this comment.
| def __init__( | |
| self, | |
| client_id: str, | |
| websc: WebSCAsync, | |
| hass: HomeAssistant, | |
| ) -> None: | |
| """Initialize an LedSC Light.""" | |
| self._hass: HomeAssistant = hass | |
| self._websc: WebSCAsync = websc | |
| self._attr_unique_id = f"{client_id}-{websc.name}" | |
| _LOGGER.debug(f"LedSC '%s' initialized!", self.name) | |
| _attr_has_entity_name = True | |
| _attr_translation_key: str = "light" | |
| def __init__( | |
| self, | |
| client_id: str, | |
| websc: WebSCAsync, | |
| hass: HomeAssistant, | |
| ) -> None: | |
| """Initialize an LedSC Light.""" | |
| self._hass: HomeAssistant = hass | |
| self._websc: WebSCAsync = websc | |
| self._attr_unique_id = f"{client_id}-{websc.name}" | |
| _LOGGER.debug("LedSC %s initialized!", websc.name) | |
| self._attr_device_info = DeviceInfo( | |
| manufacturer="LedSC", | |
| model="LedSC", | |
| name=websc.name, | |
| identifiers={(DOMAIN, f"{client_id}-{websc.name}")}, | |
| ) | |
Ok, so I fiddled around a bit and this is what i came up with. No need to set entity_description. It will create a Device with the corresponding Name from the ledsc server. The entity will have a generic name light.
You also will have to add the following to the strings.json file
"entity": {
"light": {
"light": {
"name": "[%key:component::light::title%]"
}
}
}
and as LedSC are led-strips you may also want to add an icons.json and set the icon to a led-strip
{
"entity": {
"light": {
"light": {
"default": "mdi:led-strip-variant"
}
}
}
}
| await hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={"source": config_entries.SOURCE_USER}, | ||
| data=USER_INPUT, | ||
| ) |
There was a problem hiding this comment.
You are testing if the config flow aborts, if an entry already exists. To be able to test this, you set the precondition, that there is already an entry, you just add it to hass, then initialize the config flow.
|
Also i had the impression you are the manufacturer of these devices or somehow associated with them, or am i wrong? If so, maybe this could be interesting for you: |
Co-authored-by: Mr. Bubbles <[email protected]>
|
Hey @PatrikKr010, I just checked your website, ledsc.eu, and apparently I can't order anything. Could you please elaborate on what your plans are and what the product is? Thanks! |
|
Hello, |
|
Thanks for contribution and for the information about the current state of the product. We appreciate the effort you’ve put into developing this integration. However, we only include integrations for products or services that are currently available for our users. This ensures that our community can immediately benefit from and support the integration in further open source development. We recommend publishing your integration as a custom component for now. Once your product is available on the market and the custom integration has had time to mature, we would be happy to re-evaluate and review a new pull request for inclusion of your integration in the core repository. Thank you for understanding, and looking forward to seeing your product’s hitting the market! |
Breaking change
Proposed change
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..coveragerc.To help with the load of incoming pull requests: