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

[rotel] New channels tcbypass, balance, speakera and speakerb #12447

Merged
merged 17 commits into from
Mar 22, 2022

Conversation

tonwi
Copy link
Contributor

@tonwi tonwi commented Mar 8, 2022

  • [rotel] Extension of channel list for amplifiers A11, A12 and A14

The list of channels for the Rotel devices A11, A12 and A14 has been extended by 4 channels:

  1. TONE-CONTROL-BYPASS (Switch)
    If TCBypass is ON the device "bypasses" any bass or treble adjustments.
    Instead, an optimal sound setting of the manufacturer is used
    in the case TCBypass is OFF the user is enabled to set his own bass and treble adjustments.
  2. BALANCE (Number)
    The sound balance can be adjusted.
  3. SPEAKER-A (Switch)
    A14 provides 2 speaker groups: A / B
    Channel/Item SPEAKER-A switches speaker group A ON or OFF respectively.
  4. SPEAKER-B (Switch)
    A14 provides 2 speaker groups: A / B
    Channel/Item SPEAKER-B switches speaker group B ON or OFF respectively.

Signed-off-by: Wilhelm Tonsern wto.wit01@gmx.net

@tonwi tonwi requested a review from lolodomo as a code owner March 8, 2022 16:08
@lolodomo lolodomo changed the title A14 tonwi [rotel binding] Extension of amplifier A14's channel list Mar 8, 2022
@lolodomo lolodomo changed the title [rotel binding] Extension of amplifier A14's channel list [rotel] Extension of amplifier A14's channel list Mar 8, 2022
@lolodomo
Copy link
Contributor

lolodomo commented Mar 9, 2022

Please sign off properly (just a text to add in each of your commit).

I will start with a review of what you (wrongly?) changed in the existing code.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/unable-to-git-push-my-binding-extensions/134031/7

Wilhelm Tonsern added 2 commits March 9, 2022 08:48
Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Mar 9, 2022
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review part 1 of 3

package owner's requests implemented

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@tonwi tonwi requested a review from lolodomo March 15, 2022 12:18
@lolodomo
Copy link
Contributor

@tonwi : I will continue my review this evening.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review part 2 of 3

@tonwi
Copy link
Contributor Author

tonwi commented Mar 18, 2022

I activated the "resove conversation" button: I don't know what that does. But i certainly don't want to end the conversation or delete it or....

@lolodomo
Copy link
Contributor

I prefer you let me mark the conversation as resolved. This is easier for me to follow if the comments were considered. I do it after checking the changes done in the PR.

CONSTRUCTOR: adjustments for TCBYPASS_..., BALANCE_... and SPEAKER_...

MODEL: extension of Models A11, A12

THING: A11, A12 channel extension

README: typo patched; serialPort value improved

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@tonwi tonwi requested a review from lolodomo March 18, 2022 09:47
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor

@tonwi : I just proposed changes (through a PR) to your branch. Please have a look.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

I pushed my review comments as a PR to your repo.
Here are the remaining additional comments/questions.

@lolodomo
Copy link
Contributor

@tonwi : with the changes I pushed to you, the code is now ready to add your new channels to other models including models supporting the "old" ASCII protocol. Once you merge my PR in your PR, I will propose to you a new small PR with channels added on other models.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor

@tonwi : with the changes I pushed to you, the code is now ready to add your new channels to other models including models supporting the "old" ASCII protocol. Once you merge my PR in your PR, I will propose to you a new small PR with channels added on other models.

This is now included in the PR I pushed to you.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor

This is also possible to handle speaker A and speaker B for the models RX-1050 and RX-1052. These models use the HEX protocol.
If you are fast to integrate my PR, I will do it later to not delay the merge of your PR.

@lolodomo
Copy link
Contributor

This is also possible to handle speaker A and speaker B for the models RX-1050 and RX-1052. These models use the HEX protocol. If you are fast to integrate my PR, I will do it later to not delay the merge of your PR.

I do not remember why but these 2 models (RX-1050 and RX-1052) are not yet supported by the binding. So if I do something, this will be definitively in a separate PR.

@tonwi
Copy link
Contributor Author

tonwi commented Mar 19, 2022

This is also possible to handle speaker A and speaker B for the models RX-1050 and RX-1052. These models use the HEX protocol. If you are fast to integrate my PR, I will do it later to not delay the merge of your PR.

Please give me advice how to "integrate your PR into mine"? Do I have to "git pull...." into my local branch? How should I do this? What I have to do? You must know: I do not have the slightest idea. My knowledge of "git" is very modest. Please give me advice!

@lolodomo
Copy link
Contributor

lolodomo commented Mar 19, 2022

First go to your Git page: tonwi#1
Look at my changes and if you are ok, merge the changes. That's all. Your remote branch will then include my changes and the current PR will even be updated.
If you want then to retrieve the code in your local branch from your remote branch, go into your local branch and the command should be something like: git pull tonwi a14-tonwi

@tonwi
Copy link
Contributor Author

tonwi commented Mar 20, 2022

I destroyed my local repository completely.
In order to avoid erroneous "git push" commands and conflicts with this #12447 branch , i am informing you of an unimportant typo:
file: RotelConnec tor.java
line: 479
comment:
....The firmware expects values like r05 or L04 ....
should be
....The firmware expects values like r05 or l04 ....

@lolodomo
Copy link
Contributor

I destroyed my local repository completely.

You can create it again from your remote branch:

git remote add tonwi https://github.com/tonwi/openhab-addons.git 
git fetch tonwi
git checkout -b a14-tonwi tonwi/a14-tonwi

i am informing you of an unimportant typo

Can you please fix it ?

@lolodomo
Copy link
Contributor

And please discuss the last opened point ... so that we can quickly conclude this PR.

TCBYPASS ON/OFF: update channels for BASS/TREBLE in both cases

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
…eset BASS/TREBLE to 0.

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@tonwi tonwi requested a review from lolodomo March 22, 2022 10:57
@lolodomo
Copy link
Contributor

Just to be clear, here is how I imagine the final method:

    private void handleTcbypassCmd(String channel, Command command, RotelCommand onCmd, RotelCommand offCmd)
            throws RotelException, InterruptedException {
        if (command instanceof OnOffType) {
            if (command == OnOffType.ON) {
                connector.sendCommand(onCmd);
                bass = 0;
                treble = 0;
                updateChannelState(CHANNEL_BASS);
                updateChannelState(CHANNEL_TREBLE);
            } else if (command == OnOffType.OFF) {
                connector.sendCommand(offCmd);
                Thread.sleep(200);
                connector.sendCommand(RotelCommand.BASS);
                Thread.sleep(200);
                connector.sendCommand(RotelCommand.TREBLE);
            }
        } else {
            logger.debug("Command {} from channel {} failed: invalid command value", command, channel);
        }
    }

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@tonwi
Copy link
Contributor Author

tonwi commented Mar 22, 2022

it appears that it is working

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
@tonwi tonwi requested a review from lolodomo March 22, 2022 15:16
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo
Copy link
Contributor

Thank you @tonwi for contributing to this binding. I will merge your PR this evening.

@lolodomo lolodomo merged commit b347491 into openhab:main Mar 22, 2022
@lolodomo lolodomo added this to the 3.3 milestone Mar 22, 2022
@lolodomo lolodomo changed the title [rotel] Extension of amplifier A14's channel list [rotel] New channels tcbypass, balance, speakera and speakerb Mar 22, 2022
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
* A14: new channels: tcbypass, balance, speakera and speakerb
* Specific balance set command for models A1x
* Channels added on other models
* In state TCBYPASS ON: 1) Ignore user adjustments on BASS/TREBLE. 2) Reset BASS/TREBLE to 0.

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Also-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* A14: new channels: tcbypass, balance, speakera and speakerb
* Specific balance set command for models A1x
* Channels added on other models
* In state TCBYPASS ON: 1) Ignore user adjustments on BASS/TREBLE. 2) Reset BASS/TREBLE to 0.

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Also-by: Laurent Garnier <lg.hc@free.fr>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* A14: new channels: tcbypass, balance, speakera and speakerb
* Specific balance set command for models A1x
* Channels added on other models
* In state TCBYPASS ON: 1) Ignore user adjustments on BASS/TREBLE. 2) Reset BASS/TREBLE to 0.

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Also-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* A14: new channels: tcbypass, balance, speakera and speakerb
* Specific balance set command for models A1x
* Channels added on other models
* In state TCBYPASS ON: 1) Ignore user adjustments on BASS/TREBLE. 2) Reset BASS/TREBLE to 0.

Signed-off-by: Wilhelm Tonsern <wto.wit01@gmx.net>
Also-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants