Calzim/only example cleanup #39

CalZim · 2020-03-12T16:08:25Z

No description provided.

fixed broken plots and naming conventions for all basic examples. Also fixed teacg repeat from arms examples. All examples should now run and plot data correctly.

mobile_io now has comments

fixed all but mobile control examples, mobile control only breaks after a pause and restart.

mobile device control is completely functional with no lock on rotation of the ee

removed dead code

created mobile_io_control example and fixed bugs in mobile_io

fixed up arm kit examples

fixed naming conventions and corrected impedence control setpoint

also added correct example log file

rLinks234 · 2020-03-15T16:13:47Z

basic/02c_feedback_loading_log.py


 # Plot using some handy helper functions
 hebi.util.plot_logs(log, 'position', figure_spec=101)
+
+log = hebi.util.load_log('{}/{}'.format(folder, name))
 hebi.util.plot_logs(log, 'velocity', figure_spec=102)


You can call plot_logs with a string object too, so something like:

hebi.util.plot_logs('{}/{}'.format(folder, name), 'velocity', figure_spec=102)

but this is sufficient for now.

rLinks234 · 2020-03-15T16:15:50Z

basic/03a_command_position.py

@@ -29,7 +29,7 @@
 freq    = freq_hz * 2.0 * pi  # [rad / sec]
 amp     = pi * 0.25           # [rad] (45 degrees)

-duration = 10.0               # [sec]
+duration = 4              # [sec]


@iamtesch @CalZim Does anyone know why this changed to 4 seconds? It appears to be 10 seconds in the matlab example

Not sure -- I think this might have just been a testing change. I'd prefer to keep it matching the MATLAB example for consistency.

rLinks234 · 2020-03-15T16:16:37Z

basic/03b_command_velocity.py

@@ -29,7 +29,7 @@
 freq    = freq_hz * 2.0 * pi  # [rad / sec]
 amp     = 1.0                 # [rad] (45 degrees)

-duration = 10.0               # [sec]
+duration = 4              # [sec]


To avoid spamming this PR with duplicate comments, see previous comment corresponding to 10->4 seconds in the examples... Not a big deal, just wondering why it was done

rLinks234 · 2020-03-15T16:17:51Z

basic/03d_command_pos_vel.py

@@ -47,5 +48,30 @@

 # Stop logging. `log_file` contains the contents of the file
 log_file = group.stop_log()
-hebi.util.plot_logs(log_file, 'position', figure_spec=101)


There was a bug in the Python API, which I believe has since been fixed, related to this (first plot_logs would work, but second would not, since the log file had been iterated through)... If it's fixed, we can revert the changes on the RHS here back to what it was previously.

👍 this would be great to test and revert if possible :)

rLinks234 · 2020-03-15T16:19:02Z

basic/07b_robot_6_dof_arm.py

@@ -109,7 +111,13 @@ def execute_trajectory(group, model, trajectory, feedback):
 group.get_next_feedback(reuse_fbk=feedback)
 waypoints[:, 0] = feedback.position
 waypoints[:, 1] = joint_targets[:, 0]
+print("joint targets")


These print statements are extraneous and should be removed for the public facing examples.

rLinks234 · 2020-03-15T16:23:07Z

basic/mobile_io.py

+    current_button_state = [0, 0, 0, 0, 0, 0, 0, 0]
+    current_slider_state = [0, 0, 0, 0, 0, 0, 0, 0]
+
+    btn_states = ["", "", "", "", "", "", "", ""]


These variables are class fields, not instance fields (i.e., they are static variables). This means that multiple instances of MobileIO will refer to a single instance of these variables, which I assume is not the intention here.

Move these definitions into __init__ to make them instance variables

rLinks234 · 2020-03-15T16:26:25Z

basic/mobile_io.py

+        return (self.current_button_state, self.current_slider_state)
+
+    # returns the change in button positions (currently breaks if called in same step as getState())
+    def getDiff(self): 


Generally, we have snake_case naming convention for our Python stuff (that is what is generally used in Python)... I'm not really opposed to camelCase (I prefer camel tbh), but it goes against what we generally use.

Yeah, as much as I dislike snake_case, I agree we should keep things consistent. I have a feeling this current name was influenced by porting this from C++.

rLinks234 · 2020-03-15T16:27:32Z

basic/mobile_io.py

+    # send a buzz to the mobile device (only works on iphones)
+    def sendVibrate(self):
+        self.cmd.effort = 1
+        return self.grp.send_command_with_acknowledgement(self.cmd)


@iamtesch , should we do send_command_with_ack here, or just send_command ?

Hmmm...I would probably stick with send_command, unless we are actually doing something with the feedback.

@CalZim , the biggest difference here is that under the covers, send_command_with_ack sends a message and then waits for a response. This means that this call is blocking, and if the return packet is dropped will block until the timeout is reached.

sendVibrate is something that is more likely to happen in a control loop as feedback, so the "fire and forget" style of send_command is probably more appropriate.

For functions such as setSnap which configure the interface as a "one time" action, these are much more appropriate to be done with a send_command_with_acknowledgement and have wrapping code in the app to check for success.

rLinks234 · 2020-03-15T16:28:29Z

basic/mobile_io/2a_feedback.py

+sleep(2.0)
+
+family_name = "HEBI" 
+module_name = "Cal's iPhone"


Make sure to change this back to what other examples are using (for consistency)

rLinks234 · 2020-03-15T16:32:15Z

basic/mobile_io/2c_feedback_orientation.py

+  orient = fbk[0].ar_orientation
+
+  r = R.from_quat(orient)
+  rot_mat = r.as_matrix()


We have a function (util.math_utils.quat2rot) to do this, and I'd prefer not to bring in a dependency like scipy (I forget if it's a different package than numpy or what...). However... it's not imported here (you need to add a directory to the PATH for Python to find it), so this warrants a discussion on the best way to resolve this dependency.

@CalZim -- I agree we should use our existing library functions where possible.
@rLinks234 -- I assume you mean the import is for the scipy/numpy, or is that for ours?

Ah -- I see your comment elsewhere, @rLinks234 . Yeah, I'm not a huge fan of having to alter the path; it there any better way to handle this?

rLinks234 · 2020-03-15T16:33:17Z

basic/mobile_io/2d_feedback_full_pose.py

+  ])
+
+  plt.clf()
+  ax = plt.axes(projection="3d")


I'd assume this is potentially an expensive call which should be done once (outside of the loop), as opposed to each iteration

rLinks234 · 2020-03-15T16:35:38Z

kits/arm/ex_mobile_device_control.py

+  print("state: " + str(state))
+  counter += 1
+  if counter % 100 == 0:
+      print("counter: " + str(counter))


@iamtesch , should we leave all/some of these print statements in?

I don't think so -- I think the counter was part of a debugging process? We should clean up the temporary/debug code where possible

I don't think so...

rLinks234 · 2020-03-15T16:37:39Z

kits/arm/ex_mobile_device_control.py

@@ -111,106 +116,117 @@ def get_ik(xyz_target, rot_target, ik_seed):
  # Start background logging
  if enable_logging:
    group.start_log(os.path.join(local_dir, 'logs'), mkdirs=True)
-    phone_group.start_log(os.path.join(local_dir, 'logs'), mkdirs=True)
+    #phone_group.start_log(os.path.join(local_dir, 'logs'), mkdirs=True)


Do we not want to log mobile IO feedback? Just wondering. If we don't (as implied by this line being commented out), then we should delete this commented out line.

rLinks234 · 2020-03-15T16:38:44Z

kits/arm/ex_mobile_device_control.py

-
+  first_run = True
+
+  print("check 2")


I'm assuming these check X print statements were for debugging purposes... We should remove them before merging this in

rLinks234 · 2020-03-15T16:39:31Z

kits/arm/ex_mobile_device_control.py

    # Send to robot
-    group.send_command(cmd)
+    #group.send_command(cmd)


This looks like it should be uncommented, right?

rLinks234 · 2020-03-15T16:40:02Z

kits/arm/ex_mobile_device_control_v2.py

+# Add the root folder of the repository to the search path for modules
+root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..'))
+sys.path = [root_path] + sys.path
+# ------------------------------------------------------------------------------


This is the hack necessary to add in the util package for stuff like the quat2rot functions.

rLinks234 · 2020-03-15T16:40:25Z

kits/arm/ex_mobile_device_control_v2.py

+
+# Mobile Device Setup
+phone_family = 'HEBI'
+phone_name = "Cal's iPhone"


Remember to change this back

rLinks234 · 2020-03-15T16:41:52Z

kits/arm/ex_mobile_device_control_v4.py

@@ -0,0 +1,320 @@
+import hebi


What is up with the ex_mobile_device_control_vX.py scheme here? It sounds like these were iterations to get the example working... If this is the case, we need to delete all but the one we want to keep

rLinks234

This looks great so far, but there are a few things which we need to change before we get this merged in. Thanks a lot for cleaning up my disastrous mess.

rLinks234 · 2020-03-15T16:46:24Z

So far, these changes are pertinent to get this merged in:

Remove extraneous print statements
Be consistent with the family, name values (e.g. Cal's phone --> Mobile IO or whatever)
Remove 3 of the 4 "iterations" of the mobile device example
Remove the hebilog files (These should not be in the repo). We can make sure to avoid any future .hebilog files creeping in to commits by adding a rule to the .gitignore
mobile_io.py file is in the examples. This should live somewhere else. Probably under util/input

cwbollinger

Some name changes to match convention, looks good overall!

cwbollinger · 2020-03-16T18:28:58Z

basic/01c_get_group_multiple_modules.py

+family_name = "Test Family" 
+module_names = ["J3_elbow", "J2_shoulder", "J1_base"]


Suggested change

family_name = "Test Family"

module_names = ["J3_elbow", "J2_shoulder", "J1_base"]

family_name = "Test Family"

Is there a reason the actuators in this list are backwards of normal convention? Typically we would order this [base, shoulder, elbow]

cwbollinger · 2020-03-16T18:30:35Z

basic/02b_feedback_w_logging.py


-family_name = "Test Family"
+family_name = "Test Family" 


trailing whitespace

cwbollinger · 2020-03-16T18:32:58Z

basic/03a_command_position.py


-family_name = "Test Family"
+family_name = "Test Family" 


Trailing whitespace

cwbollinger · 2020-03-16T18:33:30Z

basic/03b_command_velocity.py


-family_name = "Test Family"
+family_name = "Test Family" 


Trailing whitespace

cwbollinger · 2020-03-16T18:33:44Z

basic/03d_command_pos_vel.py


 lookup = hebi.Lookup()

 # Wait 2 seconds for the module list to populate
 sleep(2.0)

-family_name = "Test Family"
+family_name = "Test Family" 


Trailing whitespace

cwbollinger · 2020-03-16T18:55:19Z

kits/arm/ex_mobile_device_control.py

 enable_effort_comp = True

 # Mobile Device Setup
 phone_family = 'HEBI'
-phone_name = 'Mobile IO'
+phone_name = "Cal's iPhone"


Set to default

cwbollinger · 2020-03-16T18:55:46Z

kits/arm/ex_mobile_device_control.py

-arm_name = '6-DoF + gripper'
-arm_family = 'Arm'
+arm_name = '6-DoF'
+arm_family = 'Example Arm'


Shouldn't this still be "Arm"?

cwbollinger · 2020-03-16T19:08:19Z

kits/arm/ex_mobile_device_control_v3.py

+
+# Mobile device setup
+phone_family = 'HEBI'
+phone_name = "Cal's iPhone"


cwbollinger · 2020-03-16T19:10:52Z

kits/arm/mobile_io_control.py

+
+# Mobile device setup
+phone_family = 'HEBI'
+phone_name = "Phone Name"


cwbollinger · 2020-03-16T19:14:49Z

kits/arm/teach_repeat.py

+  print_and_cr("When in playback mode, 'b6' resumes training, and 'b1' quits.")
+
+
+  m = mbio.MobileIO("HEBI", "Phone Name")


cwbollinger · 2020-03-16T19:16:17Z

Also, do we want to have logfiles included in the examples repo? It doesn't bother me either way, but I'm not sure what they represent since their names are timestamps. Maybe rename them if they are for specific examples?

iamtesch · 2020-03-16T21:27:34Z

kits/arm/ex_mobile_device_control.py

  t = 0
-
+  #t = 0
+  print("check 1")


More debug printout and commented variables here and below...

iamtesch · 2020-03-16T21:30:48Z

Also, do we want to have logfiles included in the examples repo? It doesn't bother me either way, but I'm not sure what they represent since their names are timestamps. Maybe rename them if they are for specific examples?

Unless the logfiles are part of an example in loading logfiles, we shouldn't have these in general

alexhongs added 11 commits February 14, 2020 14:49

cleaned up examples

db782fd

fixed broken plots and naming conventions for all basic examples. Also fixed teacg repeat from arms examples. All examples should now run and plot data correctly.

Added comments to mobile_io

f2e5e10

mobile_io now has comments

more mobile_io comments

df631a9

mostly fixed arm kit examples

ca64621

fixed all but mobile control examples, mobile control only breaks after a pause and restart.

Fixed mobile device control

4f57c97

mobile device control is completely functional with no lock on rotation of the ee

cleaned up mobile device control

8a6b1f8

removed dead code

created mobile_io_control example

c0f8f0e

created mobile_io_control example and fixed bugs in mobile_io

Fixed arm kit examples

05c9189

fixed up arm kit examples

fixed naming conventions

3db2f0e

fixed naming conventions and corrected impedence control setpoint

implemented multithreading for mobile_device_control_v4

79a344c

also added correct example log file

created io_board and mobile io examples to match cpp and python

3e51e4a

rLinks234 self-requested a review March 15, 2020 16:11

rLinks234 reviewed Mar 15, 2020

View reviewed changes

rLinks234 suggested changes Mar 15, 2020

View reviewed changes

rLinks234 mentioned this pull request Mar 15, 2020

Calzim/arm api python #40

Closed

cwbollinger requested changes Mar 16, 2020

View reviewed changes

iamtesch reviewed Mar 16, 2020

View reviewed changes

cwbollinger closed this Sep 18, 2024

cwbollinger deleted the calzim/only_example_cleanup branch November 6, 2024 18:44

		family_name = "Test Family"
		module_names = ["J3_elbow", "J2_shoulder", "J1_base"]

		print_and_cr("When in playback mode, 'b6' resumes training, and 'b1' quits.")


		m = mbio.MobileIO("HEBI", "Phone Name")

Calzim/only example cleanup #39

Calzim/only example cleanup #39

Conversation

CalZim commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamtesch Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rLinks234 left a comment

Choose a reason for hiding this comment

rLinks234 commented Mar 15, 2020

cwbollinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwbollinger commented Mar 16, 2020

iamtesch Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

iamtesch commented Mar 16, 2020

iamtesch Mar 16, 2020 •

edited

Loading

iamtesch Mar 16, 2020 •

edited

Loading