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.

Sign up for GitHub

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

Jump to bottom

Fix clock thread issue (#1266) (#1267) #1685

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Fix clock thread issue (#1266) (#1267) #1685

merged 1 commit into from
Jun 8, 2021

Conversation

fujitatomoya
Copy link
Collaborator

  • lock before rcl_set_ros_time_override

Signed-off-by: Daisuke Sato daisukes@cmu.edu

@daisukes @fujitatomoya
Fix clock thread issue (ros2#1266) (ros2#1267)
1b75836 
* lock before rcl_set_ros_time_override

Signed-off-by: Daisuke Sato <daisukes@cmu.edu>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@KavenYau @clalancette friendly ping.

@fujitatomoya fujitatomoya mentioned this pull request Jun 2, 2021
Merged
clalancette
clalancette approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

From an API/ABI standpoint, this seems reasonable to me. We can't be breaking API, since both enable_ros_time and disable_ros_time were private static methods. And we aren't changing ABI, since we aren't changing the size of the TimeSource object. Finally, the removal of two methods from the symbol table could be considered an ABI break, but since they were private there was no way for external callers to get at them without grubbing around in the ELF sections. So this all looks good to me.

I'd like another opinion though, just to be sure my reasoning is sound. @hidmic, would you mind taking a look? Also pinging @jacobperron .

@clalancette
Copy link
Contributor

(oh, and we'll obviously need CI for this)

@ivanpauno
Copy link
Member

since both enable_ros_time and disable_ros_time

We could actually keep both methods if we wanted, but deleting them sounds ok to me.

@ivanpauno
Copy link
Member

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

The CI needs to be run against Foxy ros2.repos here :).

@ivanpauno
Copy link
Member

The CI needs to be run against Foxy ros2.repos here :).

Ups, trying again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

Third time's the charm:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

libyaml_vendor failed in both linux jobs, though it has already passed in macOS.
It's surely not related to this PR, but I have no idea of what's the issue.

@jacobperron
Copy link
Member

@ivanpauno The Ubuntu distro was set to Bionic, but it should be Focal.

@jacobperron jacobperron added this to Proposed in Foxy Patch Release 5 via automation Jun 4, 2021
@jacobperron jacobperron moved this from Proposed to Needs release in Foxy Patch Release 5 Jun 4, 2021
@ivanpauno
Copy link
Member

@ivanpauno The Ubuntu distro was set to Bionic, but it should be Focal.

🤦‍♂️, danke!

  • Linux Build Status
  • Linux-aarch64 Build Status

@fujitatomoya
Copy link
Collaborator Author

@ivanpauno thanks for taking care of CI 👍

@jacobperron requesting final review, when you got time.

jacobperron
jacobperron approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@ivanpauno ivanpauno merged commit 1fff1b7 into ros2:foxy Jun 8, 2021
@jacobperron jacobperron moved this from Needs release to Released in Foxy Patch Release 5 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@jacobperron jacobperron jacobperron approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Foxy Patch Release 5
Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fujitatomoya @clalancette @ivanpauno @jacobperron @daisukes