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

Organize directories #43

Merged
merged 10 commits into from
Jun 18, 2022
Merged

Organize directories #43

merged 10 commits into from
Jun 18, 2022

Conversation

niwhsa9
Copy link
Member

@niwhsa9 niwhsa9 commented Jun 13, 2022

have not tested, I can guarantee I broke some launch files though. #28

@CameronTressler
Copy link
Contributor

I think we should decide now, are we gonna be sticklers about adding newlines at the ends of files? I'm honestly kind of indifferent, but I know people (I think at least Matt Martin?) have made that comment on PRs in the past and I think we should be consistent about whether or not we care. What are your guys thoughts?

@niwhsa9
Copy link
Member Author

niwhsa9 commented Jun 13, 2022

i'm also indifferent but it definitely sounds like the thing that @qhdwight might have a strong opinion on lmao. What do you think Q? Its possible that our style checker is already enforcing too

@qhdwight
Copy link
Collaborator

i'm also indifferent but it definitely sounds like the thing that @qhdwight might have a strong opinion on lmao. What do you think Q? Its possible that our style checker is already enforcing too

I don't really have a strong opinion. Usually I add a newline. What I care about is consistency with bracing, newlines, etc. in the actual code. I also took a look on if we could enforce this with clang tidy but I didn't find an easy answer.

@qhdwight qhdwight changed the title restructure Organize directories Jun 13, 2022
@qhdwight
Copy link
Collaborator

I'll take a look at this after it compiles

@rbridges12
Copy link
Collaborator

Had to merge master in order to fix the merge conflicts, CI now runs and passes so this should be ready for review

@qhdwight
Copy link
Collaborator

qhdwight commented Jun 17, 2022

Including the .vscode folder was intentional? Since usually checking in project generated stuff is not done. I suppose ROS nonstandard to set up so that is why this is here?

@qhdwight
Copy link
Collaborator

I think we should also prefer the longer names, such as perception.launch as opposed to percep.launch. The folders are currently like this but not all files.

@rbridges12
Copy link
Collaborator

Including the .vscode folder was intentional? Since usually checking in project generated stuff is not done. I suppose ROS nonstandard to set up so that is why this is here?

@niwhsa9 added that, not sure if it was intentional or not

Copy link
Contributor

@tabiosg tabiosg left a comment

Choose a reason for hiding this comment

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

I think there's a bunch of issues with file paths now that this PR moves a bunch of files around.

Stuff got moved into the config/ folder, so the references to it will have to be changed (maybe also add to CMakeLists.txt?). e.g. It still says params/ekf_localization.yaml in nav.launch and loc.launch. I'm also not sure what control.yaml does. Also a bunch of files still say env_description and all of the .rviz files are not linked properly in the .launch files.

EDIT: that all being said, IDK how CI passes so maybe I'm just really really dumb

@niwhsa9
Copy link
Member Author

niwhsa9 commented Jun 17, 2022

nah @tabiosg u prob right, CI only does a catkin build so there's a good chance a ton of other stuff is broke.

@rbridges12 unintentional, we can gitignore that. Although if people like vscode I can add my launch jsons that let you run ROS python nodes properly with the debugger.

@rbridges12
Copy link
Collaborator

@niwhsa9 I think that would be useful, wanna do that here or later in a different PR?

@niwhsa9
Copy link
Member Author

niwhsa9 commented Jun 18, 2022

Diff PR because I'll need to change them to use environment variables as opposed to my hardcoded directories

@rbridges12
Copy link
Collaborator

All of @tabiosg concerns should be fixed now, I ran all the launch files and saw zero errors, and everything looked normal

Copy link
Contributor

@tabiosg tabiosg left a comment

Choose a reason for hiding this comment

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

concerns have vanished

@rbridges12
Copy link
Collaborator

I'm pretty sure config.yaml is from when we used a different sim velocity controller, so its now useless. I couldnt find any reference to it in the code. Can I delete @niwhsa9?

@niwhsa9
Copy link
Member Author

niwhsa9 commented Jun 18, 2022

Yeah I think ur right

@qhdwight
Copy link
Collaborator

I've tested this with nav stuff and everything worked well so I'm down to merge this. Perhaps we could do the short -> long names like I was talking about but that can also come later.

@rbridges12
Copy link
Collaborator

Yeah I'd say add the long names and then lets merge

@rbridges12 rbridges12 merged commit b1eb435 into master Jun 18, 2022
@rbridges12 rbridges12 deleted the restructure branch June 18, 2022 18:18
qhdwight pushed a commit to qhdwight/mrover that referenced this pull request Jul 9, 2022
* restructure

* updated config path in cmake lists

* removed useless localization node

* moved EARTH_RADIUS back to tf_utils

* linting

* removed vscode files, fixed paths and removed localization node in launch files

* deteled control.yaml which is no longer used

* changed launch and rviz filenames from short to full words

Co-authored-by: Riley Bridges <[email protected]>
tabiosg added a commit that referenced this pull request Jul 11, 2022
* Refactor naming

* Change joint msg to array (#40) (#43)

* Update ArmPosition.msg

* Finalize joints to array

* Fix variable names for publishing

* Refactor motor function naming
qhdwight pushed a commit to qhdwight/mrover that referenced this pull request Aug 8, 2022
* restructure

* updated config path in cmake lists

* removed useless localization node

* moved EARTH_RADIUS back to tf_utils

* linting

* removed vscode files, fixed paths and removed localization node in launch files

* deteled control.yaml which is no longer used

* changed launch and rviz filenames from short to full words

Co-authored-by: Riley Bridges <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants