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

[YamahaReceiver] Compatibility fixes and improvements for 2.3.0 #3264

Merged

Conversation

zarusz
Copy link
Member

@zarusz zarusz commented Feb 21, 2018

This PR has a bunch of fixes and improvements to the YamahaReceiver addon:

  • Fix #2758: [YamahaReceiver] Problem with HDMI input
  • Fix #3034: [YamahaReceiver] VolumeDB is not updated
  • Fix #3062: [YamahaReceiver] ‘Zone_2’ of Yamaha HTR-xxx receiver completely incontrollable
  • Fix RX-V3900 compatibility
  • Fix Tuner: The Tuner play info was not working (tested on HTR-4069)
  • Feature: Added Straight surround program
  • Feature: Introduced zone channel settings for minimum and maximum volume in dB
  • Feature: Added setting for default album image URL
  • Feature: Added Spotify, Server as supported preset inputs
  • Minor improvement (compatibility discovery, optimizations, developer documentation)

Testing was done on few Yamaha models (RX-V3900, HTR-4069, RX-S601D). Many thanks to everyone from the community who helped with verifying fixes and testing regressions!

Follow the Jenkins build link to access artifactory where you could download the JAR.

Signed-off-by: Tomasz Maruszak maruszaktomasz@gmail.com

@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/yamahareceiver-zone-discovery-not-working-with-rx-v3900/38350/4

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from ff3d30a to 2e1ea86 Compare February 22, 2018 22:49
@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from 2e1ea86 to ea068e5 Compare March 3, 2018 13:19
<default>data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEASABIAAD//gBKRmlsZSBzb3VyY2U6IGh0dHBzOi8vY29tbW9ucy53aWtpbWVkaWEub3JnL3dpa2kvRmlsZTpSZWNvcmQtQWxidW0tMDIuanBn/+ICHElDQ19QUk9GSUxFAAEBAAACDGxjbXMCEAAAbW50clJHQiBYWVogB9wAAQAZAAMAKQA5YWNzcEFQUEwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAPbWAAEAAAAA0y1sY21zAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAKZGVzYwAAAPwAAABeY3BydAAAAVwAAAALd3RwdAAAAWgAAAAUYmtwdAAAAXwAAAAUclhZWgAAAZAAAAAUZ1hZWgAAAaQAAAAUYlhZWgAAAbgAAAAUclRSQwAAAcwAAABAZ1RSQwAAAcwAAABAYlRSQwAAAcwAAABAZGVzYwAAAAAAAAADYzIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAdGV4dAAAAABGQgAAWFlaIAAAAAAAAPbWAAEAAAAA0y1YWVogAAAAAAAAAxYAAAMzAAACpFhZWiAAAAAAAABvogAAOPUAAAOQWFlaIAAAAAAAAGKZAAC3hQAAGNpYWVogAAAAAAAAJKAAAA+EAAC2z2N1cnYAAAAAAAAAGgAAAMsByQNjBZIIawv2ED8VURs0IfEpkDIYO5JGBVF3Xe1rcHoFibGafKxpv33Tw+kw////2wBDAAYEBQYFBAYGBQYHBwYIChAKCgkJChQODwwQFxQYGBcUFhYaHSUfGhsjHBYWICwgIyYnKSopGR8tMC0oMCUoKSj/2wBDAQcHBwoIChMKChMoGhYaKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCj/wAARCAClANwDASIAAhEBAxEB/8QAHAAAAAcBAQAAAAAAAAAAAAAAAAECAwQFBgcI/8QAQxAAAgEDAQUFBQYCCAUFAAAAAQIDAAQRBQYSITFBEyJRYXEHMoGRoRRCUrHB0WJyIzNDgpKi4fAIFRYkUzSDstLx/8QAGgEAAgMBAQAAAAAAAAAAAAAAAgMBBAUABv/EACwRAAICAQMDAwMEAwEAAAAAAAECAAMREiExBCJBUWHwE3HBMqGx4QUzgUL/2gAMAwEAAhEDEQA/APMv3vIgGiPM8uQ50XUcuooxksAOZGPGr0RFDiT54NK6nx4VJi0+6kQydj2cIHGSU7ij4molxLFESI5O2fqwBC/XifkKIqVGW2gBgxwu8cHM+AP6U20yL1zx6VDeRn5n4dKRSzb6Rgr9ZKN1gd1R8eNINxIevD5UxR0OsmHpEWZGPWiLsebH50mjrskzsQ8k9TQ+NFR4NEJ0Ug8zVpYouRnBHmAarUiZjwFXOkQRqQJn3RnjVmhSWAxK/UMAssZBB9mKmGDOPeCYPzBqkeR4JC0EjoR1ViD8xxq42jdGjjFowYAccDBrMtI59459aude4DaAOPMrdIhZdR8y8s9p9XtSNy+mYeEh7Qf5uP1q+stvpwAt9aQSjq0ZMbfLiKwy8VzxFGaqL1FicNHP0lL8rN7/ANRWjast7au8OcFllXGCOB4jgRXR7FvtunpexRObdju74GVDeGeVeeVZlPdJFXOgbT6toUu/pl9PbZ95Y2yjfzIe6flVivriP1iVbf8AHBt1M7kIFI4AVValazjUY7i3z3YuzwOB5/WqjQvaVZ3m7HrtkIZDwN1YjA9Wi/8Ar8q3Nsba+tBdafcQ3lqf7SI5x5MOYPrV5bVtHaZSNL0nuG0xGsWtnPazSXmnWzzKhYSGLdcEDhxqPtTawS6Js/b3qs9uRGxCtulid/Iz6YrSbVssWiXu4B/VFfnw/WqjaPThfX+l2rM4SFLbgPHcz+tLtXsYDk4nfUGtD4GfxK620nRLbjBo1qX8ZWaX6E4rQaal5cz26Kewt4mDbiKEUAdAorQR6ZEGPAk56cPyqakEVun3Yx58KBatJ5/EY9oYY5jSxnwpE00cT7r53sdBmmtQ1e2tEbDZbHx+X6ms8bLWNWJu4DHFE3BQ7lSR48q5798JuYtaM7tsJzW30GAEGeSSU+A7o/ejvb+w0cGOCCNrn8Cjl/Mf051C1vXsb1vp7eTTD8l/esyck5J41Tu6lKu2gb+sv09NZd3Xk49JL1DUbnUJN65kLAckHBV9BUShQrNYljljkzTVQgwowIKFHRVEKCjoUKkSIdChRgUwSIKUCPCiApQx4kUQkRyJ+QCk1bWdtLMO4VX+Y4qqjYD7zE1ZRiRYTJ2fcHNt7GPrVqjGreVr8+I3cOI2KSEAiojhW5EGnJnEvVW9TTAG70IobGyYSLge8PGBimiDSyxzw+lDIxSjGjaN8RQ50sqCOFIKEUJkwxkEHrVro+tXumXIntLiWCXl2kRwSPAjkw9aqVJHPlTygEZFSrFTkQXUEYM6tee0W62h2fGkXul6T2knZxrewQFJcBgTnieJxx5VN2z1BtL2vhVJFRY5Y99DjDhY4xjPTrXNdkITPtPp8SgnfkHAdeI6VvNvZ7RduZDfQLPGs0rMB76gSbuR4+7yq8js1ROd8zKuRRcARkAfP4mll2jmmyIYkTPRCz/lTCjVL5v6OORQep7o/etjp0NleWkV3ZMk1tKMpIpyD+xHUdKsUtlX3RgelOKFuTA+oB+kTJ6ZsyqOJb5u1ccQn3Qf1+NaRYd1QAOFTlh408sIxyqVATYRTFnO88Y0dChisCejgFHRUK6dBR0VHRCRBR0KMDNGBIgoCnkhzxY/AU+qqg5BaYFgFwJFWN25KxpYgkP3R860+jbJa/rIDabo97PGeUnZ7qf4mwK1Np7H9rZlBkgsbfylulz9M05aGPiKN85pFE6sMgfOrCaVltQisxzzWuhv7F9qlXKvpbnwFzj8xVVqXsx2wslLNo0k6L1tpFl+gOacqOgOBFM4Ygmc8IO9xBB8xSmOMAVZXtrPZTmC9gmt5h/ZzIUb5GoxhUngMHyquVIjhYDzI2KQw4cONOzIy8xlfEU0OFAYwHO8IEjzp1MMKLAbHQ9MdaG6QeHP86icd4sxg9KTgoeHOnYm3ufP8qcaPI5VOIvVjYzS+yuH7X7QtHjGQe2QcP5hT3tLmMm2VxIrYPekBHTeldv1qf7CbcP7SbBm4iJu0+ABb9Ko9s27Taa6BOWRIkPruA/rVjij/spnfqT6afz/AHLrYPa250K8JUGW2cg3Frng4/Gngw/0Nd/064ttRsIb2wlE1rMN5HH1BHQjkR0ryhEWRlZCQwOQfCuk+zTa/wD5Hd7tyWOlzsBcxjiYX/8AIo/MdR5imU2k7GJur0nIncVi606sPCpSIrKrIyujAMrKchgeIIPhinBFw4YppMWBPDXZGkshFTu2QjmKYkYMeFZpUTaVieRI2KGKkxIGyTRMg7xHIcKDRC1eJHxR4p5Ii65ojC1donahGwCTT8SUmNa2ns02RfazXhBIWTTrcCS6kXnu9FHmadVWWIUcxdjhRkwbC7C6ptbLvWwFtp6HEl3IO6PJR9413nZXYHZ3ZpUeGzW7vRzuroB2z5DkvwrR2sEFhZw2llCkNvEu7HGgwFFOqOprWroWsbcyizluZI7ZmAHQchRqX/FimhShIB1phkR0b34jTis4NNLID1p1TmhMmI1GxsNYtjbavZW95Cfuzxh/keYrk+2vsRikjku9jpTHIO8bCd8q3kjniD5H512BBmpMJKHypLqG5hCeJry0uLK7mtb2CS3uYm3JIpFwynwIqDcWuMvEPVa9b+1b2fW+2Wkvd2SJHr1shMMgGO2Uf2bePkehry4YnjkZJEZJEJVlYYKkcCDVRqoX1ChzKNR15jqKeUbwwefQ+NP3lv2bh1HcbmPA00q8cePL1qsRpODH6wwyIjdIOeo+tSYe8BRFcqHAOfD9KOIbsgA+9y9en7VI2gscidH9gcQ/6v1C5I/9PaXMn+GFqxe1UmdrNVYclm3MeSgD9K6J7Abdiu1F/uN2aWcke9jhvSOqAZ8cMa5xtBiTXdTlXk93Kf8AOcU5/wDSuPf5+8rV/wC98+3z9pHC8ARyNSrKb7PMJD7nuyDxHj8KYtOKsjchxHp1/enlGD3h5NQp6iS+N1M757HNoTNE2z95Jl4VMlk7H3o+sfwzkeR8q6gEOK8r7NahPZSQXNsxF1p8gdDnmvQfEZWvVGi3kWsaVbX9qQYp0Djjyz0qy3AMqIdyp8TwUDR5pAo6yQxm9iLDHPOj3jypsUYogxnYjiysvAHhS1lY5HjTFOQDL48qJWJgkCSY18a9LexbSk07YW2n3QJr5muJD1xnCj5D615tVcj4V6m9mc63GwOiPHyWDsz6qSDWn0Q3JlG85xNGOLE04KbXmfWnBV8xIiSSSAOZqZDAgGWBbhk45momCrq2OVTc9pEwU8wV+BGKEyRMpsZtgNo9R+xPpz20nYPO4IYdgQ+6ImLAbzle9w4DPXnWuki3OK8vCq7RtCisboT78rFARGrybwXIxnlzxVxMw3d0cSaWdjtCA9YUQyKlIlNQJgCpiLgcaUxhAR60BBwOnKvNft60CPSNvpLi3QLBqUQusAcA/J/qM/GvTVqneriH/EvuPrOz8Q/rEtpWb0LDH5GlA5bEG7ZMzhlxb79u646ZHwqoC90VrUt+4xI4bp/Ks0qd08KTcu4g9OxwYI1yT5gN+9IK4U/wmpUCd6P+Vv8A5U3OuEmPguaXp2jA287/AOxe33PZQ7HgZpAD/ekPGoG1/sF1COeaXZzVLe9Ejswt7sdjIOOeDjKn1O76VX+yDbFf+W6Ts09kXinmhWOeDLOHDDIZPDnxHL0r0siZdnb3mPrgeFMyCokEEMZ4h1vZnXNmLtE17SbyxG9jfljJRgfBxlT86ivHh8Ecxg+o4V6K9t+s2etpNshDtJY6UbZklvFuYpnWRiMqm9GpC44E5HMjwrk11sHrbQmfTEtNbhTvPJpFytyVGBkmMf0g455rXVkcGLtDbECZzR3KX8PTtVMZ9eY+o+tdl9nO0Z0/Z9rN7hYhBOyoGP3SAwx6Zx8K4qMwMWIKvBMrFSMFSGGQR051or2x7W4Yh2ULw4HHXP61cRNS4EzbbAj5Pn5+ZyKhQoVgiepgoxQoVInQ6XE27IpPKkgUKYoxBMtUFdo9g20KBbjZ65fDljPa5PvfiX9a4nYyh13G99eXmKs7SWW2uIri2kaKeJg6OpwVI61o9O+k6hKFoPBnrl0wcilqM1iPZ57QbTaOKOy1N47XWAMYY7qXHmp6HyremEqeAwfA1oZzEiJCDFLWPB7pxQXI5inkxQkwhAqv+I1IijA50SCpCYzS2MMCOxJUqNcnFNRITyFT4kWNGeQqqKMszHAUeJPSkM2IYEdgQIpZyFUDeZjyA6mvL3tI1obU7aXl/CSbRMW9t5ovX4nJrf8AtP8AaCmpQy6Js7KTaN3bq8Xh2g/Anl4nrXMYrYKBgYHSoRSNzK1zhthxKfUALexlbqV3R6nhWWEWEGaudb1Jbi77CDDQRni34j1NRIhHK3f4Ko3m9P8AfCgbubaQDpXeMwwkOOHuoB8Tx/ao1+u5a3LeQWrhYtyJnYd5iWP7fl8qg6hazTW8dtbRPNczOqJGgyzsxwFA6nlROulDFVWarAPeaT2EbJajtJrt/cW17e6fZ6ZAZ5Lu1n7F0kx3VRyCFY8cnHIEHnXZdoNtNrNgdJhvNVurPXLGdhFbyX1ubS8ViuQwKZSVV6n96tPZnsvZ6XsdpOmaVPDcB0Nxfyq26xmPFwQcHgAEHgFPWvP/ALcduztXtofsjiXS9Ozb2oPBXxwZ8D8R+gFVTpSoHkn9pfBd7TttKDULue+vJ7m5kaWaZ2lkdubsTkk/OrOHfgS3mhd4pk3CskbFWU4PEEcRVHZym5txMybpOeANaC4UxhU6qwU5/hUL+YNW6FGmZ3UudYHBj20Ot3+tW+9qtw11cQRlRcSgGVl4EBnxvPjHNiTVzcZEnqqn/KKydy2YJhjixCj4kCtrcwE3Eij7mF+QFW+nXkD2/MzetfdS3v8AicNoxRUBXnBPZQ6OhR4pgEiGvhSt0n1/OkgcccjT8eCN1udNUQScREeQQykhhxBq2tLlZMLJ3X+hqAUwcngfxdD6+FKxjgw401GKcRFgDiXQHzHEeIrfbK+1HXdESO3vCuqWa8Ak5xIo8n/fNcuiupIuHvr4GpkV7C/vEofOrSXDwcSo1TLvPSWje1fZi/CreSXGmynms8e8ufJlz+Va6x2g0C9ANrrWmyZ8LhQfkcV5KiKsMqwI8jUkQjdXKg/CnhsxRtK8z1+t1pwXeN/ZBfE3CfvUa62p2asFJutc01MdFmDt8lzXk1IFz7g+VTYIOWFA+FdpzAbq9PAnoLVfa/odqrJo9rdanL0Yr2UWfU8T8q53tJtlru1BMd/OIbLPC0t8qn97q3xrKQta2yA3E8SfzNx+VMXO0lrD3bOJ55PEjdX96WdKmdqssGTL6KNY4y7lUjUcWJwAKzeu679pRrXTiRGeDy8i3kPAVVX1/d6gR9qlxGOUa8APhTcUZZTu4SNfedjhR6n/AGaAsW2E7AXcxuOIkqkalmJwMdasLO2D4IwYUOSw5SMPD+EdPE8actbTfXiGWE8GY8GkHhj7q+XM9alu2/mOHCoo7zdFH706uvG5lW2/JwPnz5vGG78hJ/q04t5noKk6NqF3ouo2e0FrEssun3Hbxxsm+JCqnJI/CN4ZPTIxTcEP2p+zQmO2iwZHAyRnwHVjyA/SrjU76LZ3R3vHjAnKiKCHOd3mQuevVmPU5o2rDqdRwPJiRc1TroGWJ2E1W1Xtt0LXNjb8zbNxR7RzxmATxuUhIZSN9t1gWIzwU548civPEK9o+CfWmpWeWZ5ZCC7sWY4xknnVnpduYy0kq4PIAj61gompsCeodtC5MvNIEbdju4McY3m9BxwasJ5Dv97mo4+p4mqqG2MSdvBIYZW7xHNSvQEeZ/KjE8xO5NHhmOSynINaiOUXBExbKw7llMtdKh+2atp9t/5J1Zv5R3jVxrM9/JfM1hGWQ8X8mJJH+UrTGxsBaW+1B+6kSfZYj/G/vH4Ln510X2d6fFe6A99Io/7ud5VB6JwVf8qimszLUNBwSf2EqhVfqDqGQo/c/wBfxPMooUKOsUDM9TAKWtIpa8DTUgmPCLI6/qKNVxgMPQinYsHHjT+4DnkPyNPC5iC+NjGkOBx4jxpWAR3SMeB5f6Ul42TiOHry+f70jOD3gVP513HMHGdxDK46FfqKWoz03h5caCMQeBBp1ShP9ImD44xXAAyCSI/YRh5VAGTnlVrqTt3QCyEDHOmdMUxyrNEC+70YkindRL3MpdyqZPu4zWig00keTKFjarR6CV/bz9J3+Bo9+R/fmkP9404sAz70Z/un96kxW4/HGP8A2gfzzVQho3UviR4kjJAUbzeHM1J7Nl94LEp/Gd3Pw50//RBcPO5XwD7o+QxS4ZI0/qIcn8QHP40SpF2XHGwhQWu9xVC/8UmUX5e8fpU5Y4od15mEjr7uRhV/lUcqimZz78ioPBeJo4WZyTbRlsc5WPAerHgKsLgcSk+puT8+8mSSs4zITHH4feb9qOCJ7sJugxW2cAqMlj4KPvN58h1NLs9OMh7SXEnXefIjHw5t9B61bI6Q8UJaQjBduePAdAPIcKsJWW3MqPaqbLuY/axR2ka5RVKZKRg5CE82J+8x6n4DArK7ewXV+tr9nCyJFvFkDd7Jxxx8KtLzUAgbDAnqegrN39+0pKqTg8z40HUshrNcPoarBcLvImesrZjLvyqQqnkepq4t07V8tncXnjr5Cm40aVjg4A4sx6VJJCKEQYx9PM+ZrNrQKJtW2FzHJX3mxwwDk45Z/YcqaG+7qsSl5pGCRqOZJ5CkMRg8QFHM1q9krJbG1bX78iLCN9k3vuKPemPpyXxPpTUBtbSJXtYdOmo/89zJOsQjTNIs9BtTv3EpMTlfvO2O1b8kHr5Vu01Sy2ctbXTpphG0cQOAfX9QaxuxVudR1S416+Ux21uN2FX+6By+I4k+Z8q6TsJ7KLT2i6K+020N1c2v2yd/sUceB/2y4VGIPiQ59CKm7qAvcOOB9hF0dMQNLc8n7meTyMUdTr2ye0k3Jgdz7r/vUSSJo+fEeNUChU4m6rhhkRIFHjHp40VOxnxHA1KjMkmCOQp5ipcUob9qYMAIyhA/L59PjTZRkOGBBpoysWQrS0Qg5wfhSTGp5DHpy+VQklZefGpEdwDzOPWj1gxJrI4ivs2TwHyOPoaWkUqHgT8Qf0zS45FP+hqZbsCRg0SoDxFO7DmWeiws8DybxjKg5KkVAvSzSH+jd/PjU6Id3OaIxRsctGhPjir1mSgUTPVsOWMqlRieEEnyNSEhm4Ys5CPNTVikMWf6pPlUhIYQeEEOf5BVf6RjD1A9JVokwPBYUPmy5Hw4mpUdlPLjed2H8CHHzbH5VZKwQcCqjwGBSvtEYPFyfSmrUPJlZ72P6RGrfS0XBkCZ8XO+flwFWCpEmCF32XkX449ByHwFV8uoJGOg/mNV1zq2chSW9OApv1K64n6Vtx3l9PeKpO82T4Cqm91TA3c/3RVLNeSSZ47o8BTKK8hO4pOOZ6D1NIs6otsJbq6FU3aPXFy8p7xwPAUmKJpMEkqh69T6ClrEkY3pCGP0+XX8vWiklLZ5gcsnmf8AfhVY5O7S4ABssdLBQFjAGPjj9z500W545dTTTyADJO6orRaBs59ojS/1oNBp+N+ODO684HU59xPFj8KlVaw6VgWWJSupzBsvoY1Ii91BWXSo2wF5NcuPuL5Dq3QVNvbmfazWEsbEgWSMN90GEIXgMD8C8lHU8ajarqtxtFcrpukIEswvZlkG6hQfdUfdj+rVprSEbP2cGn6VbveaxeOIoYYx35ZDwAHh+gppKhSint8n19hEIru4tsHd/wCR6e59/n2vtK0B9p9b07YrSMx2W72upXC/2Nsp73H8THujzOeleqLS2gtLSG2tYlhtoEWKKNRwRFGAo9AKxnso2Jj2K2faK4dLjW75hPqNyvJ5McEU/gQHA+J4ZrdDlWdY5dszRrTSMTwHELbU7Ue62Rkjn8vKs/qOmS2DEopktz05kVp9Z2emsp2u9HXHHee3BwD5p4Hy/wDyollqsN2jR3A3XHdYMMYPgw6VuW0Bzos7W8HwZm1WlBrqOV8jyJkjbJKu/bsP5Ty/0psIUbdYFH8D1rTaloJZjPYNuOeO7ng1UrzGNuw1CHcI/EOH+lZ1lBqOHGPfxNKrqBaOw59vMaUEHqpp1Twww4eQ4fKnkgBUGCQFT91zkfBqQ6GP+sVo/M8VPx5V2krOLAxtoI393uny4/SmTbuPdw3oePyqSVPPAI8RQBI4Z+BoSohByJFCshwQVPnwqZZu/bIoOcmloxxgjh5H9OVTLFIzKrFBkHPu/tR1V5YYMXZZ2nIlteKLe0RuJZuHGqR71t73RVxfukyorcAo8SKqvs0Jbi7f4x+1WuozqwvEpU6cZYRIvn/D9aWL9+irShaQ/wDkf/GtKFrbjnI/+MfoKr6X9Y0mv0jZvpiOGB6Cm3uZW96Q49cVJ7G1HTe/vsf0o8wp7kS+u5+5NdpbyZwKeFkFQ8jYRWdvIZNOrbSE94qnkTk/IU+8zEYIAXwY8Plyplpc8N4keC8BQ6QOYQYniKEUMfvZdv4v2H6mg8p6DCjlnp6DkKZZ8DPBR4mnLCzvNSk3bC2luPFgMIvqx4Cu9hJOANTGNs+TkcT4ml2Frdalc9hYQPcTczu+6o8WPID1rRQ7M2llD9p2gv49wc4YX3U9DIefooNN3W04MQsNm7NUgzgER7qE+IXmx82+VFoC72HH8xZtZtqVz7nYD58MlW+naXs5Et5q00V5efcXG9Eh/gXnI3me6POoM02p7W3TKd+KyLAvvHO9jkXPU+CjgKd07ZyWeY3mtzs8h5hm4+hPQeQrQWT3N9qEWi7MWTXeoMMCOMbqxL1Z25Ivrx+dNI7e7tX08n7xSV9+rOp/XwPtBALbQIYrSwhe41CdhHHFGu9JLIeAAHU+XIV3v2Qezxtmw2ubQ7k201ym6ADvJYxnnGh6sfvN8BwySj2X+ziz2SzqOoSrqG0Uq7r3RXuQKfuQg+6PE8z5cq6PESSBzY8OHWqtrF9sYA8S2iBN+T6ychp4OMVkto9stE2bcw6jehr4LvCytV7a4I58UHujzYqPOuV6t7f1ivXSys9LjhXgFuJpZn+LRDc+AJ9aQUJjQZjpYVdcNxzWZ1/Z21v5BIGaG6A7s6c/j4ihQr2ZrW0aXGRPNB2r70ODMTpepzpNNbndbsmIJxwbBxy6fCtB2FvqVmhuIFZWGcHiR6GhQrL/AMextTS+4/uafXKK21JsdpnNc0VdKVZ7O4kVWPuEZ+tV9pqMhkETKOJxkcM+ooUKz+tUUdToq2G0u9Gxvo1WbmTTFEzkbm434ozu/TlSbqBrdA2+HU9CvGhQrmUaSYIY6gIzCVkPBd30NWtimDzJoUKLptyDB6jbaOXOQTxqEXweQNChRW/qi6hkQ+259wUYmJ5KBQoUvMYVEMlyud4D4UySTzY0KFcZyxiWXc+7n1NX+zmz7a0pd7wwIvEqkeSfiT+lChUUKHt0txA6xzVTrTmXMGj6XZXBRbMXEy8e1unMn+XgKpNQ2uvZZ2tLZEgRDugnvY9FwFHyoUKHqnatOzbeP6KlLbM2DOBneK07RzrN0Zb+7mlZRnLcT6DoPlWlitLbTIR9lhUEkLvHmc+JoUKf0iKKvqY39YPWEm76fjbaFsvYz7Y7ZroJvHsYVQySTRKGcgYyFzwU8efGvS2yGzGk7KaaLTRbVII2IZ35ySN+J25sf9ihQpGS2WbnJjXUIdK8S21rURpOiXuovCZxawtN2Qfc38DlnBx8jXnHa/2wbQ6posd5A/8Ay2wuWdFtLJzG/A4/pJvfb0XcoUKEgYz95y8zk17qt3do0ckgSBjvGGIbiE88kfePm2TULNChVcmME//Z</default>
<advanced>true</advanced>
</parameter>
<parameter name="INPUT_MAP" type="text" required="false">
Copy link
Member

Choose a reason for hiding this comment

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

I'm a litte disappointed here, that this is not happening in a more automated fashion. But it doesn't hurt if we give the user the choice to do his own mapping, as long as we also do auto-detection, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do not be disappointed :) There is an actual auto detection happening by analyzing the AVR descriptor and other featues, see DeviceInformationXML, DeviceDescriptorXML, InputConverterXML.

This hidden setting, is an extra layer that will give user ability to remap and fine tune input names for his AVR. That said, this will be the last resort and I think the autodetection worked well on all the tested models.

/**
* Model HTR-xxxx has a Zone_2 concept but realized as an extension to Main_Zone
*/
ZONE_B
Copy link
Member

Choose a reason for hiding this comment

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

The dialog level of the other PR will need an entry in this feature enum later on. Just a reminder.

Copy link
Member Author

Choose a reason for hiding this comment

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

What other PR do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

}

if (refreshTimer != null) {
refreshTimer.cancel(false);
}
refreshTimer = scheduler.scheduleWithFixedDelay(() -> updateAllZoneInformation(), initialWaitTime,
refrehInterval, TimeUnit.SECONDS);
refreshTimer = scheduler.scheduleWithFixedDelay(() -> updateAllZoneInformation(), initialWaitTime, settings.getRefreshInterval(), TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

The line break limit of this commit is different to how my eclipse was configured through the oomph setup. Does the PR conforms to the OH style guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am using IntelliJ. The project passed style checks in maven/Jenkins. Wouldn't this be catched if style would be violated?

.collect(Collectors.toSet());
.of(
// Note (TM): Not sure why we have NET_RADIO, we should only have one 'NET RADIO' (as the canonical input name)
INPUT_NET_RADIO, "NET_RADIO",
Copy link
Member

Choose a reason for hiding this comment

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

NET_RADIO was the original input, that worked on the RX-V671. "NET RADIO" will work as well in the current addon, I guess.

Copy link
Member

Choose a reason for hiding this comment

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

It seems the comment of @davidgraeff is still applicable, or not?

NET_RADIO was the original input, that worked on the RX-V671. "NET RADIO" will work as well in the current addon, I guess.

* @param setting
* @return
*/
private Map<String, String> createMapFromSetting(String setting) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm. I don't like this. If the framework does not support a valid use-case, the framework should be extended. This is clearly support code to workaround the limitation of missing list-like configuration entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

By saying the framework, you mean ESH?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I do. An ESH implementation could take some time and I don't want to block this PR, so this is not required to be changed right now :)


connection.setUseCaches(false);
connection.setDoInput(true);
connection.setDoOutput(true);

// Send request
DataOutputStream wr = new DataOutputStream(connection.getOutputStream());
try {
// ToDo: This should be sent using proper UTF-8 encoding
Copy link
Member

Choose a reason for hiding this comment

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

I doubt, that Yamaha implemented a proper UTF8 character-set support. The only think that users can provide own strings for, is for the input names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this is a left over comment. At some point I rewrote this and added encoding based on the returned Content-Type. As far as I remember, my yamaha was reporting and sending proper UTF-8. I am not sure I got to a point to send UTF-8 thus this left over comment.

Either way, as it is now, how about I remove the comment?

* @author Tomasz Maruszak - Initial contribution
*/
public class XMLProtocolService {

Copy link
Member

Choose a reason for hiding this comment

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

Almost all those public methods of this class do not have a documentation. The included public class doesn't as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will add documentations.

* @author Tomasz Maruszak - Initial contribution
*/
public class YamahaSettingsUtil {

Copy link
Member

Choose a reason for hiding this comment

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

Hm, this class should go in my opinion. Configuration should be read/parsed/assigned to fields by the Configuration ESH class. If required configuration is missing, the mentioned class will complain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please point me at an example how to achieve such "autobinding" to a class by ESH?

@@ -25,5 +25,6 @@
public String inputID = VALUE_EMPTY;
public String surroundProgram = VALUE_EMPTY;
public float volume = 0.0f; // volume in percent
public float volumeDB = 0.0f; // volume in dB
Copy link
Member

Choose a reason for hiding this comment

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

The volume should either be stored in db or in percentage, but not in both. Because of the introduced minDB and maxDB I would favour the volumeDB field. The percentage can be computed.

Copy link
Member Author

@zarusz zarusz Mar 12, 2018

Choose a reason for hiding this comment

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

Actually both are supported. If the user sets volumeDB the volume percentage is calculated, and vice versa. The actual updates from Yamaha come back in DB if I remember correctly. Both are used to update state on the two volume channels we have.

Copy link
Member

Choose a reason for hiding this comment

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

I agree to have both channels, I just disagree to save both values, especially if one can be computed that easily.

Copy link
Member Author

@zarusz zarusz Mar 13, 2018

Choose a reason for hiding this comment

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

I think the way it is right now fits into the existing design.

Note that in order to move the volume conversion logic out of ZoneControlXML the YamahaZoneConfiguration is also required (for min/max volume DB). Furthermore, if a set volume % command is made then similar conversion needs to be performed to dB in ZoneControlXML.

Not sure where this logic would sit? The only other option I see is a getter were to be exposed under ZineControlState but then you bring in the config class and break apart the from dB and the to dB conversion.

Please let me know what exactly you have in mind.

Copy link
Member

@davidgraeff davidgraeff Mar 14, 2018

Choose a reason for hiding this comment

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

  1. remove volume
  2. Move the conversion logic into a method YamahaZoneConfiguration.getPercentageVolume(float volumeInDB)->float
  3. Use YamahaZoneConfiguration.getPercentageVolume(zoneState.volumeDB) within the handler to return a percentage value

Is that a viable option?
Redundant values are risky for fellow developers who refactor code and forget about keeping those values consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking aside that the simple config holder is going to be in the business of volume conversion, yes this is viable. Check the change please.

@@ -0,0 +1,45 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

More of a note: The framework and other bindings prefer "configuration" over "settings" or "preferences". What do you thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will change to "Configuration" for sake of consistency.

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch 2 times, most recently from ecdbcd1 to c3fa9a8 Compare March 12, 2018 23:37
@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from c3fa9a8 to 0604e62 Compare March 13, 2018 19:49
private final DeviceDescriptorXML.ZoneDescriptor zoneDescriptor;

protected String powerCmd = "<Power_Control><Power>%s</Power></Power_Control>";
protected String powerPath = "Power_Control/Power";
Copy link
Member

Choose a reason for hiding this comment

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

Those command template strings should be static, shouldn't they?

Copy link
Member

Choose a reason for hiding this comment

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

I see, they might be changed by subclasses. Nevermind.

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from 0604e62 to d284e4f Compare March 14, 2018 22:59
@davidgraeff
Copy link
Member

@martinvw LGTM

@martinvw martinvw assigned martinvw and unassigned martinvw Mar 30, 2018
@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/yamahareceiver-binding-has-issue-with-zone-2/19731/54

@martinvw martinvw self-assigned this Apr 15, 2018
@zarusz
Copy link
Member Author

zarusz commented Apr 21, 2018

What are the next steps?

@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/yamahareceiver-zone-discovery-not-working-with-rx-v3900/38350/7

@martinvw
Copy link
Member

What are the next steps?

I have it open already for a week but I did not yet find time, the +19.000 lines of code feels kind of depressing to me as a reviewer 😁

It will be included in the next release.

@davidgraeff
Copy link
Member

davidgraeff commented Apr 24, 2018

@martinvw This is the OpenHab repository, not the ESH one :D Not every code line needs to be perfect.

Trust your

  • long time contributors,
  • the static code analysis,
  • jenkins and travis-ci.

We don't have a test suite here, that is correct. But users have tested the changes. And there are Yamaha models with 2 zones that do not work currently, and will work with this PR.

And OMG this is really 19k changes. Luckily 17.5k is just xml device documentation.

Copy link
Member

@martinvw martinvw left a comment

Choose a reason for hiding this comment

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

Sorry that it took me so long, but I was able to review it.

I added some remarks, please let me know it anything is unclear.

@@ -0,0 +1,3259 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

What is this and do we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

These XML files are feature descriptors I have collected from various Yamaha models from the community users. It is good to have them as part of the binding documentation, since they give insight for developers on the protocol and feature variations that should be supported.

I think it's good to keep them next to the binding code. Also people will be able to refer to this by linking to github pages.

Copy link
Member

Choose a reason for hiding this comment

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

Is there maybe a way to describe this in a developer readme because the goal was not clear to me at first sight

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already there. See the docs/README.md file in the PR.

try {
updateDeviceInformation();
} catch (IOException | ReceivedMessageParseException e) {
logger.error("Communication error", e);
Copy link
Member

Choose a reason for hiding this comment

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

In E.4 of the coding guidelines we listed what kind of logging level should be used when. Error should only be used:

error logging should only be used to inform the user that something is tremendously wrong in his setup, the system cannot function normally anymore, and there is a need for immediate action.

Please change this to use the correct logging levels

updateState(grpZone(CHANNEL_VOLUME_DB), new DecimalType(zoneState.volumeDB));
updateState(grpZone(CHANNEL_VOLUME), new PercentType((int) zoneConfiguration.getVolumePercentage(zoneState.volumeDB)));
updateState(grpZone(CHANNEL_MUTE), zoneState.mute ? OnOffType.ON : OnOffType.OFF);
updateState(grpZone(CHANNEL_DIALOGUE_LEVEL), new DecimalType(zoneState.dialogueLevel));

Copy link
Member

Choose a reason for hiding this comment

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

This line looks misformatted

* @param <T>
* @return true when the setting was present
*/
public static <T> boolean readSettingOptional(Thing thing, String settingName, Consumer<T> consumer) {
Copy link
Member

Choose a reason for hiding this comment

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

I think overloading the method name here is a bit confusing

}

updateConfiguration(configuration);

YamahaConfigurationUtil.<BigDecimal>readSettingOptional(thing, CONFIG_RELVOLUMECHANGE, v -> zoneConfiguration.setVolumeRelativeChangeFactor(v.floatValue()));
Copy link
Member

Choose a reason for hiding this comment

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

To me this feels quite magical, why do you do it like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How else would you avoid the boiler plate of pulling a value from settings, converting to decimal and setting a property? Does anything like this exist in ESH?

Copy link
Member

Choose a reason for hiding this comment

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

We support mapping configuration to a configuration class, it would cover some or more parts of your use case. Its not needed to change, then I should have joined earlier.

@wborn do you have any advice for a next PR how to handle the configuration, would using the config class solve this (partly)?

Copy link
Member

Choose a reason for hiding this comment

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

Configuration beans (there's still no good name for this pattern within esh), make the code much more readable. They do automatic conversions and can have default values.

Copy link
Member

Choose a reason for hiding this comment

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

Yes using binding specific configuration objects simplifies the code. When you override updateConfiguration(..) instead you can easily get the configuration object and map it to a binding specific configuration:

@Override 
protected void updateConfiguration(Configuration configuration) { 
    zoneConfiguration = configuration.as(YamahaZoneConfiguration.class); 
    super.updateConfiguration(configuration); 
} 

Calling the super is also important to keep the handler behaving like a default BaseThingHandler. I used something similar in the PlugwiseRelayDeviceHandler.

When the handler is initialized you get the configuration with:

@Override
public void initialize() {
    zoneConfiguration = getConfigAs(YamahaZoneConfiguration.class);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@wborn this is really neat, but how can I make it map a config value let say "ZONE" (or "VOLUME_DB_MIN") to the config bean property (zone, volumeDbMin)? Is there some annotation I could use to define the mapping?

Looking at ConfigMapper source from ESH, it seems expect naming in config to match bean properties.

Without this, I would have to rename the configs (breaking backwards compatibility) or set weird property names.

Copy link
Member

Choose a reason for hiding this comment

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

I proposed annotations for the configuration parser, but they got denied. Configuration keys should be lowercase, latin letters anyway, so it might be necessary to break backwards compatibility for the purpose of clean maintainable code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@martinvw @davidgraeff given the limitations of configuration bean mappings (bean properties need to match settings name) and your advise to refactor this, I have introduced the breaking change. Documentation has been updated. Essentially, users will be forced to re-update their settings when migrating to this version. I hope this will not cause too much issues for the users.

}
}

logger.trace("The charset {} will be used to parse the response", charset.name());
Copy link
Member

Choose a reason for hiding this comment

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

This will give a NPE if we just logged on line 190, maybe just log charset and trust the toString implementation

Copy link
Member Author

@zarusz zarusz Apr 28, 2018

Choose a reason for hiding this comment

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

Hmm the charset would never be null. It is assigned a default charset on beginning of method regardless of line 188.
Either way I change the implementations, so it will be different (along with relying on toString on the logging).

value = value.trim();

if (value.toLowerCase().startsWith("charset=")) {
charsetName = value.substring("charset=".length() + 1, value.length() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

This feels pretty wasted: "charset=".length() + 1 maybe make it constant?

public static final String POWER_STANDBY = "Standby";
public static final Map<String, Feature> FEATURE_BY_YNC_TAG;

static {
Copy link
Member

Choose a reason for hiding this comment

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

I personally dislike this construct, can you just call a method which returns the complete list, now it can have all values in between in a multi-threaded env

Copy link
Member Author

@zarusz zarusz Apr 26, 2018

Choose a reason for hiding this comment

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

I wanted to avoid Guava to help with map initialization and also wanted to have just one map instance.
Static class constructors are executed once per class loader thus are thread safe, so it should be good. If it's a matter of preference I could change it to a method call (or one line in guava), but then it violates the first sentence :)

Copy link
Member

Choose a reason for hiding this comment

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

Static class constructors are executed once per class loader thus are thread safe,

What a great language :-)

Lets leave it then, if there is no reason for change except my personal preference :-)

inputs.add(new InputDto(param, writable));
}

if (LOGGER.isTraceEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
public class YamahaConfigurationUtil {

private final static Logger LOGGER = LoggerFactory.getLogger(YamahaConfigurationUtil.class);
Copy link
Member

Choose a reason for hiding this comment

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

private static final please check all

Copy link
Member Author

Choose a reason for hiding this comment

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

It was just this place :)

@zarusz
Copy link
Member Author

zarusz commented Apr 26, 2018

@martinvw , @davidgraeff do you guys know where should the built JAR be with this PR changes?

The JAR from here does not seem to have changes from this PR: https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.yamahareceiver/

The Jenkins links is also broken (maybe the build history expired).

Someone on community is asking for a binary to test. I could provide my own build, but I believe something should exist by now.

@martinvw martinvw closed this Apr 26, 2018
@martinvw
Copy link
Member

Forcing a rebuild

@martinvw martinvw reopened this Apr 26, 2018
@martinvw
Copy link
Member

The Jenkins links is also broken (maybe the build history expired).

That sounds likely, can you check whether it’s fine now?

@zarusz
Copy link
Member Author

zarusz commented Apr 26, 2018

Still no luck.

The JAR (https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/binding/org.openhab.binding.yamahareceiver/2.3.0-SNAPSHOT/)
date is from today, so it was definitely built. However, it still does not have this PR changes.
Maybe this ends up being replaced by another PR?

Also, I followed the Jenkins build and ended up on this artifactory artifact. Yet, I don't seem to have permissions to download the JAR and confirm if this one has the changes.

@martinvw martinvw closed this Apr 26, 2018
@martinvw martinvw reopened this Apr 26, 2018
@martinvw
Copy link
Member

It completely overwrote all bindings when building:

https://github.com/openhab/openhab2-addons/pull/3137

I don't know whether that was caused by the change in the pom.xml but that could sound like our process is broken 😢

@martinvw
Copy link
Member

@zarusz its there now, but looking at the history it might be overwritten again later:

https://openhab.jfrog.io/openhab/webapp/#/builds/PR-openHAB2-Addons/9735/1524776821513/published/

As Yamaha introduces new models there may be variations between XML structure.
In an attempt to improve the addon maintenance and troubleshooting selected model's `desc.xml`' has been collected from community users:

* [RX-S601D](desc_RX-S601D.xml)
Copy link
Member Author

Choose a reason for hiding this comment

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

@martinvw here is the developer documentation that mentions these XML files...

@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/yamahareceiver-zone-discovery-not-working-with-rx-v3900/38350/8

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from d284e4f to c0a151e Compare April 30, 2018 13:27
* Feature: Introduced zone channel settings for minimum and maximum volume in dB.
* Feature: Added setting for default album image URL.
* Minor improvement (compatibility discovery, optimizations, [developer documentation](README.md)).
* **Compatibility break**: Changed underlying settings name to follow the lower camel case convention (see discussion [here](https://github.com/openhab/openhab2-addons/pull/3264)).
Copy link
Member Author

Choose a reason for hiding this comment

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

@davidgraeff @martinvw here is the breaking change mentioned in the new release notes page. The binding documentation is also upgraded.

Copy link
Member

Choose a reason for hiding this comment

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

Imho we should not have a releasenotes per binding, please add the breaking change to:

https://github.com/openhab/openhab-distro/wiki/%5BDRAFT%5D-openHAB-2.3-Release-Notes

The rest will were needed be included it in the global releasenotes but please do not add this file here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, please remove the release notes from this PR.
Besides adding the breaking change info to the wiki that @martinvw has mentioned, please also add a line to the update.lst as this will be shown on the console during the update process.

Copy link
Member

Choose a reason for hiding this comment

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

@zarusz if this is done I'm ready to merge :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@martinvw lst file update submitted, global release wiki updated, and removed the release notes file from here (along with references).

Thing zone ZoneID2 "Zone 2 Thing Name" @ "location" [ZONE="Zone_2"]
Thing zone ZoneID3 "Zone 3 Thing Name" @ "location" [ZONE="Zone_3"]
Thing zone ZoneID4 "Zone 4 Thing Name" @ "location" [ZONE="Zone_4"]
Bridge yamahareceiver:yamahaAV:ReceiverID "Yamaha Receiver Bridge Name" [host="a.b.c.d"] {
Copy link
Member Author

Choose a reason for hiding this comment

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

@davidgraeff @martinvw here are the new settings that follow the configuration bean requirements...

Copy link
Member

Choose a reason for hiding this comment

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

@kaikreuzer this breaking change is a result of the limitations of the configuration class mapping, do you agree that it is worth it?

Copy link
Member

Choose a reason for hiding this comment

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

In OpenHab 1.x times, it was fancy to use uppercase configuration keys. That has changed with ESH and OpenHab 2.x were the majority of all extensions and bindings are using camelcase configuration keys. Should be unified in my opinion.

Copy link
Member Author

@zarusz zarusz Apr 30, 2018

Choose a reason for hiding this comment

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

@martinvw yes, the braking change is dictated by the limitation of the config class mapping. Having a feature in ESH that David suggested would really help here. Also looking at ESH it should be easy to add to ESH.

@davidgraeff let me know if in case something doesn't look good.

In general, given all this conversations around this and the fact I already made the effort to change this, let's stick with the unification and introduce this one time break.

Copy link
Member

Choose a reason for hiding this comment

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

this one time break

@zarusz
This binding has quite a history of breaking changes 😆 . Partly because it was a port of OH1 but also because the design lacked zone support etc.
But seriously, why is it "host" on most bindings and HOST here. That confuses users.

Copy link
Member

Choose a reason for hiding this comment

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

@kaikreuzer this breaking change is a result of the limitations of the configuration class mapping, do you agree that it is worth it?

I leave that decision to the authors - any outcome is fine with me.

}

updateConfiguration(configuration);

YamahaConfigurationUtil.<BigDecimal>readSettingOptional(thing, CONFIG_RELVOLUMECHANGE, v -> zoneConfiguration.setVolumeRelativeChangeFactor(v.floatValue()));
Copy link
Member Author

Choose a reason for hiding this comment

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

@martinvw @davidgraeff given the limitations of configuration bean mappings (bean properties need to match settings name) and your advise to refactor this, I have introduced the breaking change. Documentation has been updated. Essentially, users will be forced to re-update their settings when migrating to this version. I hope this will not cause too much issues for the users.

Copy link
Member Author

@zarusz zarusz left a comment

Choose a reason for hiding this comment

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

Please see the compatibility breaks mentioned.

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from c0a151e to 0c2a790 Compare April 30, 2018 15:12
@martinvw
Copy link
Member

Thanks for your great work, is there some community testing needed, or do you feel confident about the changes?

@zarusz
Copy link
Member Author

zarusz commented Apr 30, 2018

I am fairly confident about these changes. Also tested on my AVR model. Either way, I plan to add some more features in the near term. If anything I will keep an eye on the community.

Glad to be of help for openHAB 👍

@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/yamahareceiver-binding-problems/34628/53

@zarusz zarusz force-pushed the feature/yamaha_fixes_and_improvements_2.3.0 branch from 0c2a790 to 1bbf55d Compare April 30, 2018 19:26
- Problem with HDMI input openhab#2758 (input mapping fixes for HDMIx, AUDIOx, NET RADIO)
- VolumeDB is not updated openhab#3034 - ‘Zone_2’ of Yamaha HTR-xxx receiver completely incontrollable openhab#3062
- Fixing Tuner compatibility with HTR-xxxx
- Adding support for RX-V3900 - Added Spotify, Server as supported preset inputs
- Added Server as supported playback inputs
- Added user defined custom mapping for inputs
- Adding STRAIGHT surround program
- Adding default album URL setting - Developer documentation cleanup

Signed-off-by: Tomasz Maruszak <maruszaktomasz@gmail.com>
@martinvw
Copy link
Member

martinvw commented May 1, 2018

Thanks again for your work and patience!

@martinvw martinvw merged commit 9ba758d into openhab:master May 1, 2018
@zarusz zarusz deleted the feature/yamaha_fixes_and_improvements_2.3.0 branch May 1, 2018 19:29
@martinvw martinvw added bug An unexpected problem or unintended behavior of an add-on enhancement An enhancement or new feature for an existing add-on labels May 25, 2018
@martinvw martinvw added this to the 2.3 milestone May 25, 2018
hww3 pushed a commit to hww3/openhab2-addons that referenced this pull request Jul 4, 2018
- Problem with HDMI input openhab#2758 (input mapping fixes for HDMIx, AUDIOx, NET RADIO)
- VolumeDB is not updated openhab#3034 - ‘Zone_2’ of Yamaha HTR-xxx receiver completely incontrollable openhab#3062
- Fixing Tuner compatibility with HTR-xxxx
- Adding support for RX-V3900 - Added Spotify, Server as supported preset inputs
- Added Server as supported playback inputs
- Added user defined custom mapping for inputs
- Adding STRAIGHT surround program
- Adding default album URL setting - Developer documentation cleanup

fixes openhab#3034
fixes openhab#3062
fixes openhab#2758

Signed-off-by: Tomasz Maruszak <maruszaktomasz@gmail.com>
divyachauhan25 pushed a commit to divyachauhan25/openhab2-addons that referenced this pull request Jul 6, 2018
- Problem with HDMI input openhab#2758 (input mapping fixes for HDMIx, AUDIOx, NET RADIO)
- VolumeDB is not updated openhab#3034 - ‘Zone_2’ of Yamaha HTR-xxx receiver completely incontrollable openhab#3062
- Fixing Tuner compatibility with HTR-xxxx
- Adding support for RX-V3900 - Added Spotify, Server as supported preset inputs
- Added Server as supported playback inputs
- Added user defined custom mapping for inputs
- Adding STRAIGHT surround program
- Adding default album URL setting - Developer documentation cleanup

fixes openhab#3034
fixes openhab#3062
fixes openhab#2758

Signed-off-by: Tomasz Maruszak <maruszaktomasz@gmail.com>
koa pushed a commit to koa/openhab2-addons that referenced this pull request Oct 28, 2018
- Problem with HDMI input openhab#2758 (input mapping fixes for HDMIx, AUDIOx, NET RADIO)
- VolumeDB is not updated openhab#3034 - ‘Zone_2’ of Yamaha HTR-xxx receiver completely incontrollable openhab#3062
- Fixing Tuner compatibility with HTR-xxxx
- Adding support for RX-V3900 - Added Spotify, Server as supported preset inputs
- Added Server as supported playback inputs
- Added user defined custom mapping for inputs
- Adding STRAIGHT surround program
- Adding default album URL setting - Developer documentation cleanup

fixes openhab#3034
fixes openhab#3062
fixes openhab#2758

Signed-off-by: Tomasz Maruszak <maruszaktomasz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on enhancement An enhancement or new feature for an existing add-on
Projects
None yet
6 participants