-
Notifications
You must be signed in to change notification settings - Fork 80
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
Refactor reverse interface #70
Refactor reverse interface #70
Conversation
|
||
void ScriptSender::connectionCallback(const int filedescriptor) | ||
{ | ||
URCL_LOG_DEBUG("New client connected at FD %d.", filedescriptor); |
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.
URCL_LOG_DEBUG("New client connected at FD %d.", filedescriptor); | |
URCL_LOG_DEBUG("New client connected at file descriptor %d.", filedescriptor); |
Is FD sufficiently known as a term? If yes, then it's ok.
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.
It's a debug output anyway...
|
||
void ScriptSender::disconnectionCallback(const int filedescriptor) | ||
{ | ||
URCL_LOG_DEBUG("Client at FD %d disconnected.", filedescriptor); |
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.
URCL_LOG_DEBUG("Client at FD %d disconnected.", filedescriptor); | |
URCL_LOG_DEBUG("Client at file descriptor %d disconnected.", filedescriptor); |
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 read over this and it looks fine.
It is not really plain communication-related but rather we implemented a control protocol here. In my opinion it makes sense to move this into a new "module", as this will be extended with the changes regarding Cartesian control and trajectory forwarding.
As this is used by external applications such as the ROS driver, I think it makes sense to keep the namespace for backwards compatibility. It also seems a logical choice, as this is part of the actual protocol definition.
7348ffe
to
0d70c8a
Compare
This moves the ReverseInteface and the ScriptSender from the
comm
namespace to thecontrol
namespace. As we plan to extend the control protocol structure with the changes from the beta-testing branch, I think it makes sense to clean up things a little beforehand.Note: I decided to keep the
ControlMode
enums inside thecomm
namespace first because of backwards compatibility, as they are used on application level, and second because they actually define the communication protocol and therefore seem to be not too wrong inside the comm namespace.