-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
zWave - Dimmer integration #757
Changes from all commits
84d0394
4cdd849
1ef4b04
ed41507
6391410
c08f0be
0399d26
f5c4325
75c5491
a387c21
5727e25
c0e5e15
08f2fe2
9352108
854de8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
const { transformValue, normalizeValue } = require('../utils/valueBinders'); | ||
|
||
const DefaultCommandClass = function DefaultCommandClass() {}; | ||
|
||
DefaultCommandClass.prototype = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why didn't you use functions? I don't really approve the use of this design pattern here. Just write the function and exports them, the simpler the better! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because they are not just "functions" it's a real object with real behaviors. It's not just a set of tools or utils. It represents an object with a set of functionnalities. And it's actually how Javascript is working for this. Finality here is to have the possibility to get one fully representative object per zWave CommandClass to handle specific cases on specific CommandClasses. Look at how the factory is working, look at the SwitchMultilevelCommandClass, take a deeper look on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I read the code, I don't see the added value in using POO here. It's a singleton class with only static functions, it could just be functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are not static methods. Writing this comment... I just think about one thing: I maybe mixed two things in your comment (or mixed the two comments, my bad): code splitting and POO (the other comment). This code commented here is exactly the same thing as here: https://github.com/GladysAssistant/Gladys/blob/master/server/services/zwave/lib/index.js#L64 (and all over the project). So, if your point is to explode each function in a dedicated file to finaly put it in the prototype: this code is doing exactly the same thing... but in a single file. It's far most easier to navigate in your class within a single file (knowing which properties are available) than in one class with multiple files (for example, when I'm in the file zwave.setValue About the POO, the other comment, look at the usage of the getMinMax value. Without it, you would have a function growing over and over with command classes. The function would consist of a big Look at the code when a "value changed" event is received. Without POO, you will also need a function based on switch and functions and so on... It's not relevant enough now because we have only the SwitchMultiLevelCommandClass implemented. With time, you will have more and more. You are the owner of the project, you will have the final word. For inheritance, I'm convinced this is a long term advantage and avoid errors in coming CommandClasses implementations. There is no magick here, just usual Javascript. Let me know There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They are not using "this". They are just using function parameters to return a result, it's just utils functions.
Yes, but it'll be readable. Right now, yes it works but it's hard to understand and it'll lead to weird bugs in the future. Inheritance is debatable, to me it'll just make the maintenance more complex in this case. From what I see, your function are just "utils" functions, it could be a simple file with functions and you export the functions. About the singleton problem I don't agree, it's not the same as others design patterns used. You're exporting the object, not the class. In short: you're using the filesystem (require) as a way to export a singleton. In Node.js, when you do a "require" on a file, the result is cached. So if N files are requiring your file, just one instance of your class will be created. This is not right, because it means the object is now a global, and tests are sharing a global variable. In the example you take, I'm exporting the class, not an object of the class.
Just do a simple file with functions and call the functions: simpler, easier to read & maintain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if I don't share all your points, you're the boss. As soon as I have time, I'll update the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can always debate about that in a community call, it'll be more easy to talk about that vocally ;)
Thank you very much for your time and contribution to Gladys 🙏 Don't take personally my feedbacks, computer science is a topic full of controversy and everyone has different theory. It's great to have such discussion in PR 🙂 |
||
/** | ||
* @description Get the command class ID. | ||
* | ||
* @returns {number} - Command Class ID. | ||
* | ||
* @example commandClass.getId(); | ||
*/ | ||
getId: function getId() { | ||
return -1; | ||
}, | ||
/** | ||
* @description Get the value following a value changed event. | ||
* | ||
* @param {Object} node - The node the value changed occurs. | ||
* @param {Object} valueChanged - The value changed. | ||
* | ||
* @returns {Object} - A zWave value. | ||
* | ||
* @example | ||
* const changedValue = comClass.getChangedValue(node, value); | ||
*/ | ||
getChangedValue: function getChangedValue(node, valueChanged) { | ||
return valueChanged; | ||
}, | ||
/** | ||
* @description Transform a value according to the Command Class. | ||
* | ||
* @param {Object} node - Node of the value. | ||
* @param {number} comClass - Command Class Id of the value. | ||
* @param {number} index - Index of the value. | ||
* @param {any} value - Value to transform. | ||
* | ||
* @returns {any} The transformed value. | ||
* | ||
* @example comClass.getTransformedValue(node, 110, 0, 50); | ||
*/ | ||
getTransformedValue: function getTransformedValue(node, comClass, index, value) { | ||
return transformValue(node.classes[comClass][index].type, value); | ||
}, | ||
/** | ||
* @description Normalize a value according to the Command Class. | ||
* | ||
* @param {Object} node - Node of the value. | ||
* @param {number} comClass - Command Class Id of the value. | ||
* @param {number} index - Index of the value. | ||
* @param {any} value - Value to transform. | ||
* | ||
* @returns {any} The normalized value. | ||
* | ||
* @example comClass.getNormalizedValue(node, 110, 0, 50); | ||
*/ | ||
getNormalizedValue: function getNormalizedValue(node, comClass, index, value) { | ||
return normalizeValue(node.classes[comClass][index].type, value); | ||
}, | ||
/** | ||
* @description Get the Min/Max values for a given value. | ||
* | ||
* @param {Object} node - Node to get the min/max from. | ||
* @param {number} comClassId - Command Class Id to get the min/max. | ||
* @param {number} index - Index of the value to get the min/max. | ||
* | ||
* @returns {Object} Returns min and max values for a given value. | ||
* | ||
* @example const {min, max, step} = commClass.getMinMax(node, comClassId, index); | ||
*/ | ||
getMinMax: function getMinMax(node, comClassId, index) { | ||
return { | ||
min: node.classes[comClassId][index].min, | ||
max: node.classes[comClassId][index].max, | ||
step: 1, | ||
}; | ||
}, | ||
}; | ||
|
||
module.exports = { | ||
instance: new DefaultCommandClass(), | ||
DefaultCommandClass, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
const defaultCommandClass = require('./defaultCommandClass').instance; | ||
const switchMultilevelCommandClass = require('./switchMultilevelCommandClass').instance; | ||
|
||
const commandClasses = {}; | ||
commandClasses[switchMultilevelCommandClass.getId()] = switchMultilevelCommandClass; | ||
|
||
const commandClassKnownIdList = Object.keys(commandClasses).map((comClass) => parseInt(comClass, 10)); | ||
|
||
/** | ||
* @description Get the command class corresponding to comClass. | ||
* | ||
* @param {number} comClass - Command Class Id. | ||
* | ||
* @returns {Object} The command class. | ||
* @example getCommandClass(0x38); | ||
*/ | ||
function getCommandClass(comClass) { | ||
if (commandClassKnownIdList.indexOf(comClass) === -1) { | ||
return defaultCommandClass; | ||
} | ||
|
||
return commandClasses[comClass]; | ||
} | ||
|
||
module.exports = { | ||
getCommandClass, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
const { DefaultCommandClass } = require('./defaultCommandClass'); | ||
const logger = require('../../../../utils/logger'); | ||
const { COMMAND_CLASSES, INDEXES } = require('../constants'); | ||
|
||
const SwitchMultilevelCommandClass = function SwitchMultilevelCommandClass() { | ||
DefaultCommandClass.call(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I don't think we want this kind of inheritance here, it complexifies the code for not much value added. Can you just use functions? The simpler, the better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous response, the goal is to get a real object definition here to represent CommandClasses. All CommandClasses have some generic behavior and some specific ones. POO is perfectly adapted for this. |
||
}; | ||
SwitchMultilevelCommandClass.prototype = Object.create(DefaultCommandClass.prototype); | ||
Object.defineProperty(SwitchMultilevelCommandClass.prototype, 'constructor', { | ||
value: SwitchMultilevelCommandClass, | ||
enumerable: false, | ||
writable: true, | ||
}); | ||
|
||
/** | ||
* @description Test if provided comClass support V4. | ||
* | ||
* @param {Object} comClass - The comClass zWave object. | ||
* | ||
* @returns {boolean} True if the current node supports V4. False otherwise. | ||
* | ||
* @example | ||
* const isSupportingV4 = supportV4(zWaveComClassObject); | ||
*/ | ||
function supportV4(comClass) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function name is not really explicit, which v4 is it? Gladys v4? From an outside point of view, it's not clear enough :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are here in a method from the switchMultilevelCommandClass class. We are so talking about the V4 of the SwitchMultilevelCommandClass... I can rename it if you prefer... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's not clear enough |
||
return ( | ||
Object.keys(comClass).filter((key) => parseInt(key, 10) === INDEXES.INDEX_SWITCH_MULTILEVEL_TARGET).length === 1 | ||
); | ||
} | ||
|
||
/** | ||
* @description Get the command class ID. | ||
* | ||
* @returns {number} Command Class ID. | ||
* | ||
* @example commandClass.getId(); | ||
*/ | ||
SwitchMultilevelCommandClass.prototype.getId = function getId() { | ||
return COMMAND_CLASSES.COMMAND_CLASS_SWITCH_MULTILEVEL; | ||
}; | ||
|
||
/** | ||
* @description Get the value following a value changed event. | ||
* | ||
* @param {Object} node - The node the value changed occurs. | ||
* @param {Object} valueChanged - The value changed. | ||
* | ||
* @returns {Object?} - The zWave value to update. NULL if change MUST be | ||
* prevented. | ||
* | ||
* @example | ||
* const changedValue = comClass.getChangedValue(node, value); | ||
*/ | ||
SwitchMultilevelCommandClass.prototype.getChangedValue = function getChangedValue(node, valueChanged) { | ||
const comClass = node.classes[this.getId()]; | ||
|
||
// Specific case handling for Switch Multilevel with V4 implementations. | ||
// In the V4 implementation, the level should be based on the target value | ||
// and not the level one. | ||
// See following docs: | ||
// - Silabs Application Command Class Specification §4.72 - Multilevel Switch Command Class, version 4: | ||
// https://www.silabs.com/documents/login/miscellaneous/SDS13781-Z-Wave-Application-Command-Class-Specification.pdf | ||
// - OpenZwave 1.6 Update: | ||
// https://github.com/OpenZWave/open-zwave/wiki/OpenZWave-1.6-Release-Notes#switchmultilevel-commandclass | ||
|
||
if (!supportV4(comClass)) { | ||
return valueChanged; | ||
} | ||
|
||
if (valueChanged.index === INDEXES.INDEX_SWITCH_MULTILEVEL_TARGET) { | ||
logger.debug('Target value identified. Setting the level value instead'); | ||
const levelValue = comClass[INDEXES.INDEX_SWITCH_MULTILEVEL_LEVEL]; | ||
|
||
// Clone levelValue to the expected object to set. | ||
// We need to clone the object to avoid future side effect with object | ||
// in-memory. | ||
const updatedLevel = { ...levelValue }; | ||
updatedLevel.value = valueChanged.value; | ||
|
||
return updatedLevel; | ||
} | ||
|
||
if (valueChanged.index === INDEXES.INDEX_SWITCH_MULTILEVEL_LEVEL) { | ||
// Ignore the value event | ||
logger.debug('Ignoring level'); | ||
|
||
return null; | ||
} | ||
|
||
// It's not the LEVEL neither the TARGET value. | ||
return DefaultCommandClass.prototype.getChangedValue.call(this, node, valueChanged); | ||
}; | ||
|
||
/** | ||
* @description Get the Min/Max values for a given value. | ||
* | ||
* @param {Object} node - Node to get the min/max from. | ||
* @param {number} comClassId - Command Class Id to get the min/max. | ||
* @param {number} index - Index of the value to get the min/max. | ||
* | ||
* @returns {Object} Returns min and max values for a given value. | ||
* | ||
* @example const {min, max} = commClass.getMinMax(node, comClassId, index); | ||
*/ | ||
SwitchMultilevelCommandClass.prototype.getMinMax = function getMinMax(node, comClassId, index) { | ||
if (index === INDEXES.INDEX_SWITCH_MULTILEVEL_LEVEL) { | ||
return { | ||
min: 0, | ||
max: 99, | ||
step: 1, | ||
}; | ||
} | ||
|
||
return DefaultCommandClass.prototype.getMinMax.call(this, node, comClassId, index); | ||
}; | ||
|
||
module.exports = { | ||
instance: new SwitchMultilevelCommandClass(), | ||
}; |
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.
Please use camelCase :)