TheThingsNetwork / lorawan-devices

Device Repository for LoRaWAN devices
Apache License 2.0
192 stars 372 forks source link

Add device version to `normalizeUplink()` input #527

Closed johanstokking closed 2 years ago

johanstokking commented 2 years ago

Summary

References #395 Preparations for https://github.com/TheThingsNetwork/lorawan-stack/issues/5759

cc @pablojimpas

Changes

Notes for Reviewers

Review everything but last commit; that's just making sure everything is executed sequentially. It's slower, but it means less open file descriptors (because of concurrently running validation processes). Occassionally too many open file descriptors resulted in validation failure.

johanstokking commented 2 years ago

@adriansmares https://github.com/TheThingsNetwork/lorawan-devices/pull/524#discussion_r960672057

Regarding pushing the version identifiers to the codecs - do you think it will still be helpful when using the vendorID/vendorProfileID/TBA schematic ? because then it really becomes blurry what hardware is actually running the formatter / normalizer.

The codec and the profile are identified independently, also in the Alliance. The codecs should not have anything to do with the profile ID.

As for vendor ID, indeed that is an Alliance identifier that would be an alternative to the Device Repository's vendor ID (string). Actually, the vendorID here is probably just informative; as it stands now, the normalizer always belongs to the vendor (unlike profiles, which can be referenced from other vendors).

Note that the changes here are only applied to normalizeUplink(); we don't touch the decoder and encoders. Currently, the input and output structure of the decoder and encoders align with the LoRa Alliance TS013 specification. So we should avoid adding things there as that would diverge.

I think that in the payload normalization step, the actual hardware and region can be relevant. The firmware version and the payload encoding may be the same, but the hardware may use a different sensor or probe or have other physical attributes that affects how the payload is normalized. I don't think that profile ID is relevant.

adriansmares commented 2 years ago

I'm thinking of the case in which the end device is onboarded via a QR code which contains only vendor ID and vendor profile ID. Unless I'm missing something, the payload formatter used by the Application Server won't have access to the end device version identifiers referenced here, because the AS cannot map back the (vendor ID, vendor profile ID) pair to the (hardware version, firmware version, band ID) pair that we're using in the device repository.

How would payload normalization work here ? Do we just say that payload normalization is supported only when using the device repository format of the identifiers ?

johanstokking commented 2 years ago

But in that case, how is the codec referenced in the first place?

As the identifiers are independent, there is already no way to go from profile ID to a codec, because we indeed miss hardware/firmware version.

Unless the profile ID gets repurposed to a (unique) combination of profile and codec.

adriansmares commented 2 years ago

But in that case, how is the codec referenced in the first place? Unless the profile ID gets repurposed to a (unique) combination of profile and codec.

I am getting ahead of myself, but indeed this is the situation that I'm having in mind.

johanstokking commented 2 years ago

If we repurpose that profile ID to also refer to the codec, then indeed we don't yet have the hardware and firmware version yet.

johanstokking commented 2 years ago

Actually, TR005 already suggests that the VendorProfileID may refer to payload encoding as well:

Screen Shot 2022-09-03 at 17 12 31

Currently, the profile ID in the Device Repository identifies a device profile which can be used by many devices with different payload codecs. If we want to be compatible with TR005, we would have to key this profile ID to a device profile and codec ID. That would look like this:

diff --git a/vendor/example/index.yaml b/vendor/example/index.yaml
index 9dc1af1ac..21706b6ca 100644
--- a/vendor/example/index.yaml
+++ b/vendor/example/index.yaml
@@ -3,3 +3,13 @@
 endDevices:
   # Unique identifier of the end device (lowercase, alphanumeric with dashes, max 36 characters)
   - windsensor # look in windsensor.yaml for the end device definition
+
+vendorProfileIDs:
+  42:
+    region: EU863-870
+    profile: windsensor-profile
+    codec: windsensor-codec
+  43:
+    region: US902-928
+    profile: windsensor-profile
+    codec: windsensor-codec

That is still too limiting for the Device Repository: multiple devices, and multiple firmware versions of a single device (that can run on multiple hardware versions), can use the same combination of profile and codec. So we still cannot go from vendorProfileID to our primary keys here: vendor ID, end device ID, firmware version, hardware version and region.

I think we should add this vendorProfileIDs field to the vendor index and drop vendorProfileID from the device profile. That way, we are at least compatible with TR005.

diff --git a/schema.json b/schema.json
index 778a73e87..494dbe796 100644
--- a/schema.json
+++ b/schema.json
@@ -620,12 +620,6 @@
         }
       ],
       "properties": {
-        "vendorProfileID": {
-          "type": "integer",
-          "description": "VendorProfileID managed by the vendor, as defined in TR005",
-          "minimum": 0,
-          "maximum": 65535
-        },
         "supportsClassB": {
           "type": "boolean",
           "description": "End device supports class B"

What we still miss to identifying devices: we miss the device ID and the hardware and firmware version.

However, this pull request now becomes problematic. If we can load the codec (including the normalizer), but we don't have the device ID and the hardware and firmware version while the normalizer requires this information, the normalizer may return errors. That is not a good developer UX.

So what do we do?

  1. Do not accept the changes here. Do not make normalizeUplink() take identifiers may not be known if the device has been onboarded with a QR code (and the owner didn't select the right device). This implies that normalizers that do need device specific information (unlike the decoder) cannot obtain this information. If the device maker has this foresight, they must declare multiple firmware versions for each hardware version and assign a new vendorProfileID to each unique device ID and unique hardware and firmware version (but is not advised in TR005)

  2. Do accept the changes here. Let The Things Stack only run normalizeUplink() when the device ID, hardware and firmware versions are known. However, most normalizers probably don't require these versions to be present, so this still is not really nice, but I think it's acceptable


Side note: besides TR005 (vendor + profile ID), there's also TS006 (firmware management) that allows an AS to discover the hardware and firmware version:

Screen Shot 2022-09-03 at 17 44 32

This is especially useful with FOTA.

However, even though we could map these numbers back to the hardware and firmware versions in the Device Repository (it keeps these numeric versions too), there's very little support for TS006 and we would still not have the device ID.

(Maybe we should also add TS006 support in the Device Repository, so our AS knows that it can discover these versions? This is nice especially with FUOTA, and works with TR005 for onboading and TS006 for version discovery and then we have all identifiers)


Summing up:

Key DR Primary Key TR005 TS006
Vendor ID X X
Device ID X
Hardware version X X
Firmware version X X
Region X X
Profile X
Codec X

In short: you cannot go from information in TR005 or TS006 to the DR Primary Key.


@adriansmares looking forward to hearing your thoughts. Let's make sure we have a proper plan before we merge this at least.

johanstokking commented 2 years ago

Updated the above with progressive insight

adriansmares commented 2 years ago

I think we should not add the device version as an input. As things stand right now, it will only widen the indexing gap between our repository and the specification. TR005 encourages profile and codec reuse, and with reuse this functionality is useless. I somehow expect that if by magic normalizers become a specification, they will reuse the vendor profile ID too.

The customer UX that we will have with these QR codes in general is not ideal, but that battle has been lost on TR005 publication.

In its current form, I believe that end devices that would like to have hardware / software model specific information available in the formatter or normalizer should just have an extra device profile ID / dedicated formatter. If we have to reuse the same vendor profile ID between two sensors that have different physical properties, it's not like we can do it in a stateless manner.


I like the refactoring that maps the vendor profile ID to a profile + codec - I think we should do that (but it will need some lorawan-stack work too).

johanstokking commented 2 years ago

OK, let's forget about this for the time being then!