Skip to content

Conversation

@andig
Copy link
Member

@andig andig commented Jan 6, 2026

Refs #25058 (reply in thread)

Poll measurements in addition to processing use case events.

@andig andig added the bug Something isn't working label Jan 6, 2026
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In both readValue and maUseCaseSupportUpdate you use defer c.mu.Lock() after acquiring the lock, which will deadlock; this should be defer c.mu.Unlock() so the mutex is properly released.
  • The new readPhases helper indexes res[0], res[1], and res[2] without verifying the slice length (both on cache hit and after update), whereas the previous implementation guarded against invalid lengths; consider restoring a length check to avoid potential panics.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In both `readValue` and `maUseCaseSupportUpdate` you use `defer c.mu.Lock()` after acquiring the lock, which will deadlock; this should be `defer c.mu.Unlock()` so the mutex is properly released.
- The new `readPhases` helper indexes `res[0]`, `res[1]`, and `res[2]` without verifying the slice length (both on cache hit and after `update`), whereas the previous implementation guarded against invalid lengths; consider restoring a length check to avoid potential panics.

## Individual Comments

### Comment 1
<location> `meter/eebus.go:138-139` </location>
<code_context>
-	c.log.TRACE.Printf("Power: %.0fW", data)
-	c.power.Set(data)
-}
+	c.mu.Lock()
+	defer c.mu.Lock()

-func (c *EEBus) maDataUpdateEnergyConsumed(entity spineapi.EntityRemoteInterface) {
</code_context>

<issue_to_address>
**issue (bug_risk):** Deferred call should unlock the mutex, not lock it again

In `readValue`, `defer c.mu.Lock()` will re-acquire the mutex instead of releasing it, causing a deadlock. Use `defer c.mu.Unlock()` so the mutex is released after the critical section.
</issue_to_address>

### Comment 2
<location> `meter/eebus.go:134-143` </location>
<code_context>
-	if err != nil {
-		return 0, 0, 0, api.ErrNotAvailable
+func (c *EEBus) readPhases(cache *util.Value[[]float64], update func(entity spineapi.EntityRemoteInterface) ([]float64, error)) (float64, float64, float64, error) {
+	if res, err := cache.Get(); err == nil {
+		return res[0], res[1], res[2], nil
 	}
-	if len(res) != 3 {
</code_context>

<issue_to_address>
**issue (bug_risk):** Lack of length checks on cached phase slice can cause panics

`Currents` and `Voltages` previously guarded `res[0..2]` with `len(res) == 3`. `readPhases` now indexes `res[0..2]` from both `cache.Get()` and `update(c.maEntity)` without validating the slice length, so a shorter slice will panic. Please reintroduce a length check (e.g. `len(res) == 3`) and return an error instead of indexing when the check fails on both paths.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andig andig enabled auto-merge (squash) January 7, 2026 13:04
@andig andig merged commit fea6e09 into master Jan 7, 2026
6 checks passed
@andig andig deleted the fix/eebus-meter branch January 7, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants