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

Add defines for changing transport to endianness agnostic #276

Merged
merged 5 commits into from
Jun 7, 2022

Conversation

amgross
Copy link
Contributor

@amgross amgross commented May 17, 2022

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

Added defines for user for changing the bytes that transport to some endianness agnostic way he decide on.

To Reproduce

Expected behavior

Screenshots

Desktop (please complete the following information):

  • OS:Windows
  • eRPC Version:1.9.0

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.

Additional context

Tested with two cores with different endianness, the core with little endian changed its endianness while read+write and the other didn't changed anything. 64 bit types and pointer types wasn't tested.
Fixes #106

@Hadatko
Copy link
Member

Hadatko commented May 17, 2022

Hi @amgross , thank you for your PR.
I was thinking if we can do it more generic. As you mentioned there can be some architecture dependencies (float/double). I am thinking if we should create new port files in port folder for this case. Maybe OS/standart lib we are supporting currently have functionality for this already and we can reuse them.
Also i am still thinking if it wouldn't be better to move the code under Cursor class. Myabe we can have enum variable for cursor write which would define data type like: bytestream, float,double,number -> in case of number we can do decission based on length variable. What do you think?

@Hadatko
Copy link
Member

Hadatko commented May 17, 2022

@amgross amgross changed the title Add defines for changing transport to erndianness agnostic Add defines for changing transport to endianness agnostic May 18, 2022
@amgross
Copy link
Contributor Author

amgross commented May 18, 2022

For example: https://www.freertos.org/FreeRTOS-Plus/FreeRTOS_Plus_TCP/API/htons_ntohs_htonl_ntohl.html

We can use the defines I added to make it (if we will decide to do it I can), but as you can see also they not handle float and double because it is more complex issue (about 64 bits, I don't know if always swapping all bytes or there are architecture that swapping each 4 bytes separately).

Maybe we can have enum variable for cursor write which would define data type like: bytestream, float, double, number -> in case of number we can do decision based on length variable

It can be done, but why switch-case is cleaner/faster than handling each type in its function? (BTW, it preffered to have in both read and write, so all data passes will be in same endianness)

@Hadatko
Copy link
Member

Hadatko commented May 18, 2022

I am just thinking what is better. Let's keep it as you have it, just add note to Codec class and BasicCodec class that this classes are handling endianess

@Hadatko
Copy link
Member

Hadatko commented May 18, 2022

But still i would say that it is better to create port file and include it. Maybe trough one macro in config file...

@amgross
Copy link
Contributor Author

amgross commented May 18, 2022

So you want me:

  1. Move all defines I added erpc_config to port file (if such, maybe I will even define them for one system) and leave erpc_config_internal as I done
  2. add note to Codec class and BasicCodec class that this classes are handling endianness

Sounds OK?

@Hadatko
Copy link
Member

Hadatko commented May 18, 2022

I was thinking something like:
Removing all definitions from configs->create new file someting like erpc_endianess_undefined.h->define empty macros->in erpc_config define macro something like
//#define ENDIANES_HEADER "erpc_endianess_undefined.h"
in internal config
ifndef ENDIANES_HEADER
define ENDIANES_HEADER "erpc_endianess_undefined.h"
endif

in basic_codec source file:
#include ENDIANES_HEADER

@amgross
Copy link
Contributor Author

amgross commented May 18, 2022

Sounds OK for me (will do id probably tomorrow), we don't want to add working example currently?

@Hadatko
Copy link
Member

Hadatko commented May 18, 2022

Working example would be great. What do you think @MichalPrincNXP ?

@amgross
Copy link
Contributor Author

amgross commented May 19, 2022

@Hadatko I done what we talked about,
Please look if it stands in the needed standards and add the expected license to the new files.
I did some sanity tests on the changes

@amgross
Copy link
Contributor Author

amgross commented May 31, 2022

Hi @MichalPrincNXP ,
What do you think on this PR?

@amgross
Copy link
Contributor Author

amgross commented May 31, 2022

@Hadatko I see one of the CI tests got to timeout, can you try to rerun it?

@Hadatko
Copy link
Member

Hadatko commented May 31, 2022

Yes this happens sometimes. We should fix that.

Copy link
Member

@MichalPrincNXP MichalPrincNXP left a comment

Choose a reason for hiding this comment

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

Hello @amgross , I am sorry for delayed response. I am ok with the approach and the proposed changes. Internal on-board testing passed, not affecting any test. But, we do not test the case when two different endianness is used, we rely on your case and the correct functionality on your side. Anyway, I am happy that we have a solution for these cases, thank you for the effort with the PR.

@MichalPrincNXP MichalPrincNXP added this to the 1.10.0 milestone Jun 7, 2022
@MichalPrincNXP MichalPrincNXP merged commit 3abb86d into EmbeddedRPC:develop Jun 7, 2022
@amgross
Copy link
Contributor Author

amgross commented Jun 7, 2022

@MichalPrincNXP Wasn't it better to collapse all commits to one before merging? I thought you will ask me to do it or you will do it before merging.

@MichalPrincNXP
Copy link
Member

I am doing the merge from the web and I am not sure if it allows to do the squash merge (unless you are doing it manually on command line)

@amgross
Copy link
Contributor Author

amgross commented Jun 12, 2022

@MichalPrincNXP see squash and merge

@MichalPrincNXP
Copy link
Member

Hello @amgross , I am sorry I missed that there is no copyright and license header in newly introduced erpc_endianness_undefined.h and erpc_endianness_agnostic_example.h sources which is not good and we should solve that. Could you please add copyright headers into mentioned files with BSD-3 Clause license, please? Thank you.

@amgross
Copy link
Contributor Author

amgross commented Mar 19, 2024

@MichalPrincNXP DONE #417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

endianness agnostic
3 participants