Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Initial version of FS internet radio binding. #429

Merged
merged 1 commit into from
Jan 8, 2016

Conversation

paphko
Copy link
Contributor

@paphko paphko commented Aug 27, 2015

This binding is based on openhab 1.x binding frontiersiliconradio and is
migrated to the new ESH API, including device discovery.

@paphko paphko force-pushed the fsinternetradio-binding branch 4 times, most recently from 48235cf to 289b481 Compare August 27, 2015 11:16
@kaikreuzer kaikreuzer self-assigned this Aug 27, 2015
@kaikreuzer
Copy link
Contributor

Thanks! I'll do the review in September and come back to you!


<config-description>
<parameter name="ip" type="text" required="true">
<label>Internet Radio Address</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the "Internet Radio" from all labels (config as well as channels), please - labels should be as short as possible.

@paphko
Copy link
Contributor Author

paphko commented Nov 26, 2015

@teichsta Thanks for testing :-)
@kaikreuzer I did all the changes, except for switching to another HTTP client - that would take too much time at the moment...

@paphko
Copy link
Contributor Author

paphko commented Nov 26, 2015

To be more precise: I will not change the HTTP client within the next days or even weeks, so I would suggest to finish this pull request and to open a new issue for switching to another HTTP client?

@kaikreuzer
Copy link
Contributor

I would suggest to finish this pull request and to open a new issue for switching to another HTTP client?

Ok, I am fine with that. Would be great if you could find the time in the next months to work on the http client replacement though.
Regarding finishing this PR: I would really appreciate, if you would reconsider #429 (comment) and change this part of the code accordingly.

@maggu2810
Copy link
Contributor

FYI: The ESH transport bundle upnp imports packages of jupnp. The jupnp bundle itself imports a lot of the org.apache.http.* packages. So, we can get rid off it in the ESH bundles, but as long as jupnp using it, we cannot really get rid off (using e.g. the hue bundle will add it to the dependency chain).

@kaikreuzer
Copy link
Contributor

@maggu2810, the discussion here is about Apache Commons HTTP which is yet another lib than org.apache.http, so this does not make a difference here.
Still, pointing to jUPnP is a very valid remark, which you might want to directly make at https://www.eclipse.org/forums/index.php/t/1069192/.

paphko pushed a commit to paphko/smarthome that referenced this pull request Dec 8, 2015
This binding is based on openhab 1.x binding frontiersiliconradio and is
migrated to the new ESH API, including device discovery.

Bug: eclipse-archived#429
Signed-off-by: Patrick Koenemann <patrick.koenemann@itemis.de>
Also-by: Rainer Ostendorf <github@linlab.de>
@paphko
Copy link
Contributor Author

paphko commented Dec 8, 2015

@kaikreuzer, there are now two volume items, one is the absolute 0..32 number item, the other is a percent item. I further created a new issue about the http client that is used in this binding: #682

<label>Preset</label>
<description>Preset radio stations configured in the radio.</description>
</channel-type>
<channel-type id="volume-absolute">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you define this channel type as "advanced=true"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

paphko pushed a commit to paphko/smarthome that referenced this pull request Dec 8, 2015
This binding is based on openhab 1.x binding frontiersiliconradio and is
migrated to the new ESH API, including device discovery.

Bug: eclipse-archived#429
Signed-off-by: Patrick Koenemann <patrick.koenemann@itemis.de>
Also-by: Rainer Ostendorf <github@linlab.de>
paphko pushed a commit to paphko/smarthome that referenced this pull request Dec 8, 2015
This binding is based on openhab 1.x binding frontiersiliconradio and is
migrated to the new ESH API, including device discovery.

Bug: eclipse-archived#429
Signed-off-by: Patrick Koenemann <patrick.koenemann@itemis.de>
Also-by: Rainer Ostendorf <github@linlab.de>
@kaikreuzer
Copy link
Contributor

Thanks a lot for the updates, @paphko.
I actually just did a functional test and since I do not have such a radio at hand, I only tried an error situation by manually creating a Thing and pointed it to the IP address of my Fritz!Box - we will need some improvements on the logging here :-)

try {
xml = getXmlDocFromString(requestResultString);
} catch (Exception e) {
logger.error("converting to XML failed: '" + requestResultString + "' with " + e.getClass().getName() + ": "
Copy link
Contributor

Choose a reason for hiding this comment

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

this should only be debugging or trace level - and the message prints out the full XML, which is potentially huge. So you might log the e.getMessage() as debug, but the requestResultString should definitely only be trace level.

This binding is based on openhab 1.x binding frontiersiliconradio and is
migrated to the new ESH API, including device discovery.

Bug: eclipse-archived#429
Signed-off-by: Patrick Koenemann <patrick.koenemann@itemis.de>
Also-by: Rainer Ostendorf <github@linlab.de>
@paphko
Copy link
Contributor Author

paphko commented Dec 23, 2015

@kaikreuzer Done.

@kaikreuzer
Copy link
Contributor

Thanks!
As the code is > 1000 lines, I have created CQ 10558 and am now waiting for approval from the IP team before we can merge this PR.

@kaikreuzer kaikreuzer added CQ and removed feedback labels Dec 23, 2015
@kaikreuzer kaikreuzer merged commit e16ab74 into eclipse-archived:master Jan 8, 2016
@kaikreuzer
Copy link
Contributor

CQ has been approved for parallel IP checkin!

@kaikreuzer kaikreuzer removed the CQ label Jan 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants