Skip to content

Added MecanumDriveOdometry Python wrapper#549

Merged
ahcorde merged 10 commits intoign-math6from
ahcorde/6/mecanumDriveOdometry_pytohn
Feb 13, 2024
Merged

Added MecanumDriveOdometry Python wrapper#549
ahcorde merged 10 commits intoign-math6from
ahcorde/6/mecanumDriveOdometry_pytohn

Conversation

@ahcorde
Copy link
Copy Markdown
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.

@ahcorde
Added MecanumDriveOdometry
0eb2d28 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Aug 23, 2023
@ahcorde ahcorde mentioned this pull request Aug 23, 2023
Open
@github-actions github-actions bot added 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Aug 23, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 23, 2023 •
edited
Loading

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
Copy Markdown
Contributor

azeey commented Aug 23, 2023

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

@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Aug 23, 2023

this is targeting Fortress, but we can wait no problem

@azeey
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

ahcorde commented Oct 5, 2023

friendly ping @azeey @scpeters or @adityapande-1995

azeey
azeey requested changes Oct 6, 2023
View reviewed changes
src/python_pybind11/src/MecanumDriveOdometry.cc Outdated
std::string pyclass_name = typestr;
py::class_<Class>(m,
pyclass_name.c_str(),
py::buffer_protocol(),
Copy link
Copy Markdown
Contributor

@azeey azeey Oct 6, 2023

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
Copy Markdown
Contributor

@azeey azeey Nov 27, 2023

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/test/MecanumDriveOdometry_TEST.py Outdated
# Setup the wheel parameters, and initialize
odom.set_wheel_params(wheelSeparation, wheelRadius, wheelRadius,wheelRadius)
startTime = datetime.datetime.now()
odom.init(datetime.timedelta())
Copy link
Copy Markdown
Contributor

@azeey azeey Oct 6, 2023

Choose a reason for hiding this comment

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

Should this use startTime?

src/python_pybind11/test/MecanumDriveOdometry_TEST.py Outdated
Angle(1.0 * math.pi / 180),
Angle(1.0 * math.pi / 180),
Angle(1.0 * math.pi / 180),
time1 - startTime)
Copy link
Copy Markdown
Contributor

@azeey azeey Oct 6, 2023

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.

src/python_pybind11/test/MecanumDriveOdometry_TEST.py Outdated
Angle(2.0 * math.pi / 180),
Angle(2.0 * math.pi / 180),
Angle(2.0 * math.pi / 180),
time2 - startTime)
Copy link
Copy Markdown
Contributor

@azeey azeey Oct 6, 2023

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

azeey
azeey requested changes Oct 6, 2023
View reviewed changes
Copy link
Copy Markdown
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

azeey
azeey requested changes Oct 6, 2023
View reviewed changes
Copy link
Copy Markdown
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

@ahcorde
Feedback
d1c70f2 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Merge remote-tracking branch 'origin/ign-math6' into ahcorde/6/mecanu…
f23425e 
…mDriveOdometry_pytohn
@ahcorde
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

ahcorde commented Nov 14, 2023

friendly ping @azeey

1 similar comment
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Nov 27, 2023

friendly ping @azeey

@azeey
Copy link
Copy Markdown
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
Copy Markdown
Contributor

j-rivero commented Dec 7, 2023

@osrf-jenkins run tests

@ahcorde
Added feedback
fc1b7d2 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Added feedback
88ab851 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Dec 14, 2023

@azeey I added your feedback

@azeey
Copy link
Copy Markdown
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
Merge remote-tracking branch 'origin/ign-math6' into ahcorde/6/mecanu…
baa2611 
…mDriveOdometry_pytohn
@ahcorde
Added feedback
e0aaab1 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Copy Markdown
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

azeey
azeey reviewed Feb 13, 2024
View reviewed changes
@ahcorde
Diffdrive test
811947f 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@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
@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

Reviewers

@azeey azeey azeey approved these changes

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

@ahcorde ahcorde

Labels

🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ahcorde @azeey @j-rivero