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

enable communication with new SCU app #760

Merged
merged 3 commits into from
Jun 10, 2024
Merged

enable communication with new SCU app #760

merged 3 commits into from
Jun 10, 2024

Conversation

panentheos
Copy link
Collaborator

Summary of changes

Asana Ticket: RTS: Support communication with new SCU app

Implements the code path that sends messages to Scully, passing the audio strings through Watts first. Note that this code path is not expected to be used until we start converting stations, and there will be further changes before then. Notes:

  • Currently, any unexpected response from Watts will the associated Task to crash, effectively skipping the entire message. This is a very coarse approach that we may want to tune up later.
  • The TODO in the bus code is intended, for now. This will be implemented as a separate effort.
  • This sets us up to be able to assert on the TTS strings in the high-level sign logic tests. That will be included in its own PR, since it will be a large diff.

Copy link

github-actions bot commented Jun 6, 2024

Coverage of commit 449e649

Summary coverage rate:
  lines......: 73.2% (2017 of 2757 lines)
  functions..: 75.4% (567 of 752 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio.ex                                       | 100%      3|83.3%     6|    -      0
  lib/pa_ess/scu_updater.ex                                  | 6.1%     33|33.3%     6|    -      0
  lib/pa_ess/updater.ex                                      | 0.0%     29| 0.0%     5|    -      0
  lib/realtime_signs.ex                                      | 100%     15| 100%     3|    -      0
  lib/signs/bus.ex                                           |85.4%    240|95.2%    42|    -      0
  lib/signs/utilities/audio.ex                               |87.7%    122| 100%    15|    -      0

Download coverage report

@@ -15,10 +15,11 @@ defprotocol Content.Audio do

@type language :: :english | :spanish
@type value :: canned_message() | ad_hoc_message() | nil
@type tts_value :: {String.t(), [{String.t(), String.t(), integer()}] | nil}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a clarifying question because it's relevant to my current ticket: the first String here is what gets read out and the list of pages the transcription that gets shown, which is what lets us tweak the pronunciation of the TTS from whats shown visually? Would it be helpful to add two additional types audio_text as String.t() and visual_text as [{String.t(), String.t(), integer()}] | nil to communicate that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, can do. Also, for your ticket, take a look at the new paginate_text helper if you haven't already.

Copy link

github-actions bot commented Jun 7, 2024

Coverage of commit 2cdd832

Summary coverage rate:
  lines......: 73.2% (2018 of 2758 lines)
  functions..: 75.4% (567 of 752 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio.ex                                       | 100%      3|83.3%     6|    -      0
  lib/content/message.ex                                     | 100%      2|80.0%     5|    -      0
  lib/pa_ess/scu_updater.ex                                  | 6.1%     33|33.3%     6|    -      0
  lib/pa_ess/updater.ex                                      | 0.0%     29| 0.0%     5|    -      0
  lib/pa_ess/utilities.ex                                    |68.9%    193|96.3%    27|    -      0
  lib/realtime_signs.ex                                      | 100%     15| 100%     3|    -      0
  lib/signs/bus.ex                                           |85.4%    240|95.2%    42|    -      0
  lib/signs/utilities/audio.ex                               |87.7%    122| 100%    15|    -      0

Download coverage report

list -> Enum.at(list, i, List.last(list))
end)

^top_duration = bottom_duration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will crash the process if the top and bottom durations don't match. Is that desired, or would we want to just default to the top duration and log an error instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I put that in there as an assertion to make sure they match. Looking at the code, I think everything is the same now (6 seconds), and the intention is to enforce that going forward. If this seems to harsh, though, we could log an error instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think we loosely standardized on 6 seconds a while back so I guess this assertion would rarely, if ever, fail. But at least in concept, it does seem a bit harsh since every message gets funneled through this process which I think means a few consecutive crashes could be pretty disruptive.

Copy link

Coverage of commit 51b0445

Summary coverage rate:
  lines......: 73.1% (2018 of 2760 lines)
  functions..: 75.4% (567 of 752 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio.ex                                       | 100%      3|83.3%     6|    -      0
  lib/content/message.ex                                     | 100%      2|80.0%     5|    -      0
  lib/pa_ess/scu_updater.ex                                  | 6.1%     33|33.3%     6|    -      0
  lib/pa_ess/updater.ex                                      | 0.0%     31| 0.0%     5|    -      0
  lib/pa_ess/utilities.ex                                    |68.9%    193|96.3%    27|    -      0
  lib/realtime_signs.ex                                      | 100%     15| 100%     3|    -      0
  lib/signs/bus.ex                                           |85.4%    240|95.2%    42|    -      0
  lib/signs/utilities/audio.ex                               |87.7%    122| 100%    15|    -      0

Download coverage report

Copy link
Collaborator

@PaulJKim PaulJKim left a comment

Choose a reason for hiding this comment

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

Looks good!

@panentheos panentheos merged commit 3f2c91b into main Jun 10, 2024
2 checks passed
@panentheos panentheos deleted the bhw/new-scu branch June 10, 2024 19:47
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