Skip to content

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

Merged
ivanpauno merged 1 commit intoros2:foxyfrom
fujitatomoya:foxy-backport-1267
Jun 8, 2021
Merged

Fix clock thread issue (#1266) (#1267)#1685
ivanpauno merged 1 commit intoros2:foxyfrom
fujitatomoya:foxy-backport-1267

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented Jun 2, 2021

  • 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
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 2, 2021

@KavenYau @clalancette friendly ping.

@fujitatomoya fujitatomoya mentioned this pull request Jun 2, 2021
Merged
@fujitatomoya fujitatomoya requested a review from ivanpauno June 2, 2021 23:52
clalancette
clalancette approved these changes Jun 4, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

clalancette commented Jun 4, 2021

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

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

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
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

CI:

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

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jun 4, 2021

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

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

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
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

Third time's the charm:

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

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

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
Copy Markdown
Member

jacobperron commented Jun 4, 2021

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

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Jun 4, 2021

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

🤦‍♂️, danke!

  • Linux Build Status
  • Linux-aarch64 Build Status

@ivanpauno ivanpauno requested a review from jacobperron June 4, 2021 19:40
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Jun 6, 2021

@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
Copy Markdown
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
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

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fujitatomoya @clalancette @ivanpauno @jacobperron @daisukes