feat: improve client simulator CLI options#320
Conversation
- Remove --enable-recv option and make receiving default behavior - Change server address format from tcp://localhost to localhost (accept old format for backward compatibility) - Add --transform-send-rate option to control transform update frequency in Hz Co-authored-by: Masahiro Yamaguchi <from2001@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the client simulator CLI by making receiving behavior the default, simplifying server address format, and adding configurable transform update rates. The changes align with the requirements in issue #319.
Changes:
- Changed
enable_receiveparameter default fromFalsetoTruein bothSimulatedClientandClientSimulatorclasses, making broadcast reception the default behavior - Removed
--enable-recvCLI option and hardcodedenable_receive=Truein the simulator initialization - Changed default server address format from
tcp://localhosttolocalhostwith automatic normalization to add thetcp://prefix when needed - Added
--transform-send-rateCLI option (default 10.0 Hz) to control transform update frequency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "--transform-send-rate", | ||
| type=float, | ||
| default=10.0, | ||
| help="Transform update send rate in Hz (default: 10.0)", |
There was a problem hiding this comment.
The transform_send_rate parameter lacks input validation. If a user provides zero or a negative value, this will cause a division by zero error or produce a negative interval at line 1023 where update_interval = 1.0 / self.transform_send_rate is calculated. Consider adding validation to ensure transform_send_rate is positive (e.g., > 0) when parsing the argument.
| if not args.server.startswith("tcp://") and not args.server.startswith("ipc://"): | ||
| args.server = f"tcp://{args.server}" |
There was a problem hiding this comment.
The server address normalization logic doesn't handle addresses that contain a port number. For example, if a user provides "localhost:5555", this will be transformed to "tcp://localhost:5555", which may not be the intended behavior if the port is meant to override the --dealer-port argument. Consider clarifying the expected behavior or adding validation to detect and handle port numbers in the address.
| if not args.server.startswith("tcp://") and not args.server.startswith("ipc://"): | |
| args.server = f"tcp://{args.server}" | |
| server_input = args.server | |
| if not server_input.startswith("tcp://") and not server_input.startswith("ipc://"): | |
| # Detect and reject ambiguous "host:port" inputs without a scheme. | |
| # Users should either: | |
| # - provide a full ZeroMQ URL (e.g., "tcp://localhost:5555"), or | |
| # - specify the host via --server and the port via --dealer-port. | |
| if server_input.count(":") == 1: | |
| host_part, port_part = server_input.rsplit(":", 1) | |
| if port_part.isdigit(): | |
| parser.error( | |
| "Server address must not include a port when no scheme is given. " | |
| 'Use "--server HOST --dealer-port PORT" or provide a full URL like ' | |
| '"tcp://HOST:PORT".' | |
| ) | |
| args.server = f"tcp://{server_input}" |
Summary
--enable-recvoption and make receiving default behaviortcp://localhosttolocalhost(accept old format)--transform-send-rateoption to control transform update frequency in HzCloses #319
🤖 Generated with Claude Code