Skip to content

fix: remove json encoding before setting string params#521

Merged
mvollrath merged 1 commit intoRobotWebTools:ros2from
travipross:fix-ros2-rosapi-doublequote
Aug 24, 2020
Merged

fix: remove json encoding before setting string params#521
mvollrath merged 1 commit intoRobotWebTools:ros2from
travipross:fix-ros2-rosapi-doublequote

Conversation

@travipross
Copy link
Copy Markdown
Contributor

Since porting to ROS2, it was previously impossible to use Param.set_param() to set string-type parameters.

The input argument value must be JSON-serialized, however, unlike the case of other data types, the later call to get_parameter_value() inside of Param._set_param() does not strip the JSON encoding from the string. This means that the message is constructed with a double-encoded JSON string before being sent to the appropriate node. All string type parameters previously set by rosapi were therefore wrapped with unnecessary quotes.

As the get_parameter_value() method belongs to the ros2 project, it feels appropriate for this to be handled on the rosapi side. This simple fix allows for the proper decoding of strings when passed as the value argument.

No changes appear to be necessary for the Param.get_param() function to work as expected after this modification. Tested with ROS Eloquent on Ubuntu 18.04.

@travipross
Copy link
Copy Markdown
Contributor Author

Anyone have an idea of what's causing these tests to fail and how to address it? Looks like colcon is having a hard time building rosidl_generator_c in a few of these test environments.

@flynneva
Copy link
Copy Markdown
Contributor

hey @travipross ive been looking into this actually.

It looks like I was a bit quick to use the ros-tooling github actions for this repo. They are not fully supported yet for Mac or windows10.

easiest path I think would probably be to limit the CI to only Linux and go from there.

@travipross
Copy link
Copy Markdown
Contributor Author

travipross commented Aug 19, 2020 via email

@flynneva
Copy link
Copy Markdown
Contributor

#524 just opened a PR to remove mac and windows and bump the action version. this might solve the issues seen on eloquent and dashing here

@flynneva
Copy link
Copy Markdown
Contributor

yupp. looks like the ci failure for eloquent an dashing are gone now after bumping the action and adding the vcs-repo-file-url

@travipross
Copy link
Copy Markdown
Contributor Author

travipross commented Aug 19, 2020

That's great news! How can I proceed and re-run my CI workflow with those changes?

@flynneva
Copy link
Copy Markdown
Contributor

if someone merges my pr into the ros2 branch it should trigger a re-run of the ci here

@flynneva
Copy link
Copy Markdown
Contributor

@travipross, maybe close and open this PR? that should trigger a new build i think

@mvollrath mvollrath self-requested a review August 24, 2020 20:28
@mvollrath mvollrath merged commit ed37542 into RobotWebTools:ros2 Aug 24, 2020
@travipross travipross deleted the fix-ros2-rosapi-doublequote branch August 26, 2020 19:33
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.

3 participants