Skip to content
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

Extend dump to include full deviceclass object #136

Merged
merged 4 commits into from
Feb 23, 2021
Merged

Extend dump to include full deviceclass object #136

merged 4 commits into from
Feb 23, 2021

Conversation

marcelveldt
Copy link
Collaborator

@marcelveldt marcelveldt commented Feb 23, 2021

BREAKING CHANGE

DeviceClass strings tend to change sometimes which is annoying as our discovery scheme in HA depends a lot on these deviceclass id's.

This change replaces the deviceClass to a full object containing both the key and label.

Discussed with @MartinHjelmare that we should do this breaking change before HA beta release (and adjust both integration and the lib for this change).

Closes #76
Closes #75

The dump will look like this after this change:

"deviceClass": {
            "basic": {
              "key": 2,
              "label": "Static Controller"
            },
            "generic": {
              "key": 2,
              "label": "Static Controller"
            },
            "specific": {
              "key": 1,
              "label": "PC Controller"
            },
            "mandatorySupportedCCs": [
              
            ],
            "mandatoryControlledCCs": [
              32
            ]
          },

@MartinHjelmare
Copy link
Collaborator

What's the reason for not including the full state as described here?
https://zwave-js.github.io/node-zwave-js/#/api/node?id=deviceclass

@marcelveldt
Copy link
Collaborator Author

Well because we will have to replicate that entire object and no use ?

@marcelveldt
Copy link
Collaborator Author

Like just discussed on discord with @AlCalzone the json serializations of most objects only return the string/label so we need to dump each individual object

@MartinHjelmare
Copy link
Collaborator

Ok.

@marcelveldt
Copy link
Collaborator Author

marcelveldt commented Feb 23, 2021

also included the list of supported commandClasses in the dump, this fixes #75

"commandClasses": [
            {
              "id": 48,
              "name": "Binary Sensor",
              "version": 2,
              "isSecure": false
            },
            {
              "id": 89,
              "name": "Association Group Information",
              "version": 1,
              "isSecure": false
            },
            {
              "id": 90,
              "name": "Device Reset Locally",
              "version": 1,
              "isSecure": false
            },
            {
              "id": 94,
              "name": "Z-Wave Plus Info",
              "version": 2,
              "isSecure": false
            },
            {
              "id": 112,
              "name": "Configuration",
              "version": 1,
              "isSecure": false
            },
            {
              "id": 113,
              "name": "Notification",
              "version": 8,
              "isSecure": false
            },
            {
              "id": 114,
              "name": "Manufacturer Specific",
              "version": 2,
              "isSecure": false
            },
            {
              "id": 128,
              "name": "Battery",
              "version": 1,
              "isSecure": false
            },
            {
              "id": 132,
              "name": "Wake Up",
              "version": 2,
              "isSecure": false
            },
            {
              "id": 133,
              "name": "Association",
              "version": 2,
              "isSecure": false
            },
            {
              "id": 134,
              "name": "Version",
              "version": 2,
              "isSecure": false
            }
          ],

@marcelveldt
Copy link
Collaborator Author

If we miss any values we can add it to the (custom) state object too.

): CommandClassState => ({
id: commandClass.ccId,
name: CommandClasses[commandClass.ccId],
version: commandClass.version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess whether this CC is secure or not is of no interest to HA, but it could be useful for debugging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess whether this CC is secure or not is of no interest to HA, but it could be useful for debugging.

I can add it. How to access it from the CommandClass instance ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, got it

Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty interfaces look a bit weird (I'd use plain types for that). Otherwise LGTM

@marcelveldt
Copy link
Collaborator Author

The empty interfaces look a bit weird (I'd use plain types for that). Otherwise LGTM

Yeah, we tend to override/extend from the original interfaces so it will break (intentionally) when upstream changes so we keep up with the changes instead of things silently breaking. But if you have any good ideas to deal with it, let me know!

@AlCalzone
Copy link
Member

I mean instead of

interface Foo extends SomeGeneric<Bar> {}

I'd do

type Foo = SomeGeneric<Bar>

@marcelveldt
Copy link
Collaborator Author

@AlCalzone adjusted it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend deviceclass dump Add supported CommandClasses to dumpNode
4 participants