-
Notifications
You must be signed in to change notification settings - Fork 378
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
Validate features #736
Validate features #736
Conversation
This reverts commit 1a4ac1a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now implements one of the options mentioned in #405 (comment).
The other option is that a vendorProfileID
refers to a unique combination of device ID + firmware version + hardware version + region. When we have that, we have the profile and the codec, but we also know exactly which device it is.
So I'm suggesting the following change:
- Remove
vendorProfileID
from the profile definition - Add a new section in the vendor's index file (not the vendor index but the index of the vendor), top-level (same as
endDevices
), calledprofileIDs
- This new section is a key/value mapping of vendor profile ID (numeric) to either an object with device ID + hardware version + firmware version + region, or a profile ID + codec ID
Example diff:
diff --git a/vendor/the-things-industries/index.yaml b/vendor/the-things-industries/index.yaml
index e1a0d5769..7f0366100 100644
--- a/vendor/the-things-industries/index.yaml
+++ b/vendor/the-things-industries/index.yaml
@@ -1,2 +1,8 @@
endDevices:
- generic-node-sensor-edition
+profileIDs:
+ '42':
+ endDeviceID: 'generic-node-sensor-edition'
+ firmwareVersion: '1.0'
+ hardwareVersion: '1.1'
+ region: 'EU863-870'
This requires some JSON Schema adaptations. Something like:
diff --git a/schema.json b/schema.json
index c5d3afcba..9e1f4f5a3 100644
--- a/schema.json
+++ b/schema.json
@@ -79,6 +79,48 @@
"items": {
"$ref": "#/definitions/identifier"
}
+ },
+ "profileIDs": {
+ "type": "object",
+ "patternProperties": {
+ "^[0-9]+$": {
+ "oneOf": [
+ {
+ "type": "object",
+ "properties": {
+ "endDeviceID": {
+ "$ref": "#/definitions/identifier"
+ },
+ "hardwareVersion": {
+ "type": "string",
+ "minLength": 1
+ },
+ "firmwareVersion": {
+ "type": "string",
+ "minLength": 1
+ },
+ "region": {
+ "$ref": "#/definitions/region"
+ }
+ },
+ "required": ["endDeviceID", "hardwareVersion", "firmwareVersion", "region"]
+ },
+ {
+ "type": "object",
+ "properties": {
+ "id": {
+ "$ref": "#/definitions/identifier"
+ },
+ "codec": {
+ "$ref": "#/definitions/identifier"
+ }
+ },
+ "required": ["id", "codec"]
+ }
+ ]
+ },
+ "additionalProperties": false
+ }
}
},
"additionalProperties": false
Alright @johanstokking think I finally understood it now (apologies) I think I messed up cause, yes, I initially created a check for if the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, one minor comment
Summary
I stumbled upon the issues from @johanstokking and @KrishnaIyer (#108 and #405) and decided to give it a crack. I am very curious to hear back if these implementations are correct, they would help out a lot.
Changes
Johan's issue:
vendor/index.yaml
.Krishna's issue:
vendorProfileID
is unique per vendor(Now this checks for 2 things: is the value unique and is it missing? if it's not unique it will stop the code, and if its missing it will just log it and continue)
vendorProfileID
to match this new feature to the validationOwn changes:
vendorProfileID
explanation a bit further to make it even clearer.Notes for Reviewers
@johanstokking I had a discussion with @Jaime-Trinidad about how much we should check the
vendorProfileID
and decided to just check for uniqueness and presence. Checking it against thevendorID
was a bit outside of the scope since not every vendor adds it.You asked about EIRP checking as well but i heard there can be some special exceptions so i left that out for now too.