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.

Sign up for GitHub

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

Jump to bottom

sys/luid: provide luid_get_lb(), fix documentation #14016

Merged
merged 1 commit into from
May 13, 2020
Merged

sys/luid: provide luid_get_lb(), fix documentation #14016

merged 1 commit into from
May 13, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented May 4, 2020 •
edited
Loading

Contribution description

The documentation of luid_get() is wrong, or at least confusing.

It talks about

an 8-bit incrementing counter value into the most significant byte

while the implementation does

((uint8_t *)buf)[0] ^= lastused++;

Now it could be argued that the intention was that the ID is supposed to be used in Big Endian contexts and that was an omission, however to keep everyone's sanity, let's keep it simple and just state that this actually changes the LSB.

Also add a luid_get_lb() function that does the same, but modifies the most significant last byte - or the least significant one if interpreted as Big Endian.

Testing procedure

Issues/PRs references

This can be used directly by e.g. #13743

@benpicco benpicco added Area: doc Area: Documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: sys Area: System labels May 4, 2020
@benpicco benpicco requested a review from maribu May 4, 2020 14:32
@maribu
Copy link
Member

maribu commented May 4, 2020

Note that most significant byte or least significant byte have no meaning for byte arrays. The only have a meaning on numbers.

Why not just call it the first or last byte?

@benpicco benpicco changed the title sys/luid: provide luid_get_be(), fix documentation sys/luid: provide luid_get_lb(), fix documentation May 4, 2020
@benpicco
Copy link
Contributor Author

benpicco commented May 13, 2020 •
edited
Loading

Of course last and first depend on which way you count them 😉
But yea that makes the whole thing a lot less confusing.

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels May 13, 2020
maribu
maribu approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Untested ACK. (But luid_get_lb() is literally two lines of code. I buy a round of beer on the next non-Corona real-presence RIOT summit, if these two lines contain a bug.)

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 13, 2020
@benpicco
sys/luid: provide luid_get_lb(), fix documentation
e195e02 
This does two things:

The documentation of `luid_get()` is wrong, or at least confusing.

It talks about

> an 8-bit incrementing counter value into the most significant byte

while the implementation does

    ((uint8_t *)buf)[0] ^= lastused++;	// 0 is LSB!

Now it could be argued that the intention was that the ID is supposed
to be used in Big Endian contexts and that was an omission, however
to keep everyone's sanity, let's keep it simple and just state that this
actually changes the LSB.

Also add a `luid_get_lb()` function that does the same, but modifies the
most significant byte - or the last byte if looking at the index.

This can then be used directly by e.g. RIOT-OS#13743
@benpicco benpicco merged commit 88e4c0f into RIOT-OS:master May 13, 2020
@benpicco benpicco deleted the luid_get_be branch May 13, 2020 19:14
@benpicco
Copy link
Contributor Author

Promising beer for buggy code may create bad incentives 😉

@maribu
Copy link
Member

maribu commented May 13, 2020

Quite the opposite: A habit of reviewers having to buy the beer as compensation for poor reviews should either increase review quality, or at least helps to sheer up those affected by the poor review. In any case a win ;-)

@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @maribu @miri64