Add MQTT remote API #3692
Add MQTT remote API #3692
Conversation
|
|
| private startEventListeners(): void { | ||
| // Listen for variable changes | ||
| this.#serviceApi.on('variables_changed', this.variablesChanged) | ||
| } | ||
|
|
||
| private stopEventListeners(): void { | ||
| this.#serviceApi.removeListener('variables_changed', this.variablesChanged) | ||
| } |
There was a problem hiding this comment.
I'm not keen on where these are being called from, as it makes it hard to tell when they are being called and to ensure that they are called the correct number of times.
I think it would be easier to follow if they were setup in just the constructor of the class. And teardown might not be necessary (ember+ doesnt do it), as we currently instantiate these at startup and keep them until exit.
The cost of the methods firing when there is no client connected is not enough to worry about I expect, perhaps the variablesChanged method needs a 'is connected' check too?
| if (label == 'internal' && name.startsWith('custom')) { | ||
| label = 'custom' | ||
| name = name.substring(7) | ||
| } |
There was a problem hiding this comment.
I don't think this block will do anything, while there are some internal aliases of internal:custom_* to custom:*, that is all handled at the point of reading the values, they should never be reported as being changed (I think)
There was a problem hiding this comment.
I now see that the ember+ service was doing this already, I shall look into removing it from there too
| if (label === 'custom') { | ||
| value = this.#serviceApi.getCustomVariableValue(name)?.toString() | ||
| if (value === undefined) return |
There was a problem hiding this comment.
You can skip this block. While technically more correct, its a few levels of indirection that result in the same thing as calling this.#serviceApi.getConnectionVariableValue('custom', name).
It exists primarily to allow the caller to not need to know that the label is 'custom', but you dont need to worry about that as you already have the label and name pair
| if (value === undefined) return | ||
| } else { | ||
| value = this.#serviceApi.getConnectionVariableValue(label, name)?.toString() | ||
| if (value === undefined) return |
There was a problem hiding this comment.
I suspect that you don't want to do this check, as when a variable gets removes/unset then you wont be reporting that to clients.
In ember+, this would usually manifest as the node going away, but I dont think you can do the same, so should probably report the variable becomign empty
| if (value === undefined) return | ||
| } | ||
|
|
||
| const topic = `${this.topic}/variables/${label}/${name}` |
There was a problem hiding this comment.
Just thinking about future proofing, it might be better to do ${this.topic}/variables/${label}/${name}/value, so that the description or type or something could be added as separate properties later?
Or maybe the message should be a small json blob:
{
"value": "abc"
}
to allow for other details to be added with it?
| this.handleMessage(topic, message) | ||
| this.logger.debug(`Received MQTT message on topic ${topic}: ${message.toString()}`) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
should any current values be published? having to wait for them to change could be problematic at times?
|
Thanks for the review! May be a week or so but I'll take a look and make adjustments 🙂 |
3 participants
Adds an MQTT remote API to Companion providing live updates of all Companion variables and allows pressing buttons and setting custom variables.
I used the Ember+ module as a reference, and tried to align syntax for commands with the other modules as closely as possible.
Uses the same
mqttdependency as the Generic MQTT Companion ModuleThis is my first PR against Companion core. I wasn't 100% sure how to handle the yarn.lock file for the PR.