-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
add node Location and node Name on Value metadata #57
Conversation
Pull Request Test Coverage Report for Build 413157189
💛 - Coveralls |
@billiaz IMO this should be added in a configuration options like "add node info to payload" (available only for json and valueId payloads). Adding them to valueIds is not a good practice as it will be repeated. It should be added only before sending the payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be added only to the payload and make it configurable using options in the ui
I suspect this https://github.com/zwave-js/zwavejs2mqtt/blob/03d4d0e6cb69a82156f588346ede71572ca85a2a/lib/Gateway.js#L291 is a good point to build the code. Maybe is a better spot I cannot currently see |
lib/Gateway.js
Outdated
@@ -291,6 +291,10 @@ function onValueChanged (valueId, node, changed) { | |||
case 1: // entire zwave valueId object | |||
data = copy(valueId) | |||
data.value = tmpVal | |||
if (this.config.includeNodeInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add this even for the json type, maybe after the switch:
if(typeof data === 'object' && this.config.includeNodeInfo) {
data.nodeName = node.name
data.nodeLocation = node.loc
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this. but just before this, there is a data.value
which obviously has to work only if this is an object.
maybe is better to make it
if (typeof data === 'object') {
data.value = tmpVal
if(this.config.includeNodeInfo) {
data.nodeName = node.name
data.nodeLocation = node.loc
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean add this after the switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, my question is. if data.value assumes data is object, why only check the nodeName/Location in our case?
shouldn't data.value also be checked for the same typeof?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, in the switch we check for the payload type, later if the payload is an object and the includeNodeInfo flag is true we add the node info
Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @billiaz 🙏
MQTT is a powerfull tool. Using data from to generate graphs is a good approach.
In tools like influxdb, you rely on the whole Payload read. Adding node Location and Name in metadata is a way to use and analyse the data
Added nodeLocation and nodeName as metadata, to help searching/storing data.