EEBUS: configure by default #26944
EEBUS: configure by default #26944
Conversation
Co-authored-by: Michael Geers <michael@geers.tv>
Co-authored-by: Michael Geers <michael@geers.tv>
Co-authored-by: Michael Geers <michael@geers.tv>
|
I've added shipid/ski fields and explanation. Also adjusted the json save/delete workflow. The roundtrip should work now. @Maschga Can you do the e2e? Basic flow like verify values exist on start. Change shipid and restart. Verify SKI changes after reboot when user deletes config and restarts. Also noticed that private key is not redacted yet. 
|
Since we're currently still using the hacky "list as texturea" ui I'd not do autocomplete. We should upgrade the ui in da dedicated ticket and make it a list of inputs. Doing a similar thing in #26698 for zones. Could be similar. |
|
Just noticed that ship-id is optional. It must be required, same for the certificates. |
|
According to documentation:
SHIPID ist ypically generated from the machine-id. If there is none or there is a plant ID, the plant ID is used instead. IMHO plant ID is the important param, especially if the goal is to be able to move the whole EEBUS configuration to another device which is why I don't want to use machine-id. Also see this discussion. |
|
@naltatis können wir den Dokustyle von "Du" auf matter of fact umstellen? |
The user should NOT provide it. It should be generated and currently is generated on every startup assuming the machine-id/plantid are stable. |
|
It already is ;) |
Is there any benefit in using the machine-id instead of a randomly auto-generated plantid? Wouldn't it be preferable in all cases to have it plantid based so it's portable when switching hardware? The current mechanism was built before evcc had reliable state management. |
Let's not get confused. The whole purpose for having this optionally user-defined is to use migration from whatever previous status. |
That's exactly why I switched to plant ID. Maybe somebody is still interested in this posting. I had different plant IDs in yaml and db for a reason I don't know. Migrating the configuration from yaml to db would have required me to alter the db manually since plant ID cannot be altered from UI. |
For this case the user has to know his previous SHIP-ID. It's currently not part of our backup. In a case of hardware failure he won't be able to look up his original SHIP-ID in his old setup. Maybe it's something the user has in another form (email to grid operator) or can retrieve it from another system. But to repeat my question: Is there still any benefit to have it machine id specific? I currently dont see it. For me it seems like id-stability is very important, though and having to repair is trouble. |
|
PlantID is OT here. Let's stay on topic. |
Machine ID is what gives ID stability. |
This is not correct. Plant ID is used and on-demand created, when we can't retrieve a machine id (e.g. docker). |
![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:
- The new util.Masked helper takes an
anyand compares it to"", which will panic for non-string types; consider changing the signature to acceptstring(or explicitly type-assert with a safe default) so you don't risk runtime panics when masking arbitrary values. - In EebusModal the
PropertyCertFieldupdate handlers no-op whenvalues.certificateis undefined, so users cannot create a new certificate object from an initially empty config; it would be more robust to initializevalues.certificate = { public: '', private: '' }before assigning into its fields. - The
allowPubspecial case forkeys.EEBusprevents publishing updates on config changes, which means the frontend store will not reflect edited EEBus settings until a full refresh; consider publishing a properConfigStatuswrapper instead of disabling pub entirely so the UI stays in sync without overwriting the global info structure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new util.Masked helper takes an `any` and compares it to `""`, which will panic for non-string types; consider changing the signature to accept `string` (or explicitly type-assert with a safe default) so you don't risk runtime panics when masking arbitrary values.
- In EebusModal the `PropertyCertField` update handlers no-op when `values.certificate` is undefined, so users cannot create a new certificate object from an initially empty config; it would be more robust to initialize `values.certificate = { public: '', private: '' }` before assigning into its fields.
- The `allowPub` special case for `keys.EEBus` prevents publishing updates on config changes, which means the frontend store will not reflect edited EEBus settings until a full refresh; consider publishing a proper `ConfigStatus` wrapper instead of disabling pub entirely so the UI stays in sync without overwriting the global info structure.
## Individual Comments
### Comment 1
<location> `server/eebus/types.go:55-64` </location>
<code_context>
+ return fmt.Sprintf("%s-%0x", "EVCC", protectedID[:8])
+}
+
+func DefaultConfig(conf *Config) (*Config, error) {
+ cert, err := CreateCertificate()
+ if err != nil {
+ return nil, err
+ }
+
+ public, private, err := GetX509KeyPair(cert)
+ if err != nil {
+ return nil, err
+ }
+
+ res := Config{
+ Port: 4712,
+ ShipID: createShipID(),
+ Certificate: Certificate{
+ Public: public,
+ Private: private,
+ },
+ }
+
+ return &res, nil
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** DefaultConfig ignores the incoming config and always overwrites all fields, which can unexpectedly discard user settings.
`DefaultConfig` builds a new `Config` and ignores `conf`, so any partially configured state (e.g. custom port or ShipID without a certificate) is completely replaced. If the intent is to fill in only missing values, consider copying `*conf` and only setting defaults for zero-valued fields (`Port`, `ShipID`, `Certificate`) before returning. The unused `conf *Config` parameter is a strong hint that the current behavior may not match the intended design.
</issue_to_address>
### Comment 2
<location> `cmd/helper.go:126-135` </location>
<code_context>
}
+
+// migrateYamlToJson converts a settings value from yaml to json if needed
+func migrateYamlToJson[T any](key string, res *T) error {
+ if settings.IsJson(key) {
+ return settings.Json(key, res)
+ }
+
+ if err := settings.Yaml(key, new(T), res); err != nil {
+ return err
+ }
+
+ settings.SetJson(key, res)
+
+ return nil
</code_context>
<issue_to_address>
**issue (bug_risk):** migrateYamlToJson ignores potential errors from SetJson, which can hide persistence failures.
Here you correctly return errors from `settings.Json`/`settings.Yaml` but ignore the result of `settings.SetJson`. If persisting the migrated value fails, the function still returns `nil`, so callers like `configureEEBus`/`configureModbusProxy` will treat the migration as successful.
Please capture and return the `SetJson` error, e.g.:
```go
if err := settings.SetJson(key, res); err != nil {
return err
}
return nil
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func DefaultConfig(conf *Config) (*Config, error) { | ||
| cert, err := CreateCertificate() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| public, private, err := GetX509KeyPair(cert) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): DefaultConfig ignores the incoming config and always overwrites all fields, which can unexpectedly discard user settings.
DefaultConfig builds a new Config and ignores conf, so any partially configured state (e.g. custom port or ShipID without a certificate) is completely replaced. If the intent is to fill in only missing values, consider copying *conf and only setting defaults for zero-valued fields (Port, ShipID, Certificate) before returning. The unused conf *Config parameter is a strong hint that the current behavior may not match the intended design.
| func migrateYamlToJson[T any](key string, res *T) error { | ||
| if settings.IsJson(key) { | ||
| return settings.Json(key, res) | ||
| } | ||
|
|
||
| if err := settings.Yaml(key, new(T), res); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| settings.SetJson(key, res) |
There was a problem hiding this comment.
issue (bug_risk): migrateYamlToJson ignores potential errors from SetJson, which can hide persistence failures.
Here you correctly return errors from settings.Json/settings.Yaml but ignore the result of settings.SetJson. If persisting the migrated value fails, the function still returns nil, so callers like configureEEBus/configureModbusProxy will treat the migration as successful.
Please capture and return the SetJson error, e.g.:
if err := settings.SetJson(key, res); err != nil {
return err
}
return nil
Question resolved. The new process generates and persists the SHIP-ID on first start. So portability is guaranteed through db backup/restore. MachineId/PlantId is only relevant for initial config generation. |
Thanks for the clarification. Will try EEBUS auto-configuration in tomorrow's nightly. |
5 participants
Fix #26596
Ref #26615
🏃➡️ run EEBus automatically at startup
🚩 add services section for OCPP, SHM and EEBus
📄 modal is readonly if configured in evcc.yaml
TODO:
autocompletion for interfaces (?)\cc @andig @naltatis