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

Initial contribution of Astro binding. #113

Merged
merged 1 commit into from
Apr 6, 2015
Merged

Initial contribution of Astro binding. #113

merged 1 commit into from
Apr 6, 2015

Conversation

gerrieg
Copy link
Contributor

@gerrieg gerrieg commented Jan 16, 2015

Signed-off-by: Gerhard Riegler gerhard.riegler@gmail.com

@kaikreuzer kaikreuzer self-assigned this Jan 19, 2015
@kaikreuzer kaikreuzer added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 19, 2015
http://eclipse.org/smarthome/schemas/config-description-1.0.0.xsd">

<config-description uri="config:astro-config">
<parameter name="latitude" type="decimal" required="true">
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest merging latitude and longitude into one string property (syntax "51.343,-140.32") with context "geolocation". This would allow UIs to use a map to directly pick a location. Having these both parameters separately makes it more complicated.

@kaikreuzer
Copy link
Member

Thanks a lot for this binding Gerhard and sorry for coming back to you so late!
The code looks very good overall and you are even using the very latest features like channel groups - this directly shows me how buggy some other parts still are (like the UI, which does not support this yet).

I noticed that items do not receive a status update if they are newly created - as the refresh interval is usually pretty long, it would be nice to receive values immediately. I think this is probably not a problem of your binding, but should be rather triggered by the framework itself. Did you already think about this?

In this context, I noticed that it is not possible to send a "REFRESH" command to the channels. Note that this did not exist in openHAB 1. It should trigger a status update for a certain item, so your binding should handle this, if possible.

@kaikreuzer kaikreuzer removed their assignment Feb 23, 2015
@gerrieg
Copy link
Contributor Author

gerrieg commented Mar 1, 2015

Hi Kai!

I implemented/changed the things you mentioned.

Currently i don't know when a item is newly created, there is no notification. So i can't immediately publish the value.
I think this notification should be triggered by the framework in any way.

BR, Gerhard

@kaikreuzer
Copy link
Member

Thanks Gerhard!

Currently i don't know when a item is newly created, there is no notification. So i can't immediately publish the value.
I think this notification should be triggered by the framework in any way.

Yes, I agree. I think if the framework would simply send a REFRESH command to a channel for which a new item has been linked, that should probably do the trick.

xsi:schemaLocation="http://eclipse.org/smarthome/schemas/config-description/v1.0.0
http://eclipse.org/smarthome/schemas/config-description-1.0.0.xsd">

<config-description uri="config:astro-config">
Copy link
Member

Choose a reason for hiding this comment

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

This uri needs to follow a certain naming convention in order to make sure that there are no clashes, see https://github.com/eclipse/smarthome/blob/master/docs/sources/architecture/configuration.md#xml-structure-for-configuration-descriptions. It probably should be thing-type://astro:config.

@kaikreuzer
Copy link
Member

Hi @gerrieg , after having upated the target platform with the improved lifecycle handling, I had another look at this PR and just have a few more comments. Once these are addressed, I'd be ready to merge this PR.

@gerrieg
Copy link
Contributor Author

gerrieg commented Mar 23, 2015

Hi @kaikreuzer !

1.) The mentioned jobScheduler is my own Quartz scheduler, not the scheduler in the BaseThingHandler.

2.) I implemented the EqualStateFilterHandler, because i don't want to flood the bus with unnecessary events. The user can specify a interval in the config. In this interval, all states are calculated and of course sent to the bus. My interval is 180sec, so every three minutes all items are updated, even if the state is the same. State updates may not be expensive, but what about Rules, Persistence etc. I agree, that this should be done by the framework, but currently it doesn't. So what should i do with this class? Will you implement this feature into the framework itself?

3.) If i change the uri to thing-type://astro:config, i get this exception:

Caused by: java.lang.IllegalArgumentException: The scheme specific part (token) must not start with a slash ('/')!
    at org.eclipse.smarthome.config.core.ConfigDescription.<init>(ConfigDescription.java:64) ~[org.eclipse.smarthome.config.core_0.8.0.201503191550.jar:na]
    at org.eclipse.smarthome.config.xml.ConfigDescriptionConverter.unmarshal(ConfigDescriptionConverter.java:73) ~[org.eclipse.smarthome.config.xml_0.8.0.201503191550.jar:na]
    at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:72) ~[xstream-1.4.7.jar:1.4.7]

Best, Gerhard

@kaikreuzer
Copy link
Member

1.) I did not comment the line with the jobScheduler, but the one with the statusUpdate. Btw, do you really need your own scheduler? I do not like having so many different thread pools in the system, I'd prefer to get the number of threads down.

2.) Please remove it. Sending update events is the suggested way.

but what about Rules, Persistence

They both allow reacting only on change events, so they filter by themselves already.

3.) Interesting - sounds as if the documentation is incorrect then. So maybe better have a try with thing-type:astro:config instead.

@gerrieg
Copy link
Contributor Author

gerrieg commented Mar 30, 2015

1.) I am using the provided Quartz scheduler. It's required, because i need a cron trigger for the daily midnight job. With the ScheduledExecutorService from the BaseThingHandler i can only calculate a delay to midnight which is problematic with clock drifts and daylight savings.

2.) I'm not happy about removing the EqualStateFilterHandler, but you are the boss 😉 I still hope, that the framework provides this functionality someday.

3.) I have another issue now ... i wanted to start the positional job only if at least one item is linked to a positional channel. In the initialize() method of the ThingHandler, the items are not yet linked. So how can i do that?

@kaikreuzer
Copy link
Member

  1. I am still not talking about the scheduler, but about the line with the "updateStatus" call.

  2. I don't want to be the boss, I just want a clean architecture. And you will agree that a single binding is not the right place to deal with this - besides this, it would serve as an example for others and we would have a lot of redundant code. Hope you agree :-)

  3. Very simple: Override the channelLinked method of BaseThingHandler! (That was very recently introduced (for you!), so please make sure to update your target platform)

@gerrieg
Copy link
Contributor Author

gerrieg commented Apr 3, 2015

1.) I need to set the thing ON-/OFFLINE, based on valid-/invalid thing config. So what's the correct way to do that?

2.) Yes, fully agree!

3.) Thanks, implemented

@kaikreuzer
Copy link
Member

  1. Not in the dispose method. The dispose is called when the handler is deconstructed and having no handler automatically means that the thing is OFFLINE. And the line indeed caused an exception in my tests because at that point in time the handler was already unregistered.

Where has the cancelling of the jobs now gone? This still needs to happen when a handler is disposed - or has this moved anywhere else now? I could not find it...

@gerrieg
Copy link
Contributor Author

gerrieg commented Apr 4, 2015

It has been in the restartJobs(), but because of the delay it was to late to delete the jobs at shutdown, i fixed this now.

Still have to learn how everything works exactly, so sorry for the circumstances.

@kaikreuzer
Copy link
Member

Still have to learn how everything works exactly, so sorry for the circumstances.

Well, we still have to document things in order for you to easily learn it :-)

Thanks for your fixes, looks all good to me now.
May I then ask you to squash your commits into a single one before I merge? Thanks!

@kaikreuzer kaikreuzer modified the milestone: 2.0.0 alpha2 Apr 4, 2015
Signed-off-by: Gerhard Riegler <gerhard.riegler@gmail.com>
@gerrieg
Copy link
Contributor Author

gerrieg commented Apr 6, 2015

Well, we still have to document things in order for you to easily learn it :-)

Definitely! Sometimes i don't know if it's my mistake, a bug in the framework, the feature is not implemented or i am using something wrong.

I'm still rely on help from you and the other framework genius. So i would like to say thank you for your great support!

Now i can continue with the Homematic binding and try find out how this works: https://bugs.eclipse.org/bugs/show_bug.cgi?id=461663

BR
Gerhard

@kaikreuzer kaikreuzer merged commit 5022954 into openhab:master Apr 6, 2015
@kaikreuzer
Copy link
Member

Awesome, thanks for your work!
Once we have "triggerChannels" specified (https://bugs.eclipse.org/bugs/show_bug.cgi?id=456226), I will let you know so that this missing part of the Astro binding can also be addressed.

@kaikreuzer kaikreuzer mentioned this pull request Apr 6, 2015
@gerrieg gerrieg deleted the astro branch April 9, 2015 17:05
@kaikreuzer
Copy link
Member

@gerrieg: May I ask you to create a short documentation for this binding until Sunday (May 24th)?
I have created a short guide on how to do it: https://github.com/eclipse/smarthome/blob/master/docs/sources/development/extensions/bindings/docs.md
As an example how the result could look like, please see https://github.com/eclipse/smarthome/blob/master/addons/binding/org.eclipse.smarthome.binding.hue/README.md.
Thanks!

@gerrieg
Copy link
Contributor Author

gerrieg commented May 22, 2015

Added a simple documentation with #246

@gerrieg
Copy link
Contributor Author

gerrieg commented Nov 4, 2016

Hi JP!

Since a few weeks, the core framework supports trigger-channels. I can now implement the missing functionality into the Astro binding.
In the meantime, use the OH1 version in OH2.

Regards,
Gerhard

Am 03.11.2016 um 19:20 schrieb JoergPi notifications@github.com:

Hi all,

looking at the astro documentation and the fact, that the trigger items are accepted while reading the item file but do not trigger something, I assume, that the OH1 feature "trigger items" is still not available in OH2/Astro?
Unfortunately, I used this to schedule many rules. Is there any alternative / replacement for this?

Thanks,

JP


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/openhab/openhab2-addons/pull/113#issuecomment-258229951, or mute the thread https://github.com/notifications/unsubscribe-auth/AGPelFDoKjMAblOFvfTaTzgxQSKNzZBLks5q6iXcgaJpZM4DTf3O.

@JoergPi
Copy link

JoergPi commented Nov 4, 2016

Hi Gerhard,

thanks for the hint using OH1 version!
I've seen the other discussion and the status of trigger-channels in ESH
shortly after sending my question and that's the reason, why I deleted
it: to avoid a gesture of impatience.

Thanks for your efforts!

Joerg

Am 04.11.2016 um 14:46 schrieb Gerhard R.:

Hi JP!

Since a few weeks, the core framework supports trigger-channels. I can
now implement the missing functionality into the Astro binding.
In the meantime, use the OH1 version in OH2.

Regards,
Gerhard

Am 03.11.2016 um 19:20 schrieb JoergPi notifications@github.com:

Hi all,

looking at the astro documentation and the fact, that the trigger
items are accepted while reading the item file but do not trigger
something, I assume, that the OH1 feature "trigger items" is still not
available in OH2/Astro?
Unfortunately, I used this to schedule many rules. Is there any
alternative / replacement for this?

Thanks,

JP


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openhab/openhab2-addons/pull/113#issuecomment-258229951,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGPelFDoKjMAblOFvfTaTzgxQSKNzZBLks5q6iXcgaJpZM4DTf3O.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/openhab/openhab2-addons/pull/113#issuecomment-258434817,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AWKtNer3GgMCKc4ZZV70pHgOadYuQikqks5q6zcYgaJpZM4DTf3O.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants