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

[DSL][automation][WIP] Added methods to retrieve and update thing configurations via rules #583

Conversation

cweitkamp
Copy link
Contributor

  • Added methods to retrieve and update thing configurations via rules

Instead of submitting an issue for discussion, I wrote some lines of codes as a base for the discussion.

First proposal for exposing thing configurations to rules. The idea behind this PR is to allow the user to access and maybe edit thing configurations via rules. I reused existing methods from ThingHandler interface to provide similar functionality (e.g. validation of configuration parameters).

Now there are some questions to discuss:

  1. Is this approach the way to go?
  2. Should we expose the whole configuration to the user?
  3. Is the user allowed to modify all configuration parameters - except read-only ones - via rules during runtime?

I am not sure if the validation of edited configuration parameters is sufficient as I did not find any unit tests for it. Can someone point me to them - or do I have to extend / implement those.

Signed-off-by: Christoph Weitkamp [email protected]

@maggu2810
Copy link
Contributor

I am not sure about the separation of layers at all.
In the past the interaction has been done on an item base only (correct me if I am wrong).

Accessing stuff of things has been added.
It seems there has already been an agreement that rules are does not need to work on items only but can also e.g. check the thing status.

IIRC the access is currently read only.

What's the use cases for such thing configuration updates?

What should be supported by rules at all?

Should we allow the thing, channel, item creation -- an overall system automation -- too?

@cweitkamp
Copy link
Contributor Author

the access is currently read only.

Yes, that is right. My current approach copies the read only map into a new map and returns it for the user. The user now is abled to change existing values and to update the configuration via the second method. This is a very "expensive" process.

I agree with you when we are talking about thing properties. Those could be returned as immutable map and the users do not need to edit them. A method for it has not yet been implemented. It is no big deal to add it too.

What's the use cases for such thing configuration updates?

Some use cases / examples I figured in the last few weeks:

  • audio bindings introduced a configuration for the notification volume (not the volume in general)
  • some lighting bindings are supporting a transition time / fading time / duration for applying an effect
  • Hue binding supports a motion sensor which light level thresholds for daylight / darkness values could be adjusted
  • hvac bindings hold temperature values like eco, comfort, window open, etc. in configurations

Currently the people switched to ThingActions to set the above mentioned values. Maybe that is the way to go. That is what we have to discuss and figure out here.

I can imagine a lot of configurations are set only one time (e.g. IPs, ports, credentials) and I can imagine to maybe introduce an attribute for the configuration description to allow / disallow exposing a configuration.

@5iver
Copy link

5iver commented Feb 18, 2019

1. Is this approach the way to go?

Specifically, is this the way to go for the Rules DSL? IMO, we shouldn't be adding new features to the old rule engine. Although, take a look at #6660. That PR should provide the basis for implementing what you are after, and will also expose this functionality to both rule engines. All of the Core and Cloud Actions will need to be adapted as well. The goal is to be able to use these in a rule as an Action or in a scripted Action.

  • Should we expose the whole configuration to the user?
  • Is the user allowed to modify all configuration parameters - except read-only ones - via rules during runtime?

JSR223 opened the door to do pretty much anything. It can be used in the NGRE as a scripted Action, but it can be used for much more than rules (OSGI services, bindings, etc.). So, I don't think there is really a question of whether anything should be allowed, since we already do. I haven't tried it, but using Jython, there's even...

Setting this to false will allow Jython to provide access to
non-public fields, methods, and constructors of Java objects.

python.security.respectJavaAccessibility = false

The functionality you are looking to add was implemented in the openhab2-jython helper libraries about two years ago, and shows the interest/need for this functionality. As an example, I'm almost done updating my OWM script to iterate the available Channels when creating and linking the supported Items. This will allow for Items to just appear when new Channels are added. No modification of the script is needed, and the same script will work for people using an older version of the binding. Sort of like autoupdate functionality, but for a specific Thing. I have also considered setting the forecastHours and forecastDays in the Thing, but thought that was too intrusive.

@davidgraeff
Copy link
Member

davidgraeff commented Feb 18, 2019

Although I very much appreciate that we can use JSR223 in Rules, I also love the idea of having dedicated rule module-types to interact with other OH subsystems. That way it is straight forward to realize a point and click rule editor that can influence most of OHs concepts.

I therefore strongly support the idea of exposing thing configurations to rule modules.

@maggu2810
Copy link
Contributor

@kaikreuzer Can you comment about the change thing addition to the DSL rules? Or better about all the topics in question here?
Should this be closed because of "not wanted" or should this be merged as it is or should there be some changes?

@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/zwave-device-configuration-changes/80952/4

@cweitkamp cweitkamp added awaiting feedback stale As soon as a PR is marked stale, it can be removed 6 months later. labels Oct 2, 2019
@cweitkamp cweitkamp changed the title [rules] Added methods to retrieve and update thing configurations via rules [DSL][automation][WIP] Added methods to retrieve and update thing configurations via rules Oct 2, 2019
@cweitkamp cweitkamp added the work in progress A PR that is not yet ready to be merged label Oct 2, 2019
@kaikreuzer
Copy link
Member

I think the questions that @maggu2810 asked here are very good - they take a step back and try to imagine, what we would be heading to: Allowing to edit/manipulate every aspect of openHAB through rules.

I am not so sure if this is really what we want. The rule engine was originally intended to purely work on the "functional" (=item) level. The idea behind it was that there should not be a dependency on any concrete hardware that is used underneath. Accessing the Thing status was as such already breaking this rule, but as it was a very frequently requested feature, it felt to be ok to add it.

Further adding config manipulation, item & thing editing, etc. can easily lead to an extensive feature creep and it can make the rules (and the engine implementation) overly complex. Maybe it is ok to allow it for JSR223 (as this goes into programming rather than a DSL), but also there we should define any clear rules on what can and should be done with it.

I'd therefore suggest to close it as a "won't do" and thus enforce the separation of the physical and the functional layer and make people stick to it (knowing that certain things then cannot be done through rules). But that's just my opinion, the other maintainers might see it differently.

@cweitkamp cweitkamp removed awaiting feedback stale As soon as a PR is marked stale, it can be removed 6 months later. labels Nov 18, 2019
@cweitkamp cweitkamp closed this Nov 18, 2019
@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/naming-for-items-things/87515/5

@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/aeon-zw098-light-bulb-how-do-you-change-the-config-parameters-from-rules-and-sitemaps/102839/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants