Skip to content
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

Temp review #14

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Temp review #14

wants to merge 21 commits into from

Conversation

tarasborov
Copy link
Contributor

No description provided.

README.md Outdated
@@ -65,3 +65,57 @@ chmod a+x session.yml
```
Now tmux session will start with all required tabs. All required commandsmay be added to the history, in order to simplify their usage.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix typo error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -10,8 +16,6 @@ RUN apt-get update && apt-get install -y \
ros-melodic-joint-state-publisher-gui \
ros-melodic-interactive-marker-twist-server \
ros-melodic-robot-state-publisher \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to move this installation to package.xml


rospy.loginfo_once("Started Forward Kinematics")

for i in range(len(standard_position_seq)):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for me, in Python better to use next approach
for position in standard_position_seq:
time.sleep(2.1)
spot.talker(pose_dict[position[i]])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have done so



def myhook():
pass
rospy.loginfo_once("Finished Forward Kinematics")
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need pass here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

<!-- <arg name="world_name" default="$(find rs_gazebo)/worlds/ss_hq4.world" /> -->
<arg name="world_name" default="empty.world" /> -

<arg name="world_name" default= "$(find rs_gazebo)/worlds/$(arg world)" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify the default argument world, otherwise, the launch file will be broken if arg world is not specififed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done

<!--</attenuation>-->
<!--<direction>-0.5 0.1 -0.9</direction>-->
</light>
<model name='ground_plane'>gazebo.launch
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check this line. looks like it has errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is typo, thanks.


if __name__ == '__main__':
try:
main()
rospy.on_shutdown(myhook)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a single-threaded script, you don't need this hook as it is set only after the main function is fully executed.
It makes set to set on_shutdown hook before calling the main function. The hook is used to clear some variables, send notifications, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

@@ -24,6 +22,8 @@ def __init__(self, win, pub):
self.lbl11 = Label(win, text='ClearanceHeight')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these variables should have meaningful names, as they are used in different places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants