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

Expose the latch and queue_size functionality in the RegisterPublisher sys_command #82

Closed
P3TE opened this issue Jul 14, 2021 · 6 comments
Assignees

Comments

@P3TE
Copy link
Contributor

P3TE commented Jul 14, 2021

Is your feature request related to a problem? Please describe.
I would like to be able to set the queue_size and the latch parameter when calling the RegisterPublisher sys_command.

Additional context
Line 25 of publisher.py on the 'dev' branch has "# TODO: surface latch functionality", so I plan to implement that.

@peifeng-unity
Copy link
Contributor

Hello @P3TE, thank you for the request and it is a good feature to have. I have created an internal ticket AIRO-990 to track it. It may take us some time to follow up and implement it. In the meantime, if you want to implement this feature, we are more than happy to help and review your changes to merge them into the codebase.

@P3TE
Copy link
Contributor Author

P3TE commented Jul 21, 2021

Hello again, I thought I should provide an update to this ticket.

So after some initial investigating I found that to expose the latch and queue_size parameters would also require a change to the ROS-TCP-Connector.
In fact most of the work is there.

So I went about implementing this feature, and I may have dreamed up some additional features that I thought might be nice.

These include:

  • Creating a pooling system for messages. This is particularly useful for messages with large data types (I.E. Image messages). The idea is that after sending a message, the large data buffer will lose references and the garbage collector will pick it up to handle it later. I've found that this introduces lag spikes. Specifically, on my system, I'm getting GC lag spikes of 16ms every 4-5 frames without pooling when simulating two 720p cameras simultaneously. The pooling system will recycle messages after they have been sent / are no longer being used so that you don't have to reallocate large buffers, you can just update the data within.
  • Implementing a ROS message queuing system on the Unity side in the ROS-TCP-Connector. The idea here is that similar to how ROS handles the queue_size. A publisher will queue messages up to the queue_size and let the TCP connection thread know that data is ready for sending. If the queue fills up before the data is sent, old messages from the queue are discarded. The goal is to ensure that the outgoingMessageQueue doesn't grow unbounded if the TCP connection isn't able to handle the data rate from Unity.

So these features are mostly implemented on a new branch 'publisher-latch-dev' on my fork:

I realise that I've stepped out of the scope of just exposing the queue_size and latch functionality, but I thought I should touch base with you guys to get your opinion on these features.

@mrpropellers mrpropellers self-assigned this Aug 2, 2021
@mrpropellers
Copy link
Contributor

Hey @P3TE. Looks like this issue has fallen by the wayside a bit - but if you are still open to working on these features, feel free to open a draft PR against your base branch and tag me on it. I can go over what you have so far and discuss with the team what the best approach would be to get your code into the origin dev branch.

@P3TE
Copy link
Contributor Author

P3TE commented Aug 2, 2021

@mrpropellers So the current status of this is that I have implemented most of the functionality and I've been testing it locally but I did move onto other unrelated parts of the project I'm working on.
If you are interested in the features listed above in the previous post then I'll do a clean up of the code and create a PR.

@mrpropellers
Copy link
Contributor

mrpropellers commented Aug 3, 2021

These both sound like good features to have, but they also sound pretty design heavy so I can't guarantee I could let them go in right away without a thorough review of the implementations. I'm happy to go though the code with you, though! Just wanted to give you the heads up that it might entail some additional iterations if there's a lot of new logic in there.

@P3TE
Copy link
Contributor Author

P3TE commented Aug 3, 2021

Sounds good, additional iterations are no problem. I'll aim to get a PR tomorrow and we can discuss the changes!

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

No branches or pull requests

3 participants