Feedback PR 1 - #1
Conversation
Espeer5
left a comment
Espeer5
left a comment
There was a problem hiding this comment.
Good work y'all, it's looking really good. I would add a few more docstrings and comments across the codebase in general for readability, but I understand it's not easy to spend time on that when trying to get demos up and running. You may find it helpful to do it in the project though, since you will have to go back over all your code many times across multiple weeks.
| <package format="3"> | ||
| <name>project</name> | ||
| <version>0.0.0</version> | ||
| <description>TODO: Package description</description> |
There was a problem hiding this comment.
Try to fill in these TODOs as a best practice.
| from hw6sols.KinematicChainSol import KinematicChain | ||
|
|
||
|
|
||
| CYCLE = 3 * pi / 2 |
There was a problem hiding this comment.
I like pulling things out into constants like this. One suggestion I have — for us last year, we ended up defining a lot of these kinds of constants that we could use to fine-tune the behavior of our system. We found it super useful to separate the constants out into a global constants.py file, especially for constants that we used across multiple demos like our waiting position, the gravity compensation model values, etc. This way, if you want to fine-tune something in the system, you easily go into that file and make the change and it transfers across the whole system.
| q = q + qdelta * 0.5 | ||
| xdistance.append(np.linalg.norm(xdelta)) | ||
| qstepsize.append(np.linalg.norm(qdelta)) | ||
|
|
There was a problem hiding this comment.
Just to give you an option here, you can still use the iterative solution like what you did in the early weeks and in 133a in the brain using "sim time." This way you can still use secondary tasks, weighted psuedo-inverses, etc if you want to build some of those features into your system.
By "sim time", I mean just create a variable t that you start at 0 and slowly increment to some value while plugging it into a spline - just increment it in a while loop though, so you don't need to actually run it in real time.
This method is slower but more stable than Newton Raphson, so I encourage you to play with all the options and find what works best in your system.
| strip_world_start_msg = Point() | ||
| strip_world_end_msg = Point() | ||
|
|
||
| TAP_FACTOR = 0.04 |
There was a problem hiding this comment.
Factor out constants
|
|
||
| seg1 = Segment() | ||
| seg1.px = qT[0] | ||
| seg1.py = qT[1] |
There was a problem hiding this comment.
Rather than assigning all of these across multiple lines, you can assign these in a single line using splatting/ tuple unpacking.
|
|
||
| self.get_logger().info("Running point %r, %r, %r" % (x,y,z)) | ||
| else: | ||
| self.get_logger().info('Not in the dome. Please try again dummy...') |
There was a problem hiding this comment.
Be willing to bet my life savings that I could guess who wrote this
| now = self.get_clock().now() | ||
| self.t = (now - self.starttime).nanoseconds * 1e-9 | ||
|
|
||
| if self.mode is Mode.START_UP: |
There was a problem hiding this comment.
I recommend match statements rather than big if/then blocks for state machines.
4 participants
Hi team. I'll be using this feedback PR as a way to review your code so far for 134. I am using a draft PR for this rather than creating issues wince I don't expect/need you to fix issues that are relevant only to the 3DOF.
Let me know if you have any questions about my feedback! Once you've reviewed the feedback, feel free to "close without merging."
Note that I created this PR from a review branch into an empty "base_branch", so even if you accidentally merge it somehow, it won't create any problems.