Shaun teleop - basic teleop control + estop and zero_state buttons#153
Shaun teleop - basic teleop control + estop and zero_state buttons#153
Conversation
imzaynb
left a comment
imzaynb
left a comment
There was a problem hiding this comment.
Look at what I have so far, and then when you push that, I will look at the testing stuff.
Good work!
There was a problem hiding this comment.
If possible, remove this file from your commit. The tagged line is supposed to be commented out unless you are on Mac. Thanks!
There was a problem hiding this comment.
What's the purpose of this file? From what I can see there is only one source file which is joystick_teleop_continuous.py.
The file is attempting to launch a non-existant node joystick_teleop. The only node that exists currently is joystick_teleop_continuous. These nodes are registered with ROS2 in the setup.py.
Maybe remove this file?
| Button class just wraps button state logic | ||
| """ | ||
|
|
||
| KNOWN_ACTIONS = {"estop", "zero_state"} |
There was a problem hiding this comment.
I would use an enumeration here.
| self._is_pressed = is_pressed | ||
|
|
||
| @property | ||
| def just_pressed(self) -> bool: |
There was a problem hiding this comment.
Does this suffice for debouncing? I'm curious.
| raise ValueError(f"Unknown button action '{action}'. Known: {KNOWN_ACTIONS}") | ||
| self.idx = idx | ||
| self.name = action | ||
| self._is_pressed = False |
There was a problem hiding this comment.
While I like the _ prefix for private variables a lot, we currently don't differentiate between public and private variables across the codebase. As a result, to maintain cohesion with the rest of the codebase, I would remove the _.
| def __init__(self) -> None: | ||
| super().__init__("joystick_teleop_continuous") | ||
|
|
||
| params_path = self.declare_parameter( |
There was a problem hiding this comment.
You don't need to load parameters in this way.
ROS will handle loading in the yaml file under the folder mrobosub_teleop/params/<file>.yaml provided you specify this file in the launch file. I explained this within the launch file above.
Additionally, you will want to change the format of your file to be inline with the way that ROS expects to load in parameters from your files. A good example of this is mrobosub_hal/params/thruster_controller_params.yaml
From there, you can use Muskaan's parameter loading to handle loading in the parameters.
Step 1: List out all parameters
import Param from mrobosub_lib
Then create a list of parameters, perhaps called params, based off of the parameters in your param file.
params = [Param("deadzone", Parameter.Type.INTEGER, "The deadzone of the controller"), <more parameters here>]I think they should work with the nested parameters that you provided with the dictionary. Test it out if you can. If this doesn't work, I am sure we can make our parameter abstraction more robust to handle it.
Step 2: Declare parameters
After you create the list of parameters, you need to declare them with the node, like such:
self.declare_params(params)Step 3: Use the params
The parameters will now be available under self.<param> for instance, self.deadzone, etc. And these parameters will automatically refresh if they are changed from, for instance, RQT.
| is_pressed = len(msg.buttons) > button.idx and msg.buttons[button.idx] == 1 | ||
| button.update(is_pressed) | ||
|
|
||
| # might move this to a function map if it needs to scale better |
There was a problem hiding this comment.
I like this idea. Do it if you can.
There was a problem hiding this comment.
Also, I strongly recommend using Enum here.
There was a problem hiding this comment.
Overall, the file looks really solid, good job. Please consider the changes I suggest if you can.
There was a problem hiding this comment.
See if you can change the formatting of this file to use the ros params way instead.
There was a problem hiding this comment.
In the past we haven't created the type of testing that you employ here. I will provide more thoughts on my next pass through the code. I think its a good idea though and am interested to try it out myself.
2 participants
No description provided.