-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed") | ||
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced") |
There was a problem hiding this comment.
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}
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
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.") |
meterEnergyRealSumConsumedVec.WithLabelValues(key).Set(meter.EnergyRealSumConsumed + config.Symo.OffsetConsumed) | ||
meterEnergyRealSumProducedVec.WithLabelValues(key).Set(meter.EnergyRealSumProduced + config.Symo.OffsetProduced) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymoPowerFlow struct { | |
symoPowerFlow struct { |
@@ -64,7 +66,7 @@ type ( | |||
} | |||
|
|||
// SymoArchive holds the parsed archive data from Symo API | |||
symoArchive struct { | |||
SymoArchive struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymoArchive struct { | |
symoArchive struct { |
OffsetConsumed float64 | ||
OffsetProduced float64 |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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 👍
Co-authored-by: Chris <[email protected]>
Co-authored-by: Chris <[email protected]>
Co-authored-by: Chris <[email protected]>
Co-authored-by: Chris <[email protected]>
There was a problem hiding this 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.
fs.Float64("symo.offset-consumed", config.Symo.OffsetConsumed, "Offset for consumed") | ||
fs.Float64("symo.offset-produced", config.Symo.OffsetProduced, "Offset for produced") |
There was a problem hiding this comment.
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).
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.") |
OffsetConsumed float64 | ||
OffsetProduced float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymoArchive struct { | |
symoArchive struct { |
) | ||
|
||
type ( | ||
symoPowerFlow struct { | ||
SymoPowerFlow struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p := SymoPowerFlow{} | |
p := symoPowerFlow{} |
Will this PR eventually get merged? |
I don't know. I have requested changes from the author @jheyduk but never heard anything since. |
Summary
Added fetching most important values from
/solar_api/v1/GetMeterRealtimeData.cgi
Checklist
fix
,enhancement
,documentation
,change
,breaking
,dependency
as they show up in the changelog