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

[monopriceaudio] Monoprice Whole House Amplifier Binding - initial contribution #7371

Merged
merged 58 commits into from
Sep 3, 2020

Conversation

mlobstein
Copy link
Contributor

[monopriceaudio] Initial contribution

This is the initial implementation of a Binding to control the Monoprice MPR-SG6Z or Dayton Audio DAX66 multi-zone whole house amplifier. The binding is configurable to control up to 18 zones when 3 amplifiers are connected together. The preferred connection to the amplifier controller is via a direct serial port connection but serial over IP connections are also supported.

@mlobstein mlobstein requested a review from a team as a code owner April 14, 2020 15:26
@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Apr 14, 2020
@swamiller
Copy link

swamiller commented Apr 19, 2020

Very nice to see someone make a binding for this amp, thank you! Any chance of creating a method of connecting via a globalcache serial device? I currently use a rule to connect to my amp via a globalcache iTach Flex. The only difference to a normal serial connection is the need to use URL-encoded characters for special characters in the serial commands and the send and receive are two separate channels.

See an old version of my rule for reference of the serial command differences: https://community.openhab.org/t/monoprice-6-zone-audio-amp-items-sitemap-rules/1693/5?u=swamiller

I have since implemented a much shorter version based on this rule using URL encoded commands and separate send and receive channels: https://community.openhab.org/t/monoprice-6-zone-audio-amp-items-sitemap-rules/1693/26?u=swamiller

Also see the globalcache binding: https://www.openhab.org/addons/bindings/globalcache/

@mlobstein
Copy link
Contributor Author

mlobstein commented Apr 19, 2020

Very nice to see someone make a binding for this amp, thank you! Any chance of creating a method of connecting via a globalcache serial device?

It should work as currently coded on an iTach with the Serial over IP thing example. In the iTach API it states "All serial data is passed through without interpretation or conversion via an assigned, unique IP port. The iTach serial connector is assigned to IP Port 4999." So I don't think the extra newline character and special encoding at the end of command string that you did in your rule set is needed. In this use case, the new binding will directly connect to the iTach instead of going through the openHAB globalcache binding. Do you want to be a beta tester for the binding to verify this?

@swamiller
Copy link

I would be happy to test it out.

@mlobstein
Copy link
Contributor Author

mlobstein commented Apr 19, 2020

Here is the jar file: https://github.com/mlobstein/mlobstein-beta-test/raw/master/org.openhab.binding.monopriceaudio-2.5.8-SNAPSHOT.jar

If you receive an error about openhab-transport-serial, issue the following command in the openhab console:

feature:install openhab-transport-serial

@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@swamiller
Copy link

Binding works with iTach Flex on port 4999, thank you! I have 12 zones and all seem to work.

One request, can you add bass, treble, and balance channels? Also, why is the all zone power divided into separate on and off channels?

@mlobstein
Copy link
Contributor Author

The regular zones should already have have treble, bass and balance channels baked in. If you are looking at the thing's channels in paper ui, try clicking the "SHOW MORE" link to see all the channels. I am not sure why it hides them like that.

For 'All Zones', the channels really exist in the virtual sense so I wanted it to be different button presses for 'All on' or 'All off'. It didn't make sense to me to have it be a switch that could be thrown on and off. Because once all zones are turned on, you can immediately turn an individual zone back off. And would that mean that the 'All' power switch should then go to the off position when that happens? And then what if you want to turn all the remaining zones off? It just makes more sense to have the All Zone power on and off be discreet button pushes.

@swamiller
Copy link

The all zones logic makes sense.

As for the bass/treble/balance channels, I have 12 zones showing in the "Things" menu: Power, Source Input, Volume, Mute, Do Not Disturb, Page Active, Keypad Active for each zone. No "SHOW MORE" option and no other channels show in the Paper UI.

@swamiller
Copy link

I added the bass, treble, and balance channels. I believe the multiplier for the bass and treble is incorrect. 100% bass (as an example) is showing as "7". Same for the treble channel.

Also, can the balance be set so 0 = 50%? Visually it would look like center.

Otherwise, the binding seems to connect well and is much more responsive than the rule was.

@mlobstein
Copy link
Contributor Author

I added the bass, treble, and balance channels. I believe the multiplier for the bass and treble is incorrect. 100% bass (as an example) is showing as "7". Same for the treble channel.

Also, can the balance be set so 0 = 50%? Visually it would look like center.

Otherwise, the binding seems to connect well and is much more responsive than the rule was.

I did the values of Treble, Bass and Balance to match the keypads exactly instead of scaling them to 0-100%. So for Treble & Bass the range is -7 to 7 and Balance is -10 to 10 (0 being centered). In the examples for BasicUI, it sets them up as set points with the appropriate min and max limits.

image

@swamiller
Copy link

Ok, I see that now. For bass/treble I didn't believe the amp was set up for negative values so the rule I was using had these as percentages. 0% being "flat".

@mlobstein
Copy link
Contributor Author

Ok, I see that now. For bass/treble I didn't believe the amp was set up for negative values so the rule I was using had these as percentages. 0% being "flat".

Internally the amp uses 0-14 for bass/treble. But on the keypads it displays as -7 to 7 with 0 being flat. I thought it cleaner to match the keypads for this detail. You probably don't adjust those as often and since the range is much more limited it is probably easier to think in terms of "I like the bass at 5 and the treble at 4 for zone X".

@swamiller
Copy link

Ok, I can work with that.
No other comments for me. Logs are clean, everything works. I love the responsiveness and it plays well with the 3rd party iPhone app.
Thank you!

@mlobstein
Copy link
Contributor Author

Ok, I can work with that.
No other comments for me. Logs are clean, everything works. I love the responsiveness and it plays well with the 3rd party iPhone app.
Thank you!

Thanks for the feedback and I 'm glad it is working for you!

@cpmeister what is next for getting this into the mainline?

@cpmeister
Copy link
Contributor

@cpmeister what is next for getting this into the mainline?

Next step would be to wait until a maintainer like myself has time to review your code. For now I suggest to be patient, there are a lot of bindings PRs that are many months old at this point and I am still going through them. That said, this binding is only 5/6th down my list since most of the other binding authors are not very responsive, so you might not have to wait that long.

@mlobstein
Copy link
Contributor Author

One request, can you add bass, treble, and balance channels? Also, why is the all zone power divided into separate on and off channels?

@swamiller I fixed this issue with treble, bass and balance not showing up by default when adding channels. The change will show up in when this PR is merged.

@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@bkpsu
Copy link

bkpsu commented May 22, 2020

@mlobstein - excellent! Looking forward to trying this out myself over the weekend. I assume the post from 4/19 has the latest JAR linked, since it's been edited :)

Any thoughts on incorporating the 30128 into the binding? (See this post for the (relatively) minor serial message modifications to the serial messaging. I don't own that amp, but their users would find this binding helpful for that one, as well!

Thanks!

@mlobstein
Copy link
Contributor Author

mlobstein commented May 22, 2020

@mlobstein - excellent! Looking forward to trying this out myself over the weekend. I assume the post from 4/19 has the latest JAR linked, since it's been edited :)

Any thoughts on incorporating the 30128 into the binding? (See this post for the (relatively) minor serial message modifications to the serial messaging. I don't own that amp, but their users would find this binding helpful for that one, as well!

Thanks!

Yes the jar created on May 22nd is the latest. At first glance it seems like support for the 30128 would be easy to add but every part of it's protocol is slightly different. I don't think I can make the binding do both without have that amp on hand to test while writing the code. I don't use my 10761 in the house anymore (using another WHA) so I was able to hook it up under my desk for a couple weeks and test every code change on the real hardware as I went along.

@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@mlobstein
Copy link
Contributor Author

@kaikreuzer can you assign someone else to review since cpmeister is absent?

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

In general the code is great. Some more structural comments also made apply to the Nuvo binding pr #7651 (where I made the same comments). Like how ip connection is supported and definition of channel types for zones.

Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@mlobstein mlobstein requested a review from Hilbrand August 24, 2020 22:08
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests have failed

Hey @mlobstein,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM, only needs rebase to handle conflicts.

@mlobstein
Copy link
Contributor Author

LGTM, only needs rebase to handle conflicts.

The conflicts are resolved. Ready to merge. Thank you for all the great feedback!

@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand merged commit d574e37 into openhab:2.5.x Sep 3, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Sep 3, 2020
@mlobstein mlobstein deleted the MonopriceAudio branch September 3, 2020 13:41
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
@brianlay714
Copy link

mlobstein
Trying to use this binding with OH3 and having an issue with controlling the tone and balance controls. no matter how I configure the dimmer values the amp ignores my commands.
should this binding work in 3.0?
thanks for any advice

@bkpsu
Copy link

bkpsu commented Mar 10, 2021

mlobstein
Trying to use this binding with OH3 and having an issue with controlling the tone and balance controls. no matter how I configure the dimmer values the amp ignores my commands.
should this binding work in 3.0?
thanks for any advice

Works just fine for me - I had to add metadata to the item, i.e.

Default Standalone Widget = oh-knob-card
Default List Item Widget = oh-slider-item

Then, configure each to have:
Min = -7
Max = 7
Step = 1

(for the slider, I also enabled "Display Scale" and set "Scale Steps" to 14)

@mlobstein
Copy link
Contributor Author

mlobstein commented Mar 10, 2021

@bkpsu thanks for the hint! I posted it over on the forum as well. It is a shame one has to do all this extra fiddling in OH3 even though the min/max/step values were painstakingly entered into the binding's channels.xml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants