Skip to content

Keep ErrOperationOnFunctionNotSupported matchable and stop logging it as an error - #256

Open
andig wants to merge 1 commit into
enbility:devfrom
andig:fix/optional-function-not-supported
Open

Keep ErrOperationOnFunctionNotSupported matchable and stop logging it as an error#256
andig wants to merge 1 commit into
enbility:devfrom
andig:fix/optional-function-not-supported

Conversation

@andig

@andig andig commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

ErrOperationOnFunctionNotSupported is declared in api/errors.go but has exactly one use, and that use throws the sentinel away:

// features/client/feature.go
errWithFunction := fmt.Sprintf("%s %s", api.ErrOperationOnFunctionNotSupported.Error(), function)
return nil, errors.New(errWithFunction)

errors.Is can never match it, so neither the use cases nor any downstream consumer can distinguish "the remote does not implement this optional function" from a real failure. This wraps it instead. The rendered message is unchanged.

The second half is a consistency fix. Connect-time discovery requests are best-effort — the remote is free not to implement optional functions — and most use cases already log their failures at debug level: cs/lpc, cs/lpp, eg/lpc, eg/lpp, cem/evcc, cem/evcem, cem/cevc, cem/opev, cem/evsoc, cem/ohpcf. Five files still log the same class of request at error level, and eg/lpc / eg/lpp are inconsistent within themselves — every Request* call is debug except RequestKeyValues. This aligns the outliers.

What prompted it: a NIBE VVM S320 heat pump. Its Measurement server declares measurementListData and measurementDescriptionListData and nothing else, which is legal — measurementConstraintsListData is optional. ma/mpc requests constraints unconditionally on connect for every compatible entity, so the device produces two lines like this on every connection, forever:

ERROR operation is not supported on function measurementConstraintsListData

The use case works fine — descriptions arrive, DataUpdatePower streams normally — but users reasonably read a red ERROR as the reason their device is not working and open bug reports about it.

Verified: go vet ./... clean, go test ./... green. usecases/api/types.go is reported by gofmt -l on dev as well and is left untouched.


🤖 Generated with Claude Code

… as an error

requestData rendered the sentinel to a string and rebuilt it with errors.New,
so errors.Is could never match it and callers had no way to tell an expected
capability gap from a real failure.

Wrap it instead, and align the remaining use cases with the majority that
already log these connect-time discovery requests at debug level.
@andig

andig commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

This is another consistency issue that should end up on the implementation quality checklist.

@andig

andig commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

One thing worth flagging before this is merged: 13 open PRs add new connect-time Request* calls that log failures at error level — the exact pattern this PR removes from the five outliers. New error-level logging.Log().Error(...) lines added per PR:

PR added requests
#232 4 measurement.RequestDescriptions/RequestConstraints/RequestData
#233 4 same
#240, #249 8 hvac.RequestHvacOperationModeDescriptions, RequestHvacSystemFunctionDescriptions, RequestHvacSystemFunctionSetpointRelations, setpoint.RequestSetpointDescriptions/RequestSetpoints/RequestSetpointConstraints
#241, #242, #244, #245 5 hvac/setpoint discovery
#243 8 hvac/setpoint discovery
#246, #247 7 hvac discovery incl. RequestHvacOverruns
#248 2 deviceClassification.RequestUserData
#132 5 hvac/setpoint discovery

All of them are the same class as the ones aligned here: best-effort discovery on connect, where the remote is free not to implement the optional function. setpoint.RequestSetpointConstraints and hvac.RequestHvacOverruns in particular are as optional as measurementConstraintsListData is on the NIBE, so the heat pump PRs are the most likely to produce the same spurious ERROR lines in the wild.

Whichever side merges last wins, so the outcome is either a follow-up sweep or a conflict-prone rebase. Happy to flip these to debug in the respective branches ahead of time — say the word and I will push that to mine (#232, #233, #240#249); #132 is not mine.

No merge conflicts with any currently open PR, for what it's worth: #207 touches the same events.go files but only HandleEvent and the *Connected signatures (lines 2–30, plus evcc 70–135), and #215 / #181 sit in the *DataUpdate bodies further down. All disjoint from the hunks here.

Related: #251 introduces api.ErrNotSupported for the write-side equivalent ("remote does not advertise Write()"), so between the two PRs both directions become matchable with errors.Is.

🤖 Generated with Claude Code

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.

1 participant