Allow logging data from very generic indicator #913

ArturoManzoli · 2024-04-25T19:37:21Z

Allow logging of any variable the vehicle offers;
Custom VeryGenericIndicators now must have a Name and a variable to be accepted;
Unique Display Name for VeryGenericIndicators from now on. They can monitor the same variable, but with different display names. This is the reference for logging.

Closes #600

rafaellehmkuhl · 2024-04-25T21:43:38Z

src/components/MiniWidgetContainer.vue

+
+const handleDeleteWidget = (event: DraggableEvent): void => {
+  const widgetData = container.value.widgets.find((w) => w.hash === event.item.dataset.widgetHash)
+
+  // Remove VeryGenericIndicator from Logged variables list
+  if (widgetData?.component === 'VeryGenericIndicator') {
+    CurrentlyLoggedVariables.removeVariable(widgetData.options.displayName)
+  }
+
+  trashList.value = []
+}


Thinking on a system-architecture point of view, we are adding a child-specific handler here on the parent.
What if we pass an event to the widget that is being deleted and ask him to deal with it? This way we keep the parent without child-specific code and also allow any mini-widget to decide what to do when it is removed.

rafaellehmkuhl · 2024-04-25T21:49:10Z

I think the situation where the user is already logging from a VGI will be very common, as there are default VGIs that the user will probably add to several views.

What if we put a "log that" checkbox in the VGI, and if the user clicks it and there's another one already, it offers to ovewrite the previous or change the name of this VGI?

ArturoManzoli · 2024-04-26T09:15:30Z

I think the situation where the user is already logging from a VGI will be very common, as there are default VGIs that the user will probably add to several views.

What if we put a "log that" checkbox in the VGI, and if the user clicks it and there's another one already, it offers to ovewrite the previous or change the name of this VGI?

I agree that we must differentiate VGIs from different views.
The log configuration page already have the ability to switch them on and off. Maybe a cleaner idea would be to add, only on the logs configurations page, the view that the VGI is from. This way the user can decide over there to log them or not (check image below).
This solution anyway is kind of temporary since we are rethinking the way the user set the telemetry logs (#621 #875).

rafaellehmkuhl · 2024-04-26T18:44:17Z

Will be discussing more about how to do it next week.

…gging lib Signed-off-by: Arturo Manzoli <[email protected]>

rafaellehmkuhl · 2024-04-30T14:58:23Z

From our discussion today:

Problem: The mini-widgets are being loaded/unloaded from the screen when the view is changed, which would make the logs specific to each view (not desirable).

Solution: We agreed to change the current code on the top/bottom bar to stop reloading then when views are changed, and instead render them all, showing only the one for the current view, as we are doing with the widgets themselves.

This makes the log configuration Profile specific, as desirable. This also fixes the undesirable behavior of having all the mini-widgets being visually reloaded when the view is changed, which differs from the widgets' behavior, which are pre-rendered and cause a much better experience for the user.

patrickelectric · 2024-05-07T15:58:14Z

src/views/ConfigurationLogsView.vue

      <div class="flex w-[60%] flex-wrap">
        <v-checkbox
-          v-for="variable in DatalogVariable"
+          v-for="variable in loggedVariables"


It would be nice to have the items to be organized alphabetically, otherwise it's kind of hard to predict where it is.

rafaellehmkuhl

Didn't look at the code yet, but regarding usage, as mentioned in private, there are some bugs that appear to be related to reactivity:

Adding a new VGI makes the old ones disappear from the configuration list
Acessing the configuration of an already added VGI that appears with the same name in two views of the profile forces you to rename it
When the previous situation happens, sometimes the first VGI (the one that keeps the original name) does not appear in the configuration list

Refreshing the page seems the first and last items.

rafaellehmkuhl · 2024-05-07T16:25:15Z

src/libs/sensors-logging.ts


-      this.currentCockpitLog.push({
+      const logPoint: CockpitStandardLogPoint = {


This change can also be accomplished with this.currentCockpitLog.push({...} as CockpitStandardLogPoint).

No need to change, just a tip.

rafaellehmkuhl · 2024-05-07T16:29:14Z

src/stores/widgetManager.ts

+
+    // Remove miniWidget variable from the list of currently logged variables
+    CurrentlyLoggedVariables.removeVariable(miniWidget.options.displayName)


I'm not sure. I feel like this does not belong here. This is the widget store, but here we are coupling it with the logging system.

Can't we do this on the unmount of the widget itself?

@patrickelectric your input here would be good.

For the time, so we do not hurt the roadmap, we decide to let this coupling how it is and open an issue to track a future improvement on it.

One of the ideas to solve this is to allow register of add/remove callbacks on the widget manager, so those interested can react to changes there.

rafaellehmkuhl · 2024-05-07T16:30:34Z

src/components/MiniWidgetContainer.vue

+
+const handleDeleteWidget = (event: DraggableEvent): void => {
+  const widgetData = container.value.widgets.find((w) => w.hash === event.item.dataset.widgetHash)
+  if (widgetData) {
+    // Remove miniWidget variableName from Logged variables list
+    CurrentlyLoggedVariables.removeVariable(widgetData.options.displayName)
+  }
+  trashList.value = []
+}
 </script>


Same here as the last commit. This is specific to the VGI widget, but we are coupling it with the MiniWidget instantiation system.

For the time, so we do not hurt the roadmap, we decide to let this coupling how it is and open an issue to track a future improvement on it.

One of the ideas to solve this is to allow register of add/remove callbacks on the widget manager, so those interested can react to changes there.

Signed-off-by: Arturo Manzoli <[email protected]>

rafaellehmkuhl

As discussed outside here, the bug of changing a mini-widget name causing the others with the same name to disappear persists.

We agreed to use the widget hash instead of the displayName for tracking of each one, which will fix the bugs and UX problem.

Signed-off-by: Arturo Manzoli <[email protected]>

…s on delete Signed-off-by: Arturo Manzoli <[email protected]>

…ing on drag-to-trash Signed-off-by: Arturo Manzoli <[email protected]>

Signed-off-by: Arturo Manzoli <[email protected]>

…ic for multiple views logging Signed-off-by: Arturo Manzoli <[email protected]>

ArturoManzoli · 2024-05-09T17:33:43Z

As discussed outside here, the bug of changing a mini-widget name causing the others with the same name to disappear persists.

We agreed to use the widget hash instead of the displayName for tracking of each one, which will fix the bugs and UX problem.

The problems with multiple instances of mini-widgets were solved using an instance count. Since we don't need unique instances of the mini-widgets, and there is a 'refreshWidgetsHashs' method inside the 'miniWidgetContainer' which makes the VGI hashes mutable, the solution that treats all mini-widget instances as generic instances is simpler and more reliable.

rafaellehmkuhl

Everything appears to be working fine now!

ArturoManzoli requested review from patrickelectric, joaoantoniocardoso and JoaoMario109 April 25, 2024 19:37

rafaellehmkuhl reviewed Apr 25, 2024

View reviewed changes

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from 5f6d4b6 to 6f5f88e Compare April 29, 2024 13:37

ArturoManzoli requested a review from rafaellehmkuhl April 29, 2024 13:52

Mini-widgets: Very-generic-indicator: Add integration with sensors-lo…

419b710

…gging lib Signed-off-by: Arturo Manzoli <[email protected]>

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from 6f5f88e to dda6816 Compare April 29, 2024 13:55

rafaellehmkuhl marked this pull request as draft May 3, 2024 20:25

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from dda6816 to 659060c Compare May 6, 2024 17:49

ArturoManzoli marked this pull request as ready for review May 7, 2024 14:04

patrickelectric requested changes May 7, 2024

View reviewed changes

rafaellehmkuhl requested changes May 7, 2024

View reviewed changes

rafaellehmkuhl reviewed May 7, 2024

View reviewed changes

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from 659060c to 5e49b85 Compare May 7, 2024 17:56

Configuration-Logs: Modify variables list fetching method

7cabb1e

Signed-off-by: Arturo Manzoli <[email protected]>

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from 5e49b85 to 96b39b2 Compare May 7, 2024 18:32

ArturoManzoli requested review from rafaellehmkuhl and patrickelectric May 7, 2024 18:51

rafaellehmkuhl mentioned this pull request May 8, 2024

We should decouple the logging system from the widget management system #939

Open

rafaellehmkuhl requested changes May 8, 2024

View reviewed changes

ArturoManzoli added 3 commits May 9, 2024 14:21

Sensors-logging: Add support for very-generic-indicators data logging

27783ec

Signed-off-by: Arturo Manzoli <[email protected]>

Stores: Widget-manager: Remove VeryGenericIndicator variable from log…

0c95f19

…s on delete Signed-off-by: Arturo Manzoli <[email protected]>

Mini-widget-container: Remove VeryGenericIndicator variable from logg…

fdf7ae2

…ing on drag-to-trash Signed-off-by: Arturo Manzoli <[email protected]>

ArturoManzoli added 2 commits May 9, 2024 14:21

Bottom-bar: Preload bottom-bar and mini-widgets from all views

2575303

Signed-off-by: Arturo Manzoli <[email protected]>

Mini-widgets: Very-generic-indicator: Adapt component message and log…

570241a

…ic for multiple views logging Signed-off-by: Arturo Manzoli <[email protected]>

ArturoManzoli force-pushed the 600-Allow-logging-data-from-VeryGenericIndicator branch from 96b39b2 to 570241a Compare May 9, 2024 17:24

ArturoManzoli requested a review from rafaellehmkuhl May 9, 2024 17:34

rafaellehmkuhl approved these changes May 9, 2024

View reviewed changes

rafaellehmkuhl merged commit 45c7171 into bluerobotics:master May 9, 2024
8 checks passed

rafaellehmkuhl mentioned this pull request May 10, 2024

Bottom bar is not appearing in profiles with less than 3 views #944

Closed

ArturoManzoli deleted the 600-Allow-logging-data-from-VeryGenericIndicator branch May 24, 2024 10:00

ES-Alexander added the docs-needed Change needs to be documented label Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow logging data from very generic indicator #913

Allow logging data from very generic indicator #913

ArturoManzoli commented Apr 25, 2024

rafaellehmkuhl Apr 25, 2024

rafaellehmkuhl commented Apr 25, 2024

ArturoManzoli commented Apr 26, 2024

rafaellehmkuhl commented Apr 26, 2024

rafaellehmkuhl commented Apr 30, 2024 •

edited

Loading

patrickelectric May 7, 2024

rafaellehmkuhl left a comment •

edited

Loading

rafaellehmkuhl May 7, 2024

rafaellehmkuhl May 7, 2024 •

edited

Loading

rafaellehmkuhl May 8, 2024 •

edited

Loading

rafaellehmkuhl May 7, 2024

rafaellehmkuhl May 8, 2024 •

edited

Loading

rafaellehmkuhl left a comment

ArturoManzoli commented May 9, 2024

rafaellehmkuhl left a comment


		this.currentCockpitLog.push({
		const logPoint: CockpitStandardLogPoint = {


		// Remove miniWidget variable from the list of currently logged variables
		CurrentlyLoggedVariables.removeVariable(miniWidget.options.displayName)

Allow logging data from very generic indicator #913

Allow logging data from very generic indicator #913

Conversation

ArturoManzoli commented Apr 25, 2024

rafaellehmkuhl Apr 25, 2024

Choose a reason for hiding this comment

rafaellehmkuhl commented Apr 25, 2024

ArturoManzoli commented Apr 26, 2024

rafaellehmkuhl commented Apr 26, 2024

rafaellehmkuhl commented Apr 30, 2024 • edited Loading

patrickelectric May 7, 2024

Choose a reason for hiding this comment

rafaellehmkuhl left a comment • edited Loading

Choose a reason for hiding this comment

rafaellehmkuhl May 7, 2024

Choose a reason for hiding this comment

rafaellehmkuhl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

rafaellehmkuhl May 8, 2024 • edited Loading

Choose a reason for hiding this comment

rafaellehmkuhl May 7, 2024

Choose a reason for hiding this comment

rafaellehmkuhl May 8, 2024 • edited Loading

Choose a reason for hiding this comment

rafaellehmkuhl left a comment

Choose a reason for hiding this comment

ArturoManzoli commented May 9, 2024

rafaellehmkuhl left a comment

Choose a reason for hiding this comment

rafaellehmkuhl commented Apr 30, 2024 •

edited

Loading

rafaellehmkuhl left a comment •

edited

Loading

rafaellehmkuhl May 7, 2024 •

edited

Loading

rafaellehmkuhl May 8, 2024 •

edited

Loading

rafaellehmkuhl May 8, 2024 •

edited

Loading