RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
312 stars 70 forks source link

Fixes action ClientGoalHandler getResult() and status update bug, #813 #814

Closed wayneparrott closed 2 years ago

wayneparrott commented 2 years ago

Public API Changes ClientGoalHandler (client_goal_handler.js, action_client.d.ts)

  1. deprecated accepting getter
  2. added convenience methods: isAccepting(), isExecuting(), isCanceling(), isSucceeded(), isCanceled(), isAborted(). These methods are not essential. Thus will leave it to the reviewer's discretion of whether they stay or go. The same for #1 above.
  3. status property is now properly updated with the goal lifecycle state codes, see ActionInterfaces.GoalStatus for codes
  4. getResult() now returns a ActionResult. Previously it returned an internal message with structure {status: number, result: ActionResult<T>}.
  5. ClientGoalHandle feedback function now only receives a ActionFeedback<T> argument. Previously ActionFallback<T> function was receiving an internal msg with structure{goal_id: UUID, feedback: ActionFeedback}`.

Description A bug was submitted where ClientGoalHandler.getResult() was returning an internal message that did not conform to the TypeScript declarations. Additionally the ClientGoalHandler.status was never being updated and always return UNKNOWN. Lastly while fixing the getResult() bug it was discovered that the feedbackCallback function was being sent an internal message and not the expected ActionFeedback<T> type. This fix resolves all 3 issues.

I added beforeSetResult(result) method to Deferred.js class. This is used by the ActionClient _getResult(resultMsg) method to update the ClientGoalHandler.status and to return only the msg.result property of type ActionResult<t>.

Updated all action related tests and examples.

Fixed several prettier/lint issues.

Fix #813

minggangw commented 2 years ago

Thanks for the quick action! I noticed the eslint reported 4 errors, which failed the CI. So, would you please fix them first and re-trigger the CI again? Thanks!

wayneparrott commented 2 years ago

After finding a format style that would pass eslint on CI the windows builds are failing on the test-action-xxx and the OSX build fails when compiling native module. I'll give the windows build a look and see if I can identify why the action tests are failing on that platform. Also you can see I churned 3 attempts to get eslint formatting that would pass. This is really odd as my linux dev environment eslint settings are based on rclnodejs and CI eslint is failing on format that used to pass and still passes in my dev env.

wayneparrott commented 2 years ago

update: At one time galactic was running on my window system. Yet currently ros2 galactic will not run on any of my windows configurations, e.g., ros2 -h fails with stacktrace. I even resorted to the undocumented MS version of galactic with no success:

~~// edit: don't use this install as it is a version of the rolling distro at the time of the foxy release choco upgrade ros-galactic-desktop -y --execution-timeout=0 --pre~~

The only good news regarding window is the test suite runs successfully on my ros2 foxy + windows 11 setup. I'm now looking more closely at how appveyor test env is setup.

wayneparrott commented 2 years ago

more info for test-suite failing on windows: 1) I've can successfully run the test-suite on windows w/ ros2 foxy 2) I'm not able to run the test-suite on window w/ ros2 galactic. 2a) The root issue with the test-suite on windows + galactic is due to the rclnodejs_test_msgs are failing to compile as there is a python error for a missing module em. I believe there are 2 python3 environments present: 3.7-x64 which is specified in appveyor.yml and python3.9-x64 which is part of the base image. Our yml script sets up python 3.7 with all of the ros2 dependencies. Yet when the compile_tests.js script runs, python 3.9 is being picked up in the environment and run. Python 3.9 is lacking all of the prerequisite python modules such as empy. Thus rclnodejs_test_msgs package is failing to compile prior to running the test-suite. The test-action-xxx.js test cases depend on the rclnodejs_test_msgs.

--- stderr: rclnodejs_test_msgs
CMake Error at C:/ros2-windows/share/rosidl_adapter/cmake/rosidl_adapt_interfaces.cmake:59 (message):
  execute_process(C:/Python39-x64/python.exe -m rosidl_adapter --package-name
  rclnodejs_test_msgs --arguments-file
  C:/proj/rclnodejs/build/rclnodejs_test_msgs/rosidl_adapter__arguments__rclnodejs_test_msgs.json
  --output-dir
  C:/proj/rclnodejs/build/rclnodejs_test_msgs/rosidl_adapter/rclnodejs_test_msgs
  --output-file
  C:/proj/rclnodejs/build/rclnodejs_test_msgs/rosidl_adapter/rclnodejs_test_msgs.idls)
  returned error code 1:
  Traceback (most recent call last):
    File "C:\Python39-x64\lib\runpy.py", line 197, in _run_module_as_main
      return _run_code(code, main_globals, None,
    File "C:\Python39-x64\lib\runpy.py", line 87, in _run_code
      exec(code, run_globals)
    File "c:\ros2-windows\Lib\site-packages\rosidl_adapter\__main__.py", line 19, in <module>
      sys.exit(main())
    File "c:\ros2-windows\Lib\site-packages\rosidl_adapter\main.py", line 53, in main
      abs_idl_file = convert_to_idl(
    File "c:\ros2-windows\Lib\site-packages\rosidl_adapter\__init__.py", line 18, in convert_to_idl
      from rosidl_adapter.msg import convert_msg_to_idl
    File "c:\ros2-windows\Lib\site-packages\rosidl_adapter\msg\__init__.py", line 16, in <module>
      from rosidl_adapter.resource import expand_template
    File "c:\ros2-windows\Lib\site-packages\rosidl_adapter\resource\__init__.py", line 19, in <module>
      import em
  ModuleNotFoundError: No module named 'em'
Call Stack (most recent call first):
  C:/ros2-windows/share/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake:130 (rosidl_adapt_interfaces)
  CMakeLists.txt:25 (rosidl_generate_interfaces)
---
Failed   <<< rclnodejs_test_msgs [31.6s, exited with code 1]
Summary: 0 packages finished [32.2s]
coveralls commented 2 years ago

Coverage Status

Coverage decreased (-0.08%) to 90.544% when pulling 3de02417a990ed65a39d4cc64546c7d5f77efcbf on wayneparrott:develop into 16acb5545aa87cdb229334f3394377c4c045819a on RobotWebTools:develop.

minggangw commented 2 years ago

Maybe you can reference #812 to get rid of the windows CI problem.