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

Implement Logitech Media Server in mainsteam AmpliPi #549

Merged
merged 14 commits into from
Dec 7, 2023
Merged

Conversation

rtertiaer
Copy link
Contributor

Filing this as a draft for now. When completed, this ought to close #532 . I'd like some early feedback because we'd like to ship 0.3.0 before the holidays.

amplipi/streams.py Outdated Show resolved Hide resolved
amplipi/streams.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (725ccf0) 51.52% compared to head (0d470f2) 51.52%.

Files Patch % Lines
amplipi/app.py 18.18% 18 Missing ⚠️
amplipi/streams.py 0.00% 9 Missing ⚠️
amplipi/ctrl.py 83.33% 4 Missing ⚠️
amplipi/defaults.py 86.95% 3 Missing ⚠️
tests/test_rest.py 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #549      +/-   ##
===========================================
- Coverage    51.52%   51.52%   -0.01%     
===========================================
  Files           23       24       +1     
  Lines         5624     5673      +49     
===========================================
+ Hits          2898     2923      +25     
- Misses        2726     2750      +24     
Flag Coverage Δ
unittests 51.52% <58.82%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rtertiaer rtertiaer marked this pull request as ready for review December 5, 2023 00:07
@rtertiaer
Copy link
Contributor Author

rtertiaer commented Dec 5, 2023

per in-person review:

  • remove deallocatable stuff - PersistentStreams class does most or all of what we want
  • Make the LMS mode frontend stuff a toggle-able switch, with state
  • ensure in LMS mode, streams are eagerly started/activate()d instead of lazily (when finally attached to a source)

amplipi/ctrl.py Outdated
Comment on lines 285 to 286
if self.lms_mode and stream.type == 'lms':
self.streams[stream.id].activate()
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 considered doing this in the constructor but the controller is the bit that knows & cares about if it's in LMS mode.

amplipi/streams.py Outdated Show resolved Hide resolved
@linknum23
Copy link
Contributor

Testing this on lamplipi shows the lms clients dropping after a reset. Heres the sequence I tested.

  1. Enable LMS mode
  2. Set each source to its corresponding RCA input
  3. Went to config->reset
  4. All LMS clients are missing now from the LMS Server UI hosted on port 9000

@rtertiaer
Copy link
Contributor Author

@linknum23 I was not able to replicate your problem above. I'd love to hash this out a smidge in person, because there's some wackiness with our multiple test beds that I think is complicating things a little bit.

@rtertiaer
Copy link
Contributor Author

@linknum23 I think the thing you bumped into is fixed now. The bug was that every other restart of squeezelite leaves the ALSA loopback in a funny state and it cannot properly bind to the 'device'; it then simply bails with an error. The fix ultimately required us building our own squeezelite .deb for an upstream commit, which permitted us to manage squeezelite using process_monitor.py.

I'm going to continue testing this.

Copy link
Contributor

@linknum23 linknum23 left a comment

Choose a reason for hiding this comment

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

This PR really needs #557 for zones to work right but as is it is pretty fantastic. Nice work @rtertiaer

@rtertiaer rtertiaer merged commit 6799b8e into develop Dec 7, 2023
2 checks passed
@rtertiaer rtertiaer deleted the upstream_lms branch December 7, 2023 20:56
@rtertiaer rtertiaer mentioned this pull request Dec 14, 2023
2 tasks
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.

None yet

3 participants