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.

Sign up for GitHub

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

Jump to bottom

[lcn] Make auto discovery use segment id and module id for ThingID #8120

Merged
merged 5 commits into from
Aug 1, 2020
Merged

[lcn] Make auto discovery use segment id and module id for ThingID #8120

merged 5 commits into from
Aug 1, 2020

Conversation

toweosp
Copy link

@toweosp toweosp commented Jul 13, 2020 •
edited by fwolter
Loading

When using auto discovery modules are now identified by a combination of segment id and module id in the format S<segmentID>M<moduleID> instead by serial number. Serial number is used as representation property (e.g. to identify identical modules when using auto-ignore option in PaperUI). Closes #8097.

@toweosp
Reference modules by segment id and module/group id instead of serial
2457401 
number. Serial number is used as representation property.

Signed-off-by: Thomas Weiler <[email protected]>
@toweosp toweosp requested a review from fwolter as a code owner July 13, 2020 18:11
@toweosp toweosp changed the title Auto discovery uses combination of segment id and module id for ThingID [lcn] Auto discovery uses combination of segment id and module id for ThingID Jul 13, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@fwolter fwolter added the enhancement An enhancement or new feature for an existing add-on label Jul 13, 2020
@fwolter fwolter changed the title [lcn] Auto discovery uses combination of segment id and module id for ThingID [lcn] Make auto discovery use segment id and module id for ThingID Jul 13, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter
fwolter requested changes Jul 23, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Actually the module's serial number is not needed at all anymore. The property could be dropped and replaced by an address (representation-)property, which could be set during discovery identical to the Thing ID. That would simplify code.

@toweosp
Copy link
Author

toweosp commented Jul 24, 2020

I intentionally used the serial/firmware number as the representation property and didn't remove the property. If I am not mistaken:

Serial number is the only unique and immutable property of an lcn module. So auto discovery can identify identity of modules/things even if segment id or module id get changed. Furthermore after the change provided by this PR auto discovery in existing installations using the 2.x lcn binding will not discover existing modules/things as new but ignore them if paper ui is configured accordingly. Moreover ThingID of manually added modules/things can differ from the scheme used by auto discovery, so I think ThingID will /can not be equal to the representation property in every case, might it be the serial number or an address identifier.

Last but not least I generally wouldn't remove the serial number property, because it could ease support in the not so rare cases where module handling depends on serial number/firmware age.

fwolter
fwolter requested changes Jul 26, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

OK

...ab.binding.lcn/src/main/java/org/openhab/binding/lcn/internal/LcnModuleDiscoveryService.java Outdated Show resolved Hide resolved
@fwolter
Copy link
Member

fwolter commented Jul 26, 2020

Can you change the Thing UIDs in the documentation examples from serial numbers to the new scheme?

toweosp and others added 2 commits July 31, 2020 16:55
@toweosp @fwolter
Update bundles/org.openhab.binding.lcn/src/main/java/org/openhab/bind…
72ff2c4 
…ing/lcn/internal/LcnModuleDiscoveryService.java

Changed ThingUIDs in documentation examples

Co-authored-by: Fabian Wolter <[email protected]>
Signed-off-by: Thomas Weiler <[email protected]>
@toweosp
[lcn] Improve documentation
b88f1d2 
Signed-off-by: Thomas Weiler <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @toweosp,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

1 similar comment
@TravisBuddy
Copy link

Travis tests were successful

Hey @toweosp,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@toweosp
Copy link
Author

toweosp commented Jul 31, 2020

Done.

fwolter
fwolter approved these changes Aug 1, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

@fwolter fwolter merged commit a9727ca into openhab:2.5.x Aug 1, 2020
@fwolter fwolter added this to the 2.5.8 milestone Aug 1, 2020
@toweosp toweosp deleted the 8097-Autodiscovery_uses_segId_and_modID_for_ThingUID branch August 1, 2020 13:21
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
@toweosp @MPH80
[lcn] Make auto discovery use segment id and module id for ThingID (o…
bc6eb8a 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
Signed-off-by: MPH80 <[email protected]>
@fwolter fwolter mentioned this pull request Aug 13, 2020
Merged
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@toweosp @andrewfg
[lcn] Make auto discovery use segment id and module id for ThingID (o…
ec1f470 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@toweosp @andrewfg
[lcn] Make auto discovery use segment id and module id for ThingID (o…
1f721a5 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@toweosp @andrewfg
[lcn] Make auto discovery use segment id and module id for ThingID (o…
5c5963b 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@toweosp @andrewfg
[lcn] Make auto discovery use segment id and module id for ThingID (o…
8be738e 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@toweosp @DaanMeijer
[lcn] Make auto discovery use segment id and module id for ThingID (o…
bf3c222 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
@toweosp @CSchlipp
[lcn] Make auto discovery use segment id and module id for ThingID (o…
6ad005a 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@toweosp @markus7017
[lcn] Make auto discovery use segment id and module id for ThingID (o…
7403d15 
…penhab#8120)

Signed-off-by: Thomas Weiler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fwolter fwolter fwolter approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
2.5.8
Development

Successfully merging this pull request may close these issues.

[lcn] Reference modules and groups by segment id and module/group id instead of serial number
3 participants
@toweosp @TravisBuddy @fwolter