Config UI: allow disabling devices - #29455
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 left some high level feedback:
- In
configureLoadpoints, whenconf.Disableis true theinstanceremainsnilbut is still wrapped inNewConfigurableDevice; consider ensuring all consumers guard against a nilloadpoint.APIor creating a small disabled stub implementation to avoid potential nil dereferences. - In
configurableInstance, the disabled path leavesinstanceas the zero value ofTwithout returning an error; consider making the disabled behavior explicit (e.g., early return or a dedicated disabled state) so callers don’t have to implicitly rely on a zero-value instance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `configureLoadpoints`, when `conf.Disable` is true the `instance` remains `nil` but is still wrapped in `NewConfigurableDevice`; consider ensuring all consumers guard against a nil `loadpoint.API` or creating a small disabled stub implementation to avoid potential nil dereferences.
- In `configurableInstance`, the disabled path leaves `instance` as the zero value of `T` without returning an error; consider making the disabled behavior explicit (e.g., early return or a dedicated disabled state) so callers don’t have to implicitly rely on a zero-value instance.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@marq24 this PR might not be sufficient for exposing this functionality via HA yet. Its part of our internal/undocumented |
|
@andig UI works. Only did shallow functional testing (disabling a pv meter, ...) and ran into startup errors and situations where I got stuck (not even fatal mode). I'll suggest you try this. I'll add structured e2e tests later to cover all use-cases. |
So just to check, if I understood this correctly - even when there will be documented, the functionality will have it's home in the Then HA users must use REST-API Integration in order to manipulate this [since the current HA integration only use API endpoints that are accessible without credentials] - so this new feature will then not be directly available in HA then [I just will create an appropriate documentation like this one here: https://github.com/marq24/ha-evcc#accessing-your-vehicle-soc--range-when-the-vehicle-is-not-connected-to-a-loadpoint |
|
Yes, disable flag will be part of device configuration and requires a restart after change. Both endpoints need authentication. |
|
@naltatis loadpoints can't be disabled yet? |
|
Und: das UI scheint bei noch alle |
|
Just broke it (again), but not sure why this breaks. Maybe we need to do the meter nil checks later- but why? |
mixed up error handling. should be fixed now. |
I'm unsure if we should put the disable logic in loadpoint at all or maybe leave it at device level (charger, optional meter) and just let lp initialization check if it has a functioning/non-disabled charger. Benefit: this is exactly what we have to do for handling broken devices on startup anyways. Drawback: user has to click one level deeper to the charger. If important it could easily be solved UI-only (show disable button on loadpoint which actually changes the associated charger disable state). Since loadpoint-charger is a 1:1 relationship I'd suggest to not implement both and use the charger as the source of truth here. |
|
I'd prefer loadpoint- otherwise we'd need backend logic to silently deactivate the loadpoint because its charger no longer exists. If LP is deactivated, charger and lp meter can remain active but won't be used. |
|
allow me to add my opinion here (my 'train of though' coming from the wish to 'temporary disable the ability of evcc to control my charger', since I want/must pass over the control to the Tibber backend, so that I can receive Grid rewards). @naltatis when LP and a charger device have a 1:1 relation in evcc, then disable the charger will also disable the loadpoint - right? (I assume a LP without an functional/operational charger has then also no functionality). From my personal enduser-experince I don't make any logical difference between a loadpoint (and the charger behind it)... To my best knowledge as a user I don't see 'charger devices' anywhere - I just see the corresponding loadpoint in the UI. So when you place the toggle at device level, you need to remind every single user "Yes, you want to disable the loadpoint- but technically you must disable the charger device". I understand that a LP object has different attributes/methods then a charger-device [but I doubt, that the common user will mind this difference] |
|
Plus: LP is the top level object as far as ui is concerned! Would need to think about what happens with broken meters/chargers for disabled loadpoints- they would still be initialized and consume startup time but ultimately not leads to an error. May be acceptable. |
What do you mean with this? If I'm using a mobile charger (NRGKick use case) and disable a loadpoint, I'd expect that evcc does not try to initialize the charger and eventually fail. |
That's only half true. The values you see in the loadpoint card are the sum of charger and meter status. Loadpoint does not have a status. |
|
27f33b4 addresses the remaining boot and subdevice items:
|
# Conflicts: # core/site.go
|
Disabled loadpoints are published in minimal form, but not shown in UI. Only in display settings as disabled. 
|
|
@andig added a lot of e2e tests and fixed remaining bug. All subtasks fixed. |
# Conflicts: # core/site.go
|
ping @andig |
andig
added
backlog
# Conflicts: # assets/js/types/evcc.ts # core/site_api.go # eslint.config.mts
Fix #21144, refs #14496
TODO
Out of scope
Video
disable.webm
Screenshots
disable button in device modal
confirm
disabled device in overview
enable notice
