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: show hex number along with the unknown strings #801

Merged
merged 6 commits into from
Mar 5, 2021

Conversation

murrayma
Copy link
Contributor

@murrayma murrayma commented Mar 3, 2021

The accepted convention for representing manufacturer and product identifiers appears to use hexadecimal.

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@marcus-j-davies
Copy link
Member

marcus-j-davies commented Mar 3, 2021

My 2C.

I would also add some padding here, to remain consistent padStart(4, '0')

const deviceConfig = zwaveNode.deviceConfig || {
   label: 'Unknown product 0x' + parseInt(zwaveNode.productId).toString(16).padStart(4, '0'),
   description: 'Unknown type 0x' + parseInt(zwaveNode.productType).toString(16).padStart(4, '0'),
   manufacturer: 'Unknown manufacturer 0x' + parseInt(zwaveNode.manufacturerId).toString(16).padStart(4, '0')
 }

@robertsLando,
not sure if the padded format is used in the UI?

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

1 similar comment
@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@murrayma
Copy link
Contributor Author

murrayma commented Mar 3, 2021

My 2C.

I would also add some padding here, to remain consistent padStart(4, '0')

const deviceConfig = zwaveNode.deviceConfig || {
   label: 'Unknown product 0x' + parseInt(zwaveNode.productId).toString(16).padStart(4, '0'),
   description: 'Unknown type 0x' + parseInt(zwaveNode.productType).toString(16).padStart(4, '0'),
   manufacturer: 'Unknown manufacturer 0x' + parseInt(zwaveNode.manufacturerId).toString(16).padStart(4, '0')
 }

@robertsLando,
not sure if the padded format is used in the UI?

Thank you for the suggestion. I found an existing method, utils.num2hex() meant for this purpose and updated.

Comment on lines 869 to 873
const deviceConfig = zwaveNode.deviceConfig || {
label: 'Unknown product ' + zwaveNode.productId,
description: zwaveNode.productType,
manufacturer: 'Unknown manufacturer ' + zwaveNode.manufacturerId
label: 'Unknown product ' + utils.num2hex(zwaveNode.productId),
description: utils.num2hex(zwaveNode.productType),
manufacturer: 'Unknown manufacturer ' + utils.num2hex(zwaveNode.manufacturerId)
}
Copy link
Member

Choose a reason for hiding this comment

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

I already evaluate this values later here. I suggest you to move that at the start of initNode function and use the values from the array. Also remember to use zwaveNnode instead of node

@robertsLando robertsLando changed the title Use hex for unknown manufacturer, product IDs in UI. fix: show hex number along with the unknown strings Mar 4, 2021
@zwave-js zwave-js deleted a comment from zwave-js-bot Mar 4, 2021
@zwave-js zwave-js deleted a comment from zwave-js-bot Mar 4, 2021
Per request from @robertsLando, evaluation of hexIds is moved above
the initialization of deviceConfig. Backtick templates used to generate
labels in place of string concatenation to remain consistent with
surrounding code.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 622900091

  • 0 of 11 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 18.366%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/ZwaveClient.js 0 11 0.0%
Totals Coverage Status
Change from base Build 621213741: 0.0%
Covered Lines: 2022
Relevant Lines: 11290

💛 - Coveralls

@murrayma murrayma requested a review from robertsLando March 5, 2021 01:03
@robertsLando robertsLando merged commit 056bc80 into zwave-js:master Mar 5, 2021
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