-
Notifications
You must be signed in to change notification settings - Fork 37
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
Merge obstacle status into robot status #1047
Conversation
🔔 Changes in database folder detected 🔔 |
@@ -25,6 +25,10 @@ export function RobotStatusChip({ status }: StatusProps) { | |||
chipColor = StatusColors.Busy | |||
break | |||
} | |||
case RobotStatus.Stuck: { | |||
chipColor = StatusColors.Busy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a dedicated statuscolor for Stuck. This color can be the same as the one for busy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marriane has used same one in figma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chipColor = StatusColors.Busy | |
chipColor = StatusColors.Stuck |
Agree with Øystein. Make this change and then add a StatusColors.Stuck that has the same value as busy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
d806bd0
to
8e37bce
Compare
@@ -353,6 +352,16 @@ public async Task<ActionResult<IList<VideoStream>>> GetVideoStreams([FromRoute] | |||
return NotFound("Robot not found"); | |||
} | |||
|
|||
if (robot.Status is RobotStatus.Stuck) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest renaming stuck to blocked
if (robot.Status is RobotStatus.Stuck) | ||
{ | ||
_logger.LogWarning( | ||
"Robot '{id}' is stuck ({status})", | ||
robotId, | ||
robot.Status.ToString() | ||
); | ||
return Conflict($"The Robot is stuck ({robot.Status})"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest removing. Want to keep the option to try a new mission, different mission, if the current one is blocked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@@ -229,7 +229,6 @@ public async Task<ActionResult<Robot>> DeleteRobot([FromRoute] string id) | |||
) | |||
{ | |||
_logger.LogInformation("Updating robot status with id={id}", id); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove?
if (robot.Status is RobotStatus.Stuck) | ||
{ | ||
_logger.LogWarning( | ||
"Robot '{id}' is stuck ({status})", | ||
scheduleLocalizationMissionQuery.RobotId, | ||
robot.Status.ToString() | ||
); | ||
return Conflict($"The Robot is stuck ({robot.Status})"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is desired behavior. Should be able to localize even if the robot has some obstacle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - will be removed
912fcb1
to
813f138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Small and tidy PR
@oysand review this again please :) |
Approved by another guy and updated according to your request :)
Add stuck condition to Robot Controller and frontend
No description provided.