andino_apps package created and andino_navigation package updated #235

JesusSilvaUtrera · 2024-04-10T09:17:18Z

🦟 Bug fix

Fixes #228

Summary

I have created a new andino_apps package, where I moved the andino_simulation_navigation launch file from andino_navigation. This makes the andino_navigation independent of the simulation packages.

Test it

Build the workspace and follow the new README.md from andino_apps to test the launch file and see that the functionality remains the same.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: JesusSilvaUtrera <[email protected]>

JesusSilvaUtrera · 2024-04-10T09:21:15Z

@agalbachicar FYI. Also some topics to discuss with @francocipollone along with the ones that we mentioned earlier:

Should the andino_apps package be in a different repository as andino_o3de for example?
Should the rviz files be moved to andino_apps or remain in their original package?
Should the world and map files be included in the package to have them at hand or we are good using them from their original package (nav2_bringup) so far? Obviously in the future we can include specific worlds or maps that we create.

Thanks in advance for the time!

agalbachicar · 2024-04-10T09:47:20Z

Risking some answers here, but let's wait for @francocipollone to give the final thumbs up or down:

Should the andino_apps package be in a different repository as andino_o3de for example?

I think we can move forward with it for now within this repository as it will integrate the packages present within this repository. Anything that go beyond that, e.g. a demostration of the navigation stack using the O3DE simulation, shall leave in another package/repository.

Should the rviz files be moved to andino_apps or remain in their original package?

This is a good question. I think we should keep things related together, meaning that if the RViz configuration file has been created for the purpose of the integration application we should have that within the andino_apps package. When it is a generic, application independent configuration, then it should be a top level package. Probably, an andino_visualization package to that end.

Should the world and map files be included in the package to have them at hand or we are good using them from their original package (nav2_bringup) so far? Obviously in the future we can include specific worlds or maps that we create.

IMO, yes. We need to have these world, map and other configuration files that serve the purpose of an integration application within this new andino_apps package. That will help to keep related assets together. As soon as we find a use case for sharing those assets across multiple packages we can create a new structure that serves the pupose, but I would refrain from doing an early segmentation without a use case.

agalbachicar

PTAL

andino_apps/README.md

andino_gz_classic/worlds/willowgarage.world

andino_navigation/README.md

Signed-off-by: JesusSilvaUtrera <[email protected]>

…lvaUtrera/andino into jesus/#228_simulation_dependency

agalbachicar

Looking better! PTAL

andino_apps/CHANGELOG.rst

andino_navigation/README.md

Signed-off-by: JesusSilvaUtrera <[email protected]>

agalbachicar

One final thing before approving. Look at https://github.com/Ekumen-OS/andino/blob/humble/.github/workflows/ci.yml#L32, you need to add andino_apps to the list of packages.

Signed-off-by: JesusSilvaUtrera <[email protected]>

JesusSilvaUtrera · 2024-04-12T09:03:39Z

@agalbachicar sorry, I am not really used it CI/CD and I forgot about it, I already changed that, thanks!

agalbachicar

LGTM

agalbachicar · 2024-04-12T09:45:04Z

I think you can a proper PR now (remove the draft state).

francocipollone

Thanks for the contribution! Looks good. Some minor comments to go

francocipollone · 2024-04-18T12:15:44Z

andino_apps/package.xml

+  <exec_depend>ros2launch</exec_depend>
+  <exec_depend>andino_gz_classic</exec_depend>
+  <exec_depend>nav2_bringup</exec_depend>
+  <exec_depend>turtlebot3_gazebo</exec_depend>


Do we need to be explicit about turtlebot3_gazebo. IIRC nav2_bringup manages that dependency.
We are not directly using any turtlebot_gazebo asset. (If we use the world we use it via nav2_bringup)

francocipollone · 2024-04-18T12:16:13Z

andino_navigation/README.md

@@ -4,44 +4,29 @@ We rely on [Nav2](https://github.com/ros-planning/navigation2) stack in order to

 # Usage

-## Real Robot
+## Prerequisites
+  1. Run the mobility stack in a real Andino robot  or a simulated one:


nit: Extra space?

Suggested change

1. Run the mobility stack in a real Andino robot or a simulated one:

1. Run the mobility stack in a real Andino robot or a simulated one:

francocipollone · 2024-04-18T12:17:00Z

andino_navigation/README.md


-2. We need a previously recorded map in order to navigate on.
-   Refer to [andino_slam](../andino_slam/README.md) to learn how to record a map with Andino.
+  2. Provide a recorded map. Refer to [andino_slam](https://github.com/JesusSilvaUtrera/andino/blob/jesus/%23228_simulation_dependency/andino_slam/README.md) to learn how to record a map with Andino.


You are using a link from your fork. Use a relative path to that.

Signed-off-by: JesusSilvaUtrera <[email protected]>

JesusSilvaUtrera · 2024-04-18T13:35:54Z

@francocipollone minor issues fixed. Also, do you think it is a good idea to keep the andino_apps here for now or it would be better to move it to an independent repo?

francocipollone · 2024-04-18T14:26:28Z

@francocipollone minor issues fixed. Also, do you think it is a good idea to keep the andino_apps here for now or it would be better to move it to an independent repo?

I think that for now we can keep it here

francocipollone

LGTM!

francocipollone

One extra thing I just noticed: Please add the andino_apps to the list of packages if you can see in the root README file

Signed-off-by: JesusSilvaUtrera <[email protected]>

JesusSilvaUtrera · 2024-04-18T14:57:23Z

One extra thing I just noticed: Please add the andino_apps to the list of packages if you can see in the root README file

Already done, sorry I didn't notice that earlier either.

francocipollone

LGTM!

JesusSilvaUtrera · 2024-04-19T06:27:51Z

@francocipollone Javi told me he was leaving the decision up to you, so you can merge this now if you don't want to review anything else. Thanks!

agalbachicar · 2024-04-19T07:48:58Z

Sorry, I am a bit impatient ;)

* Improves andino_navigation package. (#224) Signed-off-by: Franco Cipollone <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]> * Add basic Platform IO instructions (#229) Signed-off-by: Gary Servin <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]> * Modify Shell class API to allow dependency injection (#221) Signed-off-by: Javier Balloffet <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]> * Fixed wheel diameter in URDF (#93) Signed-off-by: JesusSilvaUtrera <[email protected]> * Added YAML file for default camera intrinsics (#169) Signed-off-by: JesusSilvaUtrera <[email protected]> * Adds references to o3de and isaac_sim simulations. (#232) Signed-off-by: Franco Cipollone <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]> * Added README for andino_bringup (#233) Signed-off-by: JesusSilvaUtrera <[email protected]> * Added binaries installation docs (#234) Signed-off-by: JesusSilvaUtrera <[email protected]> * andino_apps package created and andino_navigation package updated (#235) * andino_apps package created and andino_navigation package updated Signed-off-by: JesusSilvaUtrera <[email protected]> * Minor changes from the PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Update andino_navigation README with changes from PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Add andino_apps package to ci.yaml Signed-off-by: JesusSilvaUtrera <[email protected]> * Fixed minor issues from the PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Added 'andino_apps' to the general README Signed-off-by: JesusSilvaUtrera <[email protected]> --------- Signed-off-by: JesusSilvaUtrera <[email protected]> * Minimal change to test launch file Signed-off-by: JesusSilvaUtrera <[email protected]> * Added webcam.yaml file and intrinsics are now publishing Signed-off-by: JesusSilvaUtrera <[email protected]> * Added raspicam.yaml with camera intrinsics and updated instructions in andino_hardware/README Signed-off-by: JesusSilvaUtrera <[email protected]> * Changed the 'camera_info_url' to be relative path Signed-off-by: JesusSilvaUtrera <[email protected]> --------- Signed-off-by: Franco Cipollone <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]> Signed-off-by: Gary Servin <[email protected]> Signed-off-by: Javier Balloffet <[email protected]> Co-authored-by: Franco Cipollone <[email protected]> Co-authored-by: Gary Servin <[email protected]> Co-authored-by: Javier Balloffet <[email protected]>

andino_apps package created and andino_navigation package updated

e449534

Signed-off-by: JesusSilvaUtrera <[email protected]>

agalbachicar requested changes Apr 10, 2024

View reviewed changes

JesusSilvaUtrera and others added 3 commits April 10, 2024 12:28

Merge branch 'humble' into jesus/#228_simulation_dependency

bc819e7

Minor changes from the PR

3789b6e

Signed-off-by: JesusSilvaUtrera <[email protected]>

Merge branch 'jesus/#228_simulation_dependency' of github.com:JesusSi…

1574860

…lvaUtrera/andino into jesus/#228_simulation_dependency

agalbachicar requested changes Apr 11, 2024

View reviewed changes

andino_apps/CHANGELOG.rst Show resolved Hide resolved

andino_navigation/README.md Outdated Show resolved Hide resolved

Update andino_navigation README with changes from PR

1a0b260

Signed-off-by: JesusSilvaUtrera <[email protected]>

agalbachicar requested changes Apr 12, 2024

View reviewed changes

Add andino_apps package to ci.yaml

7ec9ec1

Signed-off-by: JesusSilvaUtrera <[email protected]>

agalbachicar previously approved these changes Apr 12, 2024

View reviewed changes

JesusSilvaUtrera marked this pull request as ready for review April 18, 2024 08:14

JesusSilvaUtrera requested review from francocipollone and jballoffet as code owners April 18, 2024 08:14

francocipollone requested changes Apr 18, 2024

View reviewed changes

Fixed minor issues from the PR

ca9582a

Signed-off-by: JesusSilvaUtrera <[email protected]>

JesusSilvaUtrera dismissed agalbachicar’s stale review via ca9582a April 18, 2024 13:34

francocipollone approved these changes Apr 18, 2024

View reviewed changes

francocipollone requested changes Apr 18, 2024

View reviewed changes

Added 'andino_apps' to the general README

0d500d0

Signed-off-by: JesusSilvaUtrera <[email protected]>

francocipollone approved these changes Apr 18, 2024

View reviewed changes

agalbachicar merged commit 07a0083 into Ekumen-OS:humble Apr 19, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

andino_apps package created and andino_navigation package updated #235

andino_apps package created and andino_navigation package updated #235

JesusSilvaUtrera commented Apr 10, 2024

JesusSilvaUtrera commented Apr 10, 2024

agalbachicar commented Apr 10, 2024

agalbachicar left a comment

agalbachicar left a comment

agalbachicar left a comment

JesusSilvaUtrera commented Apr 12, 2024

agalbachicar left a comment

agalbachicar commented Apr 12, 2024 •

edited

Loading

francocipollone left a comment

francocipollone Apr 18, 2024

francocipollone Apr 18, 2024

francocipollone Apr 18, 2024

JesusSilvaUtrera commented Apr 18, 2024

francocipollone commented Apr 18, 2024

francocipollone left a comment

francocipollone left a comment

JesusSilvaUtrera commented Apr 18, 2024

francocipollone left a comment

JesusSilvaUtrera commented Apr 19, 2024

agalbachicar commented Apr 19, 2024

	1. Run the mobility stack in a real Andino robot or a simulated one:
	1. Run the mobility stack in a real Andino robot or a simulated one:

andino_apps package created and andino_navigation package updated #235

andino_apps package created and andino_navigation package updated #235

Conversation

JesusSilvaUtrera commented Apr 10, 2024

🦟 Bug fix

Summary

Test it

Checklist

JesusSilvaUtrera commented Apr 10, 2024

agalbachicar commented Apr 10, 2024

agalbachicar left a comment

Choose a reason for hiding this comment

agalbachicar left a comment

Choose a reason for hiding this comment

agalbachicar left a comment

Choose a reason for hiding this comment

JesusSilvaUtrera commented Apr 12, 2024

agalbachicar left a comment

Choose a reason for hiding this comment

agalbachicar commented Apr 12, 2024 • edited Loading

francocipollone left a comment

Choose a reason for hiding this comment

francocipollone Apr 18, 2024

Choose a reason for hiding this comment

francocipollone Apr 18, 2024

Choose a reason for hiding this comment

francocipollone Apr 18, 2024

Choose a reason for hiding this comment

JesusSilvaUtrera commented Apr 18, 2024

francocipollone commented Apr 18, 2024

francocipollone left a comment

Choose a reason for hiding this comment

francocipollone left a comment

Choose a reason for hiding this comment

JesusSilvaUtrera commented Apr 18, 2024

francocipollone left a comment

Choose a reason for hiding this comment

JesusSilvaUtrera commented Apr 19, 2024

agalbachicar commented Apr 19, 2024

agalbachicar commented Apr 12, 2024 •

edited

Loading