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

Support Mercury bus e-ink screens #2257

merged 2 commits into from
Oct 22, 2024

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented Oct 22, 2024

We'll soon be setting up our first Bus E-ink screen provided by Mercury. This requires a couple of changes:

  • In places we previously had logic to include extra "Mercury fields" when the screen type was gl_eink, we should instead key on the vendor field.
  • Since this new screen will have an audio readout button similar to GL E-ink, the Bus E-ink screen type now needs to support audio.

In places we were previously checking that the screen type was GL E-ink,
or a combination of that and the vendor being Mercury, we now only check
the vendor.
@@ -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.

@@ -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.

@digitalcora digitalcora marked this pull request as ready for review October 22, 2024 16:34
@digitalcora digitalcora requested a review from a team as a code owner October 22, 2024 16:34
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Code looks good. We'll definitely need to communicate with Ana that these screens need to have vendor populated for them to work properly.

@digitalcora digitalcora merged commit 1187b7d into main Oct 22, 2024
14 checks passed
@digitalcora digitalcora deleted the cfg-bus-to-mercury branch October 22, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants