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

waitForResponse provider #18099

Closed
wants to merge 1 commit into from
Closed

Conversation

skrul
Copy link

@skrul skrul commented Jan 7, 2025

The waitForResponse provider (final name tbd) is useful for waiting on (and checking) a response to a setter that executes its side effect asynchronously. This provider itself is a setter and is configured with two separate providers, one a set provider that will asynchronously cause a side effect, and one get provider that will block until a response to the side effect is available. The result of the get is converted into a boolean using strconv.ParseBool to determine if the set was successful.

The classic example where this is useful is an MQTT based API where a request is made by publishing to a given topic, which is consumed by a device. The device then publishes the response on a separate topic to be consumed by the caller. This provider can connect the two legs of this exchange to make it possible to treat this style of communication as a synchronous API call.

Note that this will not work correctly if the configured get provider is does not block while waiting for the response as the implementation first attempts to read the getter and while that blocks calls the setter.

An example configuration that uses this provider looks like:

batterymode:
  source: switch
  switch:
  - case: 1
    set:
      source: sequence
      set:
        - source: waitForResponse
          set:
            source: mqtt
            topic: dongle-40:4C:CA:xx:xx:xx/update
            payload: '{"setting": "DischgPowerPercentCMD", "value": 100, "from": "evcc"}'
          get:
            source: mqtt
            topic: dongle-40:4C:CA:xx:xx:xx/response
            jq: .status == "success"
            timeout: 30s

In this example, the DischgPowerPercentCMD command is sent to the /update topic which is consumed by the LUX inverter. The result status is then published to the /response topic (which looks like {"status":"success"}). This is consumed by the get MQTT provider and transformed by the jq expression into something that strconv.ParseBool can parse.

@andig curious what you think! if you like the direction, I'm wondering if I should be implementing setters for bool and float as well. Also I'm happy to change the name if you can think of something better.

@skrul
Copy link
Author

skrul commented Jan 7, 2025

I think there is probably a race condition here since it is probably possible to invoke the setter before the getter is waiting. I'm a go noob but I'll try to figure it out!

@skrul
Copy link
Author

skrul commented Jan 7, 2025

I think there is probably a race condition here since it is probably possible to invoke the setter before the getter is waiting. I'm a go noob but I'll try to figure it out!

Ah never mind, it is fine. If the setter invoked before the getter is waiting, the getter will still get the message when it starts listing because the message will stay in the topic until it is consumed.

@premultiply
Copy link
Member

premultiply commented Jan 7, 2025

I wonder how MQTT should make synchronous communication possible in this way, since to my knowledge any client can write in any order at any time on a topic. There is missing a message ID to track the request to the response.

From my point of view the API of the device is broken. MQTT does not provide a synchronous transmission channel.
The required data points should be mapped to different topics instead like the registers using Modbus communication.
Then your clients can use QoS levels on MQTT to make sure settings are applied and received by the controlled device in proper order.

@andig
Copy link
Member

andig commented Jan 7, 2025

From my point of view the API of the device is broken.

It's certainly not nice- but then it's whats there :O.

@skrul out of curiosity: what does the dongle do with the confirmation after sending success? I.e. if you change the mode twice- will the first confirmation still be there and also be read when setting the mode for the second time? If that's the case then this plugin doesn't do much in the general sense (and we should rather go for the Go implementation?).

@andig andig added the enhancement New feature or request label Jan 7, 2025
@skrul
Copy link
Author

skrul commented Jan 7, 2025

@premultiply agree about the id, this is why I had to create a new mqtt getter every time a new value was set on the waitForResponse provider, which, when the provider is mqtt, will create a new listener on the topic, which is quite bad! Also, I can't really comment on the design of the dongle API but I do know that the device is an embedded system that is very resource constrained so tradeoffs were made. Note there was some previous discussion in #18056 as well.

@andig I had experimented with this, I created a source: sequence that chained two waitForResponse providers together. This is when I discovered that I had to create the getter inside the IntSetter closure. If I didn't do that, there would be two listeners set up at initialization time and the first set would run and both listeners would see the response. Moving the listener setup into the closure fixed that such that by the time the second listener is set up, the response from the first set would be consumed. It is quite ugly though and could be fixed by the id system that @premultiply suggested.

I think the having an ID that the set includes in its message and then the get could look for in the response would make this a bit nicer. However, I see two issues:

First, the ID would have to be generated by the waitForResponse and passed to both the set and get providers so they could incorporate it into the request or the jq expression that transform the response. It is not clear to me if this is possible.

Secondly, in the case of the LUX dongle, what the ID can actually be is pretty restrictive. The author agreed to add this capability, but due to memory constraints, the ID value can only be a byte or two. I could imagine adding a config field on waitForResponse that allows the user to specify a type or something for the ID (by default I think it should be a UUID).

So i think if the id passing issue can be solved then waitForResponse could be viable. I do need to fix my implementation though (I don't think the go channel is actually needed). You could imagine this provider supporting an option to poll the get in the case that the get was an HTTP endpoint. Maybe useful to someone in the future?

Having custom go code for the monitormysolar integration is still an option. Since the author of the dongle will add a special mqtt topic to handle the evcc battery control requests, the implementation would be even simpler than what is in #18056.

Finally, doing nothing is also an option. The serialization of a series of mqtt requests is no longer needed due to the author adding a new topic for the evcc battery control requests. The integration could just use the current mqtt provider and "fire and forget" the request and hope that it works.

Thanks for all your help on this!

@andig
Copy link
Member

andig commented Jan 8, 2025

The integration could just use the current mqtt provider and "fire and forget" the request and hope that it works.

I guess if errors are the absolue exception then that would be acceptabler. Otherwise lets stay with custom Go- seems the response pattern is too specific.

@skrul
Copy link
Author

skrul commented Jan 8, 2025

My understanding is that the commands do fail from time to time and waking up with a depleted battery would really be unfortunate. Let me incorporate the evcc battery control API that the author is creating into the original PR and let's pick up the discussion then (that API might take a few weeks to land).

@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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants