ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.31k stars 1.2k forks source link

Temporal min points #4390

Open gennartan opened 1 month ago

gennartan commented 1 month ago

Basic Info

Info Please fill out this column
Ticket(s) this addresses (#4364)
Primary OS tested on Ubuntu
Robotic platform tested on Tested using colcon test only
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

Description of documentation updates required from your changes


Future work that may be required in bullet points

For Maintainers:

mergify[bot] commented 1 month ago

This pull request is in conflict. Could you fix it @gennartan?

mergify[bot] commented 1 month ago

@gennartan, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

SteveMacenski commented 1 month ago

Make sure to pull in main so that CI works for the Rotation Shim fix. Also, make sure to actually test this functionally since this is something you want to use!

Overall nice job on this, you managed to do alot with very few lines of code!

mergify[bot] commented 6 days ago

This pull request is in conflict. Could you fix it @gennartan?

SteveMacenski commented 6 days ago

@gennartan I see that you pushed some updates, should I take a look and review or just incremental updates?

CI failed ... oddly, might be worth rebasing your PR, but I retriggered CI to see if it was just a fluke. Note the linting test failures from ament_cpplint and ament_uncrustify, those are important

mergify[bot] commented 6 days ago

@gennartan, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

gennartan commented 6 days ago

Hi, just incremental updates for now, I still need to add some testing to it. I will check why the tests are failling and fix that.

Do you know if there is a method to create tests with precise timing to simulate the clock in ROS ?

SteveMacenski commented 5 days ago

Ok!

Do you know if there is a method to create tests with precise timing to simulate the clock in ROS ?

I think an illustrative example of what you’re looking to do might help me give a suggestion. I’m not sure that should be required here, but maybe we’re thinking about different definitions of ‘precise’. Is using the clock’s stamp insufficiently accurate?

These build errors are interesting. Please try rebasing on main. If it’s still a problem, I can look into it.

gennartan commented 3 days ago

I think an illustrative example of what you’re looking to do might help me give a suggestion. I’m not sure that should be required here, but maybe we’re thinking about different definitions of ‘precise’. Is using the clock’s stamp insufficiently accurate?

My question is more about the clock control in the test. If I want to test if the polygon action is correctly applied after 10s of positive detection, I don't want to wait for 10 actual seconds on the system running the tests using a sleep function, I want to be able to change the clock vlaue to simulate those 10 seconds.

Do you know if such a function exists ?

SteveMacenski commented 3 days ago

I don't want to wait for 10 actual seconds on the system running the tests using a sleep function, I want to be able to change the clock value to simulate those 10 seconds.

You could set simulation time and have your own /clock publisher, but that seems like overkill to bake into the test. I suggest making some of the times smaller values like 2-3 seconds instead. Keep the test times down whereever reasonably possible, but don't go overboard. A few seconds here or there in our CI or running isn't a huge deal -- nav2_system_tests takes 12 minutes by itself. I prefer a bit longer but more stable and non-flaky over fast but flaky.