Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove localisation from Flotilla #1868

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

andchiind
Copy link
Contributor

@andchiind andchiind commented Dec 11, 2024

This PR removes the localisation functionality from Flotilla, and instead moves responsibility to ISAR to maintain localisation. This means that currentArea on the Robot model is no longer used in mission logic. Additionally, we are no longer using Areas to keep track of whether a robot can start a mission, but Deck. The Deck is in this case referred to "InspectionArea" since we want to move towards having a model which stores a list of areas the robot can move in. Temporarily Deck is used for this purpose, but should be generalised in the future.

Some other things to note: Map information is not included in the IsarMissionDefinition for missions that do not have corresponsing MissionDefinitions. This is fine as this value is only used for inspection metadata, and the missions which do not have MissionDefinitions (return home and emergency dock missions) do not gather inspection data anyways.

We are still sending startPoses in the isar missions, using the DefaultLocalizationPose found in Decks. This is temporary as in the future we want this Pose to be stored in Isar instead. We are also still sending the dock and undock flags in the mission. In the future we should not need to send these as Isar should know to dock when the mission is a ReturnHome mission and to undock when its last mission was a dock mission.

One known "bug" currently, which I will assign to its own issue, is that the dock mission is also run even if the robot is currently docked. The simple solution to this is that ISAR should anyways be aware whether it's docked and so we don't need to do anything if we receive a return home mission when already home. In Flotilla in the future we can avoid sending it, if we implement a telemetry from ISAR which states whether the robot is docked or not. Currently, before this PR, we guessed whether it was docked using the robot.CurrentArea value. This has been replaced by robot.CurrentInspectionArea, which is set when the robot runs a mission for the first time, and then never unset again. To move the robot to another installation or to a separate region of the same installation which is in another inspection area, we currently want to use swagger to do so, as this is not something the user should be able to do.

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like console.log, TODO, etc.
  • The PR has been tested locally
  • A test have been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that require new issues
  • The changes does not introduce dead code as unused imports, functions etc.

Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@andchiind andchiind force-pushed the remove-localisation branch 3 times, most recently from 624ccd2 to b4d9f10 Compare December 12, 2024 09:39
@andchiind andchiind added bug Something isn't working feature New feature or request backend Backend related functionality breaking-change A breaking change which introduces changes to the public APIs frontend Frontend related functionality database-change Will require migration epic Northern Lights labels Dec 12, 2024
@andchiind andchiind self-assigned this Dec 12, 2024
@andchiind andchiind force-pushed the remove-localisation branch 5 times, most recently from 942c543 to 05dbed3 Compare December 12, 2024 11:47
@andchiind andchiind linked an issue Dec 12, 2024 that may be closed by this pull request
@andchiind andchiind force-pushed the remove-localisation branch 2 times, most recently from 15f50d7 to 143e3ac Compare December 12, 2024 13:02
Copy link

🔔 Migrations changes detected 🔔
📣 Remember to comment "/UpdateDatabase" after review approval for migrations to take effect!

@andchiind
Copy link
Contributor Author

Line changes before Migrations were added
image
😎

@andchiind andchiind requested review from mrica-equinor and removed request for mrica-equinor December 12, 2024 13:21
@andchiind andchiind force-pushed the remove-localisation branch 2 times, most recently from dbf8f47 to 3914a6d Compare December 13, 2024 08:13
@andchiind andchiind force-pushed the remove-localisation branch 6 times, most recently from f67e546 to e3e2a30 Compare December 13, 2024 08:23
@andchiind andchiind marked this pull request as ready for review December 13, 2024 08:23
Copy link
Contributor

@Christdej Christdej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related functionality breaking-change A breaking change which introduces changes to the public APIs bug Something isn't working database-change Will require migration epic feature New feature or request frontend Frontend related functionality Northern Lights
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove localisation logic from Flotilla
2 participants