Skip to content
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

Support Mercury bus e-ink screens #2257

Merged
merged 2 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions assets/src/components/admin/inspector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const SCREEN_TYPES = new Set([
]);

const AUDIO_SCREEN_TYPES = new Set([
"bus_eink_v2",
"bus_shelter_v2",
"busway_v2",
"gl_eink_v2",
Expand Down
1 change: 1 addition & 0 deletions lib/screens/v2/screen_data/parameters.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ defmodule Screens.V2.ScreenData.Parameters do

@static_params %{
bus_eink_v2: %Static{
audio_active_time: @all_times,
candidate_generator: CandidateGenerator.BusEink,
refresh_rate: 30
},
Expand Down
5 changes: 4 additions & 1 deletion lib/screens/v2/widget_instance/alert.ex
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,10 @@ defmodule Screens.V2.WidgetInstance.Alert do
end
end

def audio_valid_candidate?(%__MODULE__{screen: %Screen{app_id: :gl_eink_v2}}), do: true
def audio_valid_candidate?(%__MODULE__{screen: %Screen{app_id: app_id}})
when app_id in ~w[bus_eink_v2 gl_eink_v2]a,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering about this conditional. Are there places we use this widget on audio-supporting screens where we specifically don't want it to be included in audio readouts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we first introduced audio for this widget when we started converting e-inks to Mercury. My guess is that we wanted to limit the scope of the change to just Mercury screens so we added this guard to exclude Bus Shelter. In hindsight, it seems overly cautious because bus alerts should read the same as GL. Problem is we never made a task to review the alert audio on Bus Shelters. It might be ok to just leave out the guard and ship alert audio to two screens at once. I feel confident in that but would understand leaving it to a future task.

do: true

def audio_valid_candidate?(_instance), do: false

def audio_view(_instance), do: ScreensWeb.V2.Audio.AlertView
Expand Down
8 changes: 1 addition & 7 deletions lib/screens/v2/widget_instance/normal_header.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,7 @@ defmodule Screens.V2.WidgetInstance.NormalHeader do

# Mercury adds their own time so we omit the time in the response.
# https://app.asana.com/0/1185117109217413/1206070378353406/f
def serialize(
%__MODULE__{
screen: %Screen{vendor: :mercury, app_id: :gl_eink_v2},
icon: icon,
text: text
} = t
) do
def serialize(%__MODULE__{screen: %Screen{vendor: :mercury}, icon: icon, text: text} = t) do
%{icon: icon, text: text, show_to: showing_destination?(t)}
end

Expand Down
4 changes: 2 additions & 2 deletions lib/screens/v2/widget_instance/subway_status.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ defmodule Screens.V2.WidgetInstance.SubwayStatus do
alias Screens.Stops.Stop
alias Screens.V2.WidgetInstance.SubwayStatus
alias ScreensConfig.Screen
alias ScreensConfig.V2.{Footer, GlEink, PreFare}
alias ScreensConfig.V2.{BusEink, Footer, GlEink, PreFare}

defmodule SubwayStatusAlert do
@moduledoc false
Expand Down Expand Up @@ -135,7 +135,7 @@ defmodule Screens.V2.WidgetInstance.SubwayStatus do
def audio_sort_key(_instance), do: [2]

def audio_valid_candidate?(%{screen: %Screen{app_params: %app{}}})
when app in [PreFare, GlEink],
when app in [BusEink, GlEink, PreFare],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same wondering here as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely done for the same reason as above. Something to think about here is whether it's not a bad experience to hear subway audio at a bus stop. That's the only thing I could see being an issue and even that is reaching.

do: true

def audio_valid_candidate?(_instance), do: false
Expand Down
2 changes: 1 addition & 1 deletion lib/screens_web/controllers/v2/screen_api_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ defmodule ScreensWeb.V2.ScreenApiController do
end

# Add extra fields used by the Mercury E-ink client
defp put_extra_fields(response, screen_id, %Screen{app_id: :gl_eink_v2}) do
defp put_extra_fields(response, screen_id, %Screen{vendor: :mercury}) do
response
# Used to enable audio readout without additional network requests
# https://app.asana.com/0/1176097567827729/1205748798471858/f
Expand Down
6 changes: 3 additions & 3 deletions test/screens/v2/widget_instance/alert_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -633,12 +633,12 @@ defmodule Screens.V2.WidgetInstance.AlertTest do
end

describe "audio_valid_candidate?/1" do
test "returns true for gl_eink_v2", %{widget: widget} do
test "returns true for eink screen types", %{widget: widget} do
assert AlertWidget.audio_valid_candidate?(widget)
end

test "returns false for screen types != gl_eink_v2", %{widget: widget} do
widget = %{widget | screen: %Screen{widget.screen | app_id: :bus_eink_v2}}
test "returns false for non-eink screen types", %{widget: widget} do
widget = %{widget | screen: %Screen{widget.screen | app_id: :bus_shelter_v2}}
refute AlertWidget.audio_valid_candidate?(widget)
end
end
Expand Down
3 changes: 1 addition & 2 deletions test/screens/v2/widget_instance/normal_header_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ defmodule Screens.V2.WidgetInstance.NormalHeaderTest do
mercury_eink_instance: %WidgetInstance.NormalHeader{
screen:
struct(ScreensConfig.Screen, %{
app_id: :gl_eink_v2,
vendor: :mercury,
app_params: struct(GlEink, %{header: struct(Header.Destination)})
}),
Expand All @@ -38,7 +37,7 @@ defmodule Screens.V2.WidgetInstance.NormalHeaderTest do
end

describe "serialize/1" do
test "does not return time if vendor is Mercury and app is GlEink", %{
test "does not return time if vendor is Mercury", %{
mercury_eink_instance: instance
} do
assert %{
Expand Down
Loading