Skip to content

Fixes and Improvements#43

Open
tonynajjar wants to merge 14 commits into
open-navigation:mainfrom
tonynajjar:main
Open

Fixes and Improvements#43
tonynajjar wants to merge 14 commits into
open-navigation:mainfrom
tonynajjar:main

Conversation

@tonynajjar

@tonynajjar tonynajjar commented Mar 5, 2024

Copy link
Copy Markdown

Fixes:

  • Fixes for BT upgrade
  • fix polygons not being cleared in ComputeCoveragePathAction

Improvements:

  • Make visualization topics Transient Local
  • for demo, cancel goal when exiting with CTRL+C

@tonynajjar tonynajjar mentioned this pull request Mar 5, 2024
@tonynajjar tonynajjar changed the title Fix BT.CPP upgrade Bug Fixes Mar 5, 2024
@tonynajjar tonynajjar changed the title Bug Fixes Fixes and Improvements Mar 5, 2024

@SteveMacenski SteveMacenski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tonynajjar just add in the readme the transient local-ness of those topics

Comment thread opennav_coverage/src/visualizer.cpp Outdated
Comment thread README.md
@SteveMacenski

Copy link
Copy Markdown
Member

Linting failed an an odd error basically complaining about conflicts with every BT.CPPv4 node?

Comment thread opennav_coverage/CMakeLists.txt
@tonynajjar

Copy link
Copy Markdown
Author

Linting failed an an odd error basically complaining about conflicts with every BT.CPPv4 node?

I found a couple of build issue that I fixed. The logs were super verbose though, maybe there is something else, let's see

@tonynajjar

tonynajjar commented Mar 5, 2024

Copy link
Copy Markdown
Author

It's still failing, looking at the logs, they contain both /opt/ros/iron/include/behaviortree_cpp_v3 and /opt/ros/iron/include/behaviortree_cpp, maybe that's a problem? Locally it builds...

On rolling there is only the newer version:

root@tony-xmg-22:/opt/overlay_ws# ls /opt/ros/rolling/include/ | grep behavior
behaviortree_cpp

@SteveMacenski

Copy link
Copy Markdown
Member

That is odd - this CI shouldn't have any caching in it (@emersonknapp any caching happen in the ros action ci?) for it to know about BT.CPPv4 if its gone here. Does this cause you issues locally?

@tonynajjar

tonynajjar commented Mar 6, 2024

Copy link
Copy Markdown
Author

Does this cause you issues locally?

Locally on rolling no, I didn't test on iron though, I'm on humble and the devcontainer is only for rolling I believe - setting up iron will be some effort

@tonynajjar

Copy link
Copy Markdown
Author

That is odd - this CI shouldn't have any caching in it (@emersonknapp any caching happen in the ros action ci?) for it to know about BT.CPPv4 if its gone here

@SteveMacenski I'm guessing it knows about BT.CPPv4 because it's using iron. I tried using rolling in this commit but that fails as well because it can't find nav2_util -> are there no binaries for nav2 on rolling?

@SteveMacenski

Copy link
Copy Markdown
Member

There is no Nav2 binaries for rolling, but you could add it in the https://github.com/open-navigation/opennav_coverage/blob/main/.github/deps.repos for use in CI for testing. I'm not sure if you can isolate it to only nav2_utils and its necessary dependencies, but I suppose we could change the Action's colcon build to include packages-up-to for the opennav coverage packages, which would include only the parts of Nav2 strictly required. It would though need to build the Nav2 BT libraries which is non-trivial.

@SteveMacenski

Copy link
Copy Markdown
Member

-_- action ci tools don't call rosdep, I literally can't believe that

@tonynajjar

tonynajjar commented Mar 6, 2024

Copy link
Copy Markdown
Author

-_- action ci tools don't call rosdep, I literally can't believe that

hmm I think it does
image

but for some reason it can't find "standard" packages: https://github.com/open-navigation/opennav_coverage/actions/runs/8175554856/job/22352920043?pr=43

@tonynajjar

Copy link
Copy Markdown
Author

I'm wondering if https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/8 could have anything to do with it. It still says Jammy in the CI though No definition of [geometry_msgs] for OS version [jammy]

@tonynajjar

tonynajjar commented Mar 6, 2024

Copy link
Copy Markdown
Author

I'm wondering if https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/8 could have anything to do with it. It still says Jammy in the CI though No definition of [geometry_msgs] for OS version [jammy]

Now I have a stronger suspicion:

root@tony-xmg-22:/opt/overlay_ws# rosdep resolve geometry_msgs
#apt
ros-rolling-geometry-msgs
root@tony-xmg-22:/opt/overlay_ws# rosdep update
reading in sources list data from /etc/ros/rosdep/sources.list.d
Warning: running 'rosdep update' as root is not recommended.
  You should run 'sudo rosdep fix-permissions' and invoke 'rosdep update' again without sudo.
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/osx-homebrew.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/base.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/python.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/ruby.yaml
Hit https://raw.githubusercontent.com/ros/rosdistro/master/releases/fuerte.yaml
Query rosdistro index https://raw.githubusercontent.com/ros/rosdistro/master/index-v4.yaml
Skip end-of-life distro "ardent"
Skip end-of-life distro "bouncy"
Skip end-of-life distro "crystal"
Skip end-of-life distro "dashing"
Skip end-of-life distro "eloquent"
Skip end-of-life distro "foxy"
Skip end-of-life distro "galactic"
Skip end-of-life distro "groovy"
Add distro "humble"
Skip end-of-life distro "hydro"
Skip end-of-life distro "indigo"
Add distro "iron"
Skip end-of-life distro "jade"
Skip end-of-life distro "kinetic"
Skip end-of-life distro "lunar"
Skip end-of-life distro "melodic"
Add distro "noetic"
Add distro "rolling"
updated cache in /root/.ros/rosdep/sources.cache
root@tony-xmg-22:/opt/overlay_ws# rosdep resolve geometry_msgs

ERROR: No definition of [geometry_msgs] for OS version [jammy]

No definition of [geometry_msgs] for OS version [jammy]
	rosdep key : geometry_msgs
	OS name    : ubuntu
	OS version : jammy
	Data:
_is_ros: true
		debian:
		  bookworm:
		    apt:
		      packages:
		      - ros-rolling-geometry-msgs
		osx:
		  homebrew:
		    packages:
		    - ros/rolling/common_interfaces
		rhel:
		  '9':
		    dnf:
		      packages:
		      - ros-rolling-geometry-msgs
		ubuntu:
		  noble:
		    apt:
		      packages:
		      - ros-rolling-geometry-msgs

What a timing 😄

@SteveMacenski

Copy link
Copy Markdown
Member

Oh I see that too. So I guess transient during the Rolling migration that's underway, I see the flood of build farm emails with their work to 24.04. What's the path from here? I think this was just a test to see if it would resolve on rolling since Iron was problematic. Certainly we could move the CI to rolling too, I could roll with that 😉

@SteveMacenski

SteveMacenski commented Mar 6, 2024

Copy link
Copy Markdown
Member

By the way, this is not just a CI problem, I see this locally as well after doing an apt update + upgrade using the BTv4 branch. I actually see the 'all the BT nodes complaining' in opennav_docking as well now after the update which is interesting.

@SteveMacenski

Copy link
Copy Markdown
Member

Experimenting now on the odd BT compiling issues. Did you happen to make any progress on it? I'm starting with purging v3 from my computer so its only v4. I suspect that has something to do with it.

@tonynajjar

Copy link
Copy Markdown
Author

Mmm no progress as I didn't setup Iron. On my setup on rolling there wass only BTv4 so I couldn't reproduce locally. You were able to reproduce on iron right?

@SteveMacenski

SteveMacenski commented Mar 7, 2024

Copy link
Copy Markdown
Member

I don't recall specifically what my setup was when I saw it, I was in the middle of doing something else so I just removed opennav_coverage and went back to what I was trying to do at the time when I ran into it. Its possible I had nav2 main built on iron base (which could have had btv4 in it) or some other mixed policy. I was poking around alot of projects this week so I can't recall which specifically it was I found it with respect to.

What I know I did right now:

  • Rolling, with BTv4 installed only (apt removed v3)
  • Built in a workspace with Nav2 clean with no prior caches

Doesn't happen for one of the projects, I adapted that here as well now too in a separate branch with just the v4 changes and that works too. I'll open a separate PR in a moment with just the v4 updates to see what CI says about that and that I have something working locally.

I'm guessing it has to do with having the v3 and v4 installed next to each other

@SteveMacenski

SteveMacenski commented Mar 7, 2024

Copy link
Copy Markdown
Member

Ah, in CI

executing command [apt-get install -y ros-iron-nav2-behavior-tree]

That's going to pull in BT.CPPv3. Then we're using v4 that would explain it (in my PR)

@tonynajjar

tonynajjar commented Mar 7, 2024

Copy link
Copy Markdown
Author

Yes and BTv3 is expected in Iron so the simplest solution I see is to port the CI to rolling when rosdep works again... Do you see another option?

@SteveMacenski

SteveMacenski commented Mar 7, 2024

Copy link
Copy Markdown
Member

Note that you missed adding BTCPP_format="4" to the BT XML + the inline test XML string which causes a warning on execution. Please add that and I'll close my PR (only difference)

I see is to port the CI to rolling when rosdep works again

I agree. If I'm reading the docs for action-ros-ci correctly, we can use colcon-extra-args: --packages-up-to <packages in repo> to automatically only build what is needed from Nav2. Else, we can specify exactly which packages to build with package-name:, but we'll need to figure out exactly which packages we need to build manually. Which, from a quick look, seems like ~46 packages which would be a pain in the ass to list.

https://github.com/ros-tooling/action-ros-ci/blob/634e9ac5fb970763bd6207e31e1875d0081be71e/action.yml#L31-L34

I also filed ros-tooling/setup-ros-docker#68 which is necessary to upgrade the CI to use 24.04 (in case 22.04 is totally messed up permanently, which would be really irritating). I asked for clarity in https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/9?u=smac. I suppose either way we need to update CI to 24.04, so I suppose if that comes, we should just do that anyway.

I think that's the path forward and the contingencies, do you agree? I didn't want to make this your problem so I made sure to look at this today.

@tonynajjar

tonynajjar commented Mar 7, 2024

Copy link
Copy Markdown
Author

Note that you missed adding BTCPP_format="4" to the BT XML + the inline test XML string which causes a warning on execution. Please add that and I'll close my PR (only difference)

Done

https://github.com/ros-tooling/action-ros-ci/blob/634e9ac5fb970763bd6207e31e1875d0081be71e/action.yml#L31-L34

ah nice find, there's nothing about it in the readme

I think that's the path forward and the contingencies, do you agree?

yep sounds like a plan. Let's see what they'll answer in the discourse

I'm looking forward to this moving in navigation2 (if that's gonna happen) to manage it more easily and avoid the double work.

@SteveMacenski

SteveMacenski commented Mar 7, 2024

Copy link
Copy Markdown
Member

Yeah the plan is to move it to Nav2 once the F2C updated features are in F2C & updated to be used here. I still haven't decided if I will rename these to use nav2_ prefix or keep opennav_, but that's pedantic

@tonynajjar

tonynajjar commented Mar 14, 2024

Copy link
Copy Markdown
Author

I tried again since rolling sync is out but still same issue with rosdep not finding some packages. Unfortunately I don't have the time currently to look further into it, could you maybe ask for a follow up on the issue?

@SteveMacenski

SteveMacenski commented Mar 14, 2024

Copy link
Copy Markdown
Member

It still failing everywhere, I wouldn't worry about it yet. I still see missing rosdep in Nav2 and tickets on discourse, I don't think anything changed

@aosmw aosmw mentioned this pull request Apr 29, 2024
This was referenced Aug 19, 2024
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