Closed
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- There is a lot of copy-pasted code between
meter/fritzdectandmeter/fritzdect_new(types, NewConnection, ExecCmd, session helpers); consider extracting the shared parts into a common package or file and parameterizing only the behavioral differences to avoid divergence over time. ParseFXmlinmeter/fritzdect_new/fritzdect_new.gosilently ignores XML and parsing errors and assumesv.Power.Values[0]exists; it would be more robust to return an error when unmarshalling fails or when the stats slice is empty, and haveCurrentPowerpropagate that error instead of returning a potentially misleading value.- The new API wiring uses
registry.Add("fritzdect_new", NewFritzDECTFromConfig2)with a*_2constructor name andutil.NewLogger("fritzdect")in thefritzdect_newpackage; aligning naming/logging (e.g.,NewFritzDECTNewFromConfigand a"fritzdect_new"logger) would make the intent clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is a lot of copy-pasted code between `meter/fritzdect` and `meter/fritzdect_new` (types, NewConnection, ExecCmd, session helpers); consider extracting the shared parts into a common package or file and parameterizing only the behavioral differences to avoid divergence over time.
- `ParseFXml` in `meter/fritzdect_new/fritzdect_new.go` silently ignores XML and parsing errors and assumes `v.Power.Values[0]` exists; it would be more robust to return an error when unmarshalling fails or when the stats slice is empty, and have `CurrentPower` propagate that error instead of returning a potentially misleading value.
- The new API wiring uses `registry.Add("fritzdect_new", NewFritzDECTFromConfig2)` with a `*_2` constructor name and `util.NewLogger("fritzdect")` in the `fritzdect_new` package; aligning naming/logging (e.g., `NewFritzDECTNewFromConfig` and a `"fritzdect_new"` logger) would make the intent clearer and easier to maintain.
## Individual Comments
### Comment 1
<location path="meter/fritzdect_new/fritzdect_new.go" line_range="30" />
<code_context>
+ return 0, err
+ }
+
+ var f = ParseFXml(resp, err)
+ return (f * 10) / 1000, err // 1/100W ==> W
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Parsing/decoding errors for device stats are silently ignored and can lead to incorrect zero readings or panics.
`ParseFXml` can fail at several points (XML unmarshal, missing `Power.Values`, malformed CSV, `ParseFloat`), but all errors are ignored and a float is still returned. Together with accessing `v.Power.Values[0]` without checking its length, this can either panic when the slice is empty or silently return 0 on any parse failure while `err` from `ExecCmd` remains nil. Please have `ParseFXml` return `(float64, error)`, validate `v.Power.Values`/`parts` before indexing, and propagate parsing errors back through `CurrentPower` so callers can distinguish real zeros from failures.
</issue_to_address>
### Comment 2
<location path="meter/fritzdect_new/types.go" line_range="38-47" />
<code_context>
- Energy Energy `xml:"energy"`
-}
-
-// Energy structures getbasicdevicesstats command energy response (AHA-HTTP-Interface)
-type Energy struct {
- XMLName xml.Name `xml:"energy"`
</code_context>
<issue_to_address>
**suggestion (typo):** Type comments for `Voltage` and `Power` appear to be copy-pasted and incorrectly refer to energy.
Both comments currently say `Energy structures ... energy response`, which appears to be a copy‑paste mistake. Please update them to accurately describe the `Voltage` and `Power` types to avoid confusion when reading the code.
Suggested implementation:
```golang
// Voltage structures getbasicdevicesstats command voltage response (AHA-HTTP-Interface)
type Voltage struct {
XMLName xml.Name `xml:"voltage"`
Values []string `xml:"stats"`
}
```
```golang
// Power structures getbasicdevicesstats command power response (AHA-HTTP-Interface)
```
I only see the comment for `Power` but not the full type definition. Please ensure that the updated `Power` comment directly precedes the `type Power struct { ... }` definition, mirroring the style used for `Energy` and `Voltage`.
</issue_to_address>
### Comment 3
<location path="meter/fritzdect/fritzdect_common.go" line_range="49" />
<code_context>
- return fritzdect, nil
-}
-
-// ExecCmd execautes an FritzDECT AHA-HTTP-Interface command
-func (c *Connection) ExecCmd(function string) (string, error) {
- // refresh Fritzbox session id
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo in the `ExecCmd` comment.
Please change `execautes` to `executes` in the comment to fix the typo.
```suggestion
// ExecCmd executes an FritzDECT AHA-HTTP-Interface command
```
</issue_to_address>
### Comment 4
<location path="meter/fritzdect_new/fritzdect_common.go" line_range="20" />
<code_context>
- Values []string `xml:"stats"`
-}
-
-// NewConnection creates FritzDECT connection
-func NewConnection(uri, ain, user, password string) (*Connection, error) {
- if uri == "" {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the duplicated FritzDECT connection and authentication logic into a shared internal package and have both fritzdect and fritzdect_new reuse it instead of maintaining two separate copies.
You’re duplicating the existing FritzDECT connection/auth logic under a new package name; that increases maintenance complexity unnecessarily. Instead, extract the shared pieces into a common internal helper and reuse them from both packages.
Actionable steps:
1. **Create a shared internal package** (e.g. `meter/fritzdect/internalconn`) that holds the common types and logic:
```go
// meter/fritzdect/internalconn/conn.go
package internalconn
import (
"crypto/md5"
"encoding/hex"
"encoding/xml"
"errors"
"fmt"
"net/url"
"strings"
"time"
"github.com/evcc-io/evcc/api"
"github.com/evcc-io/evcc/util"
"github.com/evcc-io/evcc/util/request"
"github.com/evcc-io/evcc/util/transport"
"golang.org/x/text/encoding/unicode"
)
const sessionTimeout = 5 * time.Minute // or import from existing code
type Settings struct {
URI string
AIN string
User string
Password string
}
type Connection struct {
*request.Helper
*Settings
SID string
updated time.Time
}
func NewConnection(loggerName, uri, ain, user, password string) (*Connection, error) {
if uri == "" {
uri = "https://fritz.box"
}
if ain == "" {
return nil, errors.New("missing ain")
}
settings := &Settings{
URI: strings.TrimRight(uri, "/"),
AIN: ain,
User: user,
Password: password,
}
log := util.NewLogger(loggerName).Redact(password)
conn := &Connection{
Helper: request.NewHelper(log),
Settings: settings,
}
conn.Client.Transport = request.NewTripper(log, transport.Insecure())
return conn, nil
}
func (c *Connection) ExecCmd(function string) (string, error) {
if time.Since(c.updated) >= sessionTimeout {
if err := c.getSessionID(); err != nil {
return "", err
}
c.updated = time.Now()
}
parameters := url.Values{
"sid": {c.SID},
"ain": {c.AIN},
"switchcmd": {function},
}
uri := fmt.Sprintf("%s/webservices/homeautoswitch.lua", c.URI)
body, err := c.GetBody(uri + "?" + parameters.Encode())
res := strings.TrimSpace(string(body))
if err == nil && res == "inval" {
err = api.ErrNotAvailable
}
return res, err
}
func (c *Connection) getSessionID() error {
uri := fmt.Sprintf("%s/login_sid.lua", c.URI)
body, err := c.GetBody(uri)
if err != nil {
return err
}
var v struct {
SID string
Challenge string
BlockTime string
}
if err = xml.Unmarshal(body, &v); err == nil && v.SID == "0000000000000000" {
var challresp string
if challresp, err = createChallengeResponse(v.Challenge, c.Password); err == nil {
params := url.Values{
"username": {c.User},
"response": {challresp},
}
if body, err = c.GetBody(uri + "?" + params.Encode()); err == nil {
err = xml.Unmarshal(body, &v)
if v.SID == "0000000000000000" {
return errors.New("invalid user or password")
}
c.SID = v.SID
}
}
}
return err
}
func createChallengeResponse(challenge, pass string) (string, error) {
encoder := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder()
utf16le, err := encoder.String(challenge + "-" + pass)
if err != nil {
return "", err
}
hash := md5.Sum([]byte(utf16le))
md5hash := hex.EncodeToString(hash[:])
return challenge + "-" + md5hash, nil
}
```
2. **Use the shared connection in the existing `fritzdect` package**:
```go
// meter/fritzdect/fritzdect_common.go
package fritzdect
import "github.com/evcc-io/evcc/meter/fritzdect/internalconn"
type Connection = internalconn.Connection
func NewConnection(uri, ain, user, password string) (*Connection, error) {
return internalconn.NewConnection("fritzdect", uri, ain, user, password)
}
```
3. **Use the same shared connection in the new `fritzdect_new` package**:
```go
// meter/fritzdect_new/fritzdect_new.go
package fritzdect_new
import "github.com/evcc-io/evcc/meter/fritzdect/internalconn"
type Connection = internalconn.Connection
func NewConnection(uri, ain, user, password string) (*Connection, error) {
return internalconn.NewConnection("fritzdect", uri, ain, user, password)
}
```
You can still customize behavior per package (e.g. logger name, additional methods) without copying all the auth/session/ExecCmd plumbing. This keeps all functionality intact while removing the duplicate implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
meter/fritzdect_new/fritzdect_new.go
Outdated
| return 0, err | ||
| } | ||
|
|
||
| var f = ParseFXml(resp, err) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Parsing/decoding errors for device stats are silently ignored and can lead to incorrect zero readings or panics.
ParseFXml can fail at several points (XML unmarshal, missing Power.Values, malformed CSV, ParseFloat), but all errors are ignored and a float is still returned. Together with accessing v.Power.Values[0] without checking its length, this can either panic when the slice is empty or silently return 0 on any parse failure while err from ExecCmd remains nil. Please have ParseFXml return (float64, error), validate v.Power.Values/parts before indexing, and propagate parsing errors back through CurrentPower so callers can distinguish real zeros from failures.
Comment on lines
+38
to
+47
| // Energy structures getbasicdevicesstats command energy response (AHA-HTTP-Interface) | ||
| type Energy struct { | ||
| XMLName xml.Name `xml:"energy"` | ||
| Values []string `xml:"stats"` | ||
| } | ||
|
|
||
| // Energy structures getbasicdevicesstats command energy response (AHA-HTTP-Interface) | ||
| type Voltage struct { | ||
| XMLName xml.Name `xml:"voltage"` | ||
| Values []string `xml:"stats"` |
Contributor
There was a problem hiding this comment.
suggestion (typo): Type comments for Voltage and Power appear to be copy-pasted and incorrectly refer to energy.
Both comments currently say Energy structures ... energy response, which appears to be a copy‑paste mistake. Please update them to accurately describe the Voltage and Power types to avoid confusion when reading the code.
Suggested implementation:
// Voltage structures getbasicdevicesstats command voltage response (AHA-HTTP-Interface)
type Voltage struct {
XMLName xml.Name `xml:"voltage"`
Values []string `xml:"stats"`
} // Power structures getbasicdevicesstats command power response (AHA-HTTP-Interface)I only see the comment for Power but not the full type definition. Please ensure that the updated Power comment directly precedes the type Power struct { ... } definition, mirroring the style used for Energy and Voltage.
| return fritzdect, nil | ||
| } | ||
|
|
||
| // ExecCmd execautes an FritzDECT AHA-HTTP-Interface command |
Contributor
There was a problem hiding this comment.
nitpick (typo): Minor typo in the ExecCmd comment.
Please change execautes to executes in the comment to fix the typo.
Suggested change
| // ExecCmd execautes an FritzDECT AHA-HTTP-Interface command | |
| // ExecCmd executes an FritzDECT AHA-HTTP-Interface command |
| "golang.org/x/text/encoding/unicode" | ||
| ) | ||
|
|
||
| // NewConnection creates FritzDECT connection |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the duplicated FritzDECT connection and authentication logic into a shared internal package and have both fritzdect and fritzdect_new reuse it instead of maintaining two separate copies.
You’re duplicating the existing FritzDECT connection/auth logic under a new package name; that increases maintenance complexity unnecessarily. Instead, extract the shared pieces into a common internal helper and reuse them from both packages.
Actionable steps:
- Create a shared internal package (e.g.
meter/fritzdect/internalconn) that holds the common types and logic:
// meter/fritzdect/internalconn/conn.go
package internalconn
import (
"crypto/md5"
"encoding/hex"
"encoding/xml"
"errors"
"fmt"
"net/url"
"strings"
"time"
"github.com/evcc-io/evcc/api"
"github.com/evcc-io/evcc/util"
"github.com/evcc-io/evcc/util/request"
"github.com/evcc-io/evcc/util/transport"
"golang.org/x/text/encoding/unicode"
)
const sessionTimeout = 5 * time.Minute // or import from existing code
type Settings struct {
URI string
AIN string
User string
Password string
}
type Connection struct {
*request.Helper
*Settings
SID string
updated time.Time
}
func NewConnection(loggerName, uri, ain, user, password string) (*Connection, error) {
if uri == "" {
uri = "https://fritz.box"
}
if ain == "" {
return nil, errors.New("missing ain")
}
settings := &Settings{
URI: strings.TrimRight(uri, "/"),
AIN: ain,
User: user,
Password: password,
}
log := util.NewLogger(loggerName).Redact(password)
conn := &Connection{
Helper: request.NewHelper(log),
Settings: settings,
}
conn.Client.Transport = request.NewTripper(log, transport.Insecure())
return conn, nil
}
func (c *Connection) ExecCmd(function string) (string, error) {
if time.Since(c.updated) >= sessionTimeout {
if err := c.getSessionID(); err != nil {
return "", err
}
c.updated = time.Now()
}
parameters := url.Values{
"sid": {c.SID},
"ain": {c.AIN},
"switchcmd": {function},
}
uri := fmt.Sprintf("%s/webservices/homeautoswitch.lua", c.URI)
body, err := c.GetBody(uri + "?" + parameters.Encode())
res := strings.TrimSpace(string(body))
if err == nil && res == "inval" {
err = api.ErrNotAvailable
}
return res, err
}
func (c *Connection) getSessionID() error {
uri := fmt.Sprintf("%s/login_sid.lua", c.URI)
body, err := c.GetBody(uri)
if err != nil {
return err
}
var v struct {
SID string
Challenge string
BlockTime string
}
if err = xml.Unmarshal(body, &v); err == nil && v.SID == "0000000000000000" {
var challresp string
if challresp, err = createChallengeResponse(v.Challenge, c.Password); err == nil {
params := url.Values{
"username": {c.User},
"response": {challresp},
}
if body, err = c.GetBody(uri + "?" + params.Encode()); err == nil {
err = xml.Unmarshal(body, &v)
if v.SID == "0000000000000000" {
return errors.New("invalid user or password")
}
c.SID = v.SID
}
}
}
return err
}
func createChallengeResponse(challenge, pass string) (string, error) {
encoder := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder()
utf16le, err := encoder.String(challenge + "-" + pass)
if err != nil {
return "", err
}
hash := md5.Sum([]byte(utf16le))
md5hash := hex.EncodeToString(hash[:])
return challenge + "-" + md5hash, nil
}- Use the shared connection in the existing
fritzdectpackage:
// meter/fritzdect/fritzdect_common.go
package fritzdect
import "github.com/evcc-io/evcc/meter/fritzdect/internalconn"
type Connection = internalconn.Connection
func NewConnection(uri, ain, user, password string) (*Connection, error) {
return internalconn.NewConnection("fritzdect", uri, ain, user, password)
}- Use the same shared connection in the new
fritzdect_newpackage:
// meter/fritzdect_new/fritzdect_new.go
package fritzdect_new
import "github.com/evcc-io/evcc/meter/fritzdect/internalconn"
type Connection = internalconn.Connection
func NewConnection(uri, ain, user, password string) (*Connection, error) {
return internalconn.NewConnection("fritzdect", uri, ain, user, password)
}You can still customize behavior per package (e.g. logger name, additional methods) without copying all the auth/session/ExecCmd plumbing. This keeps all functionality intact while removing the duplicate implementation.
4 tasks
Author
|
closed in favor of #29020 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #28906 by adding a new device using the Fritz HTTP API instead to the existing lua one.
This is a quick and dirty fix, it introduces much duplicated code and there are several other ways to implement a fix (like trying both APIs in fritzdect). Guidance on improving this PR is much appreciated.
make lint unfortunately failed because of an internal error, so linting errors might exist.
works on my machine:
