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

pass local values in bulk in bus code #789

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

pass local values in bulk in bus code #789

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a831667
pass local values in bulk in bus code
panentheos Jun 27, 2024
07e0133
use struct
panentheos Jun 28, 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
229 changes: 96 additions & 133 deletions lib/signs/bus.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ defmodule Signs.Bus do
last_read_time: DateTime.t()
}

@type locals :: %{
config: Engine.Config.sign_config(),
bridge_enabled?: boolean(),
bridge_status: map(),
current_time: DateTime.t(),
route_alerts_lookup: %{String.t() => Engine.Alerts.Fetcher.stop_status()},
stop_alerts_lookup: %{String.t() => Engine.Alerts.Fetcher.stop_status()},
predictions_lookup: %{map() => Predictions.BusPrediction.t()}
}

def start_link(sign) do
state = %__MODULE__{
id: Map.fetch!(sign, "id"),
Expand Down Expand Up @@ -168,22 +178,24 @@ defmodule Signs.Bus do
|> filter_predictions(current_time, state)
|> Enum.group_by(&{&1.stop_id, &1.route_id, &1.direction_id})

locals = %{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to think of a more descriptive name for this... "locals" sort of tells you what its function is, but not what it is. Maybe something like MessageContext — it's context used to create sign messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went back and forth on this a bunch. Ultimately it's not just "messages", but also "announcements", and perhaps other things. context works, but it's really generic. locals felt decent because it describes the lifecycle of the data, without being prescriptive about what's in there.

Copy link
Contributor

@digitalcora digitalcora Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think part of the reason I'm not a fan of locals is because it implies "local variables", which are a specific thing, and this isn't quite that thing (you could say it's... non-local variables?). context, while similarly generic, doesn't have this overlap. But don't feel like you have to change it on my preference alone 🙂

config: config,
bridge_enabled?: bridge_enabled?,
bridge_status: bridge_status,
current_time: current_time,
predictions_lookup: predictions_lookup,
route_alerts_lookup: route_alerts_lookup,
stop_alerts_lookup: stop_alerts_lookup
}

# Compute new sign text and audio
{[top, bottom], audios, tts_audios} =
cond do
config == :off ->
{_messages = ["", ""], _audios = [], _tts_audios = []}

match?({:static_text, _}, config) ->
static_text_content(
config,
bridge_status,
bridge_enabled?,
current_time,
predictions_lookup,
route_alerts_lookup,
state
)
static_text_content(locals, state)

# Special case: 71 and 73 buses board on the Harvard upper busway at certain times. If
# they are predicted there, let people on the lower busway know.
Expand All @@ -195,49 +207,30 @@ defmodule Signs.Bus do
special_harvard_content()

configs ->
platform_mode_content(
predictions_lookup,
route_alerts_lookup,
stop_alerts_lookup,
current_time,
bridge_status,
bridge_enabled?,
state
)
platform_mode_content(locals, state)

true ->
mezzanine_mode_content(
predictions_lookup,
route_alerts_lookup,
stop_alerts_lookup,
current_time,
bridge_status,
bridge_enabled?,
state
)
mezzanine_mode_content(locals, state)
end

# Update the sign (if appropriate), and record changes in state
state
|> then(fn state ->
if should_update?({top, bottom}, current_time, state) do
if should_update?({top, bottom}, locals, state) do
state.sign_updater.set_background_message(state, top, bottom)
%{state | current_messages: {top, bottom}, last_update: current_time}
else
state
end
end)
|> then(fn state ->
if should_read?(current_time, state) do
if should_read?(locals, state) do
send_audio(audios, tts_audios, state)
%{state | last_read_time: current_time}
else
if should_announce_drawbridge?(bridge_status, bridge_enabled?, current_time, state) do
bridge_audios = bridge_audio(bridge_status, bridge_enabled?, current_time, state)

bridge_tts_audios =
bridge_tts_audio(bridge_status, bridge_enabled?, current_time, state)

if should_announce_drawbridge?(locals, state) do
bridge_audios = bridge_audio(locals, state)
bridge_tts_audios = bridge_tts_audio(locals, state)
send_audio(bridge_audios, bridge_tts_audios, state)
end

Expand Down Expand Up @@ -300,43 +293,18 @@ defmodule Signs.Bus do
end

# Static text mode. Just display the configured text, and possibly the bridge message.
@spec static_text_content(
Engine.Config.sign_config(),
term(),
boolean(),
DateTime.t(),
map(),
map(),
t()
) :: content_values()
defp static_text_content(
config,
bridge_status,
bridge_enabled?,
current_time,
predictions_lookup,
route_alerts_lookup,
state
) do
@spec static_text_content(locals(), t()) :: content_values()
defp static_text_content(%{config: config} = locals, state) do
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of extracting the functions which accept this "locals" value into their own module? Sort of like a "view" for which the main module is the "controller". I think having some split of responsibilities here would make both parts easier to understand (and easier to test?) vs. all being in one mega-module, even if the "view" logic is still a high proportion. (I'm not sure off-hand how much of the state value the "view" functions use, but maybe the parts that are needed could be copied into the "locals" value to reduce coupling between the modules.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Part of the motivation for streamlining this was to avoid the temptation to move things around. By their nature, these functions are very tightly coupled together with the run_loop, via the data flow (which is why this pattern works at all). Moving some of them to another file would just mean having to jump around to multiple places to follow the logic. By the same token, the particular function boundaries are kind of arbitrary, so I'd hesitate to crystallize those internal APIs with individual test cases. The fact that the bus tests all exercise the full run_loop is what made it possible to rearrange the guts without having to rewrite a bunch of tests as well.

{_, {line1, line2}} = config

messages =
[[line1, line2]]
|> Enum.concat(
bridge_message(
bridge_status,
bridge_enabled?,
current_time,
predictions_lookup,
route_alerts_lookup,
state
)
)
|> Enum.concat(bridge_message(locals, state))
|> paginate_pairs()

audios =
[{:ad_hoc, {"#{line1} #{line2}", :audio}}]
|> Enum.concat(bridge_audio(bridge_status, bridge_enabled?, current_time, state))
|> Enum.concat(bridge_audio(locals, state))

tts_audios = [{"#{line1} #{line2}", nil}]

Expand All @@ -345,24 +313,16 @@ defmodule Signs.Bus do

# Platform mode. Display one prediction per route, but if all the predictions are for the
# same route, then show a single page of two.
@spec platform_mode_content(map(), map(), map(), DateTime.t(), term(), boolean(), t()) ::
content_values()
@spec platform_mode_content(locals(), t()) :: content_values()
defp platform_mode_content(
predictions_lookup,
route_alerts_lookup,
stop_alerts_lookup,
current_time,
bridge_status,
bridge_enabled?,
state
%{stop_alerts_lookup: stop_alerts_lookup, current_time: current_time} = locals,
%__MODULE__{configs: configs, extra_audio_configs: extra_audio_configs} = state
) do
%{configs: configs, extra_audio_configs: extra_audio_configs} = state
content = configs_content(configs, predictions_lookup, route_alerts_lookup)
content = configs_content(configs, locals)
# Special case: Nubian platform E has two separate text zones, but only one audio zone due
# to close proximity. One sign process is configured to read out the other sign's prediction
# list in addition to its own, while the other one stays silent.
audio_content =
content ++ configs_content(extra_audio_configs, predictions_lookup, route_alerts_lookup)
audio_content = content ++ configs_content(extra_audio_configs, locals)

if !Enum.any?(audio_content, &match?({:predictions, _}, &1)) &&
all_stop_ids(state)
Expand All @@ -385,16 +345,7 @@ defmodule Signs.Bus do
]
end)
end
|> Enum.concat(
bridge_message(
bridge_status,
bridge_enabled?,
current_time,
predictions_lookup,
route_alerts_lookup,
state
)
)
|> Enum.concat(bridge_message(locals, state))
|> paginate_pairs()

audios =
Expand All @@ -409,7 +360,7 @@ defmodule Signs.Bus do
|> Stream.intersperse([:_])
|> Stream.concat()
|> paginate_audio()
|> Enum.concat(bridge_audio(bridge_status, bridge_enabled?, current_time, state))
|> Enum.concat(bridge_audio(locals, state))

tts_audios =
case audio_content do
Expand All @@ -426,27 +377,20 @@ defmodule Signs.Bus do
|> List.wrap()
end
|> Enum.map(&{&1, nil})
|> Enum.concat(bridge_tts_audio(bridge_status, bridge_enabled?, current_time, state))
|> Enum.concat(bridge_tts_audio(locals, state))

{messages, audios, tts_audios}
end
end

# Mezzanine mode. Display and paginate each line separately.
@spec mezzanine_mode_content(map(), map(), map(), DateTime.t(), term(), boolean(), t()) ::
content_values()
@spec mezzanine_mode_content(locals(), t()) :: content_values()
defp mezzanine_mode_content(
predictions_lookup,
route_alerts_lookup,
stop_alerts_lookup,
current_time,
bridge_status,
bridge_enabled?,
state
%{stop_alerts_lookup: stop_alerts_lookup, current_time: current_time} = locals,
%__MODULE__{top_configs: top_configs, bottom_configs: bottom_configs} = state
) do
%{top_configs: top_configs, bottom_configs: bottom_configs} = state
top_content = configs_content(top_configs, predictions_lookup, route_alerts_lookup)
bottom_content = configs_content(bottom_configs, predictions_lookup, route_alerts_lookup)
top_content = configs_content(top_configs, locals)
bottom_content = configs_content(bottom_configs, locals)

if !Enum.any?(top_content ++ bottom_content, &match?({:predictions, _}, &1)) &&
all_stop_ids(state)
Expand All @@ -467,7 +411,7 @@ defmodule Signs.Bus do
|> Stream.intersperse([:_])
|> Stream.concat()
|> paginate_audio()
|> Enum.concat(bridge_audio(bridge_status, bridge_enabled?, current_time, state))
|> Enum.concat(bridge_audio(locals, state))

tts_audios =
case top_content ++ bottom_content do
Expand All @@ -481,7 +425,7 @@ defmodule Signs.Bus do
|> List.wrap()
end
|> Enum.map(&{&1, nil})
|> Enum.concat(bridge_tts_audio(bridge_status, bridge_enabled?, current_time, state))
|> Enum.concat(bridge_tts_audio(locals, state))

{messages, audios, tts_audios}
end
Expand All @@ -503,9 +447,12 @@ defmodule Signs.Bus do
{messages, audios, tts_audios}
end

defp configs_content(nil, _, _), do: []
defp configs_content(nil, _), do: []

defp configs_content(configs, predictions_lookup, route_alerts_lookup) do
defp configs_content(
configs,
%{predictions_lookup: predictions_lookup, route_alerts_lookup: route_alerts_lookup}
) do
Enum.flat_map(configs, fn config ->
content =
Stream.flat_map(config.sources, fn source ->
Expand Down Expand Up @@ -559,9 +506,11 @@ defmodule Signs.Bus do
# 1. it has never been updated before (we just booted up)
# 2. the sign is about to auto-blank, so refresh it
# 3. the content has changed, but wait until the existing content has paged at least once
defp should_update?(messages, current_time, state) do
%{last_update: last_update, current_messages: current_messages} = state

defp should_update?(
messages,
%{current_time: current_time},
%__MODULE__{last_update: last_update, current_messages: current_messages}
) do
!last_update ||
Timex.after?(current_time, Timex.shift(last_update, seconds: 150)) ||
(current_messages != messages &&
Expand All @@ -571,13 +520,14 @@ defmodule Signs.Bus do
))
end

defp should_read?(current_time, state) do
%{
read_loop_interval: read_loop_interval,
read_loop_offset: read_loop_offset,
last_read_time: last_read_time
} = state

defp should_read?(
%{current_time: current_time},
%__MODULE__{
read_loop_interval: read_loop_interval,
read_loop_offset: read_loop_offset,
last_read_time: last_read_time
}
) do
period = fn time -> div(Timex.to_unix(time) - read_loop_offset, read_loop_interval) end
period.(current_time) != period.(last_read_time)
end
Expand All @@ -588,9 +538,14 @@ defmodule Signs.Bus do
# 1. the drawbridge just went up
# 2. drawbridge messages are enabled
# 3. we are at a stop that is impacted, but does not show visual drawbridge messages
defp should_announce_drawbridge?(bridge_status, bridge_enabled?, current_time, state) do
%{chelsea_bridge: chelsea_bridge, prev_bridge_status: prev_bridge_status} = state

defp should_announce_drawbridge?(
%{
bridge_enabled?: bridge_enabled?,
bridge_status: bridge_status,
current_time: current_time
},
%__MODULE__{chelsea_bridge: chelsea_bridge, prev_bridge_status: prev_bridge_status}
) do
chelsea_bridge == "audio" && bridge_enabled? && prev_bridge_status &&
bridge_status_raised?(bridge_status, current_time) &&
!bridge_status_raised?(prev_bridge_status, current_time)
Expand Down Expand Up @@ -619,21 +574,19 @@ defmodule Signs.Bus do
end

defp bridge_message(
bridge_status,
bridge_enabled?,
current_time,
predictions_lookup,
route_alerts_lookup,
state
%{
bridge_enabled?: bridge_enabled?,
bridge_status: bridge_status,
current_time: current_time
} = locals,
%__MODULE__{chelsea_bridge: chelsea_bridge, configs: configs}
) do
%{chelsea_bridge: chelsea_bridge, configs: configs} = state

if bridge_enabled? && chelsea_bridge == "audio_visual" &&
bridge_status_raised?(bridge_status, current_time) do
mins = bridge_status_minutes(bridge_status, current_time)

line2 =
case {mins > 0, configs_content(configs, predictions_lookup, route_alerts_lookup) != []} do
case {mins > 0, configs_content(configs, locals) != []} do
{true, true} -> "SL3 delays #{mins} more min"
{true, false} -> "for #{mins} more minutes"
{false, true} -> "Expect SL3 delays"
Expand All @@ -647,9 +600,14 @@ defmodule Signs.Bus do
end

# Returns a list of audio messages describing the bridge status
defp bridge_audio(bridge_status, bridge_enabled?, current_time, state) do
%{chelsea_bridge: chelsea_bridge} = state

defp bridge_audio(
%{
bridge_enabled?: bridge_enabled?,
bridge_status: bridge_status,
current_time: current_time
},
%__MODULE__{chelsea_bridge: chelsea_bridge}
) do
if bridge_enabled? && chelsea_bridge &&
bridge_status_raised?(bridge_status, current_time) do
case bridge_status_minutes(bridge_status, current_time) do
Expand All @@ -670,9 +628,14 @@ defmodule Signs.Bus do
end
end

defp bridge_tts_audio(bridge_status, bridge_enabled?, current_time, state) do
%{chelsea_bridge: chelsea_bridge} = state

defp bridge_tts_audio(
%{
bridge_enabled?: bridge_enabled?,
bridge_status: bridge_status,
current_time: current_time
},
%__MODULE__{chelsea_bridge: chelsea_bridge}
) do
if bridge_enabled? && chelsea_bridge &&
bridge_status_raised?(bridge_status, current_time) do
{duration, duration_spanish} =
Expand Down
Loading