Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an annotation synchronization framework that enables bidirectional synchronization of waypoint annotations between external fleet management systems and InOrbit's Config API. The implementation provides a reusable framework that any connector can use by implementing two simple interfaces.
Changes:
- Adds annotation synchronization framework with pluggable provider/converter interfaces
- Implements InOrbit Config API client for managing SpatialAnnotation objects
- Integrates annotation sync into FleetConnector with per-frame_id manager creation
- Includes comprehensive test suite with 1191 lines of tests covering lifecycle, sync modes, and edge cases
- Adds documentation and working example implementation
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_waypoint_sync.py | Comprehensive test suite for annotation sync (config, models, API client, manager, lifecycle) |
| pyproject.toml | Adds httpx dependency for REST API client |
| inorbit_connector/models.py | Adds rest_api_url field and AnnotationSyncConfig to ConnectorConfig |
| inorbit_connector/inorbit/models.py | Defines Config API models (ConfigObject, SpatialAnnotation, WaypointAnnotationSpec) |
| inorbit_connector/inorbit/config_api.py | Implements InOrbitConfigAPI client for CRUD operations on config objects |
| inorbit_connector/inorbit/init.py | Exports Config API models and client |
| inorbit_connector/connector.py | Integrates annotation sync with FleetConnector (registration, per-frame managers, lifecycle) |
| inorbit_connector/annotation_sync/models.py | Defines AnnotationSyncConfig and AnnotationSyncMode enum |
| inorbit_connector/annotation_sync/manager.py | Implements AnnotationSyncManager for bidirectional synchronization |
| inorbit_connector/annotation_sync/interfaces.py | Defines Protocol interfaces (ExternalAnnotationProvider, AnnotationConverter) |
| inorbit_connector/annotation_sync/init.py | Exports annotation sync framework components |
| inorbit_connector/init.py | Re-exports InOrbit Config API models |
| examples/fleet-connector/waypoint_sync.py | Example implementation of provider and converter for mock external system |
| examples/fleet-connector/fleet_client.py | Updates frame_id to match example configuration |
| examples/fleet-connector/connector.py | Demonstrates annotation sync registration in connector |
| examples/example.fleet.yaml | Adds annotation_sync configuration section |
| examples/README.md | Documents annotation sync in fleet connector example |
| docs/* | Comprehensive documentation for annotation sync feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@cursor review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
bc02be4 to
41a9127
Compare
…arate concerns and reduce implementation size
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
41a9127 to
c43a839
Compare
miguelgarcia
left a comment
There was a problem hiding this comment.
Hey I made several suggestions. I have various doubts about the chosen names, how the inorbit -> robot sync work, and how this would work for multi-floor.
Anyway this is amazing progress :D
| class MyAnnotationConverter: | ||
| """Converts between positions and waypoint annotations.""" | ||
|
|
||
| def position_to_annotation( |
There was a problem hiding this comment.
Again, I would just use annotation and not position. This could be robot_to_inorbit and inorbit_to_robot. That would make super clear the conversion direction without having to remember that a position is an annotation in the robot side.
| # Update robot ID cache | ||
| self.__robot_ids = [robot.robot_id for robot in self.config.fleet] | ||
|
|
||
| def register_annotation_sync( |
There was a problem hiding this comment.
nit. I think this should be plural register_annotations_sync
| @@ -0,0 +1,135 @@ | |||
| # SPDX-FileCopyrightText: 2025 InOrbit, Inc. | |||
There was a problem hiding this comment.
I think this file should be waypoints_sync.py since it syncs waypointS, unless the name is only about the type of annotations being synced
There was a problem hiding this comment.
I actually forgot to rename it to annotation_sync, like the library module. I'll name it annotations_sync.py
| - **Annotation**: An InOrbit `SpatialAnnotation` object. Currently, only waypoint annotations are supported. | ||
| - **Position**: A waypoint/location in the external system. | ||
| - **External system**: The software the connector interacts with (fleet manager, robot software). |
There was a problem hiding this comment.
After taking a look at the whole implementation I really don't like these names. Both Annotation and Position are really annotations that come from different sources. I also don't like "external", it depends on the user's side or pov.
I suggest using:
- Annotation -> InOrbitAnnotation
- Position -> RobotAnnotation
- External -> Robot
- External System -> Robot Software or Robot Management Software
Also, let's avoid using "location" since it already has a precise meaning in InOrbit.
I believe these names can bring a lot of clarity.
|
|
||
| from pydantic import BaseModel, Field, model_validator | ||
|
|
||
| ANNOTATION_SYNC_ORIGIN_PROPERTY = "syncOrigin" |
There was a problem hiding this comment.
Origin has a strong meaning when talking about spatial things. Perhaps we could use "source" or "owner"
| ) | ||
| return stats | ||
|
|
||
| async def sync_inorbit_to_external(self) -> dict: |
| Fetches all annotations from InOrbit, filters for owned ones, | ||
| converts them to positions, and synchronizes with the external system. |
There was a problem hiding this comment.
I'm not sure this is how InOrbit to Robot should work. Without knowing anything I had thought that this should sync any waypoint defined in InOrbit for my location to the FMS, without worrying about the owner.
I imagine the user case would be that I define a waypoint in InOrbit and it gets copied to all FMSs.
There was a problem hiding this comment.
I agree. Will switch to no filtering
| EXTERNAL_TO_INORBIT: Sync from external system to InOrbit | ||
| (external system is source of truth) | ||
| INORBIT_TO_EXTERNAL: Sync from InOrbit to external system | ||
| (InOrbit is source of truth) |
There was a problem hiding this comment.
More naming I'd name these ROBOT_TO_INORBIT AND INORBIT_TO_ROBOT
There was a problem hiding this comment.
WDYT aboout FLEET_TO_INORBIT AND INORBIT_TO_FLEET? I find ROBOT to be too singular, when this feature is mostly going to be used to sync fleet managers with InOrbit
There was a problem hiding this comment.
I like it. Also a robot is a fleet of one :)
| ) | ||
|
|
||
| # Fetch existing objects | ||
| existing_objects = await self.list_objects(kind, scope) |
There was a problem hiding this comment.
Please check if the config API limits results or uses pagination
|
|
||
| x: float | ||
| y: float | ||
| theta: float |
There was a problem hiding this comment.
I think we are missing the frame id. How does this work for multi-floor ?
There was a problem hiding this comment.
Managers are initialized per frame_id. In a multifloor environment, syncing starts as soon as a robot starts publishing a pose in a certain frame_id, and the annotation is applied to such frame_id.
This has the recently noted platform limitation of not being able to transform coordinates to a sublocation, which is the reason this module is "experimental" and should be until the platform fully supports multi-area annotations.
There was a problem hiding this comment.
👍 I missed that. I thought there was a single sync manager for the whole location. I believe it should be like that. Why would we wait until a robot visits a floor before reporting the annotations ?
There was a problem hiding this comment.
Because the connector is unaware of what frame_ids are ever going to be published and therefor managed unless explicitly and manually registered before hand, which is against the current design of all mutli-area connectors. Those connectors dynamically change the frame_id published based on robot data (e.g. frame_id is mapped to the map ID the robot is publishing). This reduces a lot of manual setup.
We could also switch to a manual approach, which would require implementing more logic in the client connector. The config file could have a list of frame_ids (the areas) but then use would need to reverse map that to whatever system an FMS would use.
|
@miguelgarcia thanks for taking the time to review. I agree with the naming comments, and I'll take care of those. As for the question comments, I hope I answered all of them. Please let me know your thoughts on each thread. |
Description
Adds a reusable annotation synchronization framework for synchronizing waypoints between external systems and InOrbit's Config API.
New
annotation_syncmodule: Provides interfaces and manager for implementing annotation synchronizationAnnotationSyncManager: Handles sync operations with InOrbit Config APIExternalAnnotationProviderandAnnotationConverterinterfaces for custom implementationsNew
inorbitmodule: Extracted Config API client and models for reusabilityInOrbitConfigAPI: Client for Config API operationsConfigObject,SpatialAnnotation,WaypointAnnotationSpec, etc.Documentation: Comprehensive docs for annotation sync specification and usage
Tests: Added test suite for annotation sync functionality (
test_waypoint_sync.py)Examples: Updated fleet connector example with annotation sync implementation
Note
Introduces an experimental, framework-level annotation sync feature and a reusable Config API client, with integration points in the core connector and full docs/examples/tests.
annotation_syncmodule:AnnotationSyncManager,ExternalAnnotationProvider,AnnotationConverter, per-frame sync lifecycle (auto-start onpublish_robot_pose()), ownership viaspec.properties.syncOrigininorbitmodule:InOrbitConfigAPI(httpx-based),ConfigObject/SpatialAnnotation/Waypoint*models; exports via package__init__ConnectorConfig: newrest_api_url(readsINORBIT_REST_API_URL) and optionalannotation_syncblock; updates env var handlingFleetConnector: registration API for sync, lazy per-frame_idmanager creation, clean start/stop on connect/disconnectexample.fleet.yaml) gainsannotation_syncfieldshttpxdependencyWritten by Cursor Bugbot for commit 6ac1a36. This will update automatically on new commits. Configure here.