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.

Sign up for GitHub

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

Jump to bottom

fix delete_param in rosapi #284

Merged
merged 1 commit into from
Jul 5, 2017
Merged

fix delete_param in rosapi #284

merged 1 commit into from
Jul 5, 2017

Conversation

jihoonl
Copy link
Member

@jihoonl jihoonl commented Jun 29, 2017

To fix

[/rosapi ERROR 1498735222.314929]: Error processing request: has_param() takes exactly 2 arguments (1 given)
['Traceback (most recent call last):\n', '  File "/opt/ros/kinetic/lib/python2.7/dist-packages/rospy/impl/tcpros_service.py", line 625, in _handle_request\n    response = convert_return_to_response(self.handler(request), self.response_class)\n', '  File "/home/jihoonl/work/ros/webtools/src/rosbridge_suite/rosapi/scripts/rosapi_node", line 190, in delete_param\n    rosapi.params.delete_param(request.name, params_glob)\n', '  File "/home/jihoonl/work/ros/webtools/src/rosbridge_suite/rosapi/src/rosapi/params.py", line 97, in delete_param\n    if has_param(name):\n', 'TypeError: has_param() takes exactly 2 arguments (1 given)\n']
[/rosbridge_websocket ERROR 1498735222.315634]: [Client 1] [id: call_service:/rosapi/delete_param:6] call_service ServiceException: service [/rosapi/delete_param] responded with an error: error processing request: has_param() takes exactly 2 arguments (1 given)

adolfo-rt
adolfo-rt reviewed Jun 29, 2017
View reviewed changes
rosapi/src/rosapi/params.py
with param_server_lock:
if has_param(name):
if has_param(name, params_glob):
with param_server_lock:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't L86-87 affected by the same issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. L86-87 is to call rospy.has_param whereas this line is to call has_param. Would it be more make sense to call rospy.has_param directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping here, it would seem to me that there's another place affected by this issue as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I double-checked the code if there is another place affected by this issue. I couldn't find any other places. Can you point out which code is possibly affected by this?

Copy link
Member Author

@jihoonl jihoonl Jul 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

my comments were in pending until now...

Copy link
Contributor

Choose a reason for hiding this comment

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

I initially meant this, but on closer inspection it's not calling has_param(name), but rather rospy.has_param(name).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I assume that you meant it is no problem. I am merging it.

@jihoonl jihoonl mentioned this pull request Jun 29, 2017
Merged
adolfo-rt
adolfo-rt approved these changes Jul 4, 2017
View reviewed changes
rosapi/src/rosapi/params.py
with param_server_lock:
if has_param(name):
if has_param(name, params_glob):
with param_server_lock:
Copy link
Contributor

Choose a reason for hiding this comment

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

I initially meant this, but on closer inspection it's not calling has_param(name), but rather rospy.has_param(name).

@jihoonl jihoonl merged commit db74eca into develop Jul 5, 2017
@adolfo-rt adolfo-rt deleted the fix_rospai branch July 5, 2017 08:03
jihoonl added a commit that referenced this pull request Aug 30, 2017
@jihoonl
0.8.1 (#289)
75513f1 
* Added new parameters for topic and service security.

Added 3 new parameters to rosapi and rosbridge_server which filter the
topics, services, and parameters broadcast by the server to match an
array of glob strings.

* Two minor fixes.

* Fixed time object field definitions to match documentation.

* As per the suggestions of @T045T, fixed several typos, improved logging, and made some style fixes.

* Added services_glob to CallServices, added globs to rosbridge_tcp and rosbridge_udp, and other miscellanous fixes.

* add missing imports and correct default values for glob parameters

* correct default values for security globs

also accept empty list as the default "do not check globs" value in addition to None.

Finally, append rosapi service glob after processing command line input so it's not overwritten

* add missing import

* adjust log level for security globs

Normal operation (i.e. no globs or successful verification of requests) is now silent, with illegal requests producing a warning.

* new service: get actionlib servers

* no rospy needed, just for debug logging

* Delay unregister to mitigate !138

* Fix: Set default to publish all topics

Without better doc, one does not understand why no topics are published. I thought, something is broken.
With this defaults, everything is working out of the box. And for a more secure setup, one can change it.

* Added default topics to all launch files, and fixed bug where it would crash if nothing was put into the lists as values

* Added bug fix in rosapi

* Fixed the launch files for the tcp and udp service. Without these modifications, the rosapi node fails because some rosparams are not defined properly before. Now the launchfiles comply to the websocket version.

* update changelog

* 0.7.17

* Implemented a bson_only_mode flag for the TCP version of rosbridge; This allows you to switch to a full-duplex transmission of BSON messages and therefore eliminates the need for a base64 encoding of binary data; Use the new mode by starting:'roslaunch rosbridge_server rosbridge_tcp.launch bson_only_mode:=True' or passing '--bson_only_mode' to the rosbridge_tcp.py script

* minor change in variable usage

* Move UNREGISTER_TIMEOUT to member class so it's accessible from outside

* Fix missing tests due to delayed unregistration

* Fix test advertise errors after delayed unregister changes

* Reduce timeout for tests

Tests will sleep for 10% extra of the timeout to prevent some situations
were the test sleep ended right before the unregister timer fired

* Change class constant to module constant

* Update protocol.py

Prevent a KeyError when bson_only_mode is unset.

* Set default for bson_only_mode in websocket handler and launch file.

* Create README.md

* Update README.md

Formatting and examples

* don't try to set TCP nodelay option for UDP

* expose binary_encoder rosparam that was hidden in deep depth

* correct the possible argument

* address review comment. more explicitly describe valid args

* Cleaning up travis configuration (#283)

configure travis to use industial ci configuration. Now it uses xenial and kinetic

* fix delete_param in rosapi (#284)

* added topic and service names to warnings in order to improve immediate debugability; minor linting (#286)

* Fixing errors in test. (#287)

* appease call service using timeout and proper arrival waiting

* add queue_size on publisher

* test them individually.

* add time after wait for service

* update changelog

* 0.8.0

* remove ujson from dependency to build in trusty (#290)

* update changelog

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

@adolfo-rt adolfo-rt adolfo-rt approved these changes

Assignees

@adolfo-rt adolfo-rt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jihoonl @adolfo-rt