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

Clarify rules for packing and unpacking in Section 8.1 #456

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

JonathanGregory
Copy link
Contributor

See issue #374 for discussion of these changes.

Release checklist

  • [NA] Authors updated in cf-conventions.adoc?
  • [NA] Next version in cf-conventions.adoc up to date? Versioning inspired by SemVer.
  • [Y] history.adoc up to date?
  • [Y] Conformance document up-to-date?

@mankoff
Copy link

mankoff commented Sep 18, 2023

I think this change is a great improvement to the documentation. Thank you.

Copy link

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

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

Thanks @JonathanGregory. I agree with @mankoff, this is a great improvement.

ch08.adoc Outdated
An additional restriction in this case is that the variable containing the packed data must be of type **`byte`**, **`short`** or **`int`**.
It is not advised to unpack an **`int`** into a **`float`** as there is a potential precision loss.

When packed data is written, the **`scale_factor`** and **`add_offset`** attributes must be of the same type as the unpacked data, which must be either **`float`** or **`double`**. Data of type **`float`** must be packed into one of these types: **`byte`**, **`unsigned byte`**, **`short`**, **`unsigned short`**. Data of type **`double`** must be backed into one of these types: **`byte`**, **`unsigned byte`**, **`short`**, **`unsigned short`**, **`int`**, **`unsigned int`**.

Choose a reason for hiding this comment

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

Nit: backed -> packed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Now fixed. Thanks.

@larsbarring larsbarring merged commit b25de08 into cf-convention:main Sep 18, 2023
4 checks passed
@larsbarring larsbarring mentioned this pull request Oct 16, 2023
4 tasks
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.

Clarify what action to take when unpacking an int
4 participants