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 scraping values from Fronius Smart Meter API #101

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jheyduk
Copy link

@jheyduk jheyduk commented Mar 3, 2023

Summary

Added fetching most important values from /solar_api/v1/GetMeterRealtimeData.cgi

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    fix, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update documentation.
  • Update tests.
  • Link this PR to related issues.

Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!
I have some minor issues that I would like to address before merging it.

Comment on lines +47 to +48
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe the flags better. What does "offset for consumed/produced" mean? What unit (if there is)?
If the values are only relevant for the smart meter endpoint, maybe make it symo.smartmeter.offset-{consumed/produced}?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The offset is meant to make it possible to keep the metric in sync with the official electric meter. The unit is Wh.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. I'd propose to update the desc accordingly (better description welcome).

Suggested change
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced")
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset in Wh added to consumed energy to keep in sync with official meter. Only used if smart-meter endpoint is enabled.")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset in Wh added to produced energy to keep in sync with official meter. Only used if smart-meter endpoint is enabled.")

metrics.go Outdated Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
cfg/types.go Outdated Show resolved Hide resolved
Comment on lines +197 to +198
meterEnergyRealSumConsumedVec.WithLabelValues(key).Set(meter.EnergyRealSumConsumed + config.Symo.OffsetConsumed)
meterEnergyRealSumProducedVec.WithLabelValues(key).Set(meter.EnergyRealSumProduced + config.Symo.OffsetProduced)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the smart meter data...
Is this offset actually required or important here? What is the semantic of the offset? Or is this more relevant to your personal use case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

)

type (
symoPowerFlow struct {
SymoPowerFlow struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why expose this type? It's not meant to be public (i.e. usable by other Go packages), but is merely a struct to parse the JSON

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SymoPowerFlow struct {
symoPowerFlow struct {

@@ -64,7 +66,7 @@ type (
}

// SymoArchive holds the parsed archive data from Symo API
symoArchive struct {
SymoArchive struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SymoArchive struct {
symoArchive struct {

pkg/fronius/symo.go Outdated Show resolved Hide resolved
Comment on lines +117 to +118
OffsetConsumed float64
OffsetProduced float64
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this naming alone it's not clear to me what it means or when it's relevant. Please either try to find a better name, or add field comments describing its effects.

Comment on lines +62 to +66
func Test_Symo_GetMeterData_GivenUrl_WhenRequestData_ThenParseStruct(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
payload, err := os.ReadFile("testdata/test_meter.json")
require.NoError(t, err)
_, _ = rw.Write(payload)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing a test + testfile 👍

@ccremer ccremer added the enhancement New feature or request label Mar 4, 2023
@ccremer ccremer changed the title enhancement: Add scraping values from Fronius Smart Meter API Add scraping values from Fronius Smart Meter API Mar 4, 2023
jheyduk and others added 4 commits March 4, 2023 13:41
Co-authored-by: Chris <github.account@chrigel.net>
Co-authored-by: Chris <github.account@chrigel.net>
Co-authored-by: Chris <github.account@chrigel.net>
Co-authored-by: Chris <github.account@chrigel.net>
Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delayed response, I'm busy with work lately.

Comment on lines +47 to +48
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. I'd propose to update the desc accordingly (better description welcome).

Suggested change
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced")
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset in Wh added to consumed energy to keep in sync with official meter. Only used if smart-meter endpoint is enabled.")
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset in Wh added to produced energy to keep in sync with official meter. Only used if smart-meter endpoint is enabled.")

Comment on lines +117 to +118
OffsetConsumed float64
OffsetProduced float64
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OffsetConsumed float64
OffsetProduced float64
// OffsetConsumed in Wh is added to consumed energy to keep in sync with official meter (only used if smart meter endpoint enabled).
OffsetConsumed float64
// OffsetProduced in Wh is added to consumed energy to keep in sync with official meter (only used if smart meter endpoint enabled).
OffsetProduced float64

@@ -64,7 +66,7 @@ type (
}

// SymoArchive holds the parsed archive data from Symo API
symoArchive struct {
SymoArchive struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SymoArchive struct {
symoArchive struct {

)

type (
symoPowerFlow struct {
SymoPowerFlow struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SymoPowerFlow struct {
symoPowerFlow struct {

@@ -159,7 +177,33 @@ func (c *SymoClient) GetArchiveData() (map[string]InverterArchive, error) {
return nil, err
}
defer response.Body.Close()
p := symoArchive{}
p := SymoArchive{}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
p := SymoArchive{}
p := symoArchive{}

@@ -126,7 +144,7 @@ func (c *SymoClient) GetPowerFlowData() (*SymoData, error) {
return nil, err
}
defer response.Body.Close()
p := symoPowerFlow{}
p := SymoPowerFlow{}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
p := SymoPowerFlow{}
p := symoPowerFlow{}

@hkuehl
Copy link

hkuehl commented Sep 21, 2023

Will this PR eventually get merged?

@ccremer
Copy link
Owner

ccremer commented Sep 22, 2023

Will this PR eventually get merged?

I don't know. I have requested changes from the author @jheyduk but never heard anything since.

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

Successfully merging this pull request may close these issues.

3 participants