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

Validate features #736

Merged
merged 45 commits into from
Apr 11, 2024
Merged

Conversation

LDannijs
Copy link
Contributor

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:

    • Check if there are vendors in the vendor folder which are not in vendor/index.yaml.
    • Ability to pass an argument to the validation script to only validate a single vendor
      • Updated README with explanation
  • Krishna's issue:

    • Check if the 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)
    • Also updated every vendorProfileID to match this new feature to the validation
  • Own changes:

    • Updated the PR template slightly
    • Improved the 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 the vendorID 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.

@LDannijs
Copy link
Contributor Author

I've added another small feature: add a pre-commit message that runs the validation on it's own and displays the error if the validation failed:

Screenshot 2024-01-29 at 10 00 57

I sometimes see users failing to run the validation themselves so this might help to negate that instead of Jaime having to do it himself. I'm not entirely sure if this is something we'd need but I'll be happy to hear feedback.

Makefile Outdated Show resolved Hide resolved
bin/pre-commit Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
bin/validate.js Outdated Show resolved Hide resolved
bin/validate.js Outdated Show resolved Hide resolved
bin/validate.js Outdated Show resolved Hide resolved
bin/validate.js Outdated Show resolved Hide resolved
vendor/abeeway/abeeway-compact-tracker-pro-eu868.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@johanstokking johanstokking left a 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), called profileIDs
  • 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

@LDannijs LDannijs mentioned this pull request Mar 6, 2024
@LDannijs
Copy link
Contributor Author

Alright @johanstokking think I finally understood it now (apologies)
I now removed the profileIDs from vendors where vendorProfileID was a repeating id, a vendorid, a made up number, etc.

I think I messed up cause, yes, I initially created a check for if the vendorProfileID was unique, but I rolled back that change.

vendor/decentlab/profile-as923.yaml Show resolved Hide resolved
vendor/decentlab/index.yaml Outdated Show resolved Hide resolved
Copy link
Member

@johanstokking johanstokking left a 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

bin/validate.js Outdated Show resolved Hide resolved
@Jaime-Trinidad Jaime-Trinidad merged commit 3a8dc15 into TheThingsNetwork:master Apr 11, 2024
2 checks passed
@LDannijs LDannijs deleted the validate-features branch June 11, 2024 10:56
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.

3 participants