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

FramedTransport::Header not serialized?! #163

Closed
infn-ke opened this issue Feb 12, 2021 · 15 comments · Fixed by #378
Closed

FramedTransport::Header not serialized?! #163

infn-ke opened this issue Feb 12, 2021 · 15 comments · Fixed by #378
Assignees
Milestone

Comments

@infn-ke
Copy link

infn-ke commented Feb 12, 2021

Why is not the erpc header (FramedTransport::Header) serialized? E.g. do htons on the 16 bit members m_messageSize and m_crc.

@ierturk
Copy link
Contributor

ierturk commented Feb 17, 2021

I experienced problems due to lack of serialized header, and added some code to framed transport class to serialize header. I can share the code on request.

@infn-ke
Copy link
Author

infn-ke commented Feb 17, 2021

Maybe you can raise a PR so that it can be included in master branch of erpc? I don't want to maintain a fix on my own.

@Hadatko
Copy link
Member

Hadatko commented Feb 17, 2021

Hi, Could you explain why you need serialize framedTransport header? Currently it is send to other side before serialized message as it contains extra data in compare with other possible transports.

@ierturk
Copy link
Contributor

ierturk commented Feb 17, 2021

Hi @Hadatko , using the term serialize may be wrong here. Sometimes the transferred header may be corrupted due to lack of frame and CRC. So I have to add an extra frame and CRC to check header integrity into header.

@Hadatko
Copy link
Member

Hadatko commented Feb 17, 2021

So the issue is related to sending receiving message size. As second receive call is using size from first call. Sounds reasonable to me to have crc of header.

@ierturk
Copy link
Contributor

ierturk commented Feb 17, 2021

Yes, it is related to message size. The second side wait for message with incorrect size. So the transfer is aborted if there is a timeout for the transfer. In our particular situation, the content of message were coded as ASCII and added framing characters. May be the measures other than CRC are not needed.

@infn-ke
Copy link
Author

infn-ke commented Feb 17, 2021

@Hadatko Why is it not needed to serialize and deserialize header? Representing header in network byte order would be the only way to let little-endian and big-endian exchange header before receiving the payload.

@Hadatko
Copy link
Member

Hadatko commented Feb 17, 2021

So we have two issues here. Serializing header and validate header. If you can create PR for both of them it should be great ;) Thank you guys.

@Hadatko
Copy link
Member

Hadatko commented Aug 28, 2023

I will take a look on theese.

@amgross
Copy link
Contributor

amgross commented Aug 28, 2023

Didn't it solved enough at #276 ?
If someone want he can use htons as implementation of the endiannes agnostic defines.
Maybe we can consider add such implementation.

@Hadatko
Copy link
Member

Hadatko commented Aug 28, 2023

How is that PR solving CRC of header data?

@amgross
Copy link
Contributor

amgross commented Aug 28, 2023

ERPC_WRITE_AGNOSTIC_16(h.m_crc);

@Hadatko
Copy link
Member

Hadatko commented Aug 28, 2023

@amgross :D That is crc of next message (serialized data). But there is no check if sended header data are correct. From what i understand here we need add crcHeader into Header struct. This one will be computed from header crc and length.
Other side then need firstly check header crc and then receive body and check body crc. This way you know that length and crc in header struct was received correctly

@amgross
Copy link
Contributor

amgross commented Aug 28, 2023

The first message here talked about 'E.g. do htons on the 16 bit members m_messageSize and m_crc', so I thought we are talking on endianness issue

@amgross
Copy link
Contributor

amgross commented Aug 28, 2023

I think @infn-ke (which opened the issue) talked here on endianness and @ierturk talked about length issue (which I also faced once)

@Hadatko Hadatko linked a pull request Sep 27, 2023 that will close this issue
7 tasks
@Hadatko Hadatko added this to the 1.12.0 milestone Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants