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

fix: fix multiple parameters for hass Discovery #36

Merged
merged 17 commits into from
Nov 30, 2020

Conversation

varet80
Copy link
Collaborator

@varet80 varet80 commented Nov 28, 2020

This pull request applies two changes:

  • Binary Sensor discovery was not working, fixing it by changing the SensorTypeName source
  • Add ability to use PropertyName in case Label is not sent by driver
  • Adds unit_of_measurement on Battery level devices by default.
  • Replace valueId.units with valueId.unit
  • add README.md on prettierignore list. As some linting standards cannot be followed.

** Binary Sensor discovery**
discovery was not working, as the sensorTypeName was a number and not the name of sensor
changed the value used for this from Translated to just valueId.property

Missing Label
There are cases where the Label is missing from a Property. We can use the PropertyName in that cases.

I have create a small change in code to make this happen. Because my code was based on hassDiscovery branch, I am also making a pull request with that branch as target.

I have tested a container and I do see only Undefined labels changing to Property Name.
Any property having labels is not impacted.

Batery Level
Battery level unit was not discovered. Added a percentage under the payload of this parameter

valueId.unit
valueId.units is non existing, fixing the typo with valueId.unit

@varet80 varet80 changed the title Fix Undefined property when no label exposed by Driver fix: Undefined property when no label exposed by Driver Nov 28, 2020
@varet80 varet80 requested a review from robertsLando November 28, 2020 14:27
@varet80 varet80 changed the title fix: Undefined property when no label exposed by Driver fix: use propertyName if driver doesn't send label, also add battery unit of measurement Nov 28, 2020
@varet80 varet80 changed the title fix: use propertyName if driver doesn't send label, also add battery unit of measurement fix: fix multiple parameters for hass Discovery Nov 28, 2020
@varet80 varet80 force-pushed the fix#uiUndefinedProperty branch from 4baf5ef to 4bb2601 Compare November 28, 2020 21:04
lib/Gateway.js Outdated Show resolved Hide resolved
lib/Gateway.js Show resolved Hide resolved
lib/Gateway.js Show resolved Hide resolved
Co-authored-by: AlCalzone <[email protected]>
@AlCalzone
Copy link
Member

Ok I guess @robertsLando has to chime in here. I don't know the specifics how things are handled on this side.

lib/Gateway.js Show resolved Hide resolved
lib/Gateway.js Show resolved Hide resolved
lib/Gateway.js Show resolved Hide resolved
lib/Gateway.js Outdated Show resolved Hide resolved
@@ -650,7 +650,7 @@ function updateValueMetadata (zwaveNode, zwaveValue, zwaveValueMeta) {
readable: zwaveValueMeta.readable,
writeable: zwaveValueMeta.writeable,
description: zwaveValueMeta.description,
label: zwaveValueMeta.label,
label: zwaveValueMeta.label || zwaveValue.propertyName, // when label is missing, re use propertyName. Usefull for webinterface
Copy link
Member

Choose a reason for hiding this comment

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

@AlCalzone could the label be undefined or is this caused by an error on zwavejs? If it's the second case I would remove this so we can detect undefined labels and fix them on zwavejs

Copy link
Member

Choose a reason for hiding this comment

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

The Label can be undefined if we don't set it in zwave-js. Your code should be prepared for that possibility.
But as I wrote in slack, we should find those locations and eliminate them.

Copy link
Member

@robertsLando robertsLando Nov 30, 2020

Choose a reason for hiding this comment

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

Ok so in poor words should I keep this change or remove this so we can detect undefined labels?

I think it's a good fix but we would never see the undefined labels if I keep this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could have a suffix/prefix Property and help us still know what is, but not let it undifined
something like

label: zwaveValueMeta.label || ("%s - property", zwaveValue.propertyName),

Copy link
Member

Choose a reason for hiding this comment

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

This could be another valid solution

Copy link
Collaborator Author

@varet80 varet80 Nov 30, 2020

Choose a reason for hiding this comment

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

i think more beautifull would be:

label: zwaveValueMeta.label || ("%s (property)", zwaveValue.propertyName),

pushing this as change. If you prefer different approach. please comment here

lib/Gateway.js Outdated Show resolved Hide resolved
@robertsLando
Copy link
Member

@billiaz There are some lint issues

@varet80
Copy link
Collaborator Author

varet80 commented Nov 30, 2020

my mac has some issues with lint-fix/linting

this issue can probably be solved

        if (valueId.property) {
          let sensorTypeName = valueId.property
        }
        // unsued discovery line. for historical reasons
        // let sensorTypeId = BinarySensorType[valueId.property]
        if (sensorTypeName) {
          sensorTypeName = this.mqtt.cleanName(
            sensorTypeName.toLocaleLowerCase()
          )
        }
        // TODO: Implement all BinarySensorTypes
        cfg = hassCfg['binary_sensor_' + sensorTypeName]

by making it:

        if (valueId.property) {
          let sensorTypeName = this.mqtt.cleanName(
            valueId.property.toLocaleLowerCase()
          )
          // TODO: Implement all BinarySensorTypes
          cfg = hassCfg['binary_sensor_' + sensorTypeName]
        }

lib/Gateway.js Outdated Show resolved Hide resolved
@varet80 varet80 requested a review from robertsLando November 30, 2020 09:17
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@billiaz Seems thre are still some lint issues?

@varet80
Copy link
Collaborator Author

varet80 commented Nov 30, 2020

@billiaz Seems thre are still some lint issues?
@robertsLando
this is an issue also seen in hasDiscovery. Prettierlint fails with readme. as we need to have some double quotes there.
I suggest to add a .prettierignore for README.md (I am adding it to test if this is the same failure here too! but this is what investigation showed on my laptop)

@varet80 varet80 requested a review from robertsLando November 30, 2020 10:13
lib/ZwaveClient.js Outdated Show resolved Hide resolved
@robertsLando robertsLando merged commit 23f8cb3 into feat#hassDiscovery Nov 30, 2020
@robertsLando robertsLando deleted the fix#uiUndefinedProperty branch November 30, 2020 12:48
robertsLando added a commit that referenced this pull request Nov 30, 2020
* feat: bind target values to current

* feat(wip): hass discovery

* fix: better discovery support

* fix: hass socket apis

* fix: alarmType and meterType mapping

* fix: use commandClasses instead of hardCoded numbers

* refactor: better docs on methods

* fix: converted some values in hass devices.js

* fix: converted some more values in devices.js

* feat: skip discovety flag

* fix: lint issues

* fix: ignore discovery

* fix: converted some more valueIds of devices.json

* fix: lint issues

* fix: push target value in values

* fix: mode_map and fan _mode_map

* fix: allow empty json device

* fix: added ccSpecific to value meta

* fix: setpoints valueIds

* fix: commandclasses import

* fix: set defaiult endpoint to 0

* feat: added ccSpecific

* fix: undefined in num2hex

* fix: use 5.4.1-alpha.0

* fix: probably fix to map template bug

* fix: mode_map template creation

* fix: added brackets to keys

* fix: added comments

* fix: getMappedValuesTemplate check type of value

* fix: lint errors

* docs: fixed hass docs

* docs: migration and why zwavejs

* docs: bitmask and color values

* fix: lint issues

* docs: fix for review

* fix: typo

* fix: lint on readme

* docs: fix

* docs: added some points to why zwavejs section

* fix: payload parse of modes when discovery is enabled

* fix: missing ccSpecific values (#35)

* fix: binary sensors, units and undefined labels (#36)

* fix: lint issues

Co-authored-by: V Aretakis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants