Allow user to pass custom options to SITL autopilot executable

[Shono1]

I raised this issue #3421 last year, but just got around to writing the code for it now.

Currently, BlueOS does not provide users a way of changing the arguments to the autopilot executable aside from modifying the source code. This PR allows the user to define custom arguments to be passed to the executable in the settings.json file in the autopilot_logs folder. This file is where the existing minimal customization for sitl_frame was stored, so it seems like a logical spot to store more arguments.

As is, my code does not have a frontend and it is not configured to work with non-SITL executables--though the code is written such that extending it to other executable types would not be difficult. I decided to leave this functionality out for now, as my ROVs are mothballed and I am unable to test.

Summary by Sourcery

Allow configuration and persistence of custom execution arguments for the SITL autopilot executable via settings and API endpoints.

New Features:

• Support per-firmware, per-board custom execution arguments for the SITL autopilot executable loaded from settings and defaulted from a JSON file.
• Expose REST endpoints to set and retrieve execution arguments for specific firmware and board combinations.

Enhancements:

• Refactor SITL startup to build the autopilot subprocess argument list from configurable settings, including an optional custom endpoint with a fallback to the previous default.

[CLAassistant]

All committers have signed the CLA.

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In start_sitl, self.configuration is checked for exec_arguments before calling self.settings.load(), which means the default-arguments branch can run off stale in-memory config; consider loading settings (or otherwise ensuring self.configuration is current) before the first exec_arguments existence check to avoid surprising overwrites.
• The structure and keys of arguments["endpoint"] are passed directly into Endpoint(**...), which will raise if the user supplies unexpected keys or omits required ones; it may be safer to normalize/validate this dict before constructing the Endpoint to avoid runtime errors from malformed settings.json.
• Both set_exec_arguments and get_exec_arguments silently swallow all exceptions and either log and return None or do nothing; consider tightening the exception handling (or re-raising a domain-specific error) so callers and the HTTP API can distinguish between "no config" and actual failures.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `start_sitl`, `self.configuration` is checked for `exec_arguments` before calling `self.settings.load()`, which means the default-arguments branch can run off stale in-memory config; consider loading settings (or otherwise ensuring `self.configuration` is current) before the first `exec_arguments` existence check to avoid surprising overwrites.
- The structure and keys of `arguments["endpoint"]` are passed directly into `Endpoint(**...)`, which will raise if the user supplies unexpected keys or omits required ones; it may be safer to normalize/validate this dict before constructing the `Endpoint` to avoid runtime errors from malformed `settings.json`.
- Both `set_exec_arguments` and `get_exec_arguments` silently swallow all exceptions and either log and return `None` or do nothing; consider tightening the exception handling (or re-raising a domain-specific error) so callers and the HTTP API can distinguish between "no config" and actual failures.

## Individual Comments

### Comment 1
<location path="core/services/ardupilot_manager/autopilot_manager.py" line_range="479-483" />
<code_context>
+        else:
+            master_endpoint = Endpoint(**arguments["endpoint"])
+
+        arg_list = [firmware_path]
+        for k, v in arguments.items():
+            if k != "endpoint":
+                arg_list.append(k)
+                if v != "":
+                    arg_list.append(v)
         # pylint: disable=consider-using-with
</code_context>
<issue_to_address>
**issue (bug_risk):** Coerce argument values to strings when building `arg_list` to avoid `Popen` type errors.

In this loop, `v` can be non-string (e.g., ints, bools) depending on `arguments`. `subprocess.Popen` requires each arg to be `str`/`bytes`/`os.PathLike`, so passing arbitrary types risks a runtime error. Please normalize both keys and values to strings, e.g.:

```python
arg_list = [str(firmware_path)]
for k, v in arguments.items():
    if k == "endpoint":
        continue
    arg_list.append(str(k))
    if v != "":
        arg_list.append(str(v))
```
</issue_to_address>

### Comment 2
<location path="core/services/ardupilot_manager/autopilot_manager.py" line_range="465-477" />
<code_context>
-            argument=5760,
-            protected=True,
-        )
+        if "endpoint" not in arguments:
+            logger.warning("Using default endpoint for SITL because none was provided in settings.json.")
+            master_endpoint = Endpoint(
+                name="Master",
+                owner=self.settings.app_name,
+                connection_type=EndpointType.TCPClient,
+                place="127.0.0.1",
+                argument=5760,
+                protected=True,
+            )
+        else:
+            master_endpoint = Endpoint(**arguments["endpoint"])
+
+        arg_list = [firmware_path]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider validating or defaulting fields for the user-provided endpoint dict to avoid inconsistent endpoint instances.

The default path constructs a fully specified `Endpoint`, while the configured path unpacks `arguments["endpoint"]` as-is. If that dict is missing required fields or contains unexpected keys/types, it can raise at runtime or yield endpoints with different behavior. Normalizing the dict (e.g., apply defaults for missing keys and ignore unknown fields) before unpacking would avoid these inconsistencies.

```suggestion
        # ArduPilot SITL binary will bind TCP port 5760 (server) and the mavlink router will connect to it as a client
        default_endpoint_args = {
            "name": "Master",
            "owner": self.settings.app_name,
            "connection_type": EndpointType.TCPClient,
            "place": "127.0.0.1",
            "argument": 5760,
            "protected": True,
        }

        endpoint_config = arguments.get("endpoint")
        if endpoint_config is None:
            logger.warning("Using default endpoint for SITL because none was provided in settings.json.")
            master_endpoint_args = default_endpoint_args
        else:
            if not isinstance(endpoint_config, dict):
                logger.warning(
                    "Invalid endpoint configuration type '%s'; falling back to default endpoint for SITL.",
                    type(endpoint_config).__name__,
                )
                master_endpoint_args = default_endpoint_args
            else:
                # Start with defaults, override only known fields, ignore unknown keys
                master_endpoint_args = dict(default_endpoint_args)
                for key, value in endpoint_config.items():
                    if key in default_endpoint_args:
                        master_endpoint_args[key] = value
                    else:
                        logger.debug("Ignoring unknown endpoint field '%s' in settings.json.", key)

        master_endpoint = Endpoint(**master_endpoint_args)
```
</issue_to_address>

### Comment 3
<location path="core/services/ardupilot_manager/autopilot_manager.py" line_range="455-457" />
<code_context>
         self.firmware_manager.validate_firmware(firmware_path, self._current_board.platform)

+        if "exec_arguments" not in self.configuration or str(firmware_path) not in self.configuration["exec_arguments"]:
+            with open(pathlib.Path(__file__).parent.resolve() / "default_arguments.json", "r", encoding="ascii") as f:
+                default_config = json.load(f)
+            logger.warning(f"Setting defaults parameters for SITL to {default_config}")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using ASCII encoding for `default_arguments.json` is brittle; UTF-8 is safer for JSON.

Using `encoding="ascii"` will raise `UnicodeDecodeError` as soon as non-ASCII characters appear in this JSON (e.g., in labels or paths). Please switch to `encoding="utf-8"`, which is the de-facto standard for JSON and preserves current behavior for ASCII-only content while being more robust.

```suggestion
        if "exec_arguments" not in self.configuration or str(firmware_path) not in self.configuration["exec_arguments"]:
            with open(pathlib.Path(__file__).parent.resolve() / "default_arguments.json", "r", encoding="utf-8") as f:
                default_config = json.load(f)
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Coerce argument values to strings when building arg_list to avoid Popen type errors.

In this loop, v can be non-string (e.g., ints, bools) depending on arguments. subprocess.Popen requires each arg to be str/bytes/os.PathLike, so passing arbitrary types risks a runtime error. Please normalize both keys and values to strings, e.g.:

arg_list = [str(firmware_path)]
for k, v in arguments.items():
    if k == "endpoint":
        continue
    arg_list.append(str(k))
    if v != "":
        arg_list.append(str(v))

[sourcery-ai]

suggestion (bug_risk): Consider validating or defaulting fields for the user-provided endpoint dict to avoid inconsistent endpoint instances.

The default path constructs a fully specified Endpoint, while the configured path unpacks arguments["endpoint"] as-is. If that dict is missing required fields or contains unexpected keys/types, it can raise at runtime or yield endpoints with different behavior. Normalizing the dict (e.g., apply defaults for missing keys and ignore unknown fields) before unpacking would avoid these inconsistencies.

Suggested change

	# ArduPilot SITL binary will bind TCP port 5760 (server) and the mavlink router will connect to it as a client
	master_endpoint = Endpoint(
	name="Master",
	owner=self.settings.app_name,
	connection_type=EndpointType.TCPClient,
	place="127.0.0.1",
	argument=5760,
	protected=True,
	)
	if "endpoint" not in arguments:
	logger.warning("Using default endpoint for SITL because none was provided in settings.json.")
	master_endpoint = Endpoint(
	name="Master",
	owner=self.settings.app_name,
	connection_type=EndpointType.TCPClient,
	place="127.0.0.1",
	argument=5760,
	protected=True,
	)
	else:
	master_endpoint = Endpoint(**arguments["endpoint"])
	# ArduPilot SITL binary will bind TCP port 5760 (server) and the mavlink router will connect to it as a client
	default_endpoint_args = {
	"name": "Master",
	"owner": self.settings.app_name,
	"connection_type": EndpointType.TCPClient,
	"place": "127.0.0.1",
	"argument": 5760,
	"protected": True,
	}
	endpoint_config = arguments.get("endpoint")
	if endpoint_config is None:
	logger.warning("Using default endpoint for SITL because none was provided in settings.json.")
	master_endpoint_args = default_endpoint_args
	else:
	if not isinstance(endpoint_config, dict):
	logger.warning(
	"Invalid endpoint configuration type '%s'; falling back to default endpoint for SITL.",
	type(endpoint_config).__name__,
	)
	master_endpoint_args = default_endpoint_args
	else:
	# Start with defaults, override only known fields, ignore unknown keys
	master_endpoint_args = dict(default_endpoint_args)
	for key, value in endpoint_config.items():
	if key in default_endpoint_args:
	master_endpoint_args[key] = value
	else:
	logger.debug("Ignoring unknown endpoint field '%s' in settings.json.", key)
	master_endpoint = Endpoint(**master_endpoint_args)