Allow robot created notifications #48

EmeraldWither · 2024-06-18T17:31:16Z

2024-06-18.13-15-08.mp4

Robot code usage:

 @Override
  public void autonomousInit() {
    Elastic.sendAlert(AlertLevel.INFO, "Autonomous!", "The robot is now in auto.");

There are 3 levels of notifications:

INFO:

WARNING:

ERROR:

The idea is that the driver can get a notification when something happens (eg. an arm extends too far and gets disabled).

Users can copy the Elastic class into their robot project, and use those methods to push notifications to the dashboard.

Gold872

I like this idea, however I'm wondering where it would be better to have users use the network alerts vs the notifications

There should also be some way of unit testing this, the PR for restructuring the NT dependency makes testing it much easier, so it might make more sense to add unit tests for notifications after the rework is complete

Gold872 · 2024-06-18T21:25:08Z

lib/pages/dashboard_page.dart

+    var notifications = ntConnection.subscribe("notifications");
+
+    //dont display a notification when we first connect
+    bool notificationFirstRun = true;
+    notifications.listen((p0, p1) {
+      List<String> data = p0.toString().replaceAll("[", "").replaceAll("]", "").split(",");
+      if(notificationFirstRun) {
+        notificationFirstRun = false;
+        return;
+      }
+      Icon icon;
+      if(data[0] == "INFO") {
+        icon = const Icon(Icons.info);
+      }
+      else if(data[0] == "WARNING") {
+        icon = const Icon(Icons.warning_amber, color: Colors.orange,);
+      }
+      else if(data[0] == "ERROR") {
+        icon = const Icon(Icons.error, color: Colors.red,);
+      }
+      else {
+        icon = const Icon(Icons.question_mark);
+      }
+
+      String title = data[1];
+      String description = data[2];
+      setState(() {
+        ColorScheme colorScheme = Theme.of(context).colorScheme;
+        TextTheme textTheme = Theme.of(context).textTheme;
+        ElegantNotification notification = ElegantNotification(
+          autoDismiss: true,
+          showProgressIndicator: true,
+          background: colorScheme.surface,
+          width: 350,
+          position: Alignment.bottomRight,
+          title: Text(
+            title,
+            style: textTheme.bodyMedium!.copyWith(
+              fontWeight: FontWeight.bold,
+            ),
+          ),
+          icon: icon,
+          description: Text(description),
+          stackedOptions: StackedOptions(
+            key: 'notification',
+            type: StackedType.above,
+            itemOffset: const Offset(0, 5),
+          ),
+        );
+        if (mounted) {
+          notification.show(context);
+        }
+      });
+    });


I think the listening logic would belong better in a separate class that you pass a function into which is called when a new notification is published. The main reason is because it could eventually cause the method to become a bit cluttered.

Would a robot alert service similar be a good way of going about this?

Gold872 · 2024-06-18T21:27:00Z

elasticlib/Elastic.java

+import edu.wpi.first.networktables.StringArrayTopic;
+
+public final class Elastic {
+    private static final StringArrayTopic topic = NetworkTableInstance.getDefault().getStringArrayTopic("notifications");


Maybe change the topic to something like "elastic/notifications" in case if there's ever some library that also publishes to a topic called notifications?

Thinking of renaming it to "elastic/alerts" since I think that would make more sense.

Robot alerts vs Elastic Notifications. Maybe an icon on the notification can differentiate the two? Also renaming everywhere to alerts would probably make more sense.

Gold872 · 2024-06-18T21:32:41Z

elasticlib/Elastic.java

+    public static void sendAlert(AlertLevel level, String title, String description) {
+        publisher.set(new String[] {level.toString(), title, description});
+    }


I think it would be easier to maintain long term if it used a different format that's easier to expand on. Maybe there could be a way of publishing some sort of json map that can have different properties if we want to add more customization later on

I think a json map would be the way to go here.

EmeraldWither · 2024-06-20T22:41:59Z

Hey thanks for looking at this.

There should also be some way of unit testing this, the PR for restructuring the NT dependency makes testing it much easier, so it might make more sense to add unit tests for notifications after the rework is complete

Unit testing is something that I haven't done before (😓), but my summer break starts today, so I will take a look at how to do this and will put in later.

Gold872

So far this is looking really good, I think this would be a great addition to the dashboard. Some things that would have to be added (such as unit tests) aren't very easy to do in the main branch, so some parts would have to be added in the NT connection rework branch after this is merged

elasticlib/Elastic.java

lib/services/robot_alerts.dart

elasticlib/Elastic.java

lib/pages/dashboard_page.dart

lib/services/nt4_client.dart

addresses name conflicts with wpilibs stuff in 2025

codecov · 2024-06-22T23:47:16Z

Codecov Report

Attention: Patch coverage is 23.07692% with 20 lines in your changes missing coverage. Please review.

Project coverage is 45.45%. Comparing base (f1d29a6) to head (4666061).

❗ Current head 4666061 differs from pull request most recent head cce8f91

Please upload reports for the commit cce8f91 to get more accurate results.

Files	Patch %	Lines
lib/services/robot_notifications_listener.dart	13.63%	19 Missing ⚠️
lib/pages/dashboard_page.dart	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   45.53%   45.45%   -0.08%     
==========================================
  Files          73       74       +1     
  Lines        7465     7489      +24     
==========================================
+ Hits         3399     3404       +5     
- Misses       4066     4085      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Gold872

Looks great, I'll have to merge these changes into the rework branch and there might be some slight modifications to work better with the new NT api, but this is ready to put into main

Gold872 · 2024-06-25T01:00:08Z

lib/services/robot_notifications_listener.dart

+        color: Colors.red,
+      );
+    } else {
+      icon = const Icon(Icons.question_mark);


Gold872 · 2024-06-25T02:10:07Z

@EmeraldWither quick question, what's the reason for having it send all if it ignores the notification on initial connection?

Also, I merged all of these changes into the nt-dependency-rework branch, so you can add unit tests for this much easier. Whenever you're ready, just make a PR to that branch. Let me know if you need any help with the tests

This is definitely a great addition, thanks for making this!

EmeraldWither · 2024-06-25T13:51:57Z

On the initial connection, when I start listening, it will always call the method with whatever is currently inside the topic, even if it's old/stale data (eg, rebooting elastic will cause the notification so show again even though we haven't actually posted anything).

By ignoring the first time that this happens, this allows us to only receive the new data.

EmeraldWither added 4 commits June 18, 2024 13:18

add alerts

fcf6f27

add elasticlib

d891f58

change spacing

32daba8

remove test prntln

63c84da

Gold872 requested changes Jun 18, 2024

View reviewed changes

move alerts to own service && nt topic name change

ce2b7d9

EmeraldWither marked this pull request as draft June 21, 2024 01:57

EmeraldWither added 2 commits June 21, 2024 15:19

use json for alerts

69ebb26

use json for alerts

945c146

Gold872 requested changes Jun 22, 2024

View reviewed changes

rename "robotalerts" to "elastic notifications" etc.

4666061

addresses name conflicts with wpilibs stuff in 2025

EmeraldWither added 4 commits June 22, 2024 19:57

dont use context

ea5c1ff

turn subscribing to all topics its own method

edcdac8

make elastic robot notification variables mutable

983d8e8

change jackson annotations in elasticlib

07fb8e9

EmeraldWither marked this pull request as ready for review June 25, 2024 00:54

EmeraldWither requested a review from Gold872 June 25, 2024 00:55

Add null checks for title and description

079530d

Gold872 approved these changes Jun 25, 2024

View reviewed changes

lib/services/robot_notifications_listener.dart

color: Colors.red,

);

} else {

icon = const Icon(Icons.question_mark);

Copy link

Owner

Gold872 Jun 25, 2024

Choose a reason for hiding this comment

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

Love this

Fix formatting

cce8f91

Gold872 merged commit 79d80b6 into Gold872:main Jun 25, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow robot created notifications #48

Allow robot created notifications #48

EmeraldWither commented Jun 18, 2024

Gold872 left a comment

Gold872 Jun 18, 2024

EmeraldWither Jun 20, 2024

Gold872 Jun 18, 2024

EmeraldWither Jun 20, 2024 •

edited

Loading

Gold872 Jun 18, 2024

EmeraldWither Jun 20, 2024

EmeraldWither commented Jun 20, 2024

Gold872 left a comment

codecov bot commented Jun 22, 2024 •

edited

Loading

Gold872 left a comment

Gold872 Jun 25, 2024

Gold872 commented Jun 25, 2024

EmeraldWither commented Jun 25, 2024

Allow robot created notifications #48

Allow robot created notifications #48

Conversation

EmeraldWither commented Jun 18, 2024

Gold872 left a comment

Choose a reason for hiding this comment

Gold872 Jun 18, 2024

Choose a reason for hiding this comment

EmeraldWither Jun 20, 2024

Choose a reason for hiding this comment

Gold872 Jun 18, 2024

Choose a reason for hiding this comment

EmeraldWither Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Gold872 Jun 18, 2024

Choose a reason for hiding this comment

EmeraldWither Jun 20, 2024

Choose a reason for hiding this comment

EmeraldWither commented Jun 20, 2024

Gold872 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2024 • edited Loading

Codecov Report

Gold872 left a comment

Choose a reason for hiding this comment

Gold872 Jun 25, 2024

Choose a reason for hiding this comment

Gold872 commented Jun 25, 2024

EmeraldWither commented Jun 25, 2024

EmeraldWither Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 22, 2024 •

edited

Loading