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

[RST-10727] Publish the start/stop status of the fixed-lag smoother #375

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

svwilliams
Copy link
Contributor

@svwilliams svwilliams commented Aug 2, 2024

Now that it is possible to construct a fixed-lag smoother without immediately starting it, knowing the running state of the optimizer may be important for coordination with other nodes. Publish the start/stop state of the optimizer as a latched topic.

The status is currently being published as a std_msgs/Bool. I do wonder if I should add a dedicated message type to fuse_msgs and publish that instead? If we ever want to expand the status information in the future, having our own message type could make that easier. We would simply add new fields to the message, but we would continue to publish the same message type. Any thoughts?

@svwilliams svwilliams force-pushed the RST-10727-fix-startup-sequence branch from b3d3da6 to 517f17d Compare August 4, 2024 23:38
@svwilliams svwilliams force-pushed the RST-10727-fix-startup-sequence branch from 517f17d to b9c3c77 Compare August 5, 2024 03:27
@svwilliams svwilliams changed the title [RST-10727] Delay subscribing/advertising until plugins have started [RST-10727] Publish the start/stop status of the fixed-lag smoother Aug 5, 2024
@svwilliams svwilliams marked this pull request as ready for review August 5, 2024 04:31
Copy link

@carlos-m159 carlos-m159 left a comment

Choose a reason for hiding this comment

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

The status is currently being published as a std_msgs/Bool. I do wonder if I should add a dedicated message type to fuse_msgs and publish that instead?

My initial thoughts would be to keep it with std_msgs/Bool and change it if we ever require additional status info. For example, nodes that are toggled using std_msgs/Bool or are waiting on some signal to start up or stop: injecting a fuse_msgs dependency might be strange. But I am also fine with creating a custom message, and, for example, publish that continuously and not only at start and stop.

Comment on lines 123 to 125
auto status = std_msgs::Bool();
status.data = false;
status_publisher_.publish(status);

Choose a reason for hiding this comment

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

Maybe move this to a function?
If we at any point store the state in a member, might will be easier to maintain.

inline void notifyStatus(const bool running)
{
  // Update status topic
  auto status = std_msgs::Bool();
  status.data = running;
  status_publisher_.publish(status);
}

@svwilliams svwilliams force-pushed the RST-10727-fix-startup-sequence branch from 734ad64 to a18d68b Compare August 7, 2024 21:21
@svwilliams svwilliams merged commit 75f83cf into devel Aug 7, 2024
4 checks passed
@svwilliams svwilliams deleted the RST-10727-fix-startup-sequence branch August 7, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants