Consider scoping callback "events" to the client that issued the command

[AlCalzone]

Yeah that's the plan. Anything that requires a callback becomes an event. It's not perfect because every client gets notified, not just the one that made the request, but it hasn't been a problem yet

Originally posted by @raman325 in #1480 (comment)

[raman325]

I made this comment offhand, but what is the desired behavior here? It's a relatively easy fix, right now we loop through all clients in the message handler when sending the callback event, but we get the client in the handler call and could easily just send it to them

[AlCalzone]

We may have to think about this some more, I just wanted to put it on the list, so we don't forget.
Mostly: Are we relying on the current behavior somewhere, or do we ignore unexpected events anyways?

[raman325]

now that I think about it, we may have done this intentionally. Some of the events are used to manage state in the server for e.g. pairing, firmware progress updates, nvm restore/convert progress updates, statistics etc. so that clients can access the state without having to query the controller. Clients of course don't have to access the state and can ignore the events. IIRC the frontend for some of these functions, like firmware updates and pairing, relies on this internal state for the initial load because we only start listening to the relevant events when the modal is opened.

As a result, I can either start a pairing operation from one client and monitor it in both (if I pulled up the add node dialog in both instances it would show the same thing while the operation is in progress) or start it in one place and finish it in another.

this is a long way of saying I think we should leave things as is and close this. @MartinHjelmare do you have any other thoughts or opinions?

[MartinHjelmare]

If we don't have a reason to change it, I don't think we should change it.

[raman325]

turns out we are inconsistent about this. From Discord, @AlCalzone said: "FWIW, I still tend to lean towards scoping the callback events to the clients that initiated the process"

I am good with that approach. Come to think of it, that more cleanly models zwave-js behavior with the callbacks, so that makes sense to me. I will create a new PR that fixes this for existing code and update PRs to match this behavior if relevant

[MartinHjelmare]

What's the reason to change it?

What are the consequences if we change it?

[raman325]

In our design, we handle callbacks in zwave-js API calls by emitting corresponding events over websocket back to the client(s).

For the two inclusion commands, the callbacks require some sort of orchestration outside of zwave-js (typically user input), and only the calling client receives the corresponding events:

• controller.begin_inclusion
• controller.replace_failed_node

We have other commands that emit progress events but all clients receive them:

• controller.backup_nvm_raw
• controller.restore_nvm
• node.test_power_level
• node.check_lifeline_health
• node.check_route_health

Then we have three new commands we are adding in #1513 with events, 2 progress ones and 1 inclusion one:

• controller.restore_nvm_raw (progress)
• node.check_link_reliability (progress)
• controller.begin_joining_network (inclusion like)

It was while adding these commands and doing some refactoring that I discovered that there are these two types of events with two different behaviors:

• Inclusion related events: sent to calling client only
• Progress events: sent to all connected clients

To me, it doesn't make sense to have two behaviors, we should have one. And even if we did keep different behaviors, I'd flip them - inclusion related activities can involve moving around between devices and being able to move from e.g. a client in one location to a client in another might make sense.

I checked the python library, we don't do any special handling of these events, so the consequence is effectively a no-op from a Home Assistant perspective. As for other use cases, I don't think progress events make sense without the original context of what was being done, which only the caller has - as an example, when testing a route, you have to choose a source and target node, but only the source node is known in the events. So I think it is safe to make this change.

TLDR:

• We have two different behaviors for two different types of events generated from callbacks, and I believe that all of our callback events should only be sent to the caller
• While it is technically a breaking change, I don't think it has any practical impact to anyone

[MartinHjelmare]

Don't we use these events in the integration in Home Assistant?

[AlCalzone]

AFAIK we use these two in HA:

• controller.backup_nvm_raw
• controller.restore_nvm

But even then, if HA triggers these commands, only HA can really make use of them, since it's the only client that will receive the end result too.

A Z-Wave JS UI listening in would only see that the process is going on but it will never receive the final result, so it can't do anything with it.