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

[JTC] ⏰ remove state publish rate parameter and logic and publish on each update. #520

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

destogl
Copy link
Member

@destogl destogl commented Feb 8, 2023

No description provided.

Copy link
Member

@arne48 arne48 left a comment

Choose a reason for hiding this comment

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

I assume this is implementing the changes mentioned in #473.
To me the changes look good and complete.

Edit: I don't know if there is an intended pattern for this already, but maybe it could be considered to add an RCLCPP_INFO message that the parameter was removed if we still find it in the yaml file.
This could help users who don't read the docs too often (or don't remember the log output) explicitly why after an update a parameter has no effect anymore.
This could reduce the cluttering of config files in the field.

Comment on lines -954 to +930
last_state_publish_time_ = get_node()->now();
state_publisher_->msg_.header.stamp = last_state_publish_time_;
state_publisher_->msg_.header.stamp = time;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the only time when it would be useful to change the time in the header from now to a specified time would be in testing. Would it be better to just leave it as is and then create a standard test function that uses this publish state function correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still good to have in any case. We are getting time from controller manager to have it consistent. So I would keep this as it is sinc this is the control time

@destogl
Copy link
Member Author

destogl commented Feb 10, 2023

I don't know if there is an intended pattern for this already, but maybe it could be considered to add an RCLCPP_INFO message that the parameter was removed if we still find it in the yaml file.
This could help users who don't read the docs too often (or don't remember the log output) explicitly why after an update a parameter has no effect anymore.
This could reduce the cluttering of config files in the field.

We will do that for Humble. On the Rolling we just kick it out for the simplicity. People using Rolling should follow what is happening. Is this reasonable, or you think that would confuse too many people?

@arne48
Copy link
Member

arne48 commented Feb 13, 2023

I don't know if there is an intended pattern for this already, but maybe it could be considered to add an RCLCPP_INFO message that the parameter was removed if we still find it in the yaml file.
This could help users who don't read the docs too often (or don't remember the log output) explicitly why after an update a parameter has no effect anymore.
This could reduce the cluttering of config files in the field.

We will do that for Humble. On the Rolling we just kick it out for the simplicity. People using Rolling should follow what is happening. Is this reasonable, or you think that would confuse too many people?

This addresses my question pretty well. And I agree that "Rolling" users are (should be) more likely to keep track on the changelogs.
If an information will be prompted to the "normal distribution" users I think that would be totally sufficient.
Thank you @destogl.

@bmagyar bmagyar merged commit 9c53e68 into master Feb 22, 2023
@bmagyar bmagyar deleted the jtc-remove-state-publish-rate branch February 22, 2023 14:26
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Mar 7, 2023
Update, e.g. parameter changed with ros-controls#520
bmagyar pushed a commit that referenced this pull request Mar 7, 2023
* Update JTC documentation

Update, e.g. parameter changed with #520

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

Successfully merging this pull request may close these issues.

None yet

4 participants