Config UI: make messengers configurable devices #26782
Conversation
![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 4 issues, and left some high level feedback:
- In the
configureMessengersfunction, the goroutine overconfigurablecloses over the loop variableconfand later uses&confinconfig.NewConfigurableDevice(&conf, instance), which can lead to all goroutines referencing the last element; take a copy ofconfinside the loop before starting the goroutine (e.g.,conf := conf) and use that copy instead. - There are a couple of inconsistent/incorrect log and error messages (e.g.,
"cannot create messager %d",log.WARN.Printf("create meter %d: %v", ...)) that should be aligned with the new messenger terminology and fixed typos to avoid confusion during debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `configureMessengers` function, the goroutine over `configurable` closes over the loop variable `conf` and later uses `&conf` in `config.NewConfigurableDevice(&conf, instance)`, which can lead to all goroutines referencing the last element; take a copy of `conf` inside the loop before starting the goroutine (e.g., `conf := conf`) and use that copy instead.
- There are a couple of inconsistent/incorrect log and error messages (e.g., `"cannot create messager %d"`, `log.WARN.Printf("create meter %d: %v", ...)`) that should be aligned with the new messenger terminology and fixed typos to avoid confusion during debugging.
## Individual Comments
### Comment 1
<location> `cmd/setup.go:857` </location>
<code_context>
+ return messageChan, err
+ }
+
+ for _, conf := range configurable {
+ eg.Go(func() error {
+ cc := conf.Named()
</code_context>
<issue_to_address>
**issue (bug_risk):** Loop variable `conf` is captured by the goroutine, which can lead to all goroutines using the last `conf` value.
Because the `for _, conf := range` loop variable is reused, the closure passed to `eg.Go` may see only the final `conf` value, and `&conf` will not point to a per-iteration instance. Shadow `conf` inside the loop so each goroutine captures its own copy:
```go
for _, conf := range configurable {
conf := conf // per-iteration copy
eg.Go(func() error {
cc := conf.Named()
if e := config.Messengers().Add(config.NewConfigurableDevice(&conf, instance)); e != nil && err == nil {
err = &DeviceError{cc.Name, e}
}
return err
})
}
```
</issue_to_address>
### Comment 2
<location> `cmd/setup.go:831-833` </location>
<code_context>
+ // continue
+ // }
+
+ if err := nameValid(cc.Name); err != nil {
+ log.WARN.Printf("create meter %d: %v", i+1, err)
+ }
+
</code_context>
<issue_to_address>
**nitpick (typo):** Log message refers to "meter" instead of "messenger", which is misleading.
Since this runs in the messenger setup path, the log text should match to avoid confusion during debugging. Suggest updating the message to say "messenger" instead of "meter", e.g.:
```go
log.WARN.Printf("create messenger %d: %v", i+1, err)
```
```suggestion
if err := nameValid(cc.Name); err != nil {
log.WARN.Printf("create messenger %d: %v", i+1, err)
}
```
</issue_to_address>
### Comment 3
<location> `cmd/setup.go:823` </location>
<code_context>
+ Other: cc.Other,
+ }
+
+ if cc.Name == "" {
+ return messageChan, fmt.Errorf("cannot create messager %d: missing name", i+1)
+ }
+
</code_context>
<issue_to_address>
**nitpick (typo):** Error message has a minor typo ("messager" vs "messenger").
Please align this with existing terminology by changing the message to: `cannot create messenger %d: missing name`.
```suggestion
return messageChan, fmt.Errorf("cannot create messenger %d: missing name", i+1)
```
</issue_to_address>
### Comment 4
<location> `cmd/setup.go:814` </location>
<code_context>
+ var eg errgroup.Group
+
+ for i, cc := range conf.Services {
+ // add name
+ cc := config.Named{
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a shared helper for creating and registering messengers and tightening the goroutine usage to remove duplication and a closure-capture bug.
You can reduce complexity and also fix a subtle bug by extracting a shared helper and tightening the concurrency/error-handling.
**1. Extract shared “create + register messenger” helper**
Both loops build a `config.Named`, create a logger, construct a messenger via `push.NewFromConfig`, and register it. The only difference is whether `Other` needs `customDevice` and which `Add` method is used.
You can centralize this:
```go
func initMessengerDevice(
ctx context.Context,
named config.Named,
rawOther map[string]any,
configurable *config.Configurable, // nil for static
) error {
// decode custom device if needed
props := rawOther
var err error
if configurable != nil {
props, err = customDevice(named.Other)
if err != nil {
return &DeviceError{named.Name, fmt.Errorf("cannot decode custom messenger '%s': %w", named.Name, err)}
}
}
// create instance
instance, err := push.NewFromConfig(ctx, named.Type, props)
if err != nil {
return &DeviceError{named.Name, fmt.Errorf("cannot create messenger '%s': %w", named.Name, err)}
}
// register
if configurable == nil {
if err := config.Messengers().Add(config.NewStaticDevice(named, instance)); err != nil {
return &DeviceError{named.Name, err}
}
} else {
if err := config.Messengers().Add(config.NewConfigurableDevice(configurable, instance)); err != nil {
return &DeviceError{named.Name, err}
}
}
return nil
}
```
Then both loops collapse to simple calls with much less duplication and branching:
```go
var eg errgroup.Group
for i, svc := range conf.Services {
named := config.Named{
Name: fmt.Sprintf("push-%d", i+1),
Type: svc.Type,
Other: svc.Other,
}
if named.Name == "" {
return messageChan, fmt.Errorf("cannot create messenger %d: missing name", i+1)
}
if err := nameValid(named.Name); err != nil {
log.WARN.Printf("create messenger %d: %v", i+1, err)
}
named := named // avoid closure capture issues if you add fields later
eg.Go(func() error {
ctx := util.WithLogger(context.TODO(), util.NewLogger(named.Name))
return initMessengerDevice(ctx, named, named.Other, nil)
})
}
configurable, err := config.ConfigurationsByClass(templates.Messenger)
if err != nil {
return messageChan, err
}
for _, cfg := range configurable {
cfg := cfg // FIX: avoid capturing loop variable by reference
named := cfg.Named()
eg.Go(func() error {
ctx := util.WithLogger(context.TODO(), util.NewLogger(named.Name))
return initMessengerDevice(ctx, named, named.Other, &cfg)
})
}
if err := eg.Wait(); err != nil {
return messageChan, &ClassError{ClassMessenger, err}
}
```
Benefits:
- Removes the duplicated “decode / create / add / wrap error” logic.
- Simplifies error handling: each failure path returns immediately from the helper, callers only see a single `error`.
- Fixes the closure-capture bug in the second loop (`conf` was being captured and then used via `&conf` and `conf.Named()` inside the goroutine).
- Clarifies naming (`svc`, `cfg`, `named`) and avoids shadowing (`conf` and `cc` reuse).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Suggestion: Lets not do auto-migrate at all. If yaml based config exists (via evcc.yaml or ui) we'll simply use it. We only unlock new form based configuration if messaging is not configured yet. I'm doing the same thing in the tariffs branch right now. If we detect yaml based config and(!) device-based config we'll simply fail on boot. but this could only happen if a users tries to add messaging configuration to evcc.yaml even though he already has ui-configured it. In short config ui will have two ways to configure messaging:
This way we don't have to account for one-time logics (when, re-run, rollback, ...). Drawback is, that the user has to reenter the values in UI when he wants to go to the new model. But he retains control and is not forced. |
Was not aware that it was named Note: In UI we're calling this feature "Notifications" / "Benachrichtigungen" which better describes the use-case than the actual tech. But this does not have to match our internal naming. Renaming the keys will do more harm than good 😄. |
In first case these will still be devices internally from now on. It's up to the UI then to choose what type of config interface to offer? |
Yes, UI will stay as is: yaml-config via UI editor |
|
Can we also split the parameters in the templates here as in #25768?
Or isn't that the goal of this PR?
Will have a try and create a PR for this. 👍 |
Sure, feel free! We can also do that as follow-up, doesn't need to be here. As you like, I just did a quick AI attempt. |
|
Can also remove the templates here as you prefer. No need to introduce conflicts. |
|
I appreciate your efforts. Let's keep the templates. 👍 |
Tests are done. :) |
|
Was fehlt denn hier noch- das eigentlich UI? Sollen wir darauf warten? |
|
I can branch here and create a PR for the UI. |
|
@Maschga can I close this one? |
|
Yes. :-) |
4 participants
TODO
check stringlist type UI support @naltatis>type: listexistsOut of scope
email)/cc @Maschga