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

Refactor naming of type of mission tasks for the case of return to home #1373

Merged

Conversation

mrica-equinor
Copy link
Contributor

Closes #1350

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 😄

@mrica-equinor mrica-equinor self-assigned this Jan 29, 2024
@mrica-equinor mrica-equinor added backend Backend related functionality improvement Improvement to existing functionality labels Jan 29, 2024
@@ -203,6 +203,6 @@ public enum MissionTaskType
{
Inspection,
Localization,
DriveTo
ReturnHome
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this remove the ability to have DriveTo tasks, ie. tasks where we tell the robot to go somewhere, but we don't give it a target? I think there are several use-cases for these, and DriveTo is already supported in some Isar implementations. Could we instead add a new task type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked this with Afonso, DriveTo was created specifcally for the return to home functionality and it is not used anywhere in the code that is not related to return to home. At the time he wanted to change the name but went on vacation/offshore before he could do the refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

That is odd, since missions from echo use it in other parts of their missions. I think we may need to have a bigger discussion around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it seems I have misunderstood. I see now that this renaming will not affect the naming in the IsarTask

@@ -203,6 +203,6 @@ public enum MissionTaskType
{
Inspection,
Localization,
DriveTo
ReturnHome
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it seems I have misunderstood. I see now that this renaming will not affect the naming in the IsarTask

@mrica-equinor mrica-equinor force-pushed the 1350-cancel-return-to-home branch 2 times, most recently from e010488 to 9732c0c Compare March 4, 2024 13:39
@mrica-equinor mrica-equinor force-pushed the 1350-cancel-return-to-home branch from 9732c0c to 3751a6f Compare March 4, 2024 13:44
Copy link

github-actions bot commented Mar 4, 2024

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

@github-actions github-actions bot added the database-change Will require migration label Mar 4, 2024
@mrica-equinor
Copy link
Contributor Author

/UpdateDatabase

Copy link

github-actions bot commented Mar 4, 2024

👀 Running migration command... 👀

Copy link

github-actions bot commented Mar 4, 2024

✨ Successfully ran migration command! ✨

@mrica-equinor mrica-equinor merged commit 904b551 into equinor:main Mar 4, 2024
12 checks passed
@mrica-equinor mrica-equinor deleted the 1350-cancel-return-to-home branch May 13, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related functionality database-change Will require migration improvement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return to home mission cannot be cancelled
2 participants