-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
HEMS: add "smartgrid" session logging #25907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
Relay.setLimitedyou now always callSetMaxPower(c.maxPower)regardless of thelimitedstate, which changes prior behavior where max power was set to 0 when not limited; consider restoring the previous semantics or make the change explicit if intentional. - The
updateSessionlogic inhems/relayandhems/eebusis duplicated and guarded by a TODO; consider extracting this into a shared helper (e.g., in thesmartgridpackage) to ensure behavior stays consistent across HEMS implementations. - The new file
server/db/regostry.goappears to be misspelled; renaming it toregistry.gowould improve discoverability and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Relay.setLimited` you now always call `SetMaxPower(c.maxPower)` regardless of the `limited` state, which changes prior behavior where max power was set to 0 when not limited; consider restoring the previous semantics or make the change explicit if intentional.
- The `updateSession` logic in `hems/relay` and `hems/eebus` is duplicated and guarded by a TODO; consider extracting this into a shared helper (e.g., in the `smartgrid` package) to ensure behavior stays consistent across HEMS implementations.
- The new file `server/db/regostry.go` appears to be misspelled; renaming it to `registry.go` would improve discoverability and avoid confusion.
## Individual Comments
### Comment 1
<location> `hems/relay/relay.go:101-110` </location>
<code_context>
}
func (c *Relay) run() error {
- limit, err := c.limit()
+ limited, err := c.limit()
if err != nil {
return err
}
- var power float64
- if limit {
- power = c.maxPower
+ if err := c.setLimited(limited); err != nil {
+ return err
+ }
+
+ if err := c.updateSession(c.maxPower); err != nil {
+ return fmt.Errorf("smartgrid session: %v", err)
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Session limit should likely depend on the `limited` state rather than always using `c.maxPower`.
In `run`, `updateSession` is now always called with `c.maxPower`, so a smartgrid session will start whenever `maxPower` > 0, even if `limited` is false. Previously, no session would run when `limit` was false (`power` was `0`). To preserve that behavior, consider passing `c.maxPower` only when `limited` is true and `0` otherwise, so session start/stop matches the limiting state.
</issue_to_address>
### Comment 2
<location> `server/db/db.go:53-62` </location>
<code_context>
-func NewInstance(driver, dsn string) (err error) {
- Instance, err = New(strings.ToLower(driver), dsn)
- return
+func NewInstance(driver, dsn string) error {
+ inst, err := New(strings.ToLower(driver), dsn)
+ if err != nil {
+ return err
+ }
+
+ Instance = inst
+
+ mu.Lock()
+ defer mu.Unlock()
+
+ for _, f := range registry {
+ if err := f(); err != nil {
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** Running registered init functions while holding the global mutex can deadlock if any of them calls `db.Register`.
`NewInstance` holds `mu` while invoking each callback in `registry`. If any callback (now or in the future) calls `db.Register`, it will try to take `mu` again and deadlock. To avoid this, copy `registry` while holding the lock, release the lock, and then invoke the copied callbacks, or otherwise ensure callbacks run without `mu` held.
</issue_to_address>
### Comment 3
<location> `server/db/regostry.go:1-5` </location>
<code_context>
+package db
+
+import "sync"
+
+var (
+ mu sync.Mutex
+ registry []func() error
</code_context>
<issue_to_address>
**nitpick (typo):** The filename `regostry.go` looks like a typo and could be standardized.
Using `regostry.go` instead of `registry.go` doesn’t break anything, but it hurts discoverability and looks accidental. Please rename the file to `registry.go` for clarity and consistency.
Suggested implementation:
```golang
package db
```
Please rename the file in your repository from:
- `server/db/regostry.go`
to:
- `server/db/registry.go`
No code modifications are required inside the file; the rename alone will address the discoverability and typo concern.
</issue_to_address>
### Comment 4
<location> `hems/relay/relay.go:25` </location>
<code_context>
root api.Circuit
passthrough func(bool) error
+ gridSessionID uint
status status
statusUpdated time.Time
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the session lifecycle logic and grid session ID handling into a reusable smartgrid session manager so Relay only manages limited vs. not-limited behavior.
You can reduce the new complexity without changing behavior by moving the session lifecycle state machine into a reusable helper and keeping `Relay` focused on “limited vs. not” behavior.
### 1. Encapsulate `gridSessionID` + start/stop in a helper
Create a small session manager in `smartgrid` (or a shared HEMS package) that owns the `ID` and the start/stop logic. For example:
```go
// package smartgrid
type SessionManager struct {
id uint
}
func (m *SessionManager) Update(mode Mode, limit float64, chargePowerFunc func() float64) error {
// start session
if limit > 0 && m.id == 0 {
var power *float64
if p := chargePowerFunc(); p > 0 {
power = lo.ToPtr(p)
}
sid, err := StartManage(mode, power, limit)
if err != nil {
return err
}
m.id = sid
}
// stop session
if limit == 0 && m.id != 0 {
if err := StopManage(m.id); err != nil {
return err
}
m.id = 0
}
return nil
}
```
Then `Relay` only holds a `SessionManager` instead of a raw `gridSessionID`:
```go
type Relay struct {
log *util.Logger
root api.Circuit
passthrough func(bool) error
sessions smartgrid.SessionManager
limit func() (bool, error)
maxPower float64
interval time.Duration
}
```
And `run` becomes simpler and free of the duplicated state machine:
```go
func (c *Relay) run() error {
limited, err := c.limit()
if err != nil {
return err
}
if err := c.setLimited(limited); err != nil {
return err
}
limit := c.maxPower
if !limited {
limit = 0
}
if err := c.sessions.Update(smartgrid.Dim, limit, c.root.GetChargePower); err != nil {
return fmt.Errorf("smartgrid session: %w", err)
}
return nil
}
```
This keeps the functionality intact but:
- Removes the `gridSessionID` state machine from `Relay`.
- Makes the TODO (“keep in sync across HEMS implementations”) actionable by consolidating that logic in `smartgrid.SessionManager`.
- Keeps `setLimited` focused only on relay behavior (dim/maxPower/passthrough).
</issue_to_address>
### Comment 5
<location> `hems/eebus/eebus.go:249` </location>
<code_context>
c.setLimit(limit)
+
+ if err := c.updateSession(limit); err != nil {
+ c.log.ERROR.Printf("smartgrid session: %v", err)
+ }
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the manage-session start/stop logic into a shared smartgrid helper and calling it from EEBus (and Relay) instead of duplicating it locally.
You can reduce complexity and duplication here by extracting the session lifecycle logic into a shared helper in `smartgrid` (or a HEMS helper), and then delegating from `EEBus` (and Relay) to that helper.
### 1. Extract shared session management helper
Example in `hems/smartgrid/session.go`:
```go
package smartgrid
// UpdateManageSession starts/stops a manage session based on limit.
// Keeps sessionID in sync with the current active session.
func UpdateManageSession(sessionID *uint, mode Mode, currentPower, limit float64) error {
// start session
if limit > 0 && *sessionID == 0 {
var power *float64
if currentPower > 0 {
power = ¤tPower
}
sid, err := StartManage(mode, power, limit)
if err != nil {
return err
}
*sessionID = sid
}
// stop session
if limit == 0 && *sessionID != 0 {
if err := StopManage(*sessionID); err != nil {
return err
}
*sessionID = 0
}
return nil
}
```
(Adjust `Mode` type and import paths as needed.)
### 2. Simplify `EEBus.updateSession`
```go
func (c *EEBus) updateSession(limit float64) error {
return smartgrid.UpdateManageSession(
&c.gridSessionID,
smartgrid.Dim,
c.root.GetChargePower(),
limit,
)
}
```
This:
- Removes the copy-pasted `StartManage`/`StopManage` logic and TODO about keeping implementations in sync.
- Keeps `EEBus` focused on status/limit handling while delegating session orchestration to `smartgrid`.
- Ensures Relay and EEBus share a single implementation, reducing future drift and cognitive load.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Shutting down or restarting evcc with an active session leads to entries with no finish time. For charging sessions we check for recent unfinished sessions on evcc start and "finish" them. We should add a similar mechanism here. |
|
I wouldn‘t. This is a complicance thing- we should not invent data that we don‘t have. |
naltatis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm done with basic implementation. For now just CSV download and basic info. We can add proper table or list view later.
|
Vmtl als nächstes ein „clear log“ Button, aber das können wir erstmal nutzen. Thx! |
This PR adds smart grid control session logging for EnWG §14a (and EEG §9 depending on #25887).
✍️ log grid events in database (table
grid_sessions)🗃️ expose data via (
GET /api/gridsessions) in css and json format💾 show basic info (# events, last event date) and download link in hems modal
🖥️ show hems modal even if yaml-configured (no edit options)
🔩 refactor CSV generation
🧪 add e2e tests
UI configured

yaml configured

csv download

TODO