Skip to content

Conversation

@SteveMicroNova
Copy link
Contributor

@SteveMicroNova SteveMicroNova commented Oct 24, 2025

What does this change intend to accomplish?

This PR aims to preserve the relative distance between zone volume levels when they would otherwise exceed the min or max vol_f

Checklist

  • Have you tested your changes and ensured they work?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • If applicable, have you updated the documentation/manual?
  • If applicable, have you updated the CHANGELOG?
  • Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • Have you written new tests for your core features/changes, as applicable?
  • If this is a UI change, have you tested it across multiple browser platforms on both desktop and mobile?

@SteveMicroNova SteveMicroNova linked an issue Oct 29, 2025 that may be closed by this pull request
@SteveMicroNova SteveMicroNova marked this pull request as ready for review October 29, 2025 16:41
@SteveMicroNova SteveMicroNova linked an issue Oct 29, 2025 that may be closed by this pull request
@SteveMicroNova
Copy link
Contributor Author

This has expanded to also include #1024 due to that being one extra line beyond what I'd already done to implement that for zones, though I then discovered it was an extra chunk to App.jsx to do the same for groups so there was some slight feature creep to this PR there.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.25%. Comparing base (7499989) to head (6c049a5).
⚠️ Report is 80 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
- Coverage   50.67%   50.25%   -0.42%     
==========================================
  Files          40       41       +1     
  Lines        7154     7368     +214     
==========================================
+ Hits         3625     3703      +78     
- Misses       3529     3665     +136     
Flag Coverage Δ
unittests 50.25% <100.00%> (-0.42%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SteveMicroNova
Copy link
Contributor Author

This does not interfere with home assistant's functionality

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.

The changes here look pretty reasonable. As this is an API change we should add one or more tests (or update pre-existing ones) right?

Comment on lines +608 to +642
assert z['vol_f'] - (zones[z['id']]['vol_f'] + 0.1) < 0.0001 and z["vol_f_overflow"] == 0

# test oversized deltas
rv = client.patch('/api/zones', json={'zones': [z['id'] for z in zones], 'update': {'vol_delta_f': -10.0}})
# test overflowing deltas
rv = client.patch('/api/zones', json={'zones': [z['id'] for z in zones], 'update': {'vol_delta_f': -1.0}})
assert rv.status_code == HTTPStatus.OK
jrv = rv.json()
assert len(jrv['zones']) >= 6
# check that each update worked as expected
for z in jrv['zones']:
if z['id'] in range(6):
assert z['vol_f'] == amplipi.models.MIN_VOL_F
assert z["vol_f_overflow"] == zones[z['id']]['vol_f'] + 0.1 - 1

# test oversized overflowing deltas
rv = client.patch('/api/zones', json={'zones': [z['id'] for z in zones], 'update': {'vol_delta_f': 10.0}})
assert rv.status_code == HTTPStatus.OK
jrv = rv.json()
assert len(jrv['zones']) >= 6
# check that each update worked as expected
for z in jrv['zones']:
if z['id'] in range(6):
assert z['vol_f'] == amplipi.models.MAX_VOL_F
assert z["vol_f_overflow"] == amplipi.models.MAX_VOL_F_OVERFLOW

# test overflow reset
mid_vol_f = (amplipi.models.MIN_VOL_F + amplipi.models.MAX_VOL_F) / 2
rv = client.patch('/api/zones', json={'zones': [z['id'] for z in zones], 'update': {'vol_f': mid_vol_f}})
assert rv.status_code == HTTPStatus.OK
jrv = rv.json()
assert len(jrv['zones']) >= 6
# check that each update worked as expected
for z in jrv['zones']:
if z['id'] in range(6):
assert z['vol_f'] == mid_vol_f
assert z["vol_f_overflow"] == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this test to account for valid overflows, oversized overflows, and overflow resets

@SteveMicroNova
Copy link
Contributor Author

There's some jittering in this yet, not due to this PR but exacerbated by it. The origin of this jitter is that we send volume change requests while sliding the slider but not all of the delta is always realized during that due to a race condition. A few solutions to this:

  • Only recognize on mouse up - This loses the ability to hear changes as you scroll
  • Convert the volume change call to be some sort of loop that goes while cumulativeDelta is not 0 as to not miss any user input - This seems complicated, not nearly as useful as python would make it. Potentially better to solve our queuing problem in the backend if this is the chosen path

There's also some potential that the jitter is due to javascript only having floats for decimal values, which would mean we have to fix this by rounding every single instance of a float vol everywhere to see this fixed

@SteveMicroNova
Copy link
Contributor Author

I'm going to refrain from rebasing #1063 and #1071 until we're happy with the volume slider functionality here, so those will not be worth reviewing until that is the case

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.

Add min/max volume buffers Set child mute status on the frontend

4 participants