Add support for DIRMODE #23
Conversation
WalkthroughThe pull request introduces significant updates to the documentation and codebase related to the YNCA protocol. Key changes include the addition of a "Pure Direct mode" section in the documentation, enhancements to existing sections for clarity, and the introduction of a new Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
ynca/subunits/zone.py (1)
Line range hint
48-70: LGTM! Well-structured helper functions.Good refactoring to reduce code duplication across volume control methods. The functions are well-documented and handle step sizes appropriately.
Consider adding a note about what happens when an unsupported step size is provided:
def do_vol_up(self, step_size: float, function: str): """ Increase the volume with given stepsize. - Supported stepsizes are: 0.5, 1, 2 and 5 + Supported stepsizes are: 0.5, 1, 2 and 5. + For 0.5, uses "Up" command. For others, uses "Up X dB" format. """docs/PRACTICALITIES.md (3)
174-177: Add language identifier to code block.Specify the language for the code block to improve syntax highlighting.
-``` +```yaml @MAIN:PUREDIRMODE=? e.g. RX-A810 @MAIN:DIRMODE=? e.g. RX-V473<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 174-174: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `171-172`: **Improve sentence readability.** Add a comma for better readability. ```diff -Both ways seem to support `On` and `Off`. Unclear why there are 2 ways or to which models it applies. +Both ways seem to support `On` and `Off`. Unclear why there are 2 ways, or to which models it applies.🧰 Tools
🪛 LanguageTool
[uncategorized] ~172-~172: Possible missing comma found.
Context: ...OnandOff. Unclear why there are 2 ways or to which models it applies. ``` @ma...(AI_HYDRA_LEO_MISSING_COMMA)
181-181: Format bare URL as a proper link.Convert the bare URL to a proper markdown link.
-This was brought up in https://github.com/mvdwetering/yamaha_ynca/discussions/340 +This was brought up in [discussion #340](https://github.com/mvdwetering/yamaha_ynca/discussions/340)🧰 Tools
🪛 Markdownlint
181-181: null
Bare URL used(MD034, no-bare-urls)
ynca/enums.py (1)
80-91: Consider standardizing all ON/OFF enumsWhile reviewing the
DirModeimplementation, I noticed that several other ON/OFF enums in this file are inconsistent in their inheritance patterns. Some inherit fromstr, Enumwhile others only inherit fromEnum. Consider standardizing all simple ON/OFF enums to follow the same pattern.Affected enums:
- Party
- PartyMute
- PureDirMode
- SpeakerA
- SpeakerB
- Straight
Would you like me to create a GitHub issue to track this standardization task?
tests/test_zone.py (1)
481-493: LGTM: Test function follows established patterns.The
test_dirmodefunction follows the same pattern as other similar tests (e.g.,test_puredirmode), properly testing both writing to the device and handling updates from the device.However, consider adding tests for handling unknown values to maintain consistency with other enum tests like
test_twochdecoder.def test_dirmode(connection, initialized_zone: ZoneBase): # Writing to device initialized_zone.dirmode = DirMode.ON connection.put.assert_called_with(SUBUNIT, "DIRMODE", "On") initialized_zone.dirmode = DirMode.OFF connection.put.assert_called_with(SUBUNIT, "DIRMODE", "Off") # Updates from device connection.send_protocol_message(SUBUNIT, "DIRMODE", "On") assert initialized_zone.dirmode == DirMode.ON connection.send_protocol_message(SUBUNIT, "DIRMODE", "Off") assert initialized_zone.dirmode == DirMode.OFF + + # Unknown value + connection.send_protocol_message(SUBUNIT, "DIRMODE", "UnknownValue") + assert initialized_zone.dirmode == DirMode.UNKNOWN
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
docs/PRACTICALITIES.md(1 hunks)tests/test_enums.py(2 hunks)tests/test_zone.py(5 hunks)ynca/__init__.py(2 hunks)ynca/enums.py(1 hunks)ynca/server.py(1 hunks)ynca/subunits/zone.py(10 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/PRACTICALITIES.md
[uncategorized] ~172-~172: Possible missing comma found.
Context: ...On and Off. Unclear why there are 2 ways or to which models it applies. ``` @ma...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
docs/PRACTICALITIES.md
181-181: null
Bare URL used
(MD034, no-bare-urls)
174-174: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
ynca/__init__.py (2)
19-19: LGTM! Import statement follows the module's conventions.
The DirMode enum is correctly imported and maintains alphabetical ordering within the import group.
64-64: LGTM! Public API exposure is properly configured.
The DirMode enum is correctly exposed in the module's public API through __all__ list, maintaining alphabetical ordering.
tests/test_enums.py (2)
10-10: LGTM! Import statement is well-placed.
The DirMode enum import follows the alphabetical ordering convention and matches the existing import pattern.
47-47: LGTM! Test case follows best practices.
The test case for DirMode.UNKNOWN handling is consistent with the existing test pattern and properly validates the graceful handling of invalid values.
ynca/subunits/zone.py (4)
9-11: LGTM! Clean import organization.
The import changes are well-structured and properly organized, with clear separation between enum and function imports.
Also applies to: 30-37
77-77: LGTM! Well-integrated dirmode attribute.
The dirmode attribute is properly implemented using EnumFunctionMixin and follows the class's pattern of BASIC initialization.
146-148: LGTM! Good model-specific documentation.
The comment clearly documents that puredirmode behavior differs on the RX-V1067 model, which is valuable information for maintainers.
206-207: LGTM! Clear model-specific documentation.
The comments effectively document the difference in BASIC mode support between RX-V583 and RX-V573 models for speaker A/B functionality.
docs/PRACTICALITIES.md (1)
169-181: LGTM! Documentation structure is consistent.
The new Pure Direct mode section follows the established documentation pattern, providing clear examples and practical information about the feature's implementation variations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~172-~172: Possible missing comma found.
Context: ...On and Off. Unclear why there are 2 ways or to which models it applies. ``` @ma...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint
181-181: null
Bare URL used
(MD034, no-bare-urls)
174-174: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
ynca/server.py (1)
122-122: LGTM! Verify DIRMODE command handling.
The addition of "DIRMODE" to the multiresponse_functions_table is correct and follows the existing pattern. This enables the server to handle DIRMODE commands consistently with other BASIC functions.
Let's verify the DIRMODE implementation across the codebase:
✅ Verification successful
Let's check the actual implementation in the test file and look for any potential issues.
Let's check the actual zone implementation since we couldn't find zone.py directly.
DIRMODE implementation is properly integrated and verified
The implementation is complete and correct:
- DirMode enum is properly defined in
ynca/enums.pywith ON/OFF values - Zone implementation in
ynca/subunits/zone.pycorrectly includes dirmode property with BASIC initialization - Test coverage in
tests/test_zone.pyverifies both device updates and command handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DIRMODE implementation and test coverage
# Test 1: Check for DIRMODE command handling in tests
echo "Checking test coverage for DIRMODE..."
rg -l "DIRMODE" "tests/"
# Test 2: Check for DIRMODE usage in main code
echo "Checking DIRMODE usage in main code..."
rg -l "DIRMODE" "ynca/" --glob "!server.py"
# Test 3: Check for any DIRMODE-related enums
echo "Checking for DIRMODE-related enums..."
ast-grep --pattern 'class $_ {
$$$
DIRMODE
$$$
}'
Length of output: 398
Script:
#!/bin/bash
# Check the DIRMODE test implementation
echo "Checking DIRMODE test implementation..."
rg "DIRMODE" tests/test_zone.py -B 2 -A 2
# Check if there are any DIRMODE-related functions or properties in zone.py
echo "Checking zone.py for DIRMODE implementation..."
rg "DIRMODE" ynca/zone.py -B 2 -A 2
Length of output: 1722
Script:
#!/bin/bash
# Find the zone implementation file
echo "Finding zone implementation file..."
fd zone
# Check for DirMode enum definition
echo "Checking for DirMode enum..."
ast-grep --pattern 'class DirMode {
$$$
}'
# Check for dirmode property implementation
echo "Looking for dirmode property implementation..."
ast-grep --pattern '@property
$$$
dirmode($$$)'
Length of output: 398
Script:
#!/bin/bash
# Check the actual implementation in the correct zone file
echo "Checking zone implementation..."
rg "DIRMODE|DirMode" ynca/subunits/zone.py -B 2 -A 2
# Check for any enum definitions in the file
echo "Checking enums in zone.py..."
ast-grep --pattern 'class $_ (Enum):
$$$'
Length of output: 8678
tests/test_zone.py (2)
11-11: LGTM: Import added correctly.
The DirMode import is properly added alongside other related enums.
57-57: LGTM: Initialization test updated correctly.
The assertion for dirmode is properly added to verify the initial state in the INITIALIZE_FULL_RESPONSES fixture.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
docs/PRACTICALITIES.md (2)
174-177: Add language identifier to the code block.The code block should specify a language identifier for better syntax highlighting and to comply with markdown best practices.
-``` +```yaml @MAIN:PUREDIRMODE=? e.g. RX-A810 @MAIN:DIRMODE=? e.g. RX-V473<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 174-174: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `181-181`: **Format the URL reference properly.** The bare URL should be formatted as a proper markdown link for better readability and to comply with markdown best practices. ```diff -This was brought up in https://github.com/mvdwetering/yamaha_ynca/discussions/340 +This was brought up in [discussion #340](https://github.com/mvdwetering/yamaha_ynca/discussions/340)🧰 Tools
🪛 Markdownlint
181-181: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
docs/PRACTICALITIES.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint
docs/PRACTICALITIES.md
181-181: null
Bare URL used
(MD034, no-bare-urls)
174-174: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
docs/PRACTICALITIES.md (1)
179-179: Fix inconsistency in BASIC response behavior documentation.
The statement "Both seem to be part of BASIC" contradicts the earlier note in the BASIC Response section which states that PUREDIRMODE is not part of BASIC response for RX-V1067. Consider revising for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
docs/PRACTICALITIES.md (2)
169-183: Enhance the Pure Direct mode documentation with more specific details.Consider the following improvements to make the documentation more precise and helpful:
- Add specific details about which signal processing features are bypassed in Pure Direct mode versus Direct mode.
- Update the model applicability statement to be more precise, as per verified logs:
DIRMODE: Used by older/lower-end models (RX-V473, RX-V475, RX-V500D, RX-V583)PUREDIRMODE: Used by higher-end/newer models (RX-A series, RX-V685, RX-V2067, TSR-700)🧰 Tools
🪛 Markdownlint
178-178: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
178-181: Add language specification to the code block.Add a language identifier to the code block for better syntax highlighting.
-``` +```text @MAIN:PUREDIRMODE=? e.g. RX-A810 @MAIN:DIRMODE=? e.g. RX-V473<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 178-178: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d6e7e9c41b83f116fb7a3a9608230f35b7bdcce3 and 441628b2d57f8832fd8b9c4d2da03b2a31bed967. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/PRACTICALITIES.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary> <details> <summary>docs/PRACTICALITIES.md</summary> 178-178: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
Release Notes
New Features
DirMode, to enhance command handling related to device operational modes.Bug Fixes
Tests
DirModefunctionality, ensuring proper handling of device commands and states.Chores
ZoneBaseclass and related methods for better maintainability.