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

Add bebop ip address as ROS parameter #19

Closed
wants to merge 2 commits into
base: indigo-devel
from

Conversation

Projects
None yet
3 participants
@anuppari
Contributor

anuppari commented Nov 18, 2015

I added the IP address of the bebop as a ROS parameter. This makes it easier to run when the bebop is connected as a client to a separate AP, e.g., for running multiple bebop simultaneously.

@@ -59,7 +59,6 @@ int BebopPrintToROSLogCB(eARSAL_PRINT_LEVEL level, const char *tag, const char *
} // namespace util
BebopDriverNodelet::BebopDriverNodelet()
: bebop_ptr_(new bebop_driver::Bebop(util::BebopPrintToROSLogCB))

This comment has been minimized.

@mani-monaj

mani-monaj Nov 18, 2015

Member

This is a bit dangerous, since now there is no guarantee that this pointer is not NULL and the object is initialized when for example a callback is called when onInit() method is not finished. I am not sure if that is actually a case when using nodelets, however I like the safety that this early allocation provides.

I suggest that we leave the original constructor untouched and pass/save bebop_ip when calling Connect().

@mani-monaj

mani-monaj Nov 18, 2015

Member

This is a bit dangerous, since now there is no guarantee that this pointer is not NULL and the object is initialized when for example a callback is called when onInit() method is not finished. I am not sure if that is actually a case when using nodelets, however I like the safety that this early allocation provides.

I suggest that we leave the original constructor untouched and pass/save bebop_ip when calling Connect().

@mani-monaj

This comment has been minimized.

Show comment
Hide comment
@mani-monaj

mani-monaj Nov 18, 2015

Member

Thank you for the PR, please see my inline comments. Specifically this one.

Member

mani-monaj commented Nov 18, 2015

Thank you for the PR, please see my inline comments. Specifically this one.

Change initialization of bebop ip
- Reset bebop initialization based on https://github.com/AutonomyLab/bebop_autonomy/pull/19/files#r45271943
- Changed bebop parameter variable name
- Set default ip in launch files
@anuppari

This comment has been minimized.

Show comment
Hide comment
@anuppari

anuppari Nov 18, 2015

Contributor

OK I made those changes you recommended. Do I need to make a new PR?

Contributor

anuppari commented Nov 18, 2015

OK I made those changes you recommended. Do I need to make a new PR?

@mani-monaj

This comment has been minimized.

Show comment
Hide comment
@mani-monaj

mani-monaj Nov 19, 2015

Member

@anuppari Great. Thank you. It looks good to me. It would be nice if you can squash your commits into one and update the PR. I will merge this quite soon.

Member

mani-monaj commented Nov 19, 2015

@anuppari Great. Thank you. It looks good to me. It would be nice if you can squash your commits into one and update the PR. I will merge this quite soon.

@anuppari

This comment has been minimized.

Show comment
Hide comment
@anuppari

anuppari Nov 30, 2015

Contributor

Sorry it took a while, but I made a new PR with the squashed commits

Contributor

anuppari commented Nov 30, 2015

Sorry it took a while, but I made a new PR with the squashed commits

@mani-monaj

This comment has been minimized.

Show comment
Hide comment
@mani-monaj

mani-monaj Nov 30, 2015

Member

No worries, I've already squashed and merged it.

Member

mani-monaj commented Nov 30, 2015

No worries, I've already squashed and merged it.

@davidvar

This comment has been minimized.

Show comment
Hide comment
@davidvar

davidvar Dec 29, 2017

can anyone confirm that this change in the code works well?

davidvar commented Dec 29, 2017

can anyone confirm that this change in the code works well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment