Skip to content
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

Add support to backfill generic inverter data #253

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

suprnova32
Copy link

@suprnova32 suprnova32 commented Jul 23, 2024

This PR adds support to import historical data collected from generic inverters, in particular for data collected via Solar Assistant.

The Solectrus configurator sets the InfluxDB in such a way that it makes it incompatible with the other importers. I initially tried simply changing the headers so that I could use the Sungrow importer but quickly realized that the data was being imported into the wrong measurements.

Here is what the configurator sets up as default:

INFLUX_SENSOR_INVERTER_POWER=pv:inverter_power
INFLUX_SENSOR_HOUSE_POWER=pv:house_power
INFLUX_SENSOR_GRID_IMPORT_POWER=pv:grid_import_power
INFLUX_SENSOR_GRID_EXPORT_POWER=pv:grid_export_power
INFLUX_SENSOR_BATTERY_CHARGING_POWER=pv:battery_charging_power
INFLUX_SENSOR_BATTERY_DISCHARGING_POWER=pv:battery_discharging_power
INFLUX_SENSOR_BATTERY_SOC=pv:battery_soc

So using this adapter, plus also setting the INFLUX_MEASUREMENT_PV variable to pv allows the exported data from Solar Assistant to be imported into Solectrus.

@ledermann
Copy link
Member

Thanks a lot for your work, I really appreciated it!

Yes, to make the import work, InfluxDB must be reachable, so Port 8086 needs to be open.

And you are right, the CSV Importer is not ready for the new sensor configuration (v0.15), it pushes all data to the measurement configured as INFLUX_MEASUREMENT_PV. I need to fix this later, but this is out of scope for this PR.

Before merging, I have two things I would like to ask you:

  • The data which gets imported comes from "Solar Assistant" (never heard of it before). I think it would be a better naming to call the Adapter SolarAssistantRecord (because this is the source of the data) instead of MqttRecord.
  • There is a RuboCop warning and a small issue with the tests. Would you mind fixing them?

@suprnova32
Copy link
Author

In this case yes, the data comes direct from Solar Assistant, which is a great tool btw. It has some good reporting and graphing features (using Grafana), but the biggest feature is that you can actually control your inverter via their WebUI and set some automations for it. It needs to be directly connected to the inverter via a USB or RS2542 cable, though

I named it Mqtt because the live data that I am using is coming from MQTT (via Solar Assistant, as it pushes the data to the MQTT broker), so I wanted to be consistent with the named source. Also because the measurements being used came straight from Solectrus when I chose to configure it using an MQTT source.

I'll fix the test issue.

@suprnova32 suprnova32 force-pushed the develop branch 2 times, most recently from e609fa3 to 1045de7 Compare July 23, 2024 20:48
@ledermann
Copy link
Member

Hm, I'm still not happy with the naming - or I totally misunderstood your use case.

This repository (csv-importer) is designed to import historical data from various sources prior to installing SOLECTRUS. It supports different adapters, named after the software tools that generated the CSV files. MQTT is a protocol, not a software tool, and therefore cannot produce CSV files. I don’t like the name MqttRecord for this reason. If the CSV was created by a software called SolarAssistant, we should name it SolarAssistantRecord.

@suprnova32
Copy link
Author

I understand your point (naming is hard 😅) The reason I named it Mqtt is because I wanted it to remain as generic as possible. It can import CSV files from a variety of sources as long as they comply with the format.

If I'm being 100% honest, I did not generate the CSV file using Solar Assistant. While the data does come from it (it reads values from my inverter via a serial connection, and stores them in a InfluxDB instance), I exported this data from a Grafana dashboard (it was the easiest way to get the CSV file)

I would love for the name to remain as generic as possible, so maybe CSVRecord, GenericInverterRecord, or EnergyRecord are better names.

@suprnova32 suprnova32 changed the title Add support to backfill Mqtt generated data Add support to backfill generic inverter data Jul 25, 2024
@ledermann
Copy link
Member

Ok, I understand your point.

But I still hesitate to merge this PR, because it requires the user to have exactly the same CSV format as the one you provided (with columns PV power, Load power etc.). I'm sure it works fine for your use case, but IMHO it's not as useful for others.

Also, it does not support additional values which can be handled by SOLECTRUS, like heatpump or wallbox, for example.

So, it's not as generic as it could be. In a perfect world, the user should be able to define the columns he wants to import. But of course, implementing this would be a lot of work. Currently, I'm very busy on other parts of SOLECTRUS, so I don't have the time to work on this yet.

What do you think about that? I know the feeling when you've been working on a PR for a while and it doesn't get merged...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants