zWave - Dimmer integration #757

sescandell · 2020-04-27T22:23:21Z

Pull Request check-list

To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:

If your changes affects code, did your write the tests?
Are tests passing? (npm test on both front/server)
Is the linter passing? (npm run eslint on both front/server)
Did you run prettier? (npm run prettier on both front/server)
If your changes modify the API (REST or Node.js), did you modify the API documentation? (Documentation is based on comments in code)
If you are adding a new features/services which needs explanation, did you modify the user documentation? See the GitHub repo and the website.
Did you add fake requests data for the demo mode (front/src/config/demo.json) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

Manage zWave Dimmer devices (devices supporting Command Class Switch MultiLevel (38)).
Introducing Command Class Object (to spread all over in the zWave service).

Update to OZW 1.6 must be merged first (#675 )

VonOx · 2020-04-28T13:28:50Z

@link39 I think you and @sescandell are working on the same things ( zwave service ).

sescandell · 2020-04-28T14:31:57Z

Oh, did I missed something? A conversation somewhere else or another PR?

link39 · 2020-04-28T14:45:43Z

Hello,

I created a topic to discuss about zwave bug and improvement :
https://community.gladysassistant.com/t/zwave-aide-pour-la-v4/5312/55

I invit you to join-us.

In my side, I added :

dimmer
multiple zwave instance
mapping some different comclass
Before doing a PR I want to develop some other things.
A debug page with a button to export openzwavecache.xml file
implement the node configuration function

sescandell · 2020-04-28T14:50:42Z

Ok, talking there

VonOx · 2020-04-28T14:51:32Z

  Before doing a PR I want to develop some other things.

@link39 you can make a draft PR so others users can see what is under developpment. ;)

codecov · 2020-05-03T16:26:02Z

Codecov Report

Merging #757 into master will decrease coverage by 0.14%.
The diff coverage is 84.46%.

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
- Coverage   93.49%   93.35%   -0.15%     
==========================================
  Files         428      433       +5     
  Lines        5595     5685      +90     
==========================================
+ Hits         5231     5307      +76     
- Misses        364      378      +14

Impacted Files	Coverage Δ
...rver/services/zwave/lib/commands/zwave.setValue.js	`33.33% <12.50%> (-16.67%)`	⬇️
...services/zwave/lib/comClass/defaultCommandClass.js	`77.77% <77.77%> (ø)`
server/services/zwave/lib/utils/valueBinders.js	`83.33% <83.33%> (ø)`
...er/services/zwave/lib/events/zwave.valueChanged.js	`90.90% <88.88%> (-9.10%)`	⬇️
...zwave/lib/comClass/switchMultilevelCommandClass.js	`90.00% <90.00%> (ø)`
server/services/zwave/lib/comClass/factory.js	`100.00% <100.00%> (ø)`
...rver/services/zwave/lib/commands/zwave.getNodes.js	`96.29% <100.00%> (+0.29%)`	⬆️
server/services/zwave/lib/constants.js	`100.00% <100.00%> (ø)`
server/services/zwave/lib/utils/getMode.js	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfb6ac6...854de8b. Read the comment docs.

sescandell · 2020-05-06T22:05:50Z

@link39 here is what I was talking about for "Command Classes objects".

For now, they are only used to manage setValue and valueChanged (and a part of getNodes for min/max values).
Next step would be to use them more globally in the zWave service for all things that would play with zWave values.

For features and params, it can be based on this logic (I can provide a sample...) but this PR is not the good place for that.

…objects (to spread in other functions)

…CommandClass objects

Pierre-Gilles · 2020-08-11T08:17:44Z

Hey ! What's the status with this PR ? :)

I really want to push Z-Wave improvements in the RC, so let me know if you need help

sescandell · 2020-08-11T19:44:14Z

Hey ! What's the status with this PR ? :)

I really want to push Z-Wave improvements in the RC, so let me know if you need help

Just need to double check live updates.

I should be able to work on it soon. I'll let you know!

sescandell · 2020-08-18T21:03:26Z

Hi @Pierre-Gilles

Just pushed a last commit. Everything is working as expected.

Next to this PR, I would be able to work on managing "features" tied to devices (following far discussions with @link39 )

The @link39 PR and this one have some things in common. That one is more focus on a specific subject and is going further on how dimmer work. Don't know if @link39 has time, but I should be able to take some of its work in another PR after that one is merged and continue to improve the zWave module.

Let me know if you have any question,

Thanks,

Pierre-Gilles

Hi !

Thanks for your PR. 👍

Here is a few feedback on the code quality of the PR. Nothing much, just a few feedbacks so the code is easier to maintain and understand in the long run.

By the way, your code coverage is not enough for the PR to pass. You need to add more tests!

See CodeCov : https://codecov.io/gh/GladysAssistant/Gladys/pull/757

Pierre-Gilles · 2020-08-31T08:00:15Z

server/services/zwave/lib/comClass/defaultCommandClass.js

+
+const DefaultCommandClass = function DefaultCommandClass() {};
+
+DefaultCommandClass.prototype = {


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!

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 getMinMax method and keep in mind that zWave command classes have common and specific behavior. It would make more sense than a set of exported functions.

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.

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 this.zWave doesn't mean anything. It's totally out of its context)

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 switch().

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.
So, about splitting the code in multiple files to put all of them in the prototype in a index.js file... Whatever... if you want... but result is exactly the same: there is no design pattern difference here... This is far most usual Javascript than using export everywhere to associate them to the prototype (to be honest, when I started to deep dive in Gladys and see this, I've been surprised... but OK, result is exactly the same). But if you absolutely want to split, I'll do.

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

They are not static methods.

They are not using "this". They are just using function parameters to return a result, it's just utils functions.

result is exactly the same

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.

Let me know

Just do a simple file with functions and call the functions: simpler, easier to read & maintain.

Even if I don't share all your points, you're the boss.

As soon as I have time, I'll update the PR.

We can always debate about that in a community call, it'll be more easy to talk about that vocally ;)

As soon as I have time, I'll update the PR.

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 🙂

Pierre-Gilles · 2020-08-31T08:03:22Z

server/services/zwave/lib/comClass/switchMultilevelCommandClass.js

+const { COMMAND_CLASSES, INDEXES } = require('../constants');
+
+const SwitchMultilevelCommandClass = function SwitchMultilevelCommandClass() {
+  DefaultCommandClass.call(this);


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

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.

Pierre-Gilles · 2020-08-31T08:04:14Z

server/services/zwave/lib/constants.js

+  READ_WRITE: 3,
+};
+
+const GENRES = {


This variable is in french, please write in english :)

Pierre-Gilles · 2020-08-31T08:07:05Z

server/services/zwave/lib/comClass/switchMultilevelCommandClass.js

+ * @example
+ *  const isSupportingV4 = supportV4(zWaveComClassObject);
+ */
+function supportV4(comClass) {


This function name is not really explicit, which v4 is it? Gladys v4?

From an outside point of view, it's not clear enough :)

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...

Yes, it's not clear enough

Pierre-Gilles · 2020-08-31T08:08:25Z

front/src/actions/device.js

@@ -56,6 +54,9 @@ function createActions(store) {
                  [deviceFeatureIndex]: {
                    lastValue: {
                      $set: value
+                    },
+                    lastValue_changed: {


Please use camelCase :)

sescandell · 2021-06-27T21:52:57Z

See #1221

Pierre-Gilles · 2021-06-28T07:39:03Z

Why did you change to a new PR? All my previous comment are still valid, and are now lost!

sescandell · 2021-06-28T09:05:40Z

Why did you change to a new PR? All my previous comment are still valid, and are now lost!

Parce que suite à la migration des autres PR (notamment celle de la gestion du multi-instances) et le fait de ne pas vouloir d'héritage pour la gestion des CommandClass (ce qui, en ré-écrivant le code, ne me semble pas une bonne idée), le travail était plus important que de rebase / adapter celle-ci à priori.

Je peux tenter un foce-push sur cette branche pour autant... les noms de fichiers sont les mêmes, peut-être que Git va bien s'en sortir... je peux tester

Pierre-Gilles · 2021-06-28T09:19:01Z

Ok! Tu peux rester dans une nouvelle PR si c'est plus simple pour toi :)

sescandell · 2021-06-28T21:09:40Z

A priori, malgré un force push => impossible de ré-ouvrir ici... il se perd GIT je pense :)

sescandell force-pushed the pr-zWaveDimmer branch from 97a43ef to b0ffd83 Compare June 30, 2020 20:57

sescandell added 12 commits August 8, 2020 18:27

MultilevelDevice feature

84d0394

First step to manage zWave Dimmers

4cdd849

Just implofy a log

1ef4b04

Remove unused index for feature

ed41507

getMode tests

6391410

Intermediate useless commit

c08f0be

Fix ESLint

0399d26

zWave - Refactoring value changed handler. Introducing command class …

f5c4325

…objects (to spread in other functions)

Prettier

75c5491

Refactoring transformations/normalizations and using inheritance for …

a387c21

…CommandClass objects

Use command class objects for getNodes

5727e25

MinMax dynamic for dimmers

c0e5e15

sescandell force-pushed the pr-zWaveDimmer branch from b0ffd83 to c0e5e15 Compare August 8, 2020 16:28

Fix setValue/changedValue

08f2fe2

Prettier

9352108

sescandell changed the title ~~[WIP] zWave - Dimmer integration~~ zWave - Dimmer integration Aug 18, 2020

Merge branch 'master' into pr-zWaveDimmer

854de8b

Pierre-Gilles requested changes Aug 31, 2020

View reviewed changes

sescandell closed this Jun 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zWave - Dimmer integration #757

zWave - Dimmer integration #757

sescandell commented Apr 27, 2020 •

edited

Loading

VonOx commented Apr 28, 2020

sescandell commented Apr 28, 2020

link39 commented Apr 28, 2020 •

edited

Loading

sescandell commented Apr 28, 2020

VonOx commented Apr 28, 2020

codecov bot commented May 3, 2020 •

edited

Loading

sescandell commented May 6, 2020 •

edited

Loading

Pierre-Gilles commented Aug 11, 2020

sescandell commented Aug 11, 2020

sescandell commented Aug 18, 2020 •

edited

Loading

Pierre-Gilles left a comment

Pierre-Gilles Aug 31, 2020

sescandell Aug 31, 2020 •

edited

Loading

Pierre-Gilles Aug 31, 2020

sescandell Aug 31, 2020 •

edited

Loading

Pierre-Gilles Sep 3, 2020

sescandell Sep 3, 2020

Pierre-Gilles Sep 3, 2020 •

edited

Loading

Pierre-Gilles Aug 31, 2020

sescandell Aug 31, 2020

Pierre-Gilles Aug 31, 2020

Pierre-Gilles Aug 31, 2020

sescandell Aug 31, 2020

Pierre-Gilles Aug 31, 2020

Pierre-Gilles Aug 31, 2020

sescandell commented Jun 27, 2021

Pierre-Gilles commented Jun 28, 2021

sescandell commented Jun 28, 2021

Pierre-Gilles commented Jun 28, 2021

sescandell commented Jun 28, 2021


		const DefaultCommandClass = function DefaultCommandClass() {};

		DefaultCommandClass.prototype = {

zWave - Dimmer integration #757

zWave - Dimmer integration #757

Conversation

sescandell commented Apr 27, 2020 • edited Loading

Pull Request check-list

Description of change

VonOx commented Apr 28, 2020

sescandell commented Apr 28, 2020

link39 commented Apr 28, 2020 • edited Loading

sescandell commented Apr 28, 2020

VonOx commented Apr 28, 2020

codecov bot commented May 3, 2020 • edited Loading

Codecov Report

sescandell commented May 6, 2020 • edited Loading

Pierre-Gilles commented Aug 11, 2020

sescandell commented Aug 11, 2020

sescandell commented Aug 18, 2020 • edited Loading

Pierre-Gilles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sescandell Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sescandell Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Gilles Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sescandell commented Jun 27, 2021

Pierre-Gilles commented Jun 28, 2021

sescandell commented Jun 28, 2021

Pierre-Gilles commented Jun 28, 2021

sescandell commented Jun 28, 2021

sescandell commented Apr 27, 2020 •

edited

Loading

link39 commented Apr 28, 2020 •

edited

Loading

codecov bot commented May 3, 2020 •

edited

Loading

sescandell commented May 6, 2020 •

edited

Loading

sescandell commented Aug 18, 2020 •

edited

Loading

sescandell Aug 31, 2020 •

edited

Loading

sescandell Aug 31, 2020 •

edited

Loading

Pierre-Gilles Sep 3, 2020 •

edited

Loading