Tariffs: add separate grid and feedin fees #26483
Conversation
andig
added
needs decision
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
configureTariff, the conditionif fees != nil && fees.Type == ""looks inverted; this will only add fee tariffs when the config is empty instead of when it's configured, and likely should checkfees.Type != ""instead. - Because
configureTariffsalways passes non-nil pointers forGridFees/FeedInFees, thefees != nilpart of the condition inconfigureTariffis redundant and can be simplified once the logic aroundfees.Typeis corrected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `configureTariff`, the condition `if fees != nil && fees.Type == ""` looks inverted; this will only add fee tariffs when the config is empty instead of when it's configured, and likely should check `fees.Type != ""` instead.
- Because `configureTariffs` always passes non-nil pointers for `GridFees`/`FeedInFees`, the `fees != nil` part of the condition in `configureTariff` is redundant and can be simplified once the logic around `fees.Type` is corrected.
## Individual Comments
### Comment 1
<location> `cmd/setup.go:868-869` </location>
<code_context>
return &DeviceError{name, err}
}
+ if fees != nil && fees.Type == "" {
+ ft, err := tariffInstance(name, *fees)
+ if err != nil {
+ return &DeviceError{name, err}
</code_context>
<issue_to_address>
**issue (bug_risk):** The condition for configuring `fees` looks inverted and will instantiate a fees tariff only when `Type` is empty.
For `conf` you skip configuration when `Type == ""`, but for `fees` you enter the block when `fees.Type == ""`. This inverts the logic: you create a fees tariff when it looks unconfigured and skip it when configured. Updating the condition to `fees != nil && fees.Type != ""` would align it with the main config and prevent constructing a tariff with an empty type.
</issue_to_address>
### Comment 2
<location> `cmd/setup.go:857` </location>
<code_context>
-func configureTariff(u api.TariffUsage, conf config.Typed, t *api.Tariff) error {
- if conf.Type == "" {
+func configureTariff(u api.TariffUsage, conf, fees *config.Typed, t *api.Tariff) error {
+ if conf == nil || conf.Type == "" {
return nil
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new base+fees tariff logic to keep the main config value-based, use a single inactive rule, and move combination into a small helper function.
You can keep the new “base + fees” behavior while reducing the added complexity and avoiding the mixed nullability by:
1. **Keep `conf` as a value, only use pointer for optional `fees`**
There’s no need to make the primary config pointer-based; you immediately dereference it and don’t mutate it. This also lets call sites stay simpler and consistent with the previous API.
2. **Use a single notion of “inactive” (empty `Type`)**
Avoid mixing `nil` and `Type == ""` semantics. For `fees`, a `nil` pointer is enough to mean “no fees configured”; for both `conf` and `fees`, use `Type == ""` to mean “inactive entry”.
3. **Separate fee combination into a small helper**
Let `configureTariff` focus on configuring a single tariff, and delegate the “combine with fees” concern to a helper. This also makes testing easier.
A minimally invasive refactor could look like:
```go
func configureTariff(u api.TariffUsage, conf config.Typed, t *api.Tariff) error {
if conf.Type == "" {
return nil
}
name := u.String()
res, err := tariffInstance(name, conf)
if err != nil {
return &DeviceError{name, err}
}
*t = res
return nil
}
func configureTariffWithFees(
u api.TariffUsage,
conf config.Typed,
fees *config.Typed,
t *api.Tariff,
) error {
if conf.Type == "" && (fees == nil || fees.Type == "") {
return nil
}
// base
var base api.Tariff
if conf.Type != "" {
if err := configureTariff(u, conf, &base); err != nil {
return err
}
}
// fees
if fees != nil && fees.Type != "" {
name := u.String()
ft, err := tariffInstance(name, *fees)
if err != nil {
return &DeviceError{name, err}
}
if base != nil {
base = tariff.NewCombined([]api.Tariff{base, ft})
} else {
base = ft
}
}
*t = base
return nil
}
```
Call sites then stay simple and value-based for `conf`, with a pointer only for the optional fees:
```go
eg.Go(func() error {
return configureTariffWithFees(api.TariffUsageGrid, conf.Grid, &conf.GridFees, &tariffs.Grid)
})
eg.Go(func() error {
return configureTariffWithFees(api.TariffUsageFeedIn, conf.FeedIn, &conf.FeedInFees, &tariffs.FeedIn)
})
eg.Go(func() error {
return configureTariffWithFees(api.TariffUsageCo2, conf.Co2, nil, &tariffs.Co2)
})
eg.Go(func() error {
return configureTariffWithFees(api.TariffUsagePlanner, conf.Planner, nil, &tariffs.Planner)
})
if len(conf.Solar) > 0 {
eg.Go(func() error {
if len(conf.Solar) == 1 {
return configureTariffWithFees(api.TariffUsageSolar, conf.Solar[0], nil, &tariffs.Solar)
}
return configureSolarTariff(conf.Solar, &tariffs.Solar)
})
}
```
This preserves all existing behavior (including combined tariffs) while:
- Removing pointer indirection for the main config.
- Using a consistent “inactive = `Type == \"\"`” rule.
- Keeping `configureTariff` focused and reusable, with combination logic isolated in a small helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Technically the fee tariff can be whatever it wants. We could build a Frankenstein Awattar+Octopus Tariff with it. In UI I'd assume, we'll only offer "Fixed fee" (fixed), "Time-based fees" (fixed+zones) and "User-defined fees" (custom) and thereby limit the available options to sensible ones. Maybe we'll add some specific fee tariffs in the future (e.g. Netzentgelte via PLZ API, ...). Question: How should this feature behave in relation to Alternative idea: could the |
Yes :(
Whooo :) Still makes it a circular problem on the chargesTariff which can have charges then... |
|
the very specific scenario which triggered this is Octopus Go (time based fee, cheaper from 0-5) in combination with module 3 §14a - where the user would need to model the gridfee surcharges & reduction following whatever rules the his grid operator sets (essentially requiring the zones component for modelling). |
or a complex formula https://docs.evcc.io/docs/tariffs#beispiel-tarif-mit-jahreszeit--und-zeitabhängigen-netzgebühren |
|
When it's only time based we could simply add a ui or formula abstraction that creates the correct formula under the hood (convenience). When we need fresh data (api) formula is not sufficient. |
fixed tariff does not support formula today, tested that. And tbh, formula seems overly complex here, I do like the zones concept much better. Way more user friendly without the need to construct a programming-language like term. |
That's a simple change
It wouldn't replace it, just offer more options |
|
If I understand this commit correctly, the idea is to not only have grid as a fee, but also gridfee. You can define both separately (e.g. by using zones as a logic) and they then get summarized to get the final price. I like the approach, as it supports separating both in the data/graphs if needed later. Can the gridfee be a negative value? It would be more flexible to maintain if we can also use this fee as a surcharge/deduction. Then you don't have to substract the standard gridfee from the grid price, but can only maintain HT surcharge and NT deduction. I think that is what it will be used primarily. A graph showing the fees however would be wrong then of course. |
|
This is what the yaml would look like: Take a look on the gridfee Standardtarif. This one must normally be defined on "other" times (where it's neither HT nor NT). |
it doesn‘t. |
|
Right now it doesn't, yes. But it would only be a change in the backend if you want to separate it later. |
|
how will that go into calculation then? e.g. or there is a key like |
|
You can see the example how to set it up above. You need to extract the gridfee from your energy price to set it up separately. Both (grid price + grid fee) will be summarized from evcc. This ist totally sufficient for the time being. |
|
I'm still unsure that this is the right direction. Especially since it duplicates functionality and it's not easily explainable when to use what: With a little distance I'd rather prefer "upgrading" the existing |
|
Any idea how to do this config-wise? |
|
I checked the formula syntax earlier before creating the issue, but please stay away from this. It's a powerful yet not very intuitive tool that should not be necessary for a use-case that will become as relevant as §14 variable grid fees. We should see popularity of this rising over the coming months/years. Zones has a great syntax for this. |
|
From configuration perspective I'd expect it to look like this: Current state Option 1: grid:
type: fixed
price: 0.3 # EUR/kWh
+ charges: # complex
+ price: 0.05
+ zones:
+ - days: Mo-Fr
+ hours: 2-5
+ price: 0.02 # EUR/kWh
+ - days: Sa,So
+ price: 0.05 # EUR/kWhOption 2: new grid:
type: fixed
price: 0.3 # EUR/kWh
charges: 0.05
+ chargesZones:
+ - days: Mo-Fr
+ hours: 2-5
+ price: 0.02 # EUR/kWh
+ - days: Sa,So
+ price: 0.05 # EUR/kWh |
|
both look good to me, easy to implement by the user, no need to write actual code for the formula. If charges exists as a concept today already we certainly can use that. Would charges support negative charges (i.e. for NT tariff) and zero as regular price (ST)? |
|
If implementing like this, please test the following combination with price zone and charges zone: Maybe consider calling it "chargesprice" for better consistency. |
|
FYI: In #26698 I'll have to add a dedicated template for |
|
Waiting for #26698 |
Blocked by #26698
Fix #26164