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

[insteon] Added the ability to configure devices from the UI #8226

Merged
merged 2 commits into from
Aug 16, 2020
Merged

[insteon] Added the ability to configure devices from the UI #8226

merged 2 commits into from
Aug 16, 2020

Conversation

robnielsen
Copy link
Contributor

Initial support includes broadcast channels for PLM and disabling heartbeat for motion sensor II

…e UI

Signed-off-by: Rob Nielsen <rob.nielsen@yahoo.com>
@robnielsen robnielsen added the enhancement An enhancement or new feature for an existing add-on label Jul 30, 2020
@openhab openhab deleted a comment from TravisBuddy Aug 3, 2020
@openhab openhab deleted a comment from TravisBuddy Aug 3, 2020
@openhab openhab deleted a comment from TravisBuddy Aug 3, 2020
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.

I find it highly questionable to let the user enter JSON data for configuration. Are you aware that Channels can have config parameters as well?

@robnielsen
Copy link
Contributor Author

Yes, I'm aware that channels can have config parameters as well, but they are not accessible by the Paper UI, you must define these in a .things file which can be confusing. This changes allows to completely configure the things and channels from the UI. Other than JSON, what do you suggest to use for config data that requires key/value pairs and varies by device and channel?

@fwolter
Copy link
Member

fwolter commented Aug 16, 2020

Channel configuration parameters can be configured in PaperUI. See the pen:
LCN Binding

I'd add all config parameters that a specific Channel can have to a Channel. I think you can remove config parameters at runtime, if you only know at runtime which parameters are applicable.

@robnielsen
Copy link
Contributor Author

@fwolter Thanks for the info, I'll remove the code for configuring channels.

…tion

Signed-off-by: Rob Nielsen <rob.nielsen@yahoo.com>
@TravisBuddy
Copy link

Travis tests were successful

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

@robnielsen robnielsen changed the title [insteon] Added the ability to configure devices and channels from the UI [insteon] Added the ability to configure devices from the UI Aug 16, 2020
@openhab openhab deleted a comment from TravisBuddy Aug 16, 2020
@robnielsen
Copy link
Contributor Author

@fwolter, the ability to configure channels has been removed

@fwolter
Copy link
Member

fwolter commented Aug 16, 2020

Maybe I miss something, but why do you use JSON format for the Thing configuration? If I see correctly, there are two new config parameters. Why not adding them as normal configuration parameters to the Thing?

@robnielsen
Copy link
Contributor Author

The config parameter varies by the Insteon device that is specified by the productkey parameter of the device. Currently there are only 2 of the approximately 50 device types are using this new optional way to configure devices. The parameter will more than likely be different for each new device configuration and adding these to the thing will just complicate configuration it will be full of parameters that are useless.

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.

After looking at the rest of the code I understand why you chose this approach.

It might've been better if each productKey got its own Thing type. Then, you could define the config parameter as needed for each product and you don't need to assign Channels dynamically.

Maybe OH3 is a good opportunity to think about a breaking change.

As I don't have any better idea to integrate a product specific config in the current design, I'll approve it.

@fwolter fwolter merged commit 363730c into openhab:2.5.x Aug 16, 2020
@fwolter fwolter added this to the 2.5.8 milestone Aug 16, 2020
taboneclayton pushed a commit to taboneclayton/openhab-addons that referenced this pull request Aug 23, 2020
…#8226)

Signed-off-by: Rob Nielsen <rob.nielsen@yahoo.com>
Signed-off-by: Clayton Tabone <taboneclayton@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
…#8226)

Signed-off-by: Rob Nielsen <rob.nielsen@yahoo.com>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants