Support migration from ChirpStack V4 #75

happyRip · 2023-05-15T08:10:21Z

Summary

Closes #58

Changes

Use ChirpStack V4 API
Unify flag names between sources

Testing

Tested with local ChirpStack v4 instance.

Regressions

Flag name changes affect all sources. TTS and ChirpStack sources were tested.

Notes for Reviewers

ChirpStack v3 and v4 protobuf APIs conflict with each other and cannot be compiled into the same binary.

See https://github.com/TheThingsNetwork/lorawan-stack-migrate/pull/75/files#r1232266208 since I'm not sure if this is handled correctly

Checklist

Scope: The referenced issue is addressed, there are no unrelated changes.
Documentation: Relevant documentation is added or updated.
Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.

happyRip · 2023-06-16T13:37:50Z

pkg/source/chirpstack/source.go

+			dev.Formatters.UpFormatterParameter = fmt.Sprintf(encoderFormat, s)
 			dev.Formatters.DownFormatter = ttnpb.PayloadFormatter_FORMATTER_JAVASCRIPT
-			dev.Formatters.DownFormatterParameter = fmt.Sprintf(decoderFormat, devProfile.PayloadDecoderScript)
+			dev.Formatters.DownFormatterParameter = fmt.Sprintf(encoderFormat, s)


In ChirpStack V4 PayloadCodecScript combines V3 PayloadDecoderScript and PayloadEncoderScript together, with 2 wrapper compatibility functions above them:

// v3 to v4 compatibility wrapper function decodeUplink(input) { return { data: Decode(input.fPort, input.bytes, input.variables) }; } function encodeDownlink(input) { return { bytes: Encode(input.fPort, input.data, input.variables) }; } // PayloadDecoderScript // PayloadEncoderScript

source

adriansmares

The changes to the environment variables and flag names should also be updated in the readme.

For the context on why we cannot have two sources: both the v3 and the v4 API use the same protobuf package statements and message names, but with different contents. This is not supported by the protobuf library, as the message definition registry is global, and ChirpStack messages have same path for two different messages

My proposal here was that since the v3 ChirpStack won't receive any further development, that we can just refer people to older versions of the migration tool. Another solution would be to have a build tag dedicated just to the v3 API, but I don't know if that is worth the effort and the complexity.

adriansmares · 2023-06-16T15:05:17Z

pkg/source/chirpstack/source.go

-			dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
-				Value: ttnpb.DataRateOffset(devProfile.RxDrOffset_1),
-			}
+		dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{


I would set this only if it is greater than zero as it was originally done. Also are we sure it is desired ?

DesiredRx1DataRateOffset is unsigned now, so I removed the >= condition. Should those be kept for the sake of verbosity?

is it a desired parameter

It's hard for me to judge that, I didn't want to remove the features we had here previously and compared the values from CS v3 to v4 migration script.

I would set this only if it is greater than 0, and I believe that the original implementation was wrong, and this is actually Rx1DataRateOffset, not DesiredRx1DataRateOffset.

adriansmares · 2023-06-16T15:06:19Z

pkg/source/chirpstack/source.go

-		}
-		for _, freq := range devProfile.FactoryPresetFreqs {
-			dev.MacSettings.FactoryPresetFrequencies = append(dev.MacSettings.FactoryPresetFrequencies, uint64(freq))
+		dev.MacSettings.DesiredRx2DataRateIndex = &ttnpb.DataRateIndexValue{


This should also be set only if greater than 0. Also same question as before - is it a desired parameter ?

This seems to be Rx2DataRateIndex, not DesiredRx2DataRateIndex

pkg/source/chirpstack/util.go

adriansmares · 2023-06-30T13:50:28Z

pkg/source/chirpstack/source.go

 	// General
-	switch devProfile.MacVersion {
-	case "1.0.0":
+	switch devProfile.MacVersion.String() {


Let's use the enum instead of comparing strings.

adriansmares · 2023-06-30T13:50:42Z

pkg/source/chirpstack/source.go

+	case "LORAWAN_1_0_2":
 		dev.LorawanVersion = ttnpb.MACVersion_MAC_V1_0_2
-		switch devProfile.RegParamsRevision {
+		switch devProfile.RegParamsRevision.String() {


Let's use the enum instead of comparing strings.

adriansmares · 2023-06-30T13:51:44Z

pkg/source/chirpstack/source.go

-			dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
-				Value: ttnpb.DataRateOffset(devProfile.RxDrOffset_1),
-			}
+		dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{


I would set this only if it is greater than 0, and I believe that the original implementation was wrong, and this is actually Rx1DataRateOffset, not DesiredRx1DataRateOffset.

adriansmares · 2023-06-30T13:52:06Z

pkg/source/chirpstack/source.go

-		}
-		for _, freq := range devProfile.FactoryPresetFreqs {
-			dev.MacSettings.FactoryPresetFrequencies = append(dev.MacSettings.FactoryPresetFrequencies, uint64(freq))
+		dev.MacSettings.DesiredRx2DataRateIndex = &ttnpb.DataRateIndexValue{


This seems to be Rx2DataRateIndex, not DesiredRx2DataRateIndex

adriansmares · 2023-06-30T13:52:20Z

pkg/source/chirpstack/source.go


 	// Payload formatters
-	switch devProfile.PayloadCodec {
+	switch devProfile.PayloadCodecRuntime.String() {


Let's use their enum.

adriansmares

LGTM. @johanstokking do you have any objections to dropping support completely for ChirpStack v3 going forward (see the discussion above) ?

The only alternative that I could think of was to basically maintain our own generated code version of their proto files, with a package statement added manually in order to avoid message collisions. We would then have to edit the generated gRPC code in order to edit out the package from the RPC paths. It is very, very messy, but doable.

johanstokking · 2023-07-05T19:06:10Z

I think this is acceptable going forward indeed. I don't see another clean solution either.

KrishnaIyer · 2023-09-27T11:40:51Z

@happyRip: What's the status of addressing the review comments?

KrishnaIyer · 2024-02-28T10:28:01Z

replaced by #112

happyRip added this to the 2023 Q2 milestone May 15, 2023

happyRip self-assigned this May 15, 2023

adriansmares modified the milestones: 2023 Q2, v0.10.0 May 23, 2023

happyRip commented Jun 16, 2023

View reviewed changes

happyRip force-pushed the feature/chirpstack-v4 branch 2 times, most recently from 2988635 to 9ea354b Compare June 16, 2023 14:51

happyRip marked this pull request as ready for review June 16, 2023 15:00

happyRip requested review from adriansmares and johanstokking as code owners June 16, 2023 15:00

adriansmares reviewed Jun 16, 2023

View reviewed changes

happyRip requested a review from adriansmares June 29, 2023 14:08

adriansmares reviewed Jun 30, 2023

View reviewed changes

happyRip force-pushed the feature/chirpstack-v4 branch from 032d31e to 27b400a Compare July 3, 2023 07:43

happyRip requested a review from adriansmares July 3, 2023 07:44

adriansmares approved these changes Jul 5, 2023

View reviewed changes

KrishnaIyer modified the milestones: v0.10.0, Oct 2023 Sep 27, 2023

KrishnaIyer modified the milestones: Oct 2023, Nov 2023, Dec 2023 Nov 23, 2023

KrishnaIyer modified the milestones: Dec 2023, Feb 2023 Jan 26, 2024

KrishnaIyer added 2 commits February 28, 2024 10:43

all: Fix chirpstack v3 source

34cfd2e

all: Adapt ChirpStack to v4

c2d62ba

KrishnaIyer force-pushed the feature/chirpstack-v4 branch from 27b400a to c2d62ba Compare February 28, 2024 09:53

KrishnaIyer mentioned this pull request Feb 28, 2024

Support ChirpStack v4 #112

Merged

3 tasks

KrishnaIyer closed this Feb 28, 2024

Support migration from ChirpStack V4 #75

Support migration from ChirpStack V4 #75

Uh oh!

Conversation

happyRip commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Regressions

Notes for Reviewers

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriansmares left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriansmares left a comment

Choose a reason for hiding this comment

Uh oh!

johanstokking commented Jul 5, 2023

Uh oh!

KrishnaIyer commented Sep 27, 2023

Uh oh!

KrishnaIyer commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

happyRip commented May 15, 2023 •

edited

Loading