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

[JTC] Fix tests and set_hold_position #607

Closed
wants to merge 12 commits into from
Closed

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented May 11, 2023

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2023

Codecov Report

Merging #607 (68ca860) into master (e7f9962) will increase coverage by 0.75%.
The diff coverage is 34.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
+ Coverage   35.78%   36.53%   +0.75%     
==========================================
  Files         189        7     -182     
  Lines       17570      676   -16894     
  Branches    11592      357   -11235     
==========================================
- Hits         6287      247    -6040     
+ Misses        994      134     -860     
+ Partials    10289      295    -9994     
Flag Coverage Δ
unittests 36.53% <34.91%> (+0.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp 11.11% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 17.62% <12.08%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 47.09% <46.88%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <100.00%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 100.00% <100.00%> (ø)

... and 189 files with indirect coverage changes

@mergify
Copy link
Contributor

mergify bot commented Jun 25, 2023

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

Comment on lines +230 to 232
// TODO(anyone): should the controller even allow calling update() when it is not active?
// update loop receives a new msg and updates accordingly
updateController(rclcpp::Duration::from_seconds(0.25));
Copy link
Member

Choose a reason for hiding this comment

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

This should be clearly removed. This is something that it doesn't happen in the real world using CM - so we should not test it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 7bc67e5 in #705

// wait so controller process the third point when reactivated
std::this_thread::sleep_for(std::chrono::milliseconds(3000));
// TODO(anyone) test copied from ROS 1: it fails now!
// should the old trajectory really be processed after reactivation?
Copy link
Member

Choose a reason for hiding this comment

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

No, I think after reactivation, we should remove the trajectory that was commanded. It is basically “a reset”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with bceaf96 in #705

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And even more with 0bf9844 in #558

christophfroehlich and others added 3 commits June 26, 2023 19:09
* Fix time sources and wrong checks in tests
* Use time from update-method instead of node clock
* Readd test of last command in test_goal_tolerances_fail

---------

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
christophfroehlich and others added 6 commits June 28, 2023 19:08
* Reject receiving trajectory of last velocity point is non-zero

* Update docs

* Add tests

* Change to parameterized test

* Rename parameter

* not true -> false

---------

Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
@christophfroehlich christophfroehlich changed the title [JTC] Add new features [JTC] Fix tests and set_hold_position Jul 4, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 17, 2023

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

@bmagyar
Copy link
Member

bmagyar commented Jul 17, 2023

Question: now that #705 is merged, are there any changes/functionality on this branch that needs to be merged prior to #558? I'm thinking we may not need the jtc-features branch anymore at all...

@christophfroehlich
Copy link
Contributor Author

no, 558 is standalone now. there will be some general changes to the tests, which are the basis for tests of the other open PRs. I could extract them from 558 if it still needs more discussion, and target that to jtc-features but also onto master

@bmagyar bmagyar deleted the jtc-features branch July 25, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants