-
Notifications
You must be signed in to change notification settings - Fork 5
Add a specific management of TF topics #17
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
Conversation
Improved byte reading for large message Removed a memory leak in tcp_interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to summarize, this will look for the option:
.is_tf or is_static_tf, which could be applied to topics that have TF2 Transforms but are not /tf or /tf_static, and autosets if they are this default case.
If it is, it will keep a buffer of X->Y transforms and update them as they come. It will always publish a transform between two frames that it has seen before.
I think this method makes sense. Someone somewhere may only care about certain transforms making it across, but I'm OK with sending them all for now. Since this is a special case, do you mind adding a small blurb under the configuration section of the README?
Also appreciate cleanup and robustness improvements. Thanks a lot.
| /** | ||
| * @brief The data buffer for the subscription manager. | ||
| */ | ||
| std::mutex mtx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the executor in NetworkBridge is single threaded, I don't think a mutex is required. The subscription callbacks and data transmission timer callbacks should all be on one thread. Maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. The data reception thread is the only place that could be out of sync, but it does not touch this buffer. Do you want me to revert it ? I did not feel comfortable with a potentially large buffer being passed as reference in a multithreaded environment, but there is indeed no risk here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think since the data is copied and passed to writer thread it is safe without mutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will do
|
Sorry for the multiple commit. I need to test on the server, so I'm committing the dev branch. |
|
This last update add the possibility to filter out some of the TF to avoid cluttering the link with unused TF. |
|
Looks good beside the mutex! This is awesome. Thank you! |
define if the data should be considered valid.
brow1633
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge!
|
Thanks. As a note, my test setup is a turtlebot controlled by a jetson nano I can reach through a two-hop SSH connection. So this validates the deployment on an ARM architecture as well. |
This PR addresses partially #6
This PR adds a specific management of TF messages to ensure that no message is lost, even if the messages are sent at a lower rate. The specialization works by accumulating TF frames and sending all of them on the network. The remaining of #6 can probably be implemented in the specialized subscription manager if needed.
In addition, the PR fixes some bugs that were still leading to segfault/severe warning on exit, and a memory leak on the tcp queue management. It also cleans the Queue class to remove some of the useless code.
Tested on TCP over a SSH tunnel.