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

Support latched topics in udp bridge #33

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Conversation

timonegk
Copy link
Member

@timonegk timonegk commented Apr 17, 2024

Summary

  • Adds support for latched / transient local topics by detecting them and republishing them periodically
  • Removes base64 encoding of pickle before encryption
  • Adds zlib compression before encryption to make it possible to receive larger messages (remark: upper limit due to network properties is 65535)
  • Remove loop when receiving packets and remove packet delimiter because it is not necessary.
  • Change info log to debug log
  • Run colcon build
  • Write documentation
  • Create issues for future work
  • Test on your machine
  • Test on the robot
  • This PR is on our Software project board

@timonegk timonegk force-pushed the udp_bridge_latched branch 3 times, most recently from 1082ec7 to ef2ac16 Compare April 17, 2024 15:55
Copy link
Member

@lilioid lilioid left a comment

Choose a reason for hiding this comment

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

the idea behind the message delimiter and the looping receive was that using that method larger messages than 65535 could be sent over the wire by splitting them into multiple udp packets.
This was never implemented on the sending side though :/

You could think about whether that is a useful property to have. If all relevant data packets including images are small enough then I agree that you won't need it.

Comment on lines 38 to 46
return Fernet(key=self.encryption_key).encrypt(bytes(message, encoding="UTF-8"))
return Fernet(key=self.encryption_key).encrypt(message)

def decrypt(self, enc: bytes) -> str:
if len(enc) == 0:
raise ValueError("Cannot decrypt empty data")
if self.encryption_key is None:
return str(enc, encoding="UTF-8")

return str(Fernet(key=self.encryption_key).decrypt(enc), encoding="UTF-8")
Copy link
Member

Choose a reason for hiding this comment

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

As far as I'm aware the official documentation says that bytes are used as input to encrypt and output of decrypt.

https://cryptography.io/en/latest/fernet/#cryptography.fernet.Fernet.encrypt

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I forgot to update the type hints. message is now bytes.

texhnolyze
texhnolyze previously approved these changes Apr 17, 2024
@timonegk
Copy link
Member Author

the idea behind the message delimiter and the looping receive was that using that method larger messages than 65535 could be sent over the wire by splitting them into multiple udp packets. This was never implemented on the sending side though :/

You could think about whether that is a useful property to have. If all relevant data packets including images are small enough then I agree that you won't need it.

It would be nice to be able to do that, but the current state did not work for packets bigger than that size and even for packets smaller than that size, merging the packets did not actually work :/ (e.g. size 12000 bytes)

@timonegk
Copy link
Member Author

Also, I am not sure if that can even work because packets might arrive in a different order.

@lilioid
Copy link
Member

lilioid commented Apr 17, 2024

Also, I am not sure if that can even work because packets might arrive in a different order.

yes they might; using udp is always on a best-effort basis.

It would be nice to be able to do that, but the current state did not work for packets bigger than that size and even for packets smaller than that size, merging the packets did not actually work :/ (e.g. size 12000 bytes)

That might be because the sending side never actually did split the packets so they would just be chopped by the linux kernels udp handling.

@Flova
Copy link
Member

Flova commented Apr 17, 2024

Btw. maybe we should use the standard ROS serializer, instead of pickle. This might have security and compatibility reasons.

Flova
Flova previously approved these changes Apr 18, 2024
Copy link
Member

@Flova Flova left a comment

Choose a reason for hiding this comment

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

Some minor comments including running the auto formatter and addressing the logging

udp_bridge/sender.py Outdated Show resolved Hide resolved
@texhnolyze texhnolyze merged commit 3879fba into master Apr 18, 2024
1 check passed
@texhnolyze texhnolyze deleted the udp_bridge_latched branch April 18, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants