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

Introduce <endian.h> compatibility header (IDFGH-2715) #4784

Closed
wants to merge 2 commits into from
Closed

Introduce <endian.h> compatibility header (IDFGH-2715) #4784

wants to merge 2 commits into from

Conversation

fgiancane8
Copy link
Contributor

Programs and libraries using compiler and system information about
endianness often include the system header <endian.h>.

In xtensa-gcc compiler with newlib distribution, that file is located in
<machine/endian.h>; this means that #include <endian.h> would fail
at compile time.

This commit fixes the issue by adding a compatibility <endian.h>
header which in turn just includes <machine/endian.h>.

Signed-off-by: Francesco Giancane francesco.giancane@accenture.com

@github-actions github-actions bot changed the title Introduce <endian.h> compatibility header Introduce <endian.h> compatibility header (IDFGH-2715) Feb 17, 2020
@Alvin1Zhang
Copy link
Collaborator

@fgiancane8 Thanks for your contribution.

@Alvin1Zhang Alvin1Zhang requested a review from igrr February 18, 2020 01:15
@igrr
Copy link
Member

igrr commented Feb 18, 2020

Thanks for the PR @fgiancane8. It seems that we also need a few more definitions in endian.h, such as htobe16, be16toh, and so on. Can either leave this to you if you like, or we can add another commit with the rest of the definitions on top of yours.

@fgiancane8
Copy link
Contributor Author

Hi @igrr ,

You are right indeed. I just searched for the symbols I needed and did not check for the fully compliance.

I did a quick search inside system xtensa compiler and they are not defined elsewhere.
I think I need to implement them (or just backport from another C library implementation in a proper way).

Of course if you already did the work, I am happy with merging my commit with yours.
Otherwise, I will update my PR with missing symbols.

@igrr
Copy link
Member

igrr commented Feb 18, 2020

Perhaps you can check an existing BSD licensed version of endian.h and copy it as whole, retaining the copyright headers.

@fgiancane8
Copy link
Contributor Author

Hi @igrr ,

from man 3 endian

       These functions are nonstandard.  Similar functions are present on
       the BSDs, where the required header file is <sys/endian.h> instead of
       <endian.h>.  Unfortunately, NetBSD, FreeBSD, and glibc haven't
       followed the original OpenBSD naming convention for these functions,
       whereby the nn component always appears at the end of the function
       name (thus, for example, in NetBSD, FreeBSD, and glibc, the
       equivalent of OpenBSDs "betoh32" is "be32toh")

Being non-standard, the requirement to have it in the compatibility layer is questionable.
However, I think we may import the FreeBSD version of endian.h which is licensed BSD-2 Clause with minimal modifications.

What do you think?

@fgiancane8
Copy link
Contributor Author

@igrr ,
Imported the FreeBSD version of the symbols for endianness handling.

I tested them with this code:

#include <endian.h>
	uint16_t one = 0xbeef;
	uint32_t two = 0xcafebabe;

	uint8_t __16bytes[] = {0xbe, 0xef};
	uint8_t __32bytes[] = {0xca, 0xfe, 0xba, 0xbe};

	uint16_t test1;

	test1 = htobe16(one);
	test1 = htole16(one);

	uint8_t mem[4];

	test1 = be16dec(__16bytes);

	be16enc(mem, one);

	uint32_t test2;

	test2 = htobe32(two);
	test2 = htole32(two);
	
	test2 = be32dec(__32bytes);
	be32enc(mem, two);

No warnings and no issue so far with maximum compiler strictness.

@fgiancane8
Copy link
Contributor Author

hi @igrr ,
Any update about this? Is the second commit worth for the remaining symbols to be imported?

@fgiancane8
Copy link
Contributor Author

Hi @igrr ,
I am still waiting for your feedback :) do you think we need any other work done?
Thanks,
Francesco

@fgiancane8
Copy link
Contributor Author

@igrr , @Alvin1Zhang
Any update about this PR?

Thanks,
Francesco

Francesco Giancane added 2 commits March 27, 2020 15:08
Programs and libraries using compiler and system information about
endianness often include the system header `<endian.h>`.

In `xtensa-gcc` compiler with `newlib` distribution, that file is located in
`<machine/endian.h>`; this means that `#include <endian.h>` would fail
at compile time.

This commit fixes the issue by adding a compatibility `<endian.h>`
header which in turn just includes `<machine/endian.h>`.

Signed-off-by: Francesco Giancane <francesco.giancane@accenture.com>
BSDs and Unices defined some non standardised functions and symbols used
for endianness handling: converting from Little Endian to Big Endian,
converting from Host to a specific representation, converting from a
specific representation to Host.

With this commit, a modified version of those symbols provided by
FreeBSD is imported.

The license of the imported code is still 2-Clause BSD.

Signed-off-by: Francesco Giancane <francesco.giancane@accenture.com>
@Alvin1Zhang
Copy link
Collaborator

@fgiancane8 Thanks for your contribution and updates, and sorry for the slow turnaround, really appreciate your patience waiting for our reply. We will check and then provide feedback. Thanks.

espressif-bot pushed a commit that referenced this pull request May 7, 2020
Programs and libraries using compiler and system information about
endianness often include the system header `<endian.h>`.

In `xtensa-gcc` compiler with `newlib` distribution, that file is located in
`<machine/endian.h>`; this means that `#include <endian.h>` would fail
at compile time.

This commit fixes the issue by adding a compatibility `<endian.h>`
header which in turn just includes `<machine/endian.h>`.

Signed-off-by: Francesco Giancane <francesco.giancane@accenture.com>
Merges #4784
@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally labels May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants