formalize audio announcement logic #701

panentheos · 2023-09-25T18:20:43Z

Summary of changes

Asana Ticket: RTS Refactor: Audio Logic Revision

This overhauls the logic for immediate audio announcements, creating an explicit separation between announcements and passive readouts, removing some edge cases, and changing some announcement behavior. Details:

Announcements now use their own code path with separate logic (though they still share the various from_message calls in the audio modules).
Behavior change: Headway messages no longer trigger immediate announcements.
Behavior change: When predictions appear after not being present previously (e.g. because of a headway or custom message), the first prediction will be announced immediately.
Due to this new announcement type, many signs will make a single prediction announcement when the app is restarted.
Creation of Content.Audio structs have been standardized in from_message calls.
Mezzanine signs now behave consistently, and both sides can generate their own independent announcements.
Light rails announcements only include the relevant prediction, matching the behavior of heavy rail signs.
Audios are no longer sorted based on type. This theoretically results in a behavior change, but due to how audio readouts are timed and scheduled, it would be extremely rare to see this in practice.

panentheos · 2023-09-25T18:32:12Z

lib/signs/utilities/audio.ex

+
+  defp cache_value(list, value), do: [value | list] |> Enum.take(@announced_history_length)
+
+  defp decode_sign(sign) do


Note: This function provides a sneak peek into where this refactor is headed. It's essentially "decompiling" the granular line-oriented messages into higher-level semantic structures, which are more suitable for transforming and generating e.g. audio messages. Eventually, the goal is to make proper types for these, and create them earlier in the process, so the semantic forms can be used more naturally, without having to reverse engineer them.

Clarifying question - when you say "make proper types", do you just mean to more explicitly define them using typespecs somewhere or do you mean something else?

I meant creating named structs. The data here is already kind of leaning in that direction (with the e.g. {:predictions, ...} shapes), but since the scope of this data is very limited to start, I didn't want to jump ahead and make it concrete before we could see more of the use cases.

Right - considering structs are just maps with a __struct__ key, you could say these semantic structures are almost there. So the idea is to codify our content types at the high level with named structs, but eliminate the use of types at the end of the pipeline to streamline the serialization process. Am I understanding that correctly?

github-actions · 2023-09-25T18:32:14Z

Coverage of commit `eee03a3`

Summary coverage rate:
  lines......: 76.1% (1937 of 2545 lines)
  functions..: 78.2% (538 of 688 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/approaching.ex                           |82.9%     35| 100%     8|    -      0
  lib/content/audio/next_train_countdown.ex                  |98.1%     53| 100%    14|    -      0
  lib/content/audio/predictions.ex                           | 100%     10| 100%     1|    -      0
  lib/content/audio/train_is_arriving.ex                     |65.8%     38| 100%     6|    -      0
  lib/content/audio/train_is_boarding.ex                     |96.9%     32| 100%     8|    -      0
  lib/content/message/stopped_train.ex                       | 100%     18| 100%     6|    -      0
  lib/signs/realtime.ex                                      |93.9%     49|90.0%    10|    -      0
  lib/signs/utilities/audio.ex                               |90.0%    110| 100%    14|    -      0
  lib/signs/utilities/reader.ex                              |63.6%     11| 100%     3|    -      0

Download coverage report

PaulJKim

Re: the note Light rails announcements only include the relevant prediction, matching the behavior of heavy rail signs

Does "relevant prediction" in this case just refer to the soonest prediction? If so, is it possible we'd want to do additional immediate announcements in some cases for light rail when a platform is serving multiple branches? e.g. a rider might be waiting at Kenmore for a D train when the sign changes and triggers an immediate readout, but the first prediction is for a C train and the second prediction for a D train, so the rider would need to wait until a passive readout to hear about the D train even though for that rider, the D train prediction is arguably the relevant one

PaulJKim · 2023-09-27T14:47:20Z

lib/signs/utilities/audio.ex

-  @spec sort_audio([Content.Audio.t()]) :: [Content.Audio.t()]
-  defp sort_audio(audios) do
-    Enum.sort_by(audios, fn audio ->
-      case audio do
-        %Content.Audio.TrainIsBoarding{} -> 1
-        %Content.Audio.TrainIsArriving{} -> 2
-        %Content.Audio.Approaching{} -> 3
-        _ -> 4
-      end
-    end)
-  end
-


I see how this would rarely, if ever, affect a platform sign's logic but could it be playing a more significant role for mezzanine signs? For example, if the bottom line is on Arriving but the top line is a regular prediction we might still want to prioritize reading out the arriving message first

Even in the old code, an arriving message would suppress the readout of a normal prediction, so that particular situation wouldn't happen. However, an important point is that this sorting only happens within the context of a simultaneous update. If the statuses came in 1 second apart, the first one to change would get read first, regardless. That's why I think this isn't something we should worry about.

Ah okay, I see what you mean now

PaulJKim · 2023-09-27T15:05:11Z

lib/signs/utilities/audio.ex

+        [audio] = Audio.Custom.from_messages(top, bottom)
+
+        if sign.announced_custom_text != audio.message do
+          {audios ++ [audio], %{sign | announced_custom_text: audio.message}}


Just checking - would the immediate readout of new custom text be a behavior change from before as well?

No, the old algorithm did immediate readouts of new custom messages too.

PaulJKim · 2023-09-27T15:18:57Z

lib/signs/utilities/audio.ex

+    case Enum.find(items, &match?({:alert, _, _}, &1)) do
+      {:alert, top, bottom} ->
+        new_audios =
+          case top do


This bit makes me wonder.. Would it make sense now to make from_message part of the Content.Audio protocol to further generalize things?

Actually, I was imagining the opposite: Once we're no longer tinkering with the audio structs after the fact, the need for an abstract layer there kind of goes away, and we can just think about it as a function that takes sign context and returns audio messages.

PaulJKim · 2023-09-27T18:32:00Z

lib/signs/utilities/audio.ex

-              {new_audios ++ [audio], new_approaching_trips, new_arriving_trips}
+  defp get_prediction_announcements({audios, sign}, items) do
+    {new_audios, sign} =
+      Enum.filter(items, &match?({:predictions, _}, &1))


Nit considering I doubt it'll make a profound improvement, but we could do a Stream.filter and also a Stream.with_index on line 70 to save ourselves a couple iterations

PaulJKim · 2023-09-27T19:58:21Z

lib/signs/utilities/audio.ex

+                 &cache_value(&1, {message.trip_id, message.stops_away})
+               )}
+
+            match?(%Message.Predictions{}, message) && is_integer(message.minutes) && index == 0 &&


I'm not sure if I'm following the logic 100% so please correct me if I'm wrong - but specific to mezzanine signs, I think this would only trigger an immediate announcement when we go back to predictions for the top line. Let's say the bottom line went from paging headways and then to predictions while the top line stayed on predictions the whole time. In that case the new bottom line prediction would have an index of 1, based on the decode_sign/1 logic, and therefore not trigger an immediate announcement despite it being a new {route_id, direction_id} combo relative to sign.prev_prediction_keys. Does that sound like I'm following correctly? And if so, is that omission by design?

There are two levels of nesting at play to make this work. For platform signs, the data looks like: [{:predictions, [%P{}, %P{}]}], and for mezzanine signs it looks like [{:predictions, [%P{}]}, {:predictions, [%P{}]}]. The index is for the inner list, so the bottom message of the mezzanine sign will also have index 0, and this clause should trigger.

I know this function in particular is hairy. Do you think some targeted comments here (or elsewhere, perhaps decode_sign) would help?

Ah I see, didn't catch the distinction between the shape of the data for the platform vs mezzanine cases.

And yeah I was thinking some documentation could be helpful. Although this function is fairly verbose, I actually don't think it's terribly hard to follow or anything. But I think some documentation or possibly even just a typespec could be helpful for the decode_sign logic. I think the hard part while reading/reviewing was keeping in mind the different possible outcomes from decode_sign while thinking through the rest of the logic

panentheos · 2023-09-28T14:22:59Z

Regarding the light rail question, I think there's a larger discussion we should have around it. I carried forward the existing behavior of announcements delaying passive readouts (actually for longer than the read timer itself, since it adds), but that can definitely cause normal predictions not to be read in certain cases. It's not a simple light/heavy rail issue, either. The point you mentioned applies equally to the RL branches. Also, most mezzanine signs show multiple destinations, so you could make a similar argument there.

My thought was to do the quick standardization for now, and then start the larger conversation (which may lead to further changes), but I'm curious of your thoughts on that plan.

PaulJKim · 2023-09-28T16:05:30Z

Hmm yeah I think the thought to do the standardization first makes sense although I'd guess that we'd want to do the standardization in the other direction though if anything. Personally, it seems like the second prediction is still pertinent to riders in many situations. But like you said, that seems to be suggesting a larger discussion about audio behavior. Just to make sure I'm understanding your initial bullet point though - is the light rail behavior change you called out that immediate announcements of predictions will only include the next train and not the following train?

panentheos · 2023-09-28T17:53:27Z

Correct, light rail prediction announcements now only announce the prediction that is approaching, arriving, or boarding, and not the (following) numeric one. Though if you happen to get multiple at once, e.g. two BRD predictions on a multi-berth platform, then it will announce both, as you'd expect (I'm not sure that situation would actually happen in practice).

github-actions · 2023-09-28T18:32:38Z

Coverage of commit `3a1b1e5`

Summary coverage rate:
  lines......: 76.2% (1940 of 2547 lines)
  functions..: 78.2% (538 of 688 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/approaching.ex                           |82.9%     35| 100%     8|    -      0
  lib/content/audio/next_train_countdown.ex                  |98.1%     53| 100%    14|    -      0
  lib/content/audio/predictions.ex                           | 100%     10| 100%     1|    -      0
  lib/content/audio/train_is_arriving.ex                     |65.8%     38| 100%     6|    -      0
  lib/content/audio/train_is_boarding.ex                     |96.9%     32| 100%     8|    -      0
  lib/content/message/stopped_train.ex                       | 100%     18| 100%     6|    -      0
  lib/signs/realtime.ex                                      |93.9%     49|90.0%    10|    -      0
  lib/signs/utilities/audio.ex                               |90.1%    111| 100%    14|    -      0
  lib/signs/utilities/reader.ex                              |63.6%     11| 100%     3|    -      0

Download coverage report

github-actions · 2023-09-28T19:00:17Z

Coverage of commit `6684cd8`

Summary coverage rate:
  lines......: 76.1% (1939 of 2547 lines)
  functions..: 78.2% (538 of 688 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/approaching.ex                           |82.9%     35| 100%     8|    -      0
  lib/content/audio/next_train_countdown.ex                  |98.1%     53| 100%    14|    -      0
  lib/content/audio/predictions.ex                           | 100%     10| 100%     1|    -      0
  lib/content/audio/train_is_arriving.ex                     |65.8%     38| 100%     6|    -      0
  lib/content/audio/train_is_boarding.ex                     |96.9%     32| 100%     8|    -      0
  lib/content/message/stopped_train.ex                       | 100%     18| 100%     6|    -      0
  lib/signs/realtime.ex                                      |93.9%     49|90.0%    10|    -      0
  lib/signs/utilities/audio.ex                               |90.1%    111| 100%    14|    -      0
  lib/signs/utilities/reader.ex                              |63.6%     11| 100%     3|    -      0

Download coverage report

PaulJKim

Tested locally and looks good to me! I think this is heading things in a good direction 👍

formalize audio announcement logic

eee03a3

panentheos requested a review from PaulJKim September 25, 2023 18:20

panentheos commented Sep 25, 2023

View reviewed changes

PaulJKim reviewed Sep 27, 2023

View reviewed changes

address PR feedback and fix a bug

3a1b1e5

stream

6684cd8

PaulJKim approved these changes Sep 28, 2023

View reviewed changes

panentheos merged commit 78c953d into main Oct 5, 2023
1 check passed

panentheos deleted the bhw/audio-announcements branch October 5, 2023 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

formalize audio announcement logic #701

formalize audio announcement logic #701

panentheos commented Sep 25, 2023

panentheos Sep 25, 2023

PaulJKim Sep 28, 2023

panentheos Sep 28, 2023

PaulJKim Sep 28, 2023

github-actions bot commented Sep 25, 2023

PaulJKim left a comment

PaulJKim Sep 27, 2023

panentheos Sep 28, 2023

PaulJKim Sep 28, 2023

PaulJKim Sep 27, 2023

panentheos Sep 28, 2023

PaulJKim Sep 27, 2023

panentheos Sep 28, 2023

PaulJKim Sep 27, 2023

PaulJKim Sep 27, 2023

panentheos Sep 28, 2023

PaulJKim Sep 28, 2023

panentheos commented Sep 28, 2023

PaulJKim commented Sep 28, 2023

panentheos commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

PaulJKim left a comment


		defp cache_value(list, value), do: [value \| list] \|> Enum.take(@announced_history_length)

		defp decode_sign(sign) do

formalize audio announcement logic #701

formalize audio announcement logic #701

Conversation

panentheos commented Sep 25, 2023

Summary of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2023

Coverage of commit eee03a3

PaulJKim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panentheos commented Sep 28, 2023

PaulJKim commented Sep 28, 2023

panentheos commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

Coverage of commit 3a1b1e5

github-actions bot commented Sep 28, 2023

Coverage of commit 6684cd8

PaulJKim left a comment

Choose a reason for hiding this comment

Coverage of commit `eee03a3`

Coverage of commit `3a1b1e5`

Coverage of commit `6684cd8`