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

Create Static Transform Broadcaster and publish flange->tool0 to newly created /tf_static topic #322

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jimmy-mcelwain
Copy link
Collaborator

@jimmy-mcelwain jimmy-mcelwain commented Oct 22, 2024

In reference to #20

@jimmy-mcelwain jimmy-mcelwain marked this pull request as draft October 22, 2024 19:27
@jimmy-mcelwain
Copy link
Collaborator Author

This successfully publishes flange->tool0 to tf_static, but I need to do more testing and probably clean it up

@gavanderhoorn
Copy link
Collaborator

Nice work.

A high-level comment and a question:

  • would there be a way to implement this without adding more static variables? Some of the added functions don't take any arguments but just edit static variables. I understand why you did that, but I'd suggest trying to move away from that pattern, as it facilitates testing (can just pass in 'test values') and reduces potential for problems.
  • is the bool check has-this-been-sent-yet really needed? I was hoping the static transform broadcaster would be smart enough to not broadcast transforms it's already seen, or we can just call send(..) once outside of the loop, right after the agent connected. It would be OK to republish in case of a dropped connection / a reconnect. tf_static is for 'never' changing transforms -- or things that change very infrequently, so republishing is acceptable.

@jimmy-mcelwain
Copy link
Collaborator Author

I will look at those suggestions. I added static variables because that is how it is done with the other PositionMonitor messages and publishers. I can probably switch off of static variables. As for the check has-this-been-sent-yet, I can probably find a better way.

@jimmy-mcelwain
Copy link
Collaborator Author

jimmy-mcelwain commented Jan 15, 2025

I was hoping the static transform broadcaster would be smart enough to not broadcast transforms it's already seen

I am actually doing the same thing that is done in the rclcpp implementation. If you broadcast a transform that it has already seen, it will replace the contents of that old transform (the same way that it does in geometry2). If they are the same, this will basically just update the timestamp. But just like the other implementation, it will publish even if the only thing that changed is the timestamp.

or we can just call send(..) once outside of the loop, right after the agent connected. It would be OK to republish in case of a dropped connection / a reconnect.

I was originally planning on having the send(...) outside of the loop. My only problem is that I wanted to make sure that it sent successfully. It seems important that it gets sent if it is outside of the loop, since we would only attempt to send it once. I did not want to alarm/end the application if it does not send successfully, so I figured I would put it inside of the loop so it could keep on trying until it can be verified that it did publish successfully. But I didn't want to actually repeatedly publish (to save CPU cycles and network traffic), so I decided to have that bool to verify that it published.

We could:

  1. Move it outside of the loop and alarm/end app if it fails to publish
  2. Move it outside of the loop and don't do anything if it fails to publish
  3. Move it outside of the loop and alarm if it fails to publish, but deallocate and then reinitialize/go through the main loop again
  4. Leave it as is
  5. Leave it in the loop but let it publish every iteration

Or do something else.

would there be a way to implement this without adding more static variables?

I've pushed a new update up that does this (and 2. from the previous list, which is not my preference). It is sort of hastily made/more of a proof of concept to compare to the first version. Regardless I will need to test more and double check/verify that there aren't any new memory leaks. I don't particularly like it. It is different from all of our other publishers/messages/data, it is cumbersome to pass everything the data, publishers, etc around, and it is different from how rclcpp handles it. I think that in a lot of ways, the first commit is better, but let me know what you think @gavanderhoorn.

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

Successfully merging this pull request may close these issues.

2 participants