Skip to content

core: Provide functions for different byte orders#1699

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
Kijewski:issue-1586
Oct 10, 2014
Merged

core: Provide functions for different byte orders#1699
miri64 merged 3 commits intoRIOT-OS:masterfrom
Kijewski:issue-1586

Conversation

@Kijewski
Copy link
Copy Markdown
Contributor

Rationale: see #1586.

@Kijewski Kijewski added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Sep 24, 2014
@Kijewski Kijewski added this to the Release NEXT MAJOR milestone Sep 24, 2014
@Kijewski Kijewski added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 24, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

Depends on #1700.

@Kijewski Kijewski removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 27, 2014
@OlegHahm
Copy link
Copy Markdown
Member

I'll try to do some testing now.

@OlegHahm
Copy link
Copy Markdown
Member

And s/code/core/ again.

@Kijewski Kijewski changed the title code: Provide functions for different byte orders core: Provide functions for different byte orders Sep 28, 2014
@Kijewski Kijewski added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 28, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

Typo in title fixed. Typos in code fixed, too.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Unit tests added.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the network stack those are always above the type

typedef union __attribute__((packed)) {
    …
} …;

I get that this might be better to not confuse coding-style tools (I'm looking at you astyle ಠ_ಠ), but can we discuss this please? I think a uniform code is always better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your unicode is too strong for my font.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have no problem with

typedef union __attribute__((packed)) {
    …
} …;

will change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@OlegHahm
Copy link
Copy Markdown
Member

I second @authmillenon concerning the style, but say ACK otherwise.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Separate commit?

@Kijewski
Copy link
Copy Markdown
Contributor Author

@N8Fear, separated.

@Kijewski Kijewski removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 29, 2014
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some of these builtins seems to create warnings in other toolchains, too. E.g. with older versions of the CodeSourcery toolchain for ARM.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

E.g. with older versions of the CodeSourcery toolchain for ARM.

Then the user has to update.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought if there's an ifdef anyway we could find a solution for vintage-lovers, too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course we could. I was happy that I found that __builtin_bswapN() works for all platforms (well, for all but MSP with N=16), because the generated code for the fallback solution is extremely ugly. What do the warning messages read? If it works, then I would rather use a pragma to silence the warnings.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. I'll check as soon I can access a computer with this toolchain installed again. But it's not a blocker anyway.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Oct 6, 2014

Anything left to discuss?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Oct 7, 2014

Looks good to me. ACK

@Kijewski
Copy link
Copy Markdown
Contributor Author

I am not 100% sure if this is a core change. It adds a new file in core/includes but otherwise does not affect the core at all.
So please seconds ACK, maybe, @N8Fear?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 10, 2014

I jump in: ACK & go

miri64 added a commit that referenced this pull request Oct 10, 2014
core: Provide functions for different byte orders
@miri64 miri64 merged commit 42f96b0 into RIOT-OS:master Oct 10, 2014
@Kijewski Kijewski deleted the issue-1586 branch October 10, 2014 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants