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

UA_SecureConversationMessageHeader erroneous? #28

Closed
uleon opened this issue Mar 24, 2014 · 9 comments
Closed

UA_SecureConversationMessageHeader erroneous? #28

uleon opened this issue Mar 24, 2014 · 9 comments

Comments

@uleon
Copy link
Contributor

uleon commented Mar 24, 2014

the structure of the serialized UA_SecureConversationMessageHeader is something like:

UA_Byte[3] messageType, i.e. 'OPN'
UA_ Byte isFinal, i.e. 'F'
UA_?Int32 messageSize
UA_?Int32 secureChannelID

the ?generated? encoders/decoders are quite different. Before modifying I would like to learn the rationale behind the current definition.

@uleon uleon added the question label Mar 24, 2014
@FlorianPalm
Copy link
Contributor

I used the generator to create the functions and modified them later (enc,dec,calcSize). I encode the MessageType into a byte array and decode it by casting 3 Bytes to an UA_Int32 so I can use it in switch/case structures directly. Do you know any simpler solution? I am not really satisfied with that solution...

@uleon
Copy link
Contributor Author

uleon commented Mar 24, 2014

@FlorianPalm ok, just wanted to know if these functions will be generated once more. From your answer I learn that this is not the case, so I can modify directly in the code, right?

@FlorianPalm
Copy link
Contributor

yes

@uleon
Copy link
Contributor Author

uleon commented Mar 24, 2014

ok, then we only need to decide if we want to do the encoding/decoding of the messageType in the encoders/decoders. You'll find a messy solution in commit 8da17e3. Personally I'd propose to have the messageType as a part of the struct, so it should decode it itself, eg. by calling TL_getPackageType, shouldn't it?

@FlorianPalm
Copy link
Contributor

I do not like the function TL_getPacketType, it is too long and does too many comparisions. If the header structure holds the MessageType in an UA_Int32 instead of Byte[3] and we define an enum with the corresponding values it would just be a casting/shifting operation.

enum packetType
{
    UA_MESSAGETYPE_HEL = 0x48454C, // H E L
    UA_MESSAGETYPE_ACK = 0x41434B, // A C k
    UA_MESSAGETYPE_ERR = 0x455151, // E R R
    UA_MESSAGETYPE_OPN = 0x4F504E, // O P N
    UA_MESSAGETYPE_MSG = 0x4D5347, // M S G
    UA_MESSAGETYPE_CLO = 0x434C4F  // C L O
};

the decoding function would look like that

UA_Int32 UA_OPCUATcpMessageHeader_decode(char const * src, UA_Int32* pos, UA_OPCUATcpMessageHeader* dst) {
    UA_Int32 retval = UA_SUCCESS;
    UA_Byte tmpBuf[3];
    tmpBuf[3] = 0;
    retval |= UA_Byte_decode(src,pos,&(tmpBuf[0]));
    retval |= UA_Byte_decode(src,pos,&(tmpBuf[1]));
    retval |= UA_Byte_decode(src,pos,&(tmpBuf[2]));
    dst->messageType = (UA_Int32)(tmpBuf[0]<<16) + (UA_Int32)(tmpBuf[1]<<8) + (UA_Int32)(tmpBuf[2]);

    retval |= UA_Byte_decode(src,pos,&(dst->isFinal));
    retval |= UA_UInt32_decode(src,pos,&(dst->messageSize));
    return retval;
}

So the structure can still hold the MessageType as UA_Int32

@uleon
Copy link
Contributor Author

uleon commented Mar 24, 2014

Perfect! To encourage reuse I'd like to propose introducing a new type UA_MessageType - storage type UA_Int32, size 4; encoding/decoding UA_Byte[3], size 3.

FlorianPalm notifications@github.com schrieb:

I do not like the function TL_getPacketType, it is too long and does
too many comparisions. If the header structure holds the MessageType in
an UA_Int32 instead of Byte[3] and we define an enum with the
corresponding values it would just be a casting/shifting operation.

enum packetType
{
  UA_MESSAGETYPE_HEL = 0x48454C, // H E L
  UA_MESSAGETYPE_ACK = 0x41434B, // A C k
  UA_MESSAGETYPE_ERR = 0x455151, // E R R
  UA_MESSAGETYPE_OPN = 0x4F504E, // O P N
  UA_MESSAGETYPE_MSG = 0x4D5347, // M S G
  UA_MESSAGETYPE_CLO = 0x434C4F  // C L O
};

the decoding function would look like that

UA_Int32 UA_OPCUATcpMessageHeader_decode(char const * src, UA_Int32*
pos, UA_OPCUATcpMessageHeader* dst) {
  UA_Int32 retval = UA_SUCCESS;
  UA_Byte tmpBuf[3];
  tmpBuf[3] = 0;
  retval |= UA_Byte_decode(src,pos,&(tmpBuf[0]));
  retval |= UA_Byte_decode(src,pos,&(tmpBuf[1]));
  retval |= UA_Byte_decode(src,pos,&(tmpBuf[2]));
  dst->messageType = (UA_Int32)(tmpBuf[0]<<16) +
(UA_Int32)(tmpBuf[1]<<8) + (UA_Int32)(tmpBuf[2]);

  retval |= UA_Byte_decode(src,pos,&(dst->isFinal));
  retval |= UA_UInt32_decode(src,pos,&(dst->messageSize));
  return retval;
}

So the structure can still hold the MessageType as UA_Int32


Reply to this email directly or view it on GitHub:
#28 (comment)

Prof. L. Urbas; TU Dresden; +49-177-4665201

@Stasik0
Copy link
Contributor

Stasik0 commented Mar 24, 2014

good evening, i was following the discussion and had an idea

please look into the commit 1b36c82

i have implemented a very messy "plugin" system for the code generator, maybe you can just create a custom type for the functions above and use the generator on custom .xml files ;)

@uleon
Copy link
Contributor Author

uleon commented Mar 26, 2014

@Stasik0 : please reopen new issue for plugin system, @FlorianPalm solved this issue

@uleon uleon closed this as completed Mar 26, 2014
@lock
Copy link

lock bot commented Jul 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants