54 emulate remote controller #63
Conversation
There was a problem hiding this comment.
Are you sure that we need this kind of node?
There was a problem hiding this comment.
This is just a fixed trajectory, right?
There was a problem hiding this comment.
That is not ideal. At this point, if we have a fixed trajectory, let's just use a pre-recorded rosbag. What do you think?
There was a problem hiding this comment.
I am sure that this code is able to complete the task, however in this review I would like you to reflect more on the stylistic / design choices.
In my opinion (as an external viewer) the implementation is quite messy (there are many unused variables and very specific lines of code which are incomprehensible at first sight). Moreover it is extremely noticeable the complete absence of documentation, that in combination with a really complex code (the reading loop has an extremely complex conditional structure) makes it quite impossible (at least to me) to understand what the code is doing, even superficially.
Please, do not take this review negatively, but as a reminder that 1) when we are writing code and it becomes complex, we have always to remember that others have not worked extensively on it and they do not know how it works 2) we should always try to keep the codebase as simple as possible (KISS pattern) (within specifications) otherwise it is very easy to forget pieces and it is a short leap before having spaghetti code
| self.defaultMessage.right.x = 0.0 | ||
| self.defaultMessage.right.y = 0.0 | ||
| self.defaultMessage.right.z = 0.0 | ||
| self.defaultMessage.left.x = 0.0 | ||
| self.defaultMessage.left.y = 0.0 | ||
| self.defaultMessage.left.z = 0.0 |
There was a problem hiding this comment.
Are you sure this is needed? (especially given your comment). Why are you inserting un-needed lines of code, do you believe that these improve readability?
There was a problem hiding this comment.
Moreover I don't think this would ever be needed (you can simply call Remote())
There was a problem hiding this comment.
That is not ideal. At this point, if we have a fixed trajectory, let's just use a pre-recorded rosbag. What do you think?
| # retrieve settings from file descriptor (stdin) | ||
| try: | ||
| sys.stdin.fileno() | ||
| boolStdin = True | ||
| settings = termios.tcgetattr(sys.stdin) | ||
| except Exception: | ||
| boolStdin = False | ||
| settings = None |
There was a problem hiding this comment.
What is this? This seems an anti-pattern. Why do you check for an unexpected exception? Is this because of the tests? In that case I would strongly suggest against modifying for worse the production code to have the tests working.
| def __init__(self): | ||
| super().__init__('emulator_remote_controller') | ||
| # create teleop_twist_keyboard node | ||
| self.emulator = rclpy.create_node('teleop_twist_keyboard') |
There was a problem hiding this comment.
Why do you create a second node here? You already have self
| # create teleop_twist_keyboard node | ||
| self.emulator = rclpy.create_node('teleop_twist_keyboard') | ||
| self.publisher = self.create_publisher(Remote, '/remote', 10) | ||
| self.get_logger().info('EmulatorRemoteController node started') |
There was a problem hiding this comment.
I believe that it is not that sensible to provide here this logging message, when some operations are yet to be done during init
| self.get_logger().info('EmulatorRemoteController node started') | ||
| # create timer to start the function | ||
| self.timer = self.create_timer(0.08, self.readLoop) | ||
| self.has_to_exit = False |
There was a problem hiding this comment.
The exit is handled using an exception, this is not used. That is not to say that the exception is preferable (it is not) but inn any case this variable is unused
| if self.handleKey(key, message): | ||
| # destroy node | ||
| self.emulator.destroy_node() | ||
| termios.tcsetattr(sys.stdin, termios.TCSADRAIN, settings) |
| def main(args=None): | ||
| rclpy.init(args=args) | ||
| try: | ||
| emulator_remote_controller = EmulatorRemoteController() | ||
| except Exception as err: | ||
| print('Error while starting EmulatorRemoteController node: ' + str(err)) | ||
| raise err | ||
| else: | ||
| rclpy.spin(emulator_remote_controller) | ||
| emulator_remote_controller.emulator.destroy_node() | ||
| emulator_remote_controller.destroy_node() | ||
| rclpy.shutdown() |
There was a problem hiding this comment.
The execution point should be uniformed to the most recent ones (which provide the correct error handling and logging)
| if (key == self.previousKey): | ||
| message.left.y = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| self.previousValue = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.left.y = -self.increment | ||
| self.previousValue = -self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'a': | ||
| # print("left") | ||
| if (key == self.previousKey): | ||
| message.left.x = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| self.previousValue = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.left.x = -self.increment | ||
| self.previousValue = -self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'd': | ||
| # print("right") | ||
| if (key == self.previousKey): | ||
| message.left.x = min(1.0, self.previousValue * (self.increment+1)) | ||
| self.previousValue = min(1.0, self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.left.x = self.increment | ||
| self.previousValue = self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'q': | ||
| # print("CCW") | ||
| if (key == self.previousKey): | ||
| message.left.z = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| self.previousValue = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.left.z = -self.increment | ||
| self.previousValue = -self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'e': | ||
| # print("CW") | ||
| if (key == self.previousKey): | ||
| message.left.z = min(1.0, self.previousValue * (self.increment+1)) | ||
| self.previousValue = min(1.0, self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.left.z = self.increment | ||
| self.previousValue = self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| # Start cmd_vel commands | ||
| elif key == 'i': | ||
| # print("forward") | ||
| if (key == self.previousKey): | ||
| message.right.y = min(1.0, self.previousValue * (self.increment+1)) | ||
| self.previousValue = min(1.0, self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.right.y = self.increment | ||
| self.previousValue = self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'k': | ||
| # print("backward") | ||
| if (key == self.previousKey): | ||
| message.right.y = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| self.previousValue = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.right.y = -self.increment | ||
| self.previousValue = -self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'l': | ||
| # print("right") | ||
| if (key == self.previousKey): | ||
| message.right.x = min(1.0, self.previousValue * (self.increment+1)) | ||
| self.previousValue = min(1.0, self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.right.x = self.increment | ||
| self.previousValue = self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| elif key == 'j': | ||
| # print("left") | ||
| if (key == self.previousKey): | ||
| message.right.x = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| self.previousValue = -min(1.0, -self.previousValue * (self.increment+1)) | ||
| else: | ||
| message.right.x = -self.increment | ||
| self.previousValue = -self.increment | ||
| self.previousKey = key | ||
| self.publisher.publish(message) | ||
| # if b pressed, make cmd_vel movements faster | ||
| elif key == 'b': | ||
| self.increment /= 2 | ||
| elif key == 'h': | ||
| self.increment *= 2 | ||
| elif key == 'z': | ||
| raise Exception('Exit requested') | ||
| else: | ||
| # pass to default | ||
| self.publisher.publish(self.defaultMessage) | ||
| return self.has_to_exit |
There was a problem hiding this comment.
I do not get the approach of this code. Why are you creating a new empty message and treating differently the case of it being the same command repeated? Isn't this equal to a simple update of the previous message?
def handle_key(self, k):
if k == '...':
self.prevousMessage. ... += inc # constained
SEND self.previous ?There was a problem hiding this comment.
As for now I am not reviewing this because I have some questions about the implementation and so I am unable to review this
No description provided.