frontend: add failsafe configurations #2748

Williangalvani · 2024-06-26T02:21:38Z

This implements the (for me) more relevant/important failsafes

patrickelectric · 2024-06-26T17:02:11Z

We should ask for marketing to come up with the icons if we want that in the next stable.

patrickelectric · 2024-06-26T19:03:42Z

core/frontend/src/components/parameter-editor/InlineParameterEditor.vue

@@ -0,0 +1,231 @@
+<template>
+  <v-form


We should not use form, this can result in NS_BINDING_ABORTED problems.

I don't quite recall what the issue was. care to elaborate?
also it looks like we use forms quite a lot? is this a different situation?
(first result in google is purple but I don't recall what happened)

I don't quite recall what the issue was. care to elaborate? also it looks like we use forms quite a lot? is this a different situation? (first result in google is purple but I don't recall what happened)

https://github.com/bluerobotics/BlueOS/issues?q=is%3Aissue+NS_BINDING_ABORTED+is%3Aclosed

and: https://github.com/bluerobotics/BlueOS/pull/2351/files

ok that is weird. It must be a combination of things, though, right? maybe some request initialized in the form and then aborted because of the submit action?
We do have 15 other v-forms in the code, and I'm actively relying on the form validation in this component.
I'm not looking forward to changing it if the issue isn't really there.
@joaoantoniocardoso can you try the image plz? ~~I'll do a bunch of testing here too.~~

I tried changing a bunch of settings both on chrome and firefox, and I couldn't see any broken requests.

I would be okay if @joaoantoniocardoso fails to replicate the issue, he's pretty good on that 😆

ES-Alexander

Great start - thanks for all the hard work on this! :-)

A few comments and suggestions, mostly around language consistency and clarity.

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue

patrickelectric · 2024-06-27T16:02:33Z

We should ask for marketing to come up with the icons if we want that in the next stable.

SMS will work on that, we can merge after stable

ES-Alexander

Looking great! Just a few more small changes / suggestions, and these few extra things that I couldn't tie to specific lines of code:

It seems a little weird to be able to set negative values for the battery voltages, but that might not be something we have control over within this PR
~~When a parameter is set to a custom value (as is the case for the default value of the EKF variance threshold) the dropdown just shows up empty~~
- This is also a problem in the Parameters page, but it at least shows as the custom value in the main table there, and has the option to set other custom values (but I think it's ok for the failsafes page to not support setting custom values, would just be nice if it displayed them somehow)
Screen.Recording.2024-06-28.at.1.22.21.AM.mov
- ~~If this isn't fixable in this PR, I can open it as a separate issue~~
- Resolved
I think it would be more intuitive if fields that are linked to an Action get greyed out/disabled when the action is set to disabled
- I'm not sure how hard that is to implement, and I can add it as a future TODO in the Issue if you don't think it's worth doing in this PR
I believe this box is incorrectly being set to have a background colour, when it should be transparent so the cards are surrounded by blue (like in the other configuration pages)

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue

core/frontend/src/assets/img/configuration/failsafes/pressure.svg

core/frontend/src/components/parameter-editor/InlineParameterEditor.vue

patrickelectric · 2024-06-27T16:08:09Z

core/frontend/src/components/parameter-editor/InlineParameterEditor.vue

+      v-model.number="new_value"
+      dense
+      :items="as_select_items"
+      :rules="[() => true]"


also old code, doesnt's seem like it is required. removed

patrickelectric · 2024-06-27T16:09:39Z

core/frontend/src/components/parameter-editor/InlineParameterEditor.vue

+        custom_input
+          || (param
+            && !param.options
+            && !param.bitmask
+          )


Suggested change

custom_input

|| (param

&& !param.options

&& !param.bitmask

)

custom_input || (!param?.options && !param?.bitmask)

core/frontend/src/components/parameter-editor/InlineParameterEditor.vue

ES-Alexander · 2024-06-27T16:28:14Z

SMS will work on that, we can merge after stable

@patrickelectric feedback so far has been positive on the existing hand-drawn designs. I've mentioned that I can neaten them up or change them to not drawn ones, but that hasn't been requested or desired by the five people who've responded.

Daniel did mention that it might be a bit weird to have a different style only in this page, but we can have a think about whether that's ok since we don't have such images anywhere else, or if we perhaps want to add those kinds of images to some of our other pages as well :-)

patrickelectric · 2024-06-27T19:11:18Z

SMS will work on that, we can merge after stable

@patrickelectric feedback so far has been positive on the existing hand-drawn designs. I've mentioned that I can neaten them up or change them to not drawn ones, but that hasn't been requested or desired by the five people who've responded.

Daniel did mention that it might be a bit weird to have a different style only in this page, but we can have a think about whether that's ok since we don't have such images anywhere else, or if we perhaps want to add those kinds of images to some of our other pages as well :-)

I agree, the feedback from SMS was: "I think we could probably keep the essence of these icons, but maybe tweak them just a little!"

patrickelectric · 2024-06-29T00:46:42Z

@ES-Alexander @Williangalvani after talking with @rjehangir, we agreed that the current icons can be used, but sms will be working to make them sharp. Meanwhile, if the PR gets ready, we can merge it before 1.3 stable.

ES-Alexander · 2024-06-29T16:35:48Z

@Williangalvani there are cleaned up SVGs downloadable in the Issue, which can be switched to if you want.

ES-Alexander

SMS requested colour changes

core/frontend/src/assets/img/configuration/failsafes/battery.svg

core/frontend/src/assets/img/configuration/failsafes/crash.svg

core/frontend/src/assets/img/configuration/failsafes/ekf.svg

core/frontend/src/assets/img/configuration/failsafes/heartbeat.svg

core/frontend/src/assets/img/configuration/failsafes/leak.svg

core/frontend/src/assets/img/configuration/failsafes/pilot-input.svg

core/frontend/src/assets/img/configuration/failsafes/pressure.svg

core/frontend/src/assets/img/configuration/failsafes/temperature.svg

core/frontend/src/components/vehiclesetup/configuration/failsafes/FailsafeCard.vue

ES-Alexander

I've confirmed that both aspects of the no-value parameter thing has been fixed - thanks! 😀
I did notice that now (?¹) when you click save on a parameter in the Autopilot Parameters page the dialog goes into the "unchanged" state (with the save button greyed out). I think it would make more sense to just close the dialog, since the user is done with it, and otherwise the 'correct' interaction sequence is "change param, click save, click cancel", which is unintuitive.

Would be good if you can comment on the numbered items 1, 3, and 4 of my earlier review, as to whether they'll be included in this PR or I should open issues for them :-)

There are a couple of colour issues in my drawn70-set that I didn't notice before, but they should be fixed in the new SMS colours set anyway 👍

This might just be an existing interface issue with the Parameter page, rather than being new, but I've just noticed it now. ↩

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue

Williangalvani · 2024-07-04T20:26:33Z

@ES-Alexander the negative voltage is somewhat hard to control, the limits should be defined in the parameter metadata.

As for blocking the settings if we don't have a threshold set, I think it should be a subsequent PR, and it would probably mean we should move the action up, no? 👀

Co-authored-by: ES-Alexander <[email protected]>

ES-Alexander · 2024-07-05T00:19:53Z

the negative voltage is somewhat hard to control, the limits should be defined in the parameter metadata.

Yeah, fair enough.

As for blocking the settings if we don't have a threshold set, I think it should be a subsequent PR, and it would probably mean we should move the action up, no? 👀

Nah, I think the actions should stay at the bottom, but the rest of the card can be greyed out / disabled, so it's clearer that the other fields aren't effective until the (normal-coloured + interactive) Action field has been configured to do something :-)

ES-Alexander

Looking great! :D

Only remaining comments are:

It'd be nice to have a non-zero minimum horizontal margin around the cards, to prevent this from being possible:
I think low battery is more important than the pressure/temp ones, and it would also look better if it was moved up to directly after the Leak Sensor (so when they're paired up the pressure/temp ones go together and can shrink the card height)
I'm not sure how the flat blue background is applied, but it might be worth showing the page background gradient, or at least making sure the flat background colour is defined in terms of the theme variables

I do think 1 and 2 are worth fixing, but none of those points are critical, so approving now.

Again, excellent work with this - it'll hopefully make the failsafes more approachable and widely used :-)

Williangalvani force-pushed the failsafes branch 7 times, most recently from 0b4e816 to aada0f9 Compare June 26, 2024 16:59

Williangalvani requested review from joaoantoniocardoso and joaomariolago June 26, 2024 17:00

Williangalvani marked this pull request as ready for review June 26, 2024 17:00

patrickelectric added the merge-after-stable Should be merged only after next stable release label Jun 26, 2024

Williangalvani force-pushed the failsafes branch from aada0f9 to 37bd996 Compare June 26, 2024 20:58

patrickelectric requested changes Jun 26, 2024

View reviewed changes

ES-Alexander requested changes Jun 26, 2024

View reviewed changes

ES-Alexander reviewed Jun 27, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Outdated Show resolved Hide resolved

ES-Alexander reviewed Jun 27, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Outdated Show resolved Hide resolved

ES-Alexander reviewed Jun 27, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Outdated Show resolved Hide resolved

Williangalvani force-pushed the failsafes branch from 4baff56 to b1603e1 Compare June 27, 2024 00:42

ES-Alexander reviewed Jun 27, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Show resolved Hide resolved

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Outdated Show resolved Hide resolved

Williangalvani force-pushed the failsafes branch 3 times, most recently from 4d5e352 to 643662c Compare June 27, 2024 14:15

ES-Alexander requested changes Jun 27, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Show resolved Hide resolved

core/frontend/src/assets/img/configuration/failsafes/pressure.svg Outdated Show resolved Hide resolved

patrickelectric requested changes Jun 27, 2024

View reviewed changes

patrickelectric removed the merge-after-stable Should be merged only after next stable release label Jun 28, 2024

Williangalvani force-pushed the failsafes branch 2 times, most recently from 461b018 to 0f82b9f Compare July 1, 2024 22:06

ES-Alexander requested changes Jul 3, 2024

View reviewed changes

core/frontend/src/components/vehiclesetup/configuration/failsafes/Failsafes.vue Outdated Show resolved Hide resolved

Williangalvani force-pushed the failsafes branch 2 times, most recently from cc8b84b to 0865c27 Compare July 4, 2024 20:23

Williangalvani and others added 2 commits July 4, 2024 18:03

frontend: extract inlineParameterEditor from ParameterEditorDialog

9030a65

frontend: add failsafe configurations

6a1eff9

Co-authored-by: ES-Alexander <[email protected]>

Williangalvani force-pushed the failsafes branch from 0865c27 to 6a1eff9 Compare July 4, 2024 21:03

Williangalvani requested review from patrickelectric and ES-Alexander July 4, 2024 22:19

ES-Alexander approved these changes Jul 5, 2024

View reviewed changes

This was referenced Jul 5, 2024

core: frontend: vehicle-setup: failsafes: disable unused parameters #2786

Open

core: frontend: explanatory diagrams: use themeable colours #2787

Closed

patrickelectric approved these changes Jul 5, 2024

View reviewed changes

patrickelectric merged commit 91b3416 into bluerobotics:master Jul 5, 2024

patrickelectric mentioned this pull request Jul 5, 2024

core: frontend: vehicle-setup: failsafes: visual improvements #2788

Open

frontend: add failsafe configurations #2748

frontend: add failsafe configurations #2748

Uh oh!

Conversation

Williangalvani commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickelectric commented Jun 26, 2024

Uh oh!

patrickelectric Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Williangalvani Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaoantoniocardoso Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

joaoantoniocardoso Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Williangalvani Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickelectric Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

ES-Alexander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickelectric commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ES-Alexander left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickelectric Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

Williangalvani Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

patrickelectric Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ES-Alexander commented Jun 27, 2024

Uh oh!

patrickelectric commented Jun 27, 2024

Uh oh!

patrickelectric commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ES-Alexander commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ES-Alexander left a comment

Choose a reason for hiding this comment

Williangalvani commented Jun 26, 2024 •

edited

Loading

Williangalvani Jun 26, 2024 •

edited

Loading

Williangalvani Jun 26, 2024 •

edited

Loading

patrickelectric commented Jun 27, 2024 •

edited

Loading

ES-Alexander left a comment •

edited

Loading

patrickelectric commented Jun 29, 2024 •

edited

Loading

ES-Alexander commented Jun 29, 2024 •

edited

Loading

ES-Alexander left a comment •

edited

Loading

ES-Alexander left a comment •

edited

Loading