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

Arkadiy/proximity warning #1

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

arkadiy-telegin
Copy link

@arkadiy-telegin arkadiy-telegin commented Dec 22, 2023

@missxa missxa self-requested a review December 27, 2023 13:49
Copy link
Contributor

@missxa missxa left a comment

Choose a reason for hiding this comment

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

@arkadiy-telegin

It's a good practice to add a description of added code in a PR. Would be great to have a summary of added features in the future. Also, for the future please work on a separate branch directly in this repo, since now I cannot directly comment on your python file in the fork.

Architecture & design:

  • as discussed, it is better to implement a node for a single camera. please change the implementation accordingly. in case more cameras are added to the robot, then launch another node using the camera name for the topic and node names (like you do already)
  • publish absolute proximity in meters, not relative_proximity. for the operator meters are much more understandable and it doesn't require knowing that current threshold is

Implementation:

  • 10 hz publisher is too low, use 40 hz or higher
  • use our established topic naming scheme, i.e. /roboy/pinky/sensing/your/stuff
  • call the threshold parameter proximity_warning_threshold (also, it can be potentially different for different cameras)
  • set default threshold to 0.2 m in the launch file
  • if exception occurs, use node logger's error and not just print

@arkadiy-telegin arkadiy-telegin changed the title [WIP] Arkadiy/proximity warning Arkadiy/proximity warning Mar 28, 2024
@arkadiy-telegin arkadiy-telegin requested a review from missxa March 28, 2024 00:29
@arkadiy-telegin
Copy link
Author

@missxa cleaned up code integrating all the requested changes

@arkadiy-telegin
Copy link
Author

@missxa did you have a chance to take a look?

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