Feature/ha discovery #27

dominikandreas · 2023-11-12T21:57:52Z

This is a work in progress, but wanted to get some early feedback. The main contributions of this PR are:

Implemented support for home assistant discovery (Home Assistant Addon #17)
Add extra sensors (publish more metrics #14)

Other notable changes / additions:

added vscode devcontainer support and debug launch configuration to simplify development for future contributors
add serde dependency for json serialization
version bump to 0.2.0 because of breaking changes (mqtt paths change)
refactor log init into its own module
add mock_state function to inverter for testing purposes

Note that this is my first time developing in rust and there's probably a lot of things to improve / things that an experienced rust developer would've probably done differently. I'm happy to receive some constructive feedback and improve where necessary :-)

TODO:

update home assistant addon once PR is merged / docker build is ready
make home assistant prefix configurable (Tool configuration should come from file #25)

DennisOSRM · 2023-11-13T05:46:46Z

Thanks for the PR. It will take a couple of days before I can get around to reviewing this.

From a quick glance, it seems this PR contains a couple of things that likely should be seperate PRs, and the testing code perhaps should not bleed into the implementation. That being said, this isn't a comprehensive review and just an initial impression.

Thanks for the contribution, please keep up the great work.

DennisOSRM

Thanks again for filing this PR. I think it contains very interesting and worthwhile ideas. Since the PR is rather big and contains several unrelated changes, I'd suggest to do the following:

remove the development related files and directories
the testing code should be done in a distinct PR without having to branch between test and non-test code. I.e. the implementation should be oblivious to the fact whether it is run in a test environment or not.
move structs into their own source file
consider using protobuf_json crate instead of the rolling your own conversion to json
implement the auto discovery in a non-breaking way

While the above may seem like a lot, I think the PR is a great first stab into the right direction. The intent for the PR is great, please keep up the good work.

DennisOSRM · 2023-11-18T17:25:19Z

.devcontainer/Dockerfile

@@ -0,0 +1 @@
+FROM rust:buster


The files in .devcontainer should not be part of this PR.

DennisOSRM · 2023-11-18T17:26:08Z

.vscode/launch.json

@@ -0,0 +1,21 @@
+{


The files in .vscode/* should not be part of this PR.

DennisOSRM · 2023-11-18T17:28:37Z

src/inverter.rs

@@ -39,7 +40,42 @@ impl<'a> Inverter<'a> {
        }
    }

-    pub fn update_state(&mut self) -> Option<HMSStateResponse> {
+    pub fn mock_state(&mut self) -> Option<HMSStateResponse> {


The test code should be separated from the implementation. This could be done in a tool simulating the behavior of an actual inverter. Issue #11 is tracking such a feature. This should be dropped from the PR.

DennisOSRM · 2023-11-18T17:30:04Z

src/main.rs

            mqtt.publish(&r);
        }

        // TODO: this has to move into the Inverter struct in an async implementation
-        thread::sleep(Duration::from_millis(REQUEST_DELAY));
+        if !cli.test{


See above for the comment that API simulation should implemented in #11.

DennisOSRM · 2023-11-18T17:31:27Z

src/mqtt.rs

+impl HMSStateResponse {
+    fn get_model(&self) -> String {
+         // TODO: identify model from dtu_sn
+        "HMS-800W-T2".to_string()


This is not necessarily the inverter. While tested, the tool likely works with all devices of the HMS-XXXX-2T series.

Right, since we currently don't have a way to determine the exact model name, I've removed the mention of 800W-2T

DennisOSRM · 2023-11-18T17:34:59Z

src/mqtt.rs

+}
+
+
+impl DeviceConfig {


consider moving the struct into a separate source file

DennisOSRM · 2023-11-18T17:36:10Z

src/mqtt.rs

+            identifiers,
+            manufacturer: "Hoymiles".to_string(),
+            /// Rust compiler sets the CARGO_PKG_VERSION environment from the Cargo.toml . 
+            sw_version: env!("CARGO_PKG_VERSION").to_string(),


This is arguably not part the version of the inverter device

No, but I thought it makes sense to log the version of the publisher here (we can also change it in the future in case we figure out a way to extract the sw version of the inverter)

DennisOSRM · 2023-11-18T17:36:46Z

src/mqtt.rs

+/// https://developers.home-assistant.io/docs/core/entity/sensor/
+///
+#[derive(Serialize)]
+pub struct SensorConfig {


consider moving this to a separate source file

DennisOSRM · 2023-11-18T17:39:27Z

src/mqtt.rs

+}
+
+
+fn create_sensor_configs(hms_state: &HMSStateResponse, state_topic: &str) -> Vec<SensorConfig> {


should this be a ::from(...) member function rather than a free factory?

DennisOSRM · 2023-11-18T17:40:32Z

src/mqtt.rs

-        self.client
-            .subscribe("hms800wt2/pv_current_power", QoS::AtMostOnce)
-            .unwrap();
-        match self.client.publish(


this seems like a breaking change.

dominikandreas · 2023-11-18T19:20:12Z

Thanks for your comprehensive review!

remove the development related files and directories

I will create a separate PR for this

the testing code should be done in a distinct PR without having to branch between test and non-test code.

Good point. Didn't have time to address this properly so just hacked in something quickly that allowed me to do some minimal testing during the addon development. I will remove the testing related code for now

move structs into their own source file

Will do. Wasn't sure how to structure it or whether it's necessary to split the mqtt implementation up in two parts, one for home assistant, one without home assistant support.

consider using protobuf_json crate instead of the rolling your own conversion to json

Thought about this as well, but since the values are scaled during the serialization, I decided to just do it the manual way. I assume the protobuf schemas won't often if at all, so I hope that's okay.

implement the auto discovery in a non-breaking way

The breaking change is unfortunate. But apart from home assistant support, another upside is that it would allow utilizing multiple inverters in the future (part of the serial number is added to the topic). Another part of the breaking change is that the whole update is sent json-encoded to a single topic instead of splitting it to multiple topics. This should make it more efficient in combination with home assistant (and possibly other endpoints)

DennisOSRM · 2023-11-18T19:28:59Z

The breaking change is unfortunate. But apart from home assistant support, another upside is that it would allow utilizing multiple inverters in the future (part of the serial number is added to the topic). Another part of the breaking change is that the whole update is sent json-encoded to a single topic instead of splitting it to multiple topics. This should make it more efficient in combination with home assistant (and possibly other endpoints)

That's a good point. Thanks for the clarification. I think the json-encoded single topic could be done independently, is that right? If so, we could first add things, and consider the breaking change at a later point. Would this make sense?

dominikandreas · 2023-11-18T20:23:02Z

Yes, we could change the home assistant device configurations and publish on separate topics to delay the breaking change. But if we're certain it's going to happen sooner or later (and I think supporting multiple inverters is a enough good reason), delaying the breaking change will only cause more users to be affected.

While we're at it we should also remove the "800W-2T" references, e.g. from the topic, to prevent a future breaking change.

DennisOSRM · 2023-11-18T20:31:10Z

Ok, that's a convincing argument. Let's do the breaking change now, and aim at the same time to minimize the size of this PR.

…very

…ency scale

dominikandreas · 2023-11-19T21:36:30Z

I think I addressed all of the review comments and fixed some additional bugs. Here's an example of the provided data in home assistant:

In order to enable these changes in the addon, we first need an image on docker hub that I can reference in the Dockerfile of the home assistant addon. I have a build running right now, but it's taking a while. I will hopefully push another update tomorrow when it's done

DennisOSRM · 2023-11-21T19:00:11Z

Very cool. Please have a bit of patience, since it will again take a couple of days before I can get to it.

DennisOSRM

run cargo fmt
run cargo clippy
move non-functional changes into distinct PRs to separate concerns

This is looking really close. Couple of remarks on the PR that shouldn't take too long to resolve. Then we can get this in.

To give some perspective of where I am taking the architecture of the tool are probably helpful. Similar to the idea of supporting several inputs, I'd like the tool to support several output channels (for a lack of a better name). I.e. there's a simple MQTT publisher, there's the HA autodiscovery and publishing of this PR, and other outputs like InfluxDB and perhaps others. The user will be able to configure where to publish.

Once this PR is ready, I want to start implementing these channels by building right on top of this branch and then merge the two in short succession once both are ready.

Cargo.toml

.gitignore

src/inverter.rs

src/mqtt.rs

src/mqtt_schemas.rs

src/protos/RealData.proto

src/main.rs

DennisOSRM · 2023-11-23T17:09:21Z

I did a very hasty implementation of output channels (cf. #27 (review)) on top of your PR: 7460c0e

DennisOSRM · 2023-11-23T17:11:46Z

src/mqtt.rs

@@ -1,9 +1,12 @@
 use std::{thread, time::Duration};


in light of the output channel feature (see below) it probably makes sense to rename this file to home_assistent.rs since it will implement the output channel to a HA instance.

DennisOSRM

Will merge changes and do a couple of minor changes on top. Thanks so much for the contribution

dominikandreas · 2023-11-24T19:52:59Z

Sorry I was preoccupied with my day job and family. I'll have a look at the output channels PR

DennisOSRM · 2023-11-24T20:23:00Z

No worries. Life comes first, hacking on code second. 😉

Dominik Andreas added 13 commits November 6, 2023 20:39

initial commit for home assistant addon

21df148

added repository.json

d1b89c6

add url to config.yaml

4e29be6

update archs in config.yaml

857862c

config updates

0d64a6f

fix typo in repository.json

3a1c408

Merge remote-tracking branch 'origin_orig/main'

102375d

add mock_state to inverter for testing purposes

dab85be

refactor log init into its own module

dfb0953

implement home assistant mqtt discovery, add extra sensors

8a61018

add serde dependency for serialization, version bump

ba90157

add vscode devcontainer support to simplify development

e99dacc

add vscode debug launch configuration

97dd101

DennisOSRM requested changes Nov 18, 2023

View reviewed changes

remove unrelated .devcontainer .vscode changes from PR

60ee595

remove testing related code

5a8a5a3

dominikandreas force-pushed the feature/ha_discovery branch from 73cd9fb to d4e0540 Compare November 18, 2023 21:37

Dominik Andreas added 7 commits November 18, 2023 22:37

remove 800W from model name

d4e0540

Merge remote-tracking branch 'origin_orig/main' into feature/ha_disco…

25882c8

…very

merge fix

ccf2662

move mqtt schemas into separate file

3c46193

change dtu_sn to string, fix efficiency sensor

1a586df

ha-addon: acquire mqtt config via bashio

2f46752

fix inverter handling, add grid frequency and voltage, fix grid frequ…

95131b3

…ency scale

DennisOSRM reviewed Nov 23, 2023

View reviewed changes

DennisOSRM mentioned this pull request Nov 24, 2023

Implement output channels #34

Merged

Update Cargo.toml

417ef8b

DennisOSRM approved these changes Nov 24, 2023

View reviewed changes

DennisOSRM merged commit 0685bac into DennisOSRM:main Nov 24, 2023
1 check passed

This was referenced Nov 24, 2023

Support home assistant auto-discovery #24

Closed

Home Assistant Addon #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ha discovery #27

Feature/ha discovery #27

dominikandreas commented Nov 12, 2023 •

edited

Loading

DennisOSRM commented Nov 13, 2023

DennisOSRM left a comment

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

dominikandreas Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

dominikandreas Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

DennisOSRM Nov 18, 2023

dominikandreas commented Nov 18, 2023

DennisOSRM commented Nov 18, 2023

dominikandreas commented Nov 18, 2023

DennisOSRM commented Nov 18, 2023

dominikandreas commented Nov 19, 2023

DennisOSRM commented Nov 21, 2023

DennisOSRM left a comment •

edited

Loading

DennisOSRM commented Nov 23, 2023

DennisOSRM Nov 23, 2023

DennisOSRM left a comment

dominikandreas commented Nov 24, 2023

DennisOSRM commented Nov 24, 2023

		}


		fn create_sensor_configs(hms_state: &HMSStateResponse, state_topic: &str) -> Vec<SensorConfig> {

		}


		impl DeviceConfig {

Feature/ha discovery #27

Feature/ha discovery #27

Conversation

dominikandreas commented Nov 12, 2023 • edited Loading

DennisOSRM commented Nov 13, 2023

DennisOSRM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikandreas commented Nov 18, 2023

DennisOSRM commented Nov 18, 2023

dominikandreas commented Nov 18, 2023

DennisOSRM commented Nov 18, 2023

dominikandreas commented Nov 19, 2023

DennisOSRM commented Nov 21, 2023

DennisOSRM left a comment • edited Loading

Choose a reason for hiding this comment

DennisOSRM commented Nov 23, 2023

Choose a reason for hiding this comment

DennisOSRM left a comment

Choose a reason for hiding this comment

dominikandreas commented Nov 24, 2023

DennisOSRM commented Nov 24, 2023

dominikandreas commented Nov 12, 2023 •

edited

Loading

DennisOSRM left a comment •

edited

Loading