Skip to content

Conversation

ejalaa12
Copy link
Contributor

Public API Changes
None

Description

This PR fixes the following issues with parameters when the parameter type is an array. (in order of commits)

  • the parameter type PARAMETER_STRING_ARRAY was not handled everywhere
  • values of parameters of type PARAMETER_[BYTE/INTEGER/DOUBLE]_ARRAY are of type TypedArray which throws aTypeError('incompatible value') in the validate() method. This is because _validArray() (more specifically Array.isArray() returns false when given a TypedArray
  • arrayElementType of PARAMETER_BYTE_ARRAY was set to PARAMETER_BYTE which is a type that does not exist (undefined). We can use PARAMETER_INTEGER instead.

Unit tests were added to reproduce the bugs, then the next commits fix those bugs.

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 fixing this and the unit test! yeah, we should convert it back to Array, @wayneparrott would you please take a look from your side and see if we need to change for TS accordingly, if not please help to merge it, many thanks!

@wayneparrott wayneparrott merged commit 2e05913 into RobotWebTools:develop Jun 1, 2022
@wayneparrott
Copy link
Collaborator

@ejalaa12 thx for helping improve parameter support. Your efforts have been most appreciated.

OT - may I ask how you are using rclnodejs? Also please open an issue if you have suggestions for improving the use of rclnodejs including the onboarding experience.

@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Jun 1, 2022

@wayneparrott you're welcome. It's a great job you guys are doing already.

We're using rclnodejs as a backend to the HMI that controls our ros2 robot and some other products.

We'll be sure to drop new PR/Issue when needed :)

Thanks for the merge !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants