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

Added a configurable volume step (in setup flow) and improved refresh of volume #38

Merged
merged 8 commits into from
Jun 11, 2024

Conversation

albaintor
Copy link
Contributor

Hi,

I have added a configurable volume step in setup flow (and I added necessary checks on the datamodel which may not include this new field).
Also I added a call to async refresh when sending volume up & down commands to have a more precise volume update instead of referring to updated local variable. This avoids "glitches" when the volume is updated locally while an async update (every 10s) occur.

@zehnm zehnm self-requested a review June 4, 2024 07:31
Copy link
Contributor

@zehnm zehnm left a comment

Choose a reason for hiding this comment

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

Great, thank you! Volume step configurability is a great addition.
I'll need at least another day to test the new volume handling.
The volume step configuration can be defined as a number and limited to the valid range. This simplifies the UI handling and is less error prone.
I don't see a reason to allow larger volume steps than 10 dB, or is there a specific use-case for that?

intg-denonavr/setup_flow.py Outdated Show resolved Hide resolved
intg-denonavr/setup_flow.py Outdated Show resolved Hide resolved
@albaintor
Copy link
Contributor Author

Hi Markus, I just committed the changes

intg-denonavr/setup_flow.py Outdated Show resolved Hide resolved
intg-denonavr/setup_flow.py Outdated Show resolved Hide resolved
@albaintor
Copy link
Contributor Author

Hi, now I remember why I used text field instead of numbers : it won't support decimal numbers.
I jut rollbacked changes to text format and it should work. Maybe I didn't use the number field format properly ?

@zehnm
Copy link
Contributor

zehnm commented Jun 11, 2024

it won't support decimal numbers.

The issue is only the steps field. This causes a parsing error in the core service, because it expects an int and not a float. I'll fix this, so it's according to the integration API specification.
The volume steps field needs to be a number field. The following definition works with the current core service:

"field": {
    "number": {"value": 0.5, "min": 0.5, "max": 10, "steps": 1, "decimals": 1, "unit": {"en": "dB"}}
}

@albaintor
Copy link
Contributor Author

Okay I reverted changes and applied step to 1. I didn't understand that the problem was located only on one of the fields.
Otherwise we could keep the 0.5 value if you intend to update the library at the same time in the same firmware ?

@albaintor albaintor requested a review from zehnm June 11, 2024 07:21
Copy link
Contributor

@zehnm zehnm 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, thanks!

@zehnm zehnm merged commit 0f2aa35 into unfoldedcircle:main Jun 11, 2024
1 check passed
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