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

Support migration from ChirpStack V4 #75

Closed
wants to merge 2 commits into from
Closed

Conversation

happyRip
Copy link
Member

@happyRip happyRip commented May 15, 2023

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 happyRip added this to the 2023 Q2 milestone May 15, 2023
@happyRip happyRip self-assigned this May 15, 2023
@adriansmares adriansmares modified the milestones: 2023 Q2, v0.10.0 May 23, 2023
Comment on lines 276 to 283
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@happyRip happyRip force-pushed the feature/chirpstack-v4 branch 2 times, most recently from 2988635 to 9ea354b Compare June 16, 2023 14:51
@happyRip happyRip marked this pull request as ready for review June 16, 2023 15:00
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
Value: ttnpb.DataRateOffset(devProfile.RxDrOffset_1),
}
dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be Rx2DataRateIndex, not DesiredRx2DataRateIndex

pkg/source/chirpstack/util.go Outdated Show resolved Hide resolved
}

// Frequency Plan
dev.FrequencyPlanId = p.FrequencyPlanID

// General
switch devProfile.MacVersion {
case "1.0.0":
switch devProfile.MacVersion.String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the enum instead of comparing strings.

dev.LorawanVersion = ttnpb.MACVersion_MAC_V1_0_2
switch devProfile.RegParamsRevision {
switch devProfile.RegParamsRevision.String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the enum instead of comparing strings.

dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
Value: ttnpb.DataRateOffset(devProfile.RxDrOffset_1),
}
dev.MacSettings.DesiredRx1DataRateOffset = &ttnpb.DataRateOffsetValue{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be Rx2DataRateIndex, not DesiredRx2DataRateIndex

@@ -289,30 +266,28 @@ func (p *Source) ExportDevice(devEui string) (*ttnpb.EndDevice, error) {
}

// Payload formatters
switch devProfile.PayloadCodec {
switch devProfile.PayloadCodecRuntime.String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use their enum.

Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@KrishnaIyer KrishnaIyer modified the milestones: v0.10.0, Oct 2023 Sep 27, 2023
@KrishnaIyer
Copy link
Member

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

@KrishnaIyer KrishnaIyer modified the milestones: Oct 2023, Nov 2023, Dec 2023 Nov 23, 2023
@KrishnaIyer KrishnaIyer modified the milestones: Dec 2023, Feb 2023 Jan 26, 2024
@KrishnaIyer
Copy link
Member

replaced by #112

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.

Support migration from ChirpStack V4
4 participants