Skip to content

Conversation

@ngehrsitz
Copy link
Contributor

Looks like #20451 is done apart from some test. Continuing from #20742

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 @ngehrsitz - I've reviewed your changes - here's some feedback:

  • The watchdog timeout in "charge" mode (30s) appears to conflict with its comment (3 minutes); please clarify.
  • Consider if the hardcoded 1-second sleep in the "charge" sequence should be configurable.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

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 marked this pull request as draft May 9, 2025 16:15
@andig andig added the devices Specific device support label May 10, 2025
@ngehrsitz ngehrsitz marked this pull request as ready for review May 10, 2025 15:17
@andig andig changed the title Sonnenbatterie 5/6 support batterymode Sonnenbatterie 5/6: add battery control May 11, 2025
@andig andig marked this pull request as draft May 11, 2025 13:41
@ngehrsitz ngehrsitz force-pushed the sonnen-eco56-batterymode-static branch 2 times, most recently from e0b29b0 to 14c32de Compare May 12, 2025 18:29
@ngehrsitz ngehrsitz force-pushed the sonnen-eco56-batterymode-static branch from 14c32de to 1c98b45 Compare May 12, 2025 18:31
@ngehrsitz ngehrsitz marked this pull request as ready for review May 12, 2025 18:32
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 @ngehrsitz - I've reviewed your changes - here's some feedback:

  • In plugin/watchdog.go you can simplify make([]T, 0, 0) to the literal []T{} and consider making BoolSetter support multiple reset values like the Int/Float setters for consistency.
  • In plugin/sleep.go use time.Sleep(o.duration) instead of <-time.After(o.duration) to avoid unnecessary timer allocations for a simple delay.
  • Double-check the indentation of the maxchargepower templating in the YAML to ensure it renders correctly under both branches.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

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.

@ngehrsitz
Copy link
Contributor Author

Mein workaround funktioniert, langfristig sollte man sich überlegen, ob der ursprüngliche Fall mit einem Watchdog hinter einem switch statement nicht auch den watchdog abbrechen sollte.

@andig
Copy link
Member

andig commented May 12, 2025

Workaround wofür? Der WDT bricht bei reset ab.

Co-authored-by: andig <cpuidle@gmail.com>
body: '55' # slave mode
- source: watchdog
timeout: 30s # 3 minutes without setting a value will stop all charging
reset: 1,2
Copy link
Member

Choose a reason for hiding this comment

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

Hast Du das getestet? Reset kann laut Code nur einen Wert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deshalb habe ich das in c34d8df eingebaut

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice. Einfacher: mach config.Reset []string, dann sparst Du das Splitting. Leere Slices einfach als

var reset []int64

@ngehrsitz
Copy link
Contributor Author

Workaround wofür? Der WDT bricht bei reset ab.

Du meinst wahrscheinlich

  batterymode:
    # use a sequence to propagate resets to the watchdog
    source: watchdog
    timeout: 30s # 3 minutes without setting a value will stop all charging
    reset: 1,2
    set:
      source: switch
      switch:
        - case: 1 # normal
          set:
            source: http
            method: PUT
            uri: http://{{ .host }}:{{ .port }}/rest/devices/battery/C06
            body: '10' # Automatic
        - case: 2 # hold
          set:
            source: http
            method: PUT
            uri: http://{{ .host }}:{{ .port }}/rest/devices/battery/C06
            body: '20' # Standby
        - case: 3 # charge
          set:
            source: sequence
            set:
              - source: http
                method: PUT
                uri: http://{{ .host }}:{{ .port }}/rest/devices/battery/C06
                body: '55' # slave mode
              - source: sleep
                duration: 1s
              - source: http
                method: PUT
                uri: http://{{ .host }}:{{ .port }}/rest/devices/battery/C24
                body: {{ if .maxchargepower }}{{ .maxchargepower }}{{ else }}99000{{ end }}

Wenn ich das so definiere wird aber auch der Modus immer wieder geändert:

[batterie] TRACE 2025/05/12 20:46:35 PUT http://10.10.240.50:7979/rest/devices/battery/C06
[batterie] TRACE 2025/05/12 20:46:35 55
[batterie] TRACE 2025/05/12 20:46:36 PUT http://10.10.240.50:7979/rest/devices/battery/C24
[batterie] TRACE 2025/05/12 20:46:36 450
[main  ] INFO 2025/05/12 20:46:36 waiting for: 1m0s
[batterie] TRACE 2025/05/12 20:46:50 PUT http://10.10.240.50:7979/rest/devices/battery/C06
[batterie] TRACE 2025/05/12 20:46:51 55
[batterie] TRACE 2025/05/12 20:46:52 PUT http://10.10.240.50:7979/rest/devices/battery/C24
[batterie] TRACE 2025/05/12 20:46:53 450

Genau das will ich aber vermeiden, weil die API leider total instabil ist und bei zu vielen requests aussteigt.

@andig
Copy link
Member

andig commented May 12, 2025

Oh, ok- dann mach bitte noch einen Kommentar damit man das versteht.

@ngehrsitz
Copy link
Contributor Author

Beim debuggen dieses PR ist mir aufgefallen, dass aktuell die status requests auf /rest/devices/battery mehrfach ausgeführt werden. Vermutlich passiert das wegen der verwendung als "grid", "pv", "battery"
grafik
Bei http requests wäre es doch naheliegend clientseitiges Caching zu implementieren, damit nur ein einziger Request pro Gerät und Updatezyklus ausgeführt wird. Fehlt das noch oder funktioniert das nur bei mir nicht?

@ngehrsitz ngehrsitz requested a review from andig May 12, 2025 19:20
@andig
Copy link
Member

andig commented May 12, 2025

Du hast keins konfiguriert ;)

@ngehrsitz
Copy link
Contributor Author

Du hast keins konfiguriert ;)

Meinst du damit 6bb8612?
Die einzige Änderung die ich damit feststellen konnte ist der Cache-Control header:
grafik
Auch sonst habe ich im Code nichts gefunden?

@andig
Copy link
Member

andig commented May 13, 2025

Meinst du damit 6bb8612?

Genau. Damit sollte intern gecached werden- das siehst Du aber nur mit Wireshark.

@ngehrsitz
Copy link
Contributor Author

Meinst du damit 6bb8612?

Genau. Damit sollte intern gecached werden- das siehst Du aber nur mit Wireshark.

Der Screenshot ist von mitmproxy. Wieso sollte das dort nicht sichtbar sein?
Das Problem ist, dass die Requests als stale angesehen werden:
https://github.com/gregjones/httpcache/blob/901d90724c7919163f472a9812253fb26761123d/httpcache.go#L163-L188
Das passiert weil der server keinen Date Header zurückliefert den der Cache aber zum vergleich nutzt:
https://github.com/gregjones/httpcache/blob/901d90724c7919163f472a9812253fb26761123d/httpcache.go#L304-L307
Wenn ich diesen Header in L221 manuell setzte, dann reduziert sich die Anzahl der requests von 7 auf 3

	_, dateErr:=  Date(resp.Header)
	if errors.Is(dateErr,ErrNoDateHeader) {
		resp.Header.Set("Date", time.Now().Format(time.RFC1123))
	}

Der Grund dafür dürfte sein, das parallele Requests nicht kombiniert werden und der Cache erst dann eingreift, wenn er einen request gespeichert hat.
Da das ganze Projekt unmaintained ist sollten man wohl perspektivisch auf etwas anderes wechseln: gregjones/httpcache#117 (comment)
Ich habe den Cache mal im template aktiviert auch, wenn er noch nicht funktioniert. Den Cache zu fixen/ersetzen passt besser in einen anderen PR.

}

var reset *int64
reset := make([]int64, 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reset := make([]int64, 0, 0)
var reset []int64

dito unten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ngehrsitz ngehrsitz requested a review from andig May 13, 2025 20:37
@andig andig merged commit 34cf433 into evcc-io:master May 13, 2025
6 checks passed
guido4096 pushed a commit to guido4096/evcc that referenced this pull request May 25, 2025
StarF666 pushed a commit to StarF666/evcc that referenced this pull request Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devices Specific device support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants