Allow users to change the system ID in the Vehicle Setup component - #4365
Allow users to change the system ID in the Vehicle Setup component#4365nukelet wants to merge 4 commits into
Conversation
8705e73 to
d5fce82
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Error handling, input validation, and coordination between the two system-ID updates must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds unified vehicle system-ID configuration across ArduPilot and BlueOS services.
Changes:
- Adds persistent
MAV_SYSTEM_IDconfiguration. - Adds Vehicle Setup editing and restart controls.
- Displays the active system ID.
File summaries
| File | Description |
|---|---|
core/services/ardupilot_manager/settings.py |
Defines the startup configuration path. |
core/services/ardupilot_manager/api/v1/routers/index.py |
Adds the system-ID persistence endpoint. |
core/frontend/src/components/vehiclesetup/overview/VehicleInfo.vue |
Displays the active system ID. |
core/frontend/src/components/vehiclesetup/overview/SystemId.vue |
Adds system-ID editing and restart controls. |
core/frontend/src/components/vehiclesetup/Configure.vue |
Registers the Vehicle ID setup page. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except Exception as error: | ||
| logger.warning(f"Unable to write MAV_SYSTEM_ID to bootstrap/startup.json: {error}") |
|
Cool! I didn't test it yet, but I have one point: if we move the parameter set part to the backend, we allow non-UI clients to complete the full change-mavlink-id journey. If so, I'd adjust the endpoint name to be more generic. Thanks |
|
Yeah, that's a good idea, I'll move the |
c489361 to
cac5a82
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The update can leave IDs inconsistent, and the verification loop unnecessarily delays every save.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Balanced
| with open(autopilot.settings.startup_settings_file, "r+", encoding="utf-8") as startup_settings: | ||
| settings = json.load(startup_settings) | ||
| environment = settings["core"].get("environment", []) | ||
|
|
||
| # make sure to remove MAV_SYSTEM_ID if it is already defined in | ||
| # bootstrap/startup.json | ||
| environment = [v for v in environment if not v.startswith("MAV_SYSTEM_ID=")] | ||
| environment.append(f"MAV_SYSTEM_ID={value}") | ||
| settings["core"]["environment"] = environment | ||
|
|
||
| startup_settings.seek(0) | ||
| startup_settings.write(json.dumps(settings, indent=2)) | ||
| startup_settings.truncate() |
There was a problem hiding this comment.
i disagree with the bot here, if we fail to write to startup.json then we have bigger problems to worry about. it's definitely not the weakest link here
Implement support for changing the vehicle's system id on the firmware side through a PARAM_SET message that configures the MAV_SYSID parameter. Signed-off-by: Vinicius Peixoto <vinicius@nukelet.dev>
…ration This endpoint sets the MAV_SYSID firmware parameter and also sets the MAV_SYSTEM_ID environment variable (used by mavlink2rest) to its new value by updating bootstrap/startup.json. Signed-off-by: Vinicius Peixoto <vinicius@nukelet.dev>
Introduce the SystemId component with an input field for setting the vehicle's system ID. It also comes with two buttons: - A "Save" button that calls ardupilot_manager's /system_id endpoint to change the system id - A "Reboot Core" button which restarts the blueos-core container since the MAV_SYSTEM_ID environment variable change requires a container reboot to propagate Signed-off-by: Vinicius Peixoto <vinicius@nukelet.dev>
Follow the same naming convention as the command_long_message and param_set_message helpers. Signed-off-by: Vinicius Peixoto <vinicius@nukelet.dev>
cac5a82 to
ddefebf
Compare
|
I moved the system ID change operation entirely into the backend and renamed the endpoint to curl -X POST http://<blueos-addr>/autopilot-manager/v1.0/system_id\?value\=42 -vThis was a bit more painful to figure out than I anticipated since there isn't a clean way for the onboard computer to know that the |
patrickelectric
left a comment
There was a problem hiding this comment.
It's in a good direction, let's continue
| while time.time() - start_time < timeout: | ||
| if await self.is_heart_beating(): | ||
| continue |
There was a problem hiding this comment.
yes, this should have been a break instead of continue. definitely needed more coffee when wrapping up these changes
| logger.warning(message) | ||
| return PlainTextResponse(message, status_code=503) | ||
|
|
||
| try: |
There was a problem hiding this comment.
IIRC try here is unnecessary since you are using the index_to_http_exception decorator
| url: '/version-chooser/v1.0/version/restart', | ||
| }).finally(() => { | ||
| // Give the backend a bit to go down, then reload so the user reconnects to the fresh core | ||
| setTimeout(() => window.location.reload(), 15000) |
There was a problem hiding this comment.
we should not guess, take a look in PowerMenu: waitForBackendToBeOnline
| try: | ||
| await autopilot.vehicle_manager.set_system_id(value) | ||
|
|
||
| if autopilot.vehicle_manager.target_system != value: |
There was a problem hiding this comment.
wait, isn't this supposed to be done already in set_system_id ?
| if autopilot.vehicle_manager.target_system != value: | ||
| return PlainTextResponse("Failed to set system ID", status_code=500) | ||
|
|
||
| with open(autopilot.settings.startup_settings_file, "r+", encoding="utf-8") as startup_settings: |
There was a problem hiding this comment.
it would be better to have it encapsulated by a class or something, maybe the settings itself, over opening and manipulating a settings file inside an api endpoint.
There was a problem hiding this comment.
also.. maybe not even in autopilot.settings, but maybe autopilot.bootstrap.settings, with that you can create the abstraction in autopilot.bootstrap; also, if we are going to have shared logic of bootstrap here and with other services, does it worth to have a commonwealth place for it ?

We would like to provide a unified way for users to configure the system ID for a vehicle. As it currently stands, there are two sources of truth for the vehicle ID that need to be kept in sync:
MAV_SYSIDparameter in the Ardupilot firmwareMAV_SYSTEM_IDenvironment variable, which is used to determine the vehicle ID inmavlink-server/mavlink2restThis PR aims to provide an interface in the UI for users to change the vehicle ID:
autopilot_managerendpoint,/system_id, to set the value forMAV_SYSTEM_IDthrough a POST request, making the change persistent across reboots by storing the variable in theenvironmentfield ofbootstrap/startup.jsonNote that (currently) it is necessary to restart the BlueOS container in order to have the
MAV_SYSTEM_IDenvironment variable propagated to all services (there is a button in the Vehicle ID setup UI for restarting the container).Depends on #4364
Fixes #3604