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.

Sign up for GitHub

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

Jump to bottom

Reorganize and Rename to have multiple autonomy algs #107

Merged
merged 20 commits into from
Nov 19, 2023
Merged

Reorganize and Rename to have multiple autonomy algs #107

merged 20 commits into from
Nov 19, 2023

Conversation

StefanCaldararu
Copy link
Contributor

@StefanCaldararu StefanCaldararu marked this pull request as ready for review November 15, 2023 14:50
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@StefanCaldararu StefanCaldararu self-assigned this Nov 15, 2023
@AaronYoung5

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@AaronYoung5

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@AaronYoung5

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@StefanCaldararu

This comment was marked as outdated.

Sign in to view
AaronYoung5
AaronYoung5 reviewed Nov 17, 2023
View reviewed changes
workspace/src/control/pid_controller/pid_controller/pid_controller.py Outdated
class PIDControllerNode(Node):
"""A PID controller.

This node subscribes to a path which is the data type published by the Cone Path Planner node, and publishes vehicle inputs to follow the path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no mention anywhere of what file type it is. Can you say that here or type hint it in the callbacks? And is it specific to cone path planner? I thought it was just a geometry msgs path or something?

Copy link
Contributor Author

@StefanCaldararu StefanCaldararu Nov 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There's no mention anywhere of what file type it is.

Not sure what you mean by this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the comment says "Cone Path Planner", but this node isn't specific to the cone path planner right? It should instead say what message types it receives and which it publishes. This is probably a best practice we should try to do in the future. I wouldn't worry about this rn, unless you want to change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll fix this when I go through and fix all the docstrings.

workspace/src/control/pid_controller/setup.py Outdated Show resolved Hide resolved
workspace/src/control/pid_controller/pid_controller/pid_controller.py Outdated Show resolved Hide resolved
workspace/src/common/launch/art_localization_launch/launch/pf_estimation.launch.py Outdated Show resolved Hide resolved
workspace/src/localization/shared_utils/package.xml Outdated Show resolved Hide resolved
...line_objects_path_planner/centerline_objects_path_planner/centerline_objects_path_planner.py Show resolved Hide resolved
AaronYoung5
AaronYoung5 reviewed Nov 18, 2023
View reviewed changes
Copy link
Collaborator

@AaronYoung5 AaronYoung5 left a comment

Choose a reason for hiding this comment

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

overall confused about the difference between the localization nodes. overall looks good tho, just a few comments.

workspace/src/common/launch/art_launch/package.xml Outdated Show resolved Hide resolved
workspace/src/common/launch/art_localization_launch/launch/art_localization.launch.py Outdated Show resolved Hide resolved
workspace/src/common/launch/art_localization_launch/launch/art_localization.launch.py Outdated Show resolved Hide resolved
workspace/src/common/launch/art_localization_launch/launch/ekf_estimation.launch.py Show resolved Hide resolved
workspace/src/common/launch/art_localization_launch/launch/ground_truth.launch.py Outdated Show resolved Hide resolved
...lization/particle_filter_estimation/particle_filter_estimation/particle_filter_estimation.py Outdated Show resolved Hide resolved
...lization/particle_filter_estimation/particle_filter_estimation/particle_filter_estimation.py Outdated Show resolved Hide resolved
...lization/particle_filter_estimation/particle_filter_estimation/particle_filter_estimation.py Show resolved Hide resolved
workspace/src/localization/ground_truth/ground_truth/ground_truth.py Show resolved Hide resolved
workspace/src/localization/ground_truth/ground_truth/ground_truth.py Outdated Show resolved Hide resolved
@AaronYoung5
Copy link
Collaborator

but honestly too much code to actually look through properly. If you test thoroughly (like all nodes and how they interact), i think it should be fine.

@StefanCaldararu

This comment was marked as outdated.

Sign in to view
@StefanCaldararu
Copy link
Contributor Author

StefanCaldararu commented Nov 18, 2023 •
edited
Loading

Only difference between the actual nodes for localization is the publisher. Everything that would be different is moved outside of the actual node file, into another file (ex. workspace/src/localization/particle_filter_estimation/particle_filter_estimation/particle_filter.py). This is mostly done for consistency with other nodes, and for when there ARE substantially different localization packages added..

Would there have been a better way to go about this?

AaronYoung5
AaronYoung5 reviewed Nov 19, 2023
View reviewed changes
workspace/src/common/launch/art_localization_launch/launch/ground_truth.launch.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should instead publish the message directly in the sim than have a separate node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree, because for certain applications we might want this info packaged in one message. I could see moving this to sensing because it is somehow pre-processing of data / packaging of data, but I still think it is necessary to have this in sim/real.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, if we are using RTK GPS and IMU, we DO want localization to just package the info into one message for the path planner, but not do any processing / estimation.

AaronYoung5
AaronYoung5 approved these changes Nov 19, 2023
View reviewed changes
Copy link
Collaborator

@AaronYoung5 AaronYoung5 left a comment

Choose a reason for hiding this comment

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

Assume you've tested. I honestly don't care too much about code, I just think we should follow best practices for file organization and stuff like that. Looks good to me from that perspective at least.

@StefanCaldararu StefanCaldararu merged commit 345a560 into master Nov 19, 2023
1 check passed
@StefanCaldararu StefanCaldararu deleted the 105/multiple-autonomy-algs branch November 19, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronYoung5 AaronYoung5 AaronYoung5 approved these changes

Assignees

@StefanCaldararu StefanCaldararu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@StefanCaldararu @AaronYoung5