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.

Sign up for GitHub

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

Jump to bottom

Support Mercury bus e-ink screens #2257

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Support Mercury bus e-ink screens #2257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a2f097
feat: enable Mercury vendor fields for Bus E-ink
digitalcora Oct 15, 2024
efd5897
feat: support audio readouts on Bus E-ink
digitalcora Oct 22, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions lib/screens/v2/widget_instance/subway_status.ex
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions test/screens/v2/widget_instance/alert_test.exs
View file
Open in desktop
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
Loading