Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Tracker] ROS1 nodes updates and fixes #305

Closed
23 tasks done
tsampazk opened this issue Sep 13, 2022 · 3 comments · Fixed by #364
Closed
23 tasks done

[Tracker] ROS1 nodes updates and fixes #305

tsampazk opened this issue Sep 13, 2022 · 3 comments · Fixed by #364
Assignees

Comments

@tsampazk
Copy link
Collaborator

tsampazk commented Sep 13, 2022

This issue serves to track the progress on the updates on all ROS1 nodes, as per the use-case partner feedback as well as fixes regarding consistency across all nodes. Also, the delay issues mentioned in #275 should be fixed along with the other changes, as well as issue #209 relating to input topics etc.

Main issues to fix:

Below there's a list of main issues that need fixing, i tried to check out all the nodes and made some comments in parentheses on what i noticed, so use them to get a general sense on the issues:

  1. Argparse usage as seen here.
  2. Argument names and default values consistency across nodes, as seen in node class ctor and argparse. Arguments especially in argparse which is the interface the users interact with, should have descriptive names like input_rgb_image_topic and input_depth_image_topic, etc, as well as default values to run out-of-the-box if possible.
  3. Updated device selection as seen here.
  4. Preferably, if the node has multiple outputs, we should provide the option to disable them as seen here, with appropriate changes in the callback too.
  5. Make sure you update your docstrings for wrong argument names, typos, some nodes mentioning different tools, etc.
  6. Please test whether your node has delay issues as described in ROS nodes delay #275 and if you need to adjust the queue sizes.
  7. Keep in mind the feedback provided by the use-case partners.
  8. Use a lambda function for argparse type as seen here to handle passing of None values to disable topics. This only applies to topics that can be disabled like these ones. This gives the option to users to entirely disable some unused topic from publishing, to save resources.

Tracking list:

For any questions or doubts feel free to ask below!

Edit: Added 8. in the list of issues to fix.
Edit: ROS1 GEM node updates are merged into the ros2 branch, related PR #256

@negarhdr
Copy link
Collaborator

Two new branches (on top of develop) for skeleton-based_action_recognition and landmark-based_facial_expression_recognition are created which are called "har_ros1_fix", "fer_ros1_fix", respectively.
In "har_ros1_fix", the script "skeleton_based_action_recognition" is updated and in "fer_ros1_fix", the script "landmark_based_facial_expression_recognition" is updated.
Please check them out and let me know if there are still problems.

@tsampazk
Copy link
Collaborator Author

Hey @negarhdr!

I checked the changes out, they seem pretty good apart from some really minor things.

Feel free to open pull requests to merge them into the develop branch and i can comment on the minor issues on the review.

@tsampazk tsampazk changed the title [Tracker] ROS1 nodes updates and dixes [Tracker] ROS1 nodes updates and fixes Sep 14, 2022
@tsampazk
Copy link
Collaborator Author

I am not yet closing this issue as i am going to go through all the listed ROS1 nodes, make sure that nothing related to this issue was missed and apply any minor fixes that might be needed.

This was referenced Nov 28, 2022
@tsampazk tsampazk linked a pull request Nov 30, 2022 that will close this issue
@tsampazk tsampazk closed this as completed Dec 6, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants