Skip to content
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

meter for monitormysolar dongle #18056

Closed
wants to merge 1 commit into from
Closed

Conversation

skrul
Copy link

@skrul skrul commented Jan 5, 2025

No description provided.

@andig
Copy link
Member

andig commented Jan 5, 2025

Thank you for the PR. Afaikt this is all „just“ mqtt. Do we really need Go code or is a template with mqtt plugins sufficient?
update I see- it's for the mqtt response. Unless there's a high probability of responses failing, that part could potentially be omitted? The less Go code the core team has to maintain, the better...

@andig andig added the devices Specific device support label Jan 5, 2025
@skrul
Copy link
Author

skrul commented Jan 6, 2025

@andig thanks for looking at this! for context, the MQTT provider was sufficient for reading the grid/pv/battery power (see this config file). However, implementing battery control could not be done with the MQTT provider since it requires several commands to be sent in serial to the inverter. Specifically, for the lux inverter, you have to turn on AC charging mode and also set up an AC charging schedule that includes current time to actually start charging. You also need to make sure battery discharge is set back to zero, etc. The MQTT API for the inverter is pretty low level!

All that said, I was talking to the person who builds the interface for these inverters and he agreed to add some new MQTT based APIs that implement the EVCC battery control API. This simplification could obviate the need for the go code here and we can just use the MQTT provider.

However, one deficiency of the MQTT provider is that it is sort of "fire and forget". The MQTT API for the inverter does provide a separate topic for responses, so when you send a message to the command topic, you can wait for a message on the response topic to get a success or error result. Due to the fact that setting the battery mode on this inverter requires several commands internally, it could take upwards of 30 seconds to complete and it also does fail from time to time. The implementation in this PR does all of this, and calls to SetBatteryMode will block until the setting change completes (or times out) and returns a proper error value.

So please let me know your thoughts. If native support for the battery control APIs are added to the dongle, that would simplify things enough to create a template using the MQTT provider. However, I do feel like the error handling via the response topic may be valuable, but it is unclear if this tips the scales enough to justify adding more go code to the repo.

@andig
Copy link
Member

andig commented Jan 6, 2025

I‘m wondering- unless the Lux inverter does something very specific- can‘t we integrate that without the dongle, maxbe using Modbus? Dongle seems a good solution if we could address a wider range of inverters? And if that‘s the case, Go implementation might be the way to go.

@skrul
Copy link
Author

skrul commented Jan 6, 2025

@andig my understanding is that, while the lux inverter ostensibly has a modbus interface, the protocol is non-standard lux proprietary and will not work with standard modbus implementations. the monitorymysolar dongle was built to make it possible for third parties to interface with the lux. eventually the dongle will be extended to support inverters from Solis, SolArk, Growwatt, Solax, Synsnk, Deye, and others. another benefit of the dongle is that it is a drop in replacement for the manufacturer supplied dongle, so it opens up the data while still functioning like the manufacturer supplied dongle (sends data to the LUX cloud). So I don't think LUX can work with evcc's modbus support.

I agree with you that the go version of this meter needs to justify its existence. I do think the blocking version of SetBatteryMode is pretty critical since, for example, failure to set your battery to hold mode during a scheduled fast charge could result in your home battery getting drained overnight.

One possibility would be to add response handling to the existing MQTT provider - there could be config for a response topic, a timeout, and a jq expression that could be used to parse the response payload into a success/failure result. When provided, the Set* methods on the MQTT provider would block until the response is received (or timeout occurs). With this I feel like we could get good enough support for the dongle without needing the go code.

@andig
Copy link
Member

andig commented Jan 6, 2025

Solis, SolArk, Growwatt, Solax, Synsnk, Deye

We can do all of these except SolArk and Lux. Happy to merge but it would be great if we could rely on you for maintenance? If you want to try a wait_for_response, name tbd, plugin (that in turn could read data from an mqtt plugin, or maybe is a specific option for mqtt only), that would of course be great!

@skrul
Copy link
Author

skrul commented Jan 6, 2025

@andig can you go into a little more detail on how you think wait_for_response should be implemented (like show what the config would look like)? I think building something that would be useful to others would be my first choice here!

@andig
Copy link
Member

andig commented Jan 6, 2025

Not sure. I'd probably try something like this:

batterymode:
  source: switch
  switch:
  - case: 1 # normal
    set:
      source: wait_for_response  # maybe `validate`
      set:
        # set logic
        source: mqtt
      get:
        # validate logic
        source: mqtt
        timeout: 10s
        jq : ... # use any modifiers here
      expect: # maybe add an expected value

Set is executed by batterymode. It will then execute get and return with it's result code. For example

get:
  source: error
  error: ErrNotAvailable

would always mark the set as failed.

Unfortunately you can't use the go plugin here which would simplify things. Maybe it could also be enhanced to allow this?

@skrul
Copy link
Author

skrul commented Jan 6, 2025

@andig ok I see! let me give this a try!

@skrul
Copy link
Author

skrul commented Jan 7, 2025

See #18099 for waitForResponse implementation

@skrul skrul mentioned this pull request Jan 7, 2025
@andig andig closed this Jan 9, 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