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 payload normalization #5730

Merged
merged 26 commits into from
Aug 29, 2022
Merged

Support payload normalization #5730

merged 26 commits into from
Aug 29, 2022

Conversation

johanstokking
Copy link
Member

@johanstokking johanstokking commented Aug 23, 2022

Summary

Closes #5429
References TheThingsNetwork/lorawan-devices#395

Changes

  • This adds normalized_payload to upstream messages
  • Also includes initial parsing of normalized payload per the draft schema (see referenced Device Repository PR)

Testing

Unit testing and local integration testing

Regressions

None expected

Notes for Reviewers

Skip the first commit; all lint warning removals are squashed in there

This includes some commits from #5727 to show non-LoRa data rates in the event preview

@kschiffer please review console: commit only

We can probably do improve, but this is already pretty cool:

Screen Shot 2022-08-27 at 16 13 09

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@johanstokking johanstokking added this to the v3.21.2 milestone Aug 23, 2022
@johanstokking johanstokking self-assigned this Aug 23, 2022
@github-actions github-actions bot added c/application server This is related to the Application Server compat/api This could affect API compatibility dependencies Pull requests that update a dependency file labels Aug 23, 2022
api/messages.proto Outdated Show resolved Hide resolved
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.

I like the direction in which this is going.

Now that the uplink payload formatter parameter encapsulates two functions at once, should we consider increasing the character limit, or do we keep the one we have so far ?

pkg/messageprocessors/javascript/javascript.go Outdated Show resolved Hide resolved
@johanstokking
Copy link
Member Author

Now that the uplink payload formatter parameter encapsulates two functions at once, should we consider increasing the character limit, or do we keep the one we have so far ?

Let's not do that until we run into problems. I think we're already quite generous in the Device Repository with 64KB.

Inline codecs in The Things Stack have a lower limit, but since we support detecting whether the payload is already normalized, what is really needed is that codec developers just return normalized payload going forward.

@github-actions github-actions bot added the tooling Development tooling label Aug 24, 2022
@johanstokking johanstokking force-pushed the feature/normalized-payload branch 3 times, most recently from 1a770b0 to 94c35a4 Compare August 26, 2022 20:09
@johanstokking johanstokking marked this pull request as ready for review August 26, 2022 20:21
@johanstokking
Copy link
Member Author

@adriansmares this is ready for another full pass

You can skip the first commit; all lint warning removals are squashed in there, no functional changes otherwise

@github-actions github-actions bot added the ui/web This is related to a web interface label Aug 26, 2022
@johanstokking

This comment was marked as resolved.

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.

Webhook template store schema (the YAML files that describe the templates) require an update with the new message type too. See

type webhookTemplatePaths struct {
UplinkMessage *string `yaml:"uplink-message,omitempty"`
JoinAccept *string `yaml:"join-accept,omitempty"`
DownlinkAck *string `yaml:"downlink-ack,omitempty"`
DownlinkNack *string `yaml:"downlink-nack,omitempty"`
DownlinkSent *string `yaml:"downlink-sent,omitempty"`
DownlinkFailed *string `yaml:"downlink-failed,omitempty"`
DownlinkQueued *string `yaml:"downlink-queued,omitempty"`
DownlinkQueueInvalidated *string `yaml:"downlink-queue-invalidated,omitempty"`
LocationSolved *string `yaml:"location-solved,omitempty"`
ServiceData *string `yaml:"service-data,omitempty"`
}

pkg/messageprocessors/normalizedpayload/uplink.go Outdated Show resolved Hide resolved
pkg/messageprocessors/normalizedpayload/uplink.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
@johanstokking johanstokking merged commit a5445f0 into v3.21 Aug 29, 2022
@johanstokking johanstokking deleted the feature/normalized-payload branch August 29, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server compat/api This could affect API compatibility dependencies Pull requests that update a dependency file tooling Development tooling ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for normalized_payload field
4 participants