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

Added MecanumDriveOdometry Python wrapper #549

Merged
merged 10 commits into from
Feb 13, 2024

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 23, 2023

🎉 New feature

Closes #548

Summary

Added MecanumDriveOdometry

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde self-assigned this Aug 23, 2023
@github-actions github-actions bot added 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Aug 23, 2023
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c5c3fc4) 99.38% compared to head (811947f) 99.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           ign-math6     #549   +/-   ##
==========================================
  Coverage      99.38%   99.38%           
==========================================
  Files             75       75           
  Lines           7029     7029           
==========================================
  Hits            6986     6986           
  Misses            43       43           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azeey
Copy link
Contributor

azeey commented Aug 23, 2023

Is it okay if we wait till after Harmonic for this?

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 23, 2023

this is targeting Fortress, but we can wait no problem

@azeey
Copy link
Contributor

azeey commented Aug 23, 2023

this is targeting Fortress, but we can wait no problem

I understand, but I think we should focus our review efforts on beta labeled PRs.

@ahcorde
Copy link
Contributor Author

ahcorde commented Oct 5, 2023

friendly ping @azeey @scpeters or @adityapande-1995

std::string pyclass_name = typestr;
py::class_<Class>(m,
pyclass_name.c_str(),
py::buffer_protocol(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need py::buffer_protocol here? If so, doesn't it require implementing def_buffer (https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this has been addressed

src/python_pybind11/src/MecanumDriveOdometry.cc Outdated Show resolved Hide resolved
# Setup the wheel parameters, and initialize
odom.set_wheel_params(wheelSeparation, wheelRadius, wheelRadius,wheelRadius)
startTime = datetime.datetime.now()
odom.init(datetime.timedelta())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use startTime?

Angle(1.0 * math.pi / 180),
Angle(1.0 * math.pi / 180),
Angle(1.0 * math.pi / 180),
time1 - startTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

The C++ version uses just time1 here.

Angle(2.0 * math.pi / 180),
Angle(2.0 * math.pi / 180),
Angle(2.0 * math.pi / 180),
time2 - startTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also different from the C++ test

src/python_pybind11/test/MecanumDriveOdometry_TEST.py Outdated Show resolved Hide resolved
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few issues with pybind11 options and differences with the C++ test

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few issues with pybind11 options and differences with the C++ test

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Oct 16, 2023

@azeey Time is a little bit different in Python, you can take a look the DiffDriveOdometry_TEST.py test,

@ahcorde ahcorde requested a review from azeey October 16, 2023 13:02
@ahcorde ahcorde changed the title Added MecanumDriveOdometry Added MecanumDriveOdometry Python wrapper Nov 14, 2023
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 14, 2023

friendly ping @azeey

1 similar comment
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 27, 2023

friendly ping @azeey

@azeey
Copy link
Contributor

azeey commented Nov 27, 2023

@azeey Time is a little bit different in Python, you can take a look the DiffDriveOdometry_TEST.py test,

So from the pybind11 docs, it looks like std::chrono::steady_clock::time_point is converted to datetime.timedelta. This makes it really inconvenient to use since datetime.timedelta() always returns a "0" time delta as opposed to the current monotonic (in C++ steady_clock) time. One option is to use datetime.timedelta(time.monotonic()) to initialize startTime instead of datetime.datetime.now().

@j-rivero
Copy link
Contributor

j-rivero commented Dec 7, 2023

@osrf-jenkins run tests

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 14, 2023

@azeey I added your feedback

@azeey
Copy link
Contributor

azeey commented Dec 14, 2023

@azeey I added your feedback

Thanks. I think now the test can be updated to match the C++ version. Specifically, odom.init(startTime) instead of odom.init(datetime.timedelta()) and remove startTime from odom.update calls.

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 13, 2024

Thanks. I think now the test can be updated to match the C++ version. Specifically, odom.init(startTime) instead of odom.init(datetime.timedelta()) and remove startTime from odom.update calls.

@azeey Included feedback

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from azeey February 13, 2024 21:46
@ahcorde ahcorde enabled auto-merge (squash) February 13, 2024 21:57
@ahcorde ahcorde merged commit 5b2522a into ign-math6 Feb 13, 2024
11 checks passed
@ahcorde ahcorde deleted the ahcorde/6/mecanumDriveOdometry_pytohn branch February 13, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants