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

[modbus] Change bridge to offline during reinitialization #5244

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

mrbig
Copy link

@mrbig mrbig commented Mar 25, 2019

I noticed that things connected to a modbus bridge won't be reinitialized when the bridge changes. Digging down to openhab core I found out that connected things are only notified when the bridge changes status.

This small PR changes the bridge to OFFLINE if it was ONLINE when a change happened.

This is a split of the original PR #4220 as requested by @davidgraeff .

This seems to be necessary to get the children notified about the change

Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
@mrbig mrbig requested a review from ssalonen as a code owner March 25, 2019 17:24
Copy link
Member

@davidgraeff davidgraeff left a comment

Choose a reason for hiding this comment

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

LGTM

@ssalonen
Copy link
Contributor

ssalonen commented Mar 25, 2019

Ah in the original PR the bridge was switched to offline in dispose: https://github.com/openhab/openhab2-addons/pull/2246#discussion_r206529831 and openhab/openhab2-addons@e23af94

I think this bridge status notification was exactly the reason for that in the first place but I forgot it during review when Kai asked me to remove it as "extra".

Perhaps the dispose method would be a cleaner place for this? The comment is probably still necessary to avoid accidentally removing it in the future.

Alternatively one can ask whether framework should in fact update thing OFFLINE when disposing it?

@davidgraeff
Copy link
Member

Alternatively one can ask whether framework should in fact update thing OFFLINE when disposing it?

Because a config change makes the thing handler by default go through the dispose/init cycle that would cause a lot of status changes. That's probably why the framework is not doing it.

@ssalonen
Copy link
Contributor

ssalonen commented Mar 25, 2019

@davidgraeff that makes sense then to leave it to binding developers.

Thinking more about this:

For this particular PR, it actually makes more sense to update status right in dispose to "notify" child things as soon as possible that bridge status has changed. Most likely children would become offline as well as they should

But on the other hand it would have a downside of extra status updated considering what davidgraeff just mentioned :/

@davidgraeff
Copy link
Member

I'm sure that @mrbig has tested this particluar change. I'm not sure what the framework will do when the status is updated in dispose(), there are a lot of special cases in the bridge-status-change method.

Let's rather have this change in. If it can be made faster / more responsive in a later PR, that's fine.

@mrbig
Copy link
Author

mrbig commented Mar 25, 2019

I can confirm @davidgraeff : when only the config was changes on a running bridge, then the initialize has been called in the ONLINE state. For the connected things to get notified I had to forcefully move the bridge to an OFFLINE state, eg by setting an invalid config first.

@ssalonen
Copy link
Contributor

ssalonen commented Mar 25, 2019

Davidgraeff: I take your word on the details/special cases and agree it makes sense to move incrementally and conservatively here, and merge this as is

@davidgraeff davidgraeff merged commit 7c1b83e into openhab:master Mar 25, 2019
@wborn wborn added this to the 2.5 milestone Apr 14, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Jun 19, 2019
This seems to be necessary to get the children notified about the change

Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
Signed-off-by: Pshatsillo <pshatsillo@gmail.com>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
This seems to be necessary to get the children notified about the change

Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
Signed-off-by: Maximilian Hess <mail@ne0h.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants