Add Victron Venus integration #130505
Add Victron Venus integration #130505
Conversation
home-assistant
bot
added
cla-error
has-tests
integration: victronvenus
new-integration
labels
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
JohansLab
force-pushed
the
victronvenusinitial
branch
from
94867d9 to
774d7f6
Compare
There was a problem hiding this comment.
Hi @JohansLab
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!
There was a problem hiding this comment.
those things (details of device communication) need to be extracted to a public repository and published to pypi
There was a problem hiding this comment.
I started a library -- but it essentially just had to duplicate all the code, because this integration really only maps messages. If I create a library I would need to map to a proprietary format, and then map again to home assistant format.
There was a problem hiding this comment.
tbh this is never really needed, if it is you might need to restructure
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
please fix the cla errors and remove the mentioned code before you set this back to "ready for review" |
How do I remove the cla-error? I did sign my CLA and the label cla-signed was attached, but the cla-error label wasn't removed. Am I missing something? |
|
Based on comments on discord server the cla-error is sometimes not removed after the error was corrected. The cla-bot does indicate that everything is ok. |
…Pi. Added quality scale.
…ta_description for config flow.
|
Refactors the code and have it depend on new PyPi library. Also updated to comply with new quality standard guidelines. |
| vol.Optional(CONF_USERNAME): str, | ||
| vol.Optional(CONF_PASSWORD): str, |
There was a problem hiding this comment.
can we detect if we need to auth and only show this (in a second step) if we need it?
MartinHjelmare
changed the title
|
All review comments addressed. |
| vol.Optional(CONF_USERNAME): str, | ||
| vol.Optional(CONF_PASSWORD): str, |
| errors["base"] = "cannot_connect" | ||
| except InvalidAuthError: | ||
| errors["base"] = "invalid_auth" | ||
| except Exception: # pylint: disable=broad-except # noqa: BLE001 |
There was a problem hiding this comment.
log the exception here as exception to get rid of the noqa
| data[CONF_INSTALLATION_ID] = installation_id | ||
| unique_id = installation_id |
There was a problem hiding this comment.
| data[CONF_INSTALLATION_ID] = installation_id | |
| unique_id = installation_id | |
| data[CONF_INSTALLATION_ID] = unique_id = installation_id |
| self.friendlyName: str | None = None | ||
| self.modelName: str | None = None |
There was a problem hiding this comment.
snake case
| self.friendlyName: str | None = None | |
| self.modelName: str | None = None | |
| self.friendly_name: str | None = None | |
| self.model_name: str | None = None |
| """Initialize.""" | ||
| self.hostname: str | None = None | ||
| self.serial: str | None = None | ||
| self.installation_id: str | None = None | ||
| self.friendlyName: str | None = None | ||
| self.modelName: str | None = None |
There was a problem hiding this comment.
move them all out of the init and type them only
| assert result["data"][CONF_HOST] == "1.1.1.1" | ||
| assert result["data"][CONF_PORT] == 1883 | ||
| assert result["data"][CONF_USERNAME] == "test-username" | ||
| assert result["data"][CONF_PASSWORD] == "test-password" | ||
| assert result["data"][CONF_SSL] == False # noqa: E712 |
There was a problem hiding this comment.
instead of checking each dict item independently rather check the entire dict for readability
| with patch( | ||
| "homeassistant.components.victronvenus.config_flow.validate_input", | ||
| return_value="INSTALLID", |
There was a problem hiding this comment.
those two tests can be parametrized
There was a problem hiding this comment.
missing a test for checking abort when unique id configure
|
|
||
| type VictronVenusConfigEntry = ConfigEntry[VictronVenusHub] | ||
|
|
||
| __all__ = ["DOMAIN"] |
There was a problem hiding this comment.
| __all__ = ["DOMAIN"] |
shouldn't be necessary
| if hub is not None: | ||
| if isinstance(hub, VictronVenusHub): |
There was a problem hiding this comment.
the hub should always be typed VictronVenusHub, so those two lines should be unnecessary
| "host": "Hostname or IP address of Victron Device, usually mDNS name like 'venus.local'", | ||
| "port": "The MQTT port on the host. Normally it is 1883.", | ||
| "username": "Username for the Victron Device, default is empty. This is not your VRM username.", | ||
| "password": "Password for the Victron Device, default is empty. This is not your VRM password.", | ||
| "ssl": "Indicates whether to use SSL to connect to the Victron Device. Normally it is disabled." |
There was a problem hiding this comment.
All occurrences of "Victron Device" should be "Victron device" as "device" is not part of the product name and will be translated in all languages.
Besides that, all strings look OK to me, can't wait to see this integration going live being a Victron owner myself. As mine is a mostly DC system having a sensor for DC loads would be great, too. 😄
Proposed change
This is a new integration for Victron Venus OS based devices.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Documentation PR: home-assistant/home-assistant.io#35739
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.To help with the load of incoming pull requests: