-
Notifications
You must be signed in to change notification settings - Fork 46
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
Replanning trigger for the navigation stack. #144
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary comments about the structure
src/WalkingModule/include/WalkingControllers/WalkingModule/Module.h
Outdated
Show resolved
Hide resolved
src/WalkingModule/app/robots/ergoCubGazeboV1/dcm_walking_with_joypad.ini
Outdated
Show resolved
Hide resolved
I have removed more code possible from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @SimoneMic, it is much clearer now. I have some more punctual comments
src/WalkingModule/app/robots/ergoCubGazeboV1/dcm_walking/common/navigation.ini
Outdated
Show resolved
Hide resolved
src/WalkingModule/include/WalkingControllers/WalkingModule/Module.h
Outdated
Show resolved
Hide resolved
…n/navigation.ini Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed a potential concurrency issue.
src/WalkingModule/app/robots/ergoCubGazeboV1/dcm_walking/common/navigation.ini
Outdated
Show resolved
Hide resolved
src/NavigationHelper/include/WalkingControllers/NavigationHelper/NavigationHelper.h
Outdated
Show resolved
Hide resolved
src/NavigationHelper/include/WalkingControllers/NavigationHelper/NavigationHelper.h
Outdated
Show resolved
Hide resolved
src/NavigationHelper/include/WalkingControllers/NavigationHelper/NavigationHelper.h
Outdated
Show resolved
Hide resolved
src/NavigationHelper/include/WalkingControllers/NavigationHelper/NavigationHelper.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final minor comments, but overall the code looks good to me. @GiulioRomualdi how do you want to proceed?
/** | ||
* Close the Navigation Helper Threads and ports | ||
* @return true/false in case of success/failure. | ||
*/ | ||
bool closeThreads(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably also this can be private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and I would also add in the destructor (the same is valid for closeHelper
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved it to private and added/fixed some comments/descriptions.
The destructor should never be called inside closeThreads()
, since the ports could still be open.
In closeHelper()
I don't see the reason to call the destructor inside a class method, since we are not dealing with special manual memory management. I would avoid that and let the owner handle it.
src/WalkingModule/include/WalkingControllers/WalkingModule/Module.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
On this PR a parallel thread monitors the status of the feet of the robot.
Once each double support we will stream a boolean value on a port that will be red by the navigation stack. This value is the command to replan the navigation path.
Part of the changes required for the integration with the navigation stack #142 and its PR #143