Skip to content

Conversation

bjsowa
Copy link
Member

@bjsowa bjsowa commented Aug 8, 2025

Let's be honest here. The way parameters are passed down from rosbridge_server to the Rosbridge protocol and internal classes is a complete mess:

  • Rosbridge library retrieves some parameters either from:
    1. Protocol.parameters dictionary.
    2. Node handle parameters.
    3. Class variables overwritten by rosbridge_server.
  • RosbridgeWebsocketNode parameter handling is very complex. It's very hard to tell which parameters relate to the server and which are passed to the protocol.
  • The cli argument parsing contains a lot of repeated boilerplate code.

This PR contains the following modifications:

  • Protocol class now allows passing the parameters through the constructor instead of class variable
  • All of the Capability classes retrieve their parameters through protocol.parameters upon initialization (No need to set topics_glob class variables) and use instance variables to get them later (e.g. self.topics_glob instead of Advertise.topics_glob).
  • The default values are defined in class level and are only overwritten in the instance if protocol.parameters dictionary contains the keys.
  • The ROS parameter and cli argument handling in RosbridgeWebsocketNode has been reworked:
    • Added new module constants SERVER_PARAMETERS and PROTOCOL_PARAMETERS that contain parameter name, type, default value and description.
    • Used argparse for parsing cli arguments (less boilerplate code and more user-friendly interface)
    • Made ROS parameters take precedence over cli args (passing cli args modifies the default values for ROS parameters)
    • A single class variable protocol_parameters is set in RosbridgeWebSocket handler, no need to create it on every opened connection
    • Added missing actions_glob parameter

Some changes in behavior:

  • Setting Node parameters before opening Protocol won't affect protocol parameters. The dictionary of parameters needs to be passed in the constructor. This is reflected in tests.
  • (Opinionated) max_message_size actually has effect now, that is, even when client sets the fragment_size, the actual fragment size is capped to max_message_size value.

What's left to do:

  • I need to figure out how the bson parameters are handled, especially the relation between binary_encoder and bson_only_mode.
  • Some real life application testing

Even though this is still WIP, I would appreciate a review

bjsowa added 8 commits August 9, 2025 00:40
Don't retrieve ROS parameters in rosbridge_library, instead, use parameters dictionary passed to RosbridgeProtocol for everything
Instead of passing them using a class variable
Respect max_message_size parameter when determining fragment size, even when client provides fragment_size parameter
@bjsowa bjsowa requested review from sea-bass and EzraBrooks August 9, 2025 13:29
@sea-bass
Copy link
Contributor

sea-bass commented Aug 9, 2025

I won't get to review this for a bit, but have you considered possibly using https://github.com/PickNikRobotics/generate_parameter_library?

Not sure how well this will do with hybrid ROS params and CLI arguments though, but at least may package up your params in a neat struct.

@bjsowa
Copy link
Member Author

bjsowa commented Aug 9, 2025

I won't get to review this for a bit, but have you considered possibly using https://github.com/PickNikRobotics/generate_parameter_library?

Not sure how well this will do with hybrid ROS params and CLI arguments though, but at least may package up your params in a neat struct.

Might be a good idea. I'll try to figure out whether I can use it together with cli args

@bjsowa
Copy link
Member Author

bjsowa commented Aug 9, 2025

@sea-bass I looked at the possibility of using generate_parameter_library and IMO it's not worth it. It will only add complexity instead of simplifying it and won't provide any useful features. All of the parameters are read-only (read only once at the start) and don't require any validation. Also it's not possible to retrieve parameter descriptions for cli args before declaring ROS parameters.

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.

2 participants