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

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

Merged
merged 3 commits into from
Oct 21, 2021
Merged

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

merged 3 commits into from
Oct 21, 2021

Conversation

wayneparrott
Copy link
Collaborator

@wayneparrott wayneparrott commented Oct 6, 2021

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 Initiate the project #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

@wayneparrott wayneparrott changed the title Fixes action goalclienthandler getResult() and status update bug #813 Fixes action goalclienthandler getResult() and status update bug, #813 Oct 6, 2021
@wayneparrott wayneparrott changed the title Fixes action goalclienthandler getResult() and status update bug, #813 Fixes action ClientGoalHandler getResult() and status update bug, #813 Oct 7, 2021
@minggangw
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

wayneparrott commented Oct 12, 2021

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
Copy link
Collaborator Author

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
Copy link

coveralls commented Oct 13, 2021

Coverage Status

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

@minggangw
Copy link
Member

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

Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, LGTM!

@minggangw minggangw merged commit 5f84174 into RobotWebTools:develop Oct 21, 2021
minggangw pushed a commit that referenced this pull request Nov 2, 2021
#814)

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

deprecated accepting getter
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 Initiate the project #1 above.
status property is now properly updated with the goal lifecycle state codes, see ActionInterfaces.GoalStatus for codes
getResult() now returns a ActionResult. Previously it returned an internal message with structure {status: number, result: ActionResult<T>}.
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
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.

Incorrect Type for ClientGoalHandle.getResult
3 participants