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

[basicprofiles] Initial contribution #95

Merged
merged 7 commits into from
May 16, 2021

Conversation

cweitkamp
Copy link
Contributor

  • Initial contribution of Basic Profiles bundle

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp changed the title [WIP] Initial contribution of Basic Profiles bundle Initial contribution of Basic Profiles bundle May 13, 2021
@cweitkamp cweitkamp requested a review from J-N-K May 13, 2021 14:11
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks. These look like great additions. I have left some comments.

pom.xml Outdated Show resolved Hide resolved
bundles/org.smarthomej.transform.basicprofiles/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.transform.basicprofiles/README.md Outdated Show resolved Hide resolved

@Override
public void onStateUpdateFromItem(State state) {
callback.sendUpdate((State) applyRound(state));
Copy link
Member

Choose a reason for hiding this comment

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

Is that correct? I thought that sendUpdate updates the item's state. But shouldn't this result in a state that is processed by the handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Round is a one-way Profile. It should be applied only in updates / commands from handler towards Item.

Copy link
Member

Choose a reason for hiding this comment

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

@cweitkamp I would like to come back to this comment, as the current implementation causes issues with latest core changes. I also came across this when using the invert/negate profile.

As a result of this line we end up in an endless loop:

  1. The item updates its state to 123.12 and sends an ItemStateUpdatedEvent with a state of 123.12.
  2. The CommunicationManager forwards this state update to the and according to its configuration the profile rounds that to 123 and forwards that to the callback's sendUpdate method.
  3. The callback issues an ItemStateEvent which is received by the item, which sets its state to 123. After that, it sends an ItemStateUpdatedEvent with a state of 123.
  4. The CommunicationManager receives that, ... (start again at 2).

the same issue arises with the invert profile, because CLOSED is inverted to OPEN and after updating the item state to OPEN it is again inverted to CLOSED, resulting in the same update loop.

We can't prevent that by checking event source, because the ItemUpdater (that receives the ItemStateEvent) calls GenericItem.setState (which creates the ItemStateUpdatedEvent/ItemStateChangedEvent) and the source is lost (otherwise we could have set the source of the ItemStateUpdatedEvent to the same value as the ItemStateEvent.

IMO a profile should only change values that are transferred from the handler to the item (or from the item to the handler), not interact with the item state itself.

Choose a reason for hiding this comment

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

And, obviously, it is right now a blocking issue to migrate to OH 4 with M2 or newer snapshots, so maybe this can be resolved somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your explanation sounds reasonable. I won't argue against it. Feel free to change/remove this part of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After upgrading to latest OH milestone I am suffering from this myself. I submitted #478 to fix this.

@J-N-K J-N-K changed the title Initial contribution of Basic Profiles bundle [transform.basicprofiles] Initial contribution of Basic Profiles bundle May 15, 2021
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp
Copy link
Contributor Author

One more question. Should I contribute against the "main" branch?

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Just two comments. Otherwise looks good. Jenkins fails unrelated.

Regarding the branch: I sync both branches manually by cherry-pciking the commits, so it doesn't matter wher you contribute. Just use what better fits your need.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@J-N-K J-N-K merged commit 7a2637c into smarthomej:3.2.x May 16, 2021
@J-N-K J-N-K changed the title [transform.basicprofiles] Initial contribution of Basic Profiles bundle [transform.basicprofiles] Initial contribution May 16, 2021
@J-N-K J-N-K added this to the 3.1.2 milestone May 16, 2021
@cweitkamp cweitkamp deleted the feature-basic-profiles branch May 16, 2021 13:46
J-N-K pushed a commit that referenced this pull request May 16, 2021
…le (#95)

* Initial contribution

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@J-N-K J-N-K changed the title [transform.basicprofiles] Initial contribution [basicprofiles] Initial contribution Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants