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

Start ROSCore for launch debugging, if not running (ROS1) #429

Merged
merged 8 commits into from
Jun 7, 2021

Conversation

RyanDraves
Copy link

Resolving #282 for ROS1, I chose option 2 for what was discussed with how to implement this feature. Since launch debugging needs to set rosparams, I found it more suitable to active the core at the beginning then monitor for it to become active. The core that is started will close when the VSCode Window is closed, same as having run the ROS: Start Core command manually. As I'm not familiar with ROS2, I set this feature to only work for ROS1.

Using the method I described in #330, I tested this implementation by using launch debugging on some rostests. Test cases includ asserting the values of rosparams, so I can verify that they are properly set.

@RyanDraves RyanDraves requested a review from a team as a code owner April 18, 2021 03:31
@ghost
Copy link

ghost commented Apr 18, 2021

CLA assistant check
All CLA requirements met.

@ooeygui
Copy link
Member

ooeygui commented Apr 18, 2021

Thank you!

@ooeygui
Copy link
Member

ooeygui commented Apr 18, 2021

@RyanDraves I'd love to chat with you about your contributions. If you are interested, could you connect with me on linked in? https://www.linkedin.com/in/louamadio/

Copy link
Member

@ooeygui ooeygui left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. If you can add the timeout to the loop and an issue for the todo, I'll be happy to pull that. (If you aren't able to, let me know, I'll pull it and add those)

Thanks!


// Wait for the core to start
while (!(await rosApi.getCoreStatus()).valueOf()) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add a maximum timeout - Wouldn't want to cause issues with vscode if roscore fails for some other reason.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I wasn't sure on the exact timeout to set, so I settled for 3 seconds. From my local testing, roscore seems to activate at about the 700 ms mark, but I figured it could be slower if you were running this on a Raspberry Pi or something. The error message should help users if a super slow machine can't start roscore in 3 seconds.

break;
}
case "2": {
// TODO, support starting the ROS2 daemon automatically
Copy link
Member

Choose a reason for hiding this comment

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

With a Todo comment, could you link it to a github issue so we don't forget to add this?

Copy link
Author

Choose a reason for hiding this comment

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

I created #431 for the issue. I wasn't sure on the exact format you wanted to link it, so I went with // TODO(#431) on both relevant comments if that's alright.

@ooeygui ooeygui merged commit ef4cbe3 into ms-iot:master Jun 7, 2021
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