VIDs should be hexadecimal values

[wc-smith]

CSA has traditionally maintained Vendor IDs (Manufacturer Codes) as hexadecimal values, see https://groups.csa-iot.org/wg/members-all/document/10905.

DCL reports, and presumably accepts, decimal values for VIDs. This discrepancy raises the prospect that incorrect values for VID might be entered, either by error in converting from hex to decimal or by misinterpreting a hex value as decimal (when all digits are less than 'A').

DCL should accept and report hex values to conform with Alliance historical precedent.

[jmeg-sfy]

+1 it should be even both way
and VID:PID display like USB style

[hawk248]

Need more investigation.
Current VID are stored as INT ... changing to hex means modifying state data.

[jmeg-sfy]

maybe not the storage just the way it is displayed

[ashcherbakov]

I believe we need to apply the same change for PIDs, not for VIDs only.

[abdulla-ashurov]

We can send REST API and CLI requests in hex format:
REST API:

• dcl/model/models/0x0B816/0x1

CLI:

• dcld query model get-model --vid=0x0B816 --pid=0x1

[abdulla-ashurov]

We cannot output hexadecimal numbers in JSON format.
https://stackoverflow.com/questions/52671719/can-hex-format-be-used-with-json-files-if-so-how

Summary: we can only change in structure integer to string.

[uabjabborov]

There are two options for this issue:

1. Keep VID/PID type as an integer

• pros:
  • no breaking changes
  • REST - supports hex as a path variable for queries (ex: dcl/model/models/0x0B816/0x1)
  • CLI - supports hex as an argument (ex: dcld query model get-model --vid=0x0B816 --pid=0x1)
  • gRPC - generated int fields support hex value assignment
• cons:
  • REST - json does not support hex values for number types, so query results will be in decimal format
  • CLI - non trivial modifications are required from server side to make query responses in hex
  • inconsistency between query arguments (in hex) and response parameters (in decimal) for vid and pid

2. Use string type for VID/PID

• pros:
  • full consistency between query arguments and response parameters (both in string)
  • no conversions are needed on client-side
• cons:
  • breaking change, needs migration for old values
  • requires API change for gRPC/Rest
  • does not conform to DCL specification

[ashcherbakov]

Our current recommendation is to go with Option 1 (non-breaking change), but please note that it will have the following consequences (in particular, please see the text in bold):

• Write Requests
  • CLI: possible to use VID/PID in hex in CLI tx commands (it's possible even now, no changes needed, just more tests)
  • gRPC/REST: possible to use VID/PID in hex in CLI tx commands (it's possible even now, no changes needed, just more tests)
• Read Requests (Queries)
  • CLI: query commands will return VID/PID in hex as a string (it will require a change in CLI)
  • REST:
    • possible to use hex VID/PID in query URLs (for example dcl/model/models/0x0B816/0x1), so no changes needed, just more tests
    • JSON reply will contain VID/PID in decimal format, not hex. Conversion to hex can be done on the application level.
  • gRPC:
    • possible to use hex VID/PID in query requests, so no changes needed, just more tests
    • gRPC reply will contain VID/PID in decimal format. Conversion to hex can be done on the application level.

So, the following changes need to be done for that Option:

• Write unit and integration tests for hex VID/PID values in transactions and queries. We need to have integration tests for CLI, REST and gRPC.
• CLI change: post-process all query replies where VID/PID is present, and convert the VID/PID values from decimal to hex in the output JSON.

[wc-smith]

We could always ask Zigbee to update their VID table to include a decimal value column.

…

On Mar 11, 2022, at 12:23 AM, Jonathan Mégevand ***@***.***> wrote: +1 it should be even both way and VID:PID display like USB style — Reply to this email directly, view it on GitHub <#272 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEZS5ZSDU37QXCNC66ZPA3U7L7JDANCNFSM5P4ATCGQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.

[robszewczyk]

There are two options for this issue:

1. Keep VID/PID type as an integer

• pros:

  • no breaking changes
  • REST - supports hex as a path variable for queries (ex: dcl/model/models/0x0B816/0x1)
  • CLI - supports hex as an argument (ex: dcld query model get-model --vid=0x0B816 --pid=0x1)
  • gRPC - generated int fields support hex value assignment

• cons:

  • REST - json does not support hex values for number types, so query results will be in decimal format
  • CLI - non trivial modifications are required from server side to make query responses in hex
  • inconsistency between query arguments (in hex) and response parameters (in decimal) for vid and pid

At least on the input side of the CLI I would advocate for falling back onto unambiguous format for numbers and accepting commonly understood (at the very least) 1234 (no leading prefix implies decimal format) and 0x1234 (hexadecimal format).

From brief browsing around, it looked like there are provisions for expressing hexadecimal numbers in JSON5, but that is not a likely solution for us.

[hawk248]

Now is the time to not shy from breaking changes for long term stability .. in Option 1, we will have one case where REST Api calls can lead to confusion ... for sake of consistency (same response regardless of which interface is used), I recommend option 2.

[tcarmelveilleux]

I would recommend option 1, and the confusion people may have, they will have to just take. It's not that confusing that a format that looks like a decimal number is a decimal number.

[hawk248]

TT decided on option 1. It will allow insertion using hex 0x designator for VID to suppress confusion of INT -> hex conversion during DCL key input (proposal).
Option 1 keeps INT as the internal representation and is a non breaking change.

[abdulla-ashurov]

We did not change the VID/PID output format for the CLI, as this can lead to non trivial consequences.
Corresponding PRs:

• Abdulla ashurov/issue272 #318
• Abdulla ashurov/issue272 #322