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

feat: Add audio config to Sectionals. #31

Merged
merged 4 commits into from
Sep 5, 2024
Merged

feat: Add audio config to Sectionals. #31

merged 4 commits into from
Sep 5, 2024

Conversation

cmaddox5
Copy link
Contributor

@cmaddox5 cmaddox5 commented Sep 5, 2024

This PR adds the Audio config to Sectionals. This will be used by screens to determine if the screen should play audio.

@cmaddox5 cmaddox5 requested a review from a team September 5, 2024 18:10
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

Curious if this is technically required — afaict the existing v1 Solari config structure does not include any of this customizability, and it seems like most of it would only be meaningful in a context where audio readouts are constantly happening on an interval, rather than on-demand as we have with these screens.

lib/config/v2/busway.ex Outdated Show resolved Hide resolved
@cmaddox5
Copy link
Contributor Author

cmaddox5 commented Sep 5, 2024

Curious if this is technically required — afaict the existing v1 Solari config structure does not include any of this customizability, and it seems like most of it would only be meaningful in a context where audio readouts are constantly happening on an interval, rather than on-demand as we have with these screens.

As of right now, it is required to get through this function. This sort of gets at something I mentioned in the last eng meeting where outside of the three fields called in that function, most of the struct is specific to bus shelter. What we should do is have two separate structs for push button and auto-readout screens. I can tackle that, but I'll make it a separate change.

@digitalcora
Copy link
Contributor

Hm, yes, I think we should consider separately (maybe as a group in breakdown) changing how audio is configured so we don't need to set fields that ultimately do nothing.

I guess my main concern would be: After merging this, will the default values for Audio ensure that audio always plays, or are we then going to have to manually reconfigure all Sectionals (and remember to configure new ones) with non-default values if we want audio to play?

@cmaddox5
Copy link
Contributor Author

cmaddox5 commented Sep 5, 2024

I see what you're saying. So maybe instead of this, Pre Fare, and GL E-Ink having an audio field, we just keep a list of push button screen types in the screens code and they just automatically work on deploy?

Just realized I may have jumped ahead a step in my thought process. To answer your question, yes it would be a two step process right now: deploy the change and then update each config with a start/stop time and day of week list.

@digitalcora
Copy link
Contributor

digitalcora commented Sep 5, 2024

I see what you're saying. So maybe instead of this, Pre Fare, and GL E-Ink having an audio field, we just keep a list of push button screen types in the screens code and they just automatically work on deploy?

That seems reasonable. But also not strictly necessary to sort out as part of this. I'd just want to know if we'd need further manual config changes from here to get Sectional audio to work, once it's implemented in the code. (Pre-Fares seem to use a single "standard" audio config that is not equal to Audio.from_json(:default). Maybe we could specify that here, as the default value for the field in defstruct, and then it would automatically populate the first time?)

Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

🔈 ✅

@cmaddox5
Copy link
Contributor Author

cmaddox5 commented Sep 5, 2024

With this change, we will be able to validate and save the config in place in the admin tool. That will apply the default json we want.

@cmaddox5 cmaddox5 merged commit 2c67758 into main Sep 5, 2024
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/sectional-audio branch September 5, 2024 19:46
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