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

fix for Noetic #4

Merged
merged 11 commits into from
Jun 26, 2024
Merged

fix for Noetic #4

merged 11 commits into from
Jun 26, 2024

Conversation

kirohy
Copy link

@kirohy kirohy commented Apr 3, 2024

To run rtmlaunch hrpsys_ros_bridge samplerobot.launch USE_UNSTABLE_RTC:=true, this pull req and k-okada/rtmros_common#11 are needed.

@k-okada
Copy link
Owner

k-okada commented Apr 4, 2024

please split this into 3 commits, 2to3 -f map, 2to3 -f filter, and 2to3 -f print, because we need to check that this branch also works with Python2 andw we can easly roll back each commit in case it fails.

I'm not sure if print((a+b)) is more appropriate than print(a+b), so I'll have to wait for Pythonista's comments...

@kirohy
Copy link
Author

kirohy commented Apr 4, 2024

We have to use -p flag when applying 2to3 -f print, if print is already used as a function. Without -p flag and from __future__ import print_function, 2to3 automatically adds extra parentheses to print(hoge).
https://stackoverflow.com/questions/55559825/how-to-fix-printdouble-parentheses-after-2to3-conversion
https://docs.python.org/3.8/library/2to3.html#using-2to3
Double parenthese do not make sense, so I'll add a commit to manually remove them. Because some file have both print "hoge" and print("hoge").

@kirohy
Copy link
Author

kirohy commented Apr 4, 2024

fix bugs to run sample simulations in https://github.com/fkanehiro/hrpsys-base/blob/master/sample/SampleRobot/README.md except for KalmanFilter. This rtc could not invoke resetKalmanFilterState().

Traceback (most recent call last):
  File "./samplerobot_kalman_filter.py", line 196, in <module>
    demo()
  File "./samplerobot_kalman_filter.py", line 168, in demo
    hcf.kf_svc.resetKalmanFilterState()
  File "/home/leus/catkin_ws/noetic_test/devel/lib/python3/dist-packages/hrpsys/KalmanFilterService_idl.py", line 101, in resetKalmanFilterState
    return self._obj.invoke("resetKalmanFilterState", _0_OpenHRP.KalmanFilterService._d_resetKalmanFilterState, args)
omniORB.CORBA.COMM_FAILURE: CORBA.COMM_FAILURE(omniORB.COMM_FAILURE_WaitingForReply, CORBA.COMPLETED_MAYBE)

This KalmanFilter sample works well in melodic.

@kirohy kirohy changed the title apply 2to3 on hrpsys_config.py fix for python3 in Noetic Apr 5, 2024
@kirohy kirohy changed the title fix for python3 in Noetic fix for Noetic Apr 11, 2024
@kirohy
Copy link
Author

kirohy commented Apr 11, 2024

Fixed Kalmanfilter sample problem.

Copy link
Owner

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

I have cherry picked reliable commits. I am sorry for the conflict, but hop you can rebase and fix it.

@@ -1,247 +1,247 @@
from javax.swing import *
Copy link
Owner

Choose a reason for hiding this comment

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

please split PR for python3 and style change. I would recommend revert this fix and apply 2to3.

Copy link
Author

@kirohy kirohy Apr 12, 2024

Choose a reason for hiding this comment

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

This file is originally written in ASCII (CRLF), but 2to3 changed into UTF-8 (LF). style change means this? or should I keep this file as ASCII style?

Copy link
Owner

@k-okada k-okada Jun 13, 2024

Choose a reason for hiding this comment

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

style change means this?

Yes

or should I keep this file as ASCII style?

I prefer this option, because I personally won't be using waitInput.py in the future and do not want to make unncessary changes to this file.

package.xml Outdated
@@ -51,6 +51,7 @@
<run_depend>openhrp3</run_depend>
<run_depend>python-tk</run_depend>
<run_depend>sdl</run_depend>
<run_depend>python3-packaging</run_depend>
Copy link
Owner

Choose a reason for hiding this comment

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

use <exec_depend condition="$ROS_PYTHON_VERSION == 3" />

@@ -772,7 +772,7 @@ def demoStandingPosResetting():
def demo():
start_time = time.time()
init()
from distutils.version import StrictVersion
from packaging.version import parse as StrictVersion
Copy link
Owner

Choose a reason for hiding this comment

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

What is the pupose of this chnage? to support parse('"314.0.0"') > parse('"314.10.0"') ? but it not work with parse('"314.0.0"') > parse('314.10.0')

Copy link
Author

@kirohy kirohy Apr 12, 2024

Choose a reason for hiding this comment

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

I know this problem when parsing a version string with double quatations. kirohy@9488858 can avoid such situations.

This change is because you have already replaced distutils into packaging in ecfefb9#diff-9a55a4c37694757a5f9204a027881ffaa4793e2f5c144b586d7248ae5db441a5R15. Should I revert them? Then kirohy@10b4db8 will be deleted.

Copy link
Owner

Choose a reason for hiding this comment

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

If this works for both python2/python3 this is ok, if not, then we need to make changes as small as possibe and switch the code using python2/python3 conditional branches.

@kirohy kirohy force-pushed the fix_20.04 branch 2 times, most recently from 345fc1c to f07ba6a Compare June 13, 2024 11:48
Copy link
Author

@kirohy kirohy left a comment

Choose a reason for hiding this comment

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

check simulation on melodic and noetic, and edit code. To revert old changes, force pushed

CMakeLists.txt Outdated
@@ -190,7 +190,7 @@ install(FILES
${CMAKE_CURRENT_BINARY_DIR}/hrpsys-base.pc
DESTINATION lib/pkgconfig)

add_definitions(-DHRPSYS_PACKAGE_VERSION=\"\\"${CPACK_PACKAGE_VERSION_MAJOR}.${CPACK_PACKAGE_VERSION_MINOR}.${CPACK_PACKAGE_VERSION_PATCH}\\"\")
add_definitions(-DHRPSYS_PACKAGE_VERSION=\"${CPACK_PACKAGE_VERSION_MAJOR}.${CPACK_PACKAGE_VERSION_MINOR}.${CPACK_PACKAGE_VERSION_PATCH}\")
Copy link
Owner

Choose a reason for hiding this comment

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

@kirohy what happens without this changes?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this change cannot built in Melodic. In Noetic wthout this change, hrpsys sample fails with ValueError: invalid version number '"315.15.0"'. This caued by python3 problem, so I'll revert this file and edit hrpsys python sample scripts.

@k-okada k-okada merged commit ae40b59 into k-okada:fix_20.04 Jun 26, 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