Victron GX communication center integration

[tomer-w]

Proposed change

This PR brings the Victron mqtt custom integration to HA. This integration allows monitoring Victron installations locally using the MQTT protocol. It communicates with Victron GX communication centre devices and brings lot of information back to HA. The custom integration which is ported to here is owned by me and mature enough to be ported to HA.

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [x] New integration (thank you!)
• [ ] New feature (which adds functionality to an existing integration)
• [ ] Deprecation (breaking change to happen in the future)
• [ ] Breaking change (fix/feature causing existing functionality to break)
• [ ] Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/41677
• Link to developer documentation pull request:
• Link to frontend pull request:

Checklist

• [x] I understand the code I am submitting and can explain how it works.
• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [x] I have followed the development checklist
• [x] I have followed the perfect PR recommendations
• [x] The code has been formatted using Ruff (ruff format homeassistant tests)
• [x] Tests have been added to verify that the new code works.
• [x] Any generated code has been carefully reviewed for correctness and compliance with project standards.

If user exposed functionality or configuration variables are added/changed:

• [x] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• [x] The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• [x] New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

• [x] I have reviewed two other [open pull requests][prs] in this repository.

[prs]: https://github.com/home-assistant/core/pull/156166, https://github.com/home-assistant/core/pull/156202

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[tomer-w]

When adding new integrations, limit included platforms to a single platform. Please reduce this PR to a single platform

There are really few different domains in this integration, but it will not be useful if I add only one. This PR is full merge of the already working for months custom integration. Also, the code in the integration itself is minimal as all the complexities are in the Victron client library. This is just a thin wrapper. Hopefully, you will be able to approve it as is.

[tomer-w]

Hey @joostlek , thanks for taking a look on this! With regards to your questions:

1. The code diff between one platform or all of them is quite minimal as all the heavy lifting happens in the victron-mqtt package. If I keep only SENSOR it will have really limited functionality. Let me know what you think, Can I later submit it the next day? Do I need to wait for the next HA version? Just FYI, I currently have 344 different entities extracted out of Victron and more are added on a weekly basis. What is nice that this does not require any code change in the HA integration side, just the package it is using.
2. The basic configuration is to connect to remote MQTT broker which is running on the Victron Cerbo GX device itself. Although I have some users which deployed a centralized MQTT server which fetch data from multiple Cerbo GX devices and point the integration to that MQTT server.
3. The operation mode has 3 options: READ_ONLY - This exposes all 344 entities as SENSOR and BINARY_SENSOR. FULL - Entities which are editable by Victron will be editable. This add some risk as just sliding something in the UI can create some real impact on connected system / turn them off / change cost / income generated, etc. EXPERIMENTAL - add latest entities which still didnt get enough feedback yet. I usually mark entities as experimental if they are using some new code base and not just different MQTT topic.

Will be glad to do whatever is needed to get it in.

[joostlek]

1. There's one thing we generally see with these approaches and that is that we can't properly contextualize entities. As in, generally we want to give them a translated name. I have to admit, I haven't looked into the code yet, but what other benefit does having it in the library have over having it in the codebase? To answer the other question, yes we still want to have one platform at a time. This way the review burden is just way lower and that way we can focus on the internals first.
2. Sounds great. However, we don't really want to support custom setups like that. As in, if it works, sure I guess, but we should not maintain code that does not directly integrate with the device IMO.
3. I think if we are adding the sensor entity first, this wouldn't be an issue right now. In generally you get every entity with a core integration, however, if we can make a good argument against something, we can always discuss that. For the experimental one, I think experimental is an interesting term as it isn't clear what this would mean for the end user. As in, will this break at any time? Will it get removed? I think the tinkering phase shouldn't be in production code (as in, if you're confident that it will work, let's merge features, but if you are still testing with users (as you don't have 10 inverters of course) we don't want to merge that directly into core and have people gamble with their setups the next release. We have scripts and ways to help this process, but let's not include this for now.

In any case, feel free to reach out on Discord :)

[tomer-w]

@joostlek,, thanks for the feedback. Here is what I think.

1. You are right that translated names is the only big issue with having the entities defined outside of the integration code. I spent lot of time finding the right solution. What I came with is GitHub workflow which runs nightly, it takes the latest victron_mqtt package, if it is newer from what is already used in the integration, it will open a PR against the integration repo which update the manifest to latest version and fetch all the entities and their English name and update the strings.json. This way I can take new versions with one click and zero manual intervention. I will do the same against the HA core integration when I get it approved. You should take a quick look on the code, you will see it is really empty. There is almost nothing on the HA side.
2. I agree that customers who will try to pull data from a gateway mqtt server is not common, but I had to do really minor tweaks to support it. There are 2 relevant options in the config flow to support that. one is the mqtt prefix and the 2nd is simple naming of entities. With simple naming I remove the Victron deployment ID from the entities ID. This makes the entities much more readable but will not work well if you have multiple Victron attached. Of course, the default is simple naming.
3. I agree I should not expose the experimental feature in the built in integration. I can keep it only in the HACS variant. With regards to the read-only, If you insist on sensors only then it becomes not relevant but when we will add back the editable entities, I think it is really important safety feature. I was surprised that there is no built-in support for defining editable entities as read-only in HA.

Can I at least have sensors and binary sensors to begin with? trying to cut a deal here :). Take a look on the code, it is 40 lines of code for each of those platforms.

[joostlek]

Okay so for

1. I kinda want to push back on that solution, while the platforms get really clear, it becomes a lot harder to maintain and to add new featues to. We currently have a similar issue with Solax where all the entities are described in the library, but the library maintainer is busy with their ever lasting rewrite so they don't merge anything until that's finished, so all kinds of changes are stuck in limbo. So I kinda want to push back and see what we can do to make the libraries task to just gather data (And maybe this can be in a very generic way) and then the integration becomes the part where we match the right data to the right entity and allow it to also be future maintainable. I know that this is probably a different task than you originally came here for, but I think the end result will be more maintainable for both the project and you (as one of the benefits of a core is that you share responsibiltiy with the project, so the integration doesn't pause when you're on holiday for example). But I do think it's worth it in the long run and I am happy to help out where I can here.
2. Right, so that'd be something I have to discuss with others. We can always leave that for a later PR.
3. I don't insist on keeping it sensor for now, it's more like, we want the initial PR to be small, after which we can add more platforms and the code needed to support these platforms. There are some other integrations where security is a thing, but so far that never posed to be a problem. But maybe I am missing some impact info, and we can always find out what the best way to support those entities is.

[tomer-w]

I removed all platforms other than sensor and binary_sensor. I also removed the operation mode as it becomes obsolete till we add writable entity types. I also did some other minor cleanup in the config flow of option which can be removed from the built in integration.

With regards to where entities are defined. I dont see easy way to disconnect this from the library itself. Some entities are simple, some entities depend on multiple mqtt topics to be able to define their value, name, are they writable or not, etc. There is CLI which expose it to people who need CLI support and standalone app for testing which gives the same capabilities of what you can do in HA without using HA. The library has its own reason to live, and also tons and tons of tests, some of them are depended on the Victron simulator docker. I dont think I want to move any of this to HA repo as of complexity of coding and the complexity of involved processes. I actually love HA guidance about separating this all to a pypi package which has its own life. In a way, if Victron itself would generate this library it would be the same way, they will expose all the entities, the HA integration would just map those to HA constructs.

I think the integration is ready for you / community review. We can discuss more after you take a look on how it is built.

[tomer-w]

Forgot to mention that from the pypi package I also generate the most detailed Victron MQTT documentation exists currently. So, this is another value of having it sperate from HA.

[joostlek]

In a way, if Victron itself would generate this library it would be the same way, they will expose all the entities, the HA integration would just map those to HA constructs.

I would disagree here. Entities are a HA construct. I don't think that if you'd build a library for your own inverters that you will think in entities like we do.

Are all the created entities in https://github.com/tomer-w/victron_mqtt/blob/main/victron_mqtt/_victron_topics.py? I don't see any special logic here though, also regarding write and state topics. I also see things that should not be entities in HA as they contain static data or belong in other parts of the integration. Are there other parts I should look at here that you think are of significance? I will also discuss this with some others to make sure I am not biased. It's just slightly annoying that with this approach we shut down a lot of possible UX improvements and future improvements to come.

[tomer-w]

1. The package does not expose entities, it exposes metrics. There is no 1x1 mapping between metric types and entity types. It is correct that you can map one to the other but this will be true to any package which exposes something which is later becomes HA entity. You can see the package has zero dependency on HA constructs.
2. Yes, that file has most of raw data but if you look closely, you will see it is complex. There are topics defined as MetricKind.ATTRIBUTE which are helping other metrics or define device properties. There are topics starting with $$func as they need helper functions to be calculated. There are metrics which contains {cell_id(1-16)} or other monikers which map between different part of the topics and the generated metrics. And I can go on with more complexities. The bottom line is that the library has life of its own. It exposes Victron complex topic structure as useful metric objects, the integration is mapping them to entities which is fully align with the documentation of how to build an integration. Not to mention the amount of testing which is done in the library for all the specific topics that has unique problems against the Victron official simulator. If you want to understand better the functionality of the library, you will really need to review the rest of the files there.
3. You mentioned problem with some other integration which constant refactor in the supporting library is blocking us from adding new entities easily. I agree this sucks, but the right solution is that if someone is doing major work on the supporting library, they should do it on a side branch which is separate from the main branch which can still take small additions. This is the right coding practice in any major work. I dont think there is anything unique in this case. If they dont do it this way, I dont think it is fair to say that the system is broken. They are just not using it right.
4. You mentioned that this design will block future HA innovation. Can you specify why this is the case? I dont see why this would happen. If the innovation is in HA side, everything should work as expected. If there are changes which can be done in the library, it will be similar to any 3rd party library which integrations depends on. I really dont see what is unique in my case.

Will be glad for further thinking about this topic, I think Victron with each wide adoption in boating / RV / EV / home power generation deserve full blown integration and I spent hundreds of hours creating one which is actually very easy to extend and maintain.

[joostlek]

1. Check
2. Yea I saw that there are more files, but to me it didn't seem very complex hence I had the feeling I missed some parts hence the question :)
3. Oh I absolutely agree, but as a project we can't demand libraries to work like we want or expect. I also remember moments where I was refactoring things, and I can agree that if you're refactoring a lot and there are still changes happening on your main branch which you also need to incorporate in your refactor, it can become a scope and time creep. But then to keep blocking all new innovations until you have time is also a bit shit, but like said, we can't force people to do things (and I also don't want to).
4. I think most of the innovation that we will see happening is adding even more context to an integration. The current approach would already make adding an entity category, or disabling less popular entities by default very hard, unless you define that in the library. Also things like translations. There are some ideas floating around to add even more context to entities and devices (as today a device is just a generic device) and to enhance that experience (for example, if you have 2 temperature sensors in your room and a 3D printer, today we would also add your nozzle temperature to the rooms average, which isn't fair, because we currently don't know what the purpose of a temperature sensor is. While these ideas aren't here today yet, I expect these to have an impact.

Side question, would it make sense to have a secondary mapping in Home Assistant? This way we can still keep the metrics in the library, but we add the context in Home Assistant.

But yes, I do see the value of having such integration in core and I also don't want to devalue or throw away all the work you put in so far, hence I'm loving this back and forth where we're just sharing concerns and trying to understand each other and our concerns :)

[tomer-w]

I'm not sure I understand your question / idea about side mapping. The metric coming from the library already has all the information needed to construct HA entity and to put it in the relevant device. The only thing which still need to be done in the integration side is to add it to string.json and I'm doing it as a github action.

[joostlek]

Yes and no, there are some things that I saw in the list that we don't make entities for (I don't have the list at me right now, but things like serial number, mac address, device name, device model). But my question now was more like, would it make sense to map from a metric to an entity description and add necessary context that way.

We also don't have any github action in place for a specific integration. Doesn't mean we can't get any, but I currently don't see a high chance of that being included right away.

[tomer-w]

The GitHub action will run from the library repository when I publish new version. It will generate PR with the required changes. I don't expect core to run any actions for me.

[joostlek]

Ah check. Could you also elaborate on the extra mapping? If you don't have an answer right now or would like examples, I can check if I can find some. But then I have a complete picture and can discuss it with some others to see how they look at it :)

[tomer-w]

I am not sure what is the question about "extra mapping". I will be glad to provide more information but missing the context.

[joostlek]

My question is more like, currently we have the entities in HA interpret the metric and derive the info from there. What if we add a mapping in between so that every metric that we want to add to HA needs to have an entry in HA with extra context (like translation key, enum options, entity category). This would mean that we can't fully automate this (while that action wouldn't run on our repository, it's still something I don't expect we want at this point in time), but this does ensure that we have the ability to provide more context and to not create entities for the things we don't want in the state machine.

[tomer-w]

@joostlek, I am probably missing your point. Each metric which is exposed from the library is exposing the information needed for generating translation key, enum options, min and max values, etc. Everything is defined in a single place which is the library. This library has an option to generate json file which contains the most accurate specification of Victron MQTT topics. This JSON is also the source for this documentation site. This is the same approach which is used by the canboat project which is a source repository contains C code which defines all known NMEA 2000 data and scripts creating json / HTML files with the raw data which other library use to create python code, which later becomes HA entities. There is no way that everytime someone finds new NMEA 2000 message or Victron topic, they will need to update HA inetgration. Those data sources have life of their own which is much wider than HA. You cant expect people who work with those protocols to also come here and update HA.

I did finish creating all the tests so from my perspective, the code is ready to be checked in. Let me know what is missing or if you find specific problems with my code.

[joostlek]

Each metric which is exposed from the library is exposing the information needed for generating translation key, enum options, min and max values, etc. Everything is defined in a single place which is the library.

But then still you'd be missing out on the entity category and disabling entities by default and any future context.

There is no way that everytime someone finds new NMEA 2000 message or Victron topic, they will need to update HA inetgration.

Well that is how we currently work. Like said, we don't accept all entities as is as not everything should belong in the state machine.

But like said before, this is just trying to figure out how this can work and doing due diligence for that, I am not saying that this is a must. I will now check with others what they believe is the best way forward here.

[tomer-w]

Not sure what you mean by entities categories. In Victron there are devices: batteries, solar panels, inverters, generators, etc. They are exposed by the library as well and each entity will show up under the right device. You can have 6 batteries each expose the exact same entities with different values.

[tomer-w]

@joostlek , adding a screenshot of sample Victron setup with Inverter, solar panel, Shunt and couple of tanks:

[joostlek]

We have entity_category to allow us to mark entities as diagnostic to avoid rendering on the default dashboard and to avoid using in for example area temperatures

[tomer-w]

Ok, not sure if this is super relevant in my case, but if needed, I can add it to the library or to have list of ids in the integration which if exists will be marked as diagnostics. I don't think there is anything in the overall design which blocks any of the options.

[tomer-w]

@joostlek, I liked your diagnostic idea. I changed the integration to mark some entities as diagnostics and some as disabled by default. There was nothing blocking in doing it. Only couple of lines of code. I think it demonstarte there is no major problem in the design.

[tomer-w]

I removed the binary sensor platform so now it is fully aligned with the "single platform" rule for new integration. I created another PR for adding it back here: https://github.com/home-assistant/core/pull/157617. I will keep it as draft till this one is approved.

[joostlek]

Okay so I discussed this with the core team and first off, thanks for your patience.

In a way we're fine with this approach in this case. Although there are some architectural things we should keep in mind, but that's just a matter of how we implement it. Like mentioned before, there are things that we strictly don't want in the state machine as we have other places for them, and I think it would be nice to come up with a nice test suite that will help us

1. make sure that all entities that can possibly be created are tested (we can use snapshots for this).
2. Make sure that every entity has a translation key (the first step will help with this)
3. Make sure changes in the library doesn't result in breaking changes in core. For example, if you change the unique id (which is sourced from the library) we should make sure that HA migrates that properly. And I think having a proper test suite that ensures this will be useful for the maintabiltiy.

Furthermore we think that using entity descriptions to map from a MetricType to the details of the metric type (Device class, state class and whatnot) would be useful to avoid having large if statements in the entities.

And for the automated CI action you talked about, I haven't spoke about that yet, let's first get this merged, but I still can't give guarantees if that's expected or wanted.

If there are any questions, feel free to ask or send me a message on Discord :)

[tomer-w]

@NoRi2909, I appreciate your valuable feedback about the strings. I fixed all the issue you raised. Thanks!!

[NoRi2909]

@tomer-w I'm very happy to see the progress you make in bringing this integration into Home Assistant Core as I have a live Victron system here myself. 😃

As one of the two main contributors to the German translation I will try to help you get the English strings at 100% accuracy so we can produce a comparably good translation here, too.

[tomer-w]

@NoRi2909, I didnt noticed you are a victron_mqtt expert :). If you have lot of additional comments, it might be better if you will open a PR on the real source which is here: https://github.com/tomer-w/victron_mqtt/blob/main/victron_mqtt/_victron_topics.py For Now, I just re-apply your comments manually.

[NoRi2909]

@tomer-w I'm neither a programmer nor an MQTT expert. I just happen to be very active as a German translator on Home Assistant and have also built our Victron PV system here at home.

So my comments will focus entirely on the user-facing strings, not actual code. There will probably be just a few more strings to address at this point.