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

Increase size limit for payload formatter scripts #4312

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

neoaggelos
Copy link
Contributor

Summary

References #4053
Closes #4278

Testing

...

Regressions

...

Notes for Reviewers

I think I saw somewhere a standard limit of 64KB being mentioned, but went with the 40KB limit proposed in the issue.

Let's choose a value and avoid more PRs like this in the future.

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.

@neoaggelos neoaggelos added this to the v3.13.3 milestone Jun 24, 2021
@neoaggelos neoaggelos self-assigned this Jun 24, 2021
@github-actions github-actions bot added compat/api This could affect API compatibility compat/config This could affect Configuration compatibility labels Jun 24, 2021
@neoaggelos neoaggelos force-pushed the issue/4278-formatters-length branch from 0a932c2 to 6ee4224 Compare June 24, 2021 20:33
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

As @johanstokking wrote in #4278 (comment):

Currently, there's a LoRa Alliance task force under TC about Codec API where a limit is being defined. I think we'll end up somewhere in the range of 32 to 40 KB.

So I think that for now we should go for 32kB, and depending on the outcome of LoRa Alliance discussion increase it to 40kB.

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.

I also said #4278 (comment), echoing @adriansmares's proposal. That's implemented here.

The LoRa Alliance decision is going to take a while because we also want to settle on a library that gets pre-loaded, i.e. Node's Buffer polyfill, maybe some crypto functions and maybe some CRC functions. Once that is defined, we have a better idea of what is sensible for the device maker to implement themselves, and come up with a sane limit.

Now, until we have that, I would in fact relax this for now, because this is really limiting at the moment. So let's settle on 40 KB in API and 40 KB in AS allowed maximum. Whenever we have Buffer, crypto and CRC32, we will once again reduce the AS allowed maximum, aligning with the LoRa Alliance technical recommendation. So this is good to go as-is.

@neoaggelos neoaggelos force-pushed the issue/4278-formatters-length branch from 6ee4224 to 8e4ff63 Compare June 29, 2021 08:55
@neoaggelos neoaggelos merged commit 05e264f into v3.13 Jun 29, 2021
@neoaggelos neoaggelos deleted the issue/4278-formatters-length branch June 29, 2021 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat/api This could affect API compatibility compat/config This could affect Configuration compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Available payload formatter size is too small
4 participants