search

openigtlink / OpenIGTLink

Free, open-source network communication library for image-guided therapy
http://openigtlink.org
BSD 3-Clause "New" or "Revised" License
102 stars 184 forks source link

ENH: Adding parameter to Pack() to make crc calculation optional #230

Closed adamrankin closed 4 years ago

adamrankin commented 4 years ago

Referencing #229

adamrankin commented 4 years ago

@tokjun @leochan2009

tokjun commented 4 years ago

Thanks @adamrankin

leochan2009 commented 4 years ago

Hi Both,

i found a minor issue here. if the sender didn't do a crc calculation, the receiver doesn't know if the crc wasn't calculated. So if receiver tries to perform the crc check, it will fail though data integrity is ok. I would add a check at the receiver side inside the unpack() function to avoid this situation

leochan2009 commented 4 years ago

Also, a test should be added to cover this new feature.

adamrankin commented 4 years ago

Hi Both,

i found a minor issue here. if the sender didn't do a crc calculation, the receiver doesn't know if the crc wasn't calculated. So if receiver tries to perform the crc check, it will fail though data integrity is ok. I would add a check at the receiver side inside the unpack() function to avoid this situation

Maybe a new meta data field, HasCRC? Also, CRC behaviour should be documented in the spec if possible.

leochan2009 commented 4 years ago

Hi,

meta data field is one approach, however, we would like to ensure backward compatibility for messages that has not meta data field. If mathematically we can proof that the CRC will not equal to zero for any data, then we can just check if the crc is equal to zero or not at the receiver end. I am looking into some literature now to verify if CRC calculation will ever return zero or not

tokjun commented 4 years ago

I think CRC can take zero, so strictly speaking, you cannot use zero as a flag to disable CRC check.

leochan2009 commented 4 years ago

Yes, CRC calculation can return zero. so we will need to leverage the meta data for optional CRC.

tokjun commented 4 years ago

As for the backward compatibility, I don’t now if we should change the version 1 protocol now to allow disabling CRC. Can we simply specify that the option is available only for version >= 2 messages?

@adamrankin I agree that CRC should be defined better in the protocol document.

leochan2009 commented 4 years ago

yes, As we are using the meta data, it will only be available for version 2. Version 1 won't have the option to disable the CRC.

leochan2009 commented 4 years ago

@adamrankin, will you provide the code to send crccheck via meta data?