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

[tr064] Description in LanDevice MAC "comma separated list" wrong #13988

Closed
wants to merge 3 commits into from
Closed

[tr064] Description in LanDevice MAC "comma separated list" wrong #13988

wants to merge 3 commits into from

Conversation

Spiev
Copy link
Contributor

@Spiev Spiev commented Dec 17, 2022

In the opehab binding configuration for the tr064 LANDevice configuration is wrong. It says, that (translate from german in english):
comma separated list of MAC-Addresses.

But this dosen't work :-D

The setting requires "one MAC-Address per row".

I did the changes already to the files where I found the corresponding wording.

Screenshot 2022-12-17 150628
Screenshot 2022-12-17 150535

@Spiev Spiev requested a review from a team as a code owner December 17, 2022 14:36
@jlaur jlaur changed the title tr064: Description in LanDevice MAC "comma separated list" wrong [tr064] Description in LanDevice MAC "comma separated list" wrong Dec 17, 2022
@lsiepel lsiepel linked an issue Dec 17, 2022 that may be closed by this pull request
@jlaur
Copy link
Contributor

jlaur commented Dec 17, 2022

Related to: #13798

@lsiepel
Copy link
Contributor

lsiepel commented Dec 17, 2022

In the related issue it was noticed that the english and german translation where off. I think this PR fixes the issue by proper documenting both translations.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 17, 2022

I'm not familiair with textual config combined with such lists, but maybe double check the Readme.md:

Thing subdeviceLan LAN "label LAN"   [ uuid="uuid:xxxxxxxx-xxxx-xxxx-yyyy-xxxxxxxxxxxx",
                                                macOnline="XX:XX:XX:XX:XX:XX",  
                                                          "YY:YY:YY:YY:YY:YY"] 

If that does not need a change, LGTM

@Spiev
Copy link
Contributor Author

Spiev commented Dec 17, 2022

Thanks @lsiepel,

I tested around a little bit to find out which setting is right. With OH 3.3.0, the only working setting in the UI is to set one mac address per row. So I tried to find a wording that discripes the setting more precise, in english and in german. And changed that in the source code with the pull request linked. The german translations is wrong for shure and dosen't function. :-D

@Spiev Spiev closed this Dec 17, 2022
@jlaur
Copy link
Contributor

jlaur commented Dec 17, 2022

@Spiev - you can reopen the PR if you would still suggest to add "One MAC-Address per row."

@Spiev Spiev reopened this Dec 18, 2022
@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

@jlaur Sorry, I tought, that you rejected the whole PR. :-) I reopened the PR to suggest repharsing.

@lolodomo
Copy link
Contributor

Could it be also changed here ?
https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.tr064/src/main/resources/OH-INF/thing/thing-types.xml#L149
This will avoid loosing the change if default translations are rebuilt with the i18n tool.

@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

@lolodomo Thx! I changed the file also and pushed it to fork / the PR.

@lolodomo
Copy link
Contributor

I'm not familiair with textual config combined with such lists, but maybe double check the Readme.md:

Thing subdeviceLan LAN "label LAN"   [ uuid="uuid:xxxxxxxx-xxxx-xxxx-yyyy-xxxxxxxxxxxx",
                                                macOnline="XX:XX:XX:XX:XX:XX",  
                                                          "YY:YY:YY:YY:YY:YY"] 

If that does not need a change, LGTM

This syntax is certainly wrong.

I was not aware that it is possible to enter multiple lines in a setting, either through UI or config file.

@lolodomo
Copy link
Contributor

Maybe like that ?

Thing subdeviceLan LAN "label LAN"   [ uuid="uuid:xxxxxxxx-xxxx-xxxx-yyyy-xxxxxxxxxxxx",
                                                macOnline="XX:XX:XX:XX:XX:XX
YY:YY:YY:YY:YY:YY"]

or using \\n ?

Thing subdeviceLan LAN "label LAN"   [ uuid="uuid:xxxxxxxx-xxxx-xxxx-yyyy-xxxxxxxxxxxx",
                                                macOnline="XX:XX:XX:XX:XX:XX\\nYY:YY:YY:YY:YY:YY"]

To be tested first.

@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

@lolodomo Thx, I tested different settings, the only thing that worked was writing the MAC addresses on a new line. If something else works, I would be grateful for information.

@lolodomo
Copy link
Contributor

@lolodomo Thx, I tested different settings, the only thing that worked was writing the MAC addresses on a new line. If something else works, I would be grateful for information.

Like that ?

Thing subdeviceLan LAN "label LAN"   [ uuid="uuid:xxxxxxxx-xxxx-xxxx-yyyy-xxxxxxxxxxxx",
                                                macOnline="XX:XX:XX:XX:XX:XX
YY:YY:YY:YY:YY:YY"]

If you tested it and it worked, please update the README file.

@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

@lolodomo I've configured the binding in the UX / UI only, not via text file direct, as described above. :-) This setup above shown in the screenshots works in my OH installation. All other various settings, that I tested (comma separated, semicolon separated, blank space separated) didn't worked. To be clear: All in the UX / UI, not in the text file directly. :-)

@Spiev Spiev closed this Dec 18, 2022
@lsiepel
Copy link
Contributor

lsiepel commented Dec 18, 2022

Why close it? It still needs to be merged. Your changes are good. We need to also fix the readme.md
I know you set it up by the gui. Are you able to test the textual setup? If not I’ll open an issue and ask someone else to test. Would be nice if this can be properly documented for 3.4.0
Thanks for your work

@lsiepel lsiepel reopened this Dec 18, 2022
@lolodomo
Copy link
Contributor

lolodomo commented Dec 18, 2022

I see the "multiple" attribute that is certainly the trigger for UI:

<parameter name="macOnline" type="text" multiple="true">

If we don't know what is the syntax in config file, I suggest to at least update the example in README to have only one Mac address.

@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

@lsiepel Sorry, I'm not a hardcore programmer and had some issues with a the DCO check in the PR, which I couldn't get fixed. The original error was a translation error which is resolved already. So I choosed to close the PR :-)

I test the setting in the text file described from @lolodomo above an will come back to here.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 18, 2022

No problem. The DCO will probably not be an issue as this is only a minor (but useful) documentation correction.
Thanks for performing a test!

@Spiev
Copy link
Contributor Author

Spiev commented Dec 18, 2022

Ok @lsiepel , @lolodomo,

I've teste differend configuration versions in the file configuration fritzbox.things (which I created for the test). As @lolodomo said above, the one that is working with the text file and multiple mac-addresses is:

Thing subdeviceLan LAN "label LAN"  [ uuid="uuid:75802409-bccb-40e7-8e6b-F0B014C513D7",
                                                macOnline=  "42:93:9C:27:C7:5B",
                                                "0A:F8:03:A4:65:20"
                                        ] 

In the Web UI (will name it "gui" in the following :-)) the configuration is shown as in the picture:
image

I guess that the translation "error" in german is based here.

So if I would configure the binding and its subchannels via text file, the behavior is different from the gui. When I configure the mac addresses as above in the gui, no channels would be shown under "Channels":

image

image

The openhab.log writes down the following errors:

`2022-12-18 15:41:56.915 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macOnline, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.
2022-12-18 15:41:56.930 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macIP, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.
2022-12-18 15:41:56.938 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macSignalStrength1, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.
2022-12-18 15:41:56.942 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macSpeed1, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.
2022-12-18 15:41:56.946 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macSignalStrength2, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.
2022-12-18 15:41:56.950 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, 0A:F8:03:A4:65:20 while processing macSpeed2, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.`

I've also tested this configuration in the gui:
image

But with the same result, an error in the openhab.log:

2022-12-18 15:51:26.740 [WARN ] [hab.binding.tr064.internal.util.Util] - Removing 42:93:9C:27:C7:5B, while processing macOnline, does not match pattern ([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}(\s*#.*)*, check config.

When I change the config in the gui as shown here:
image

The regarding MAC-Addresses showing up in "Channels":
image

Hope that helps :-)

@lolodomo
Copy link
Contributor

The problem is probably the conjunction of "multiple" and list as parameter.
It looks like it is working with GUI.
Maybe we are missing the way to assign a list of strings in config file.
By the way, this parameter could have been a simple string containing a comma separated list of IPs. Like that, no difficulty.

I would suggest to not hurry and to propose a clean fix.

@lolodomo
Copy link
Contributor

I am currently searching what other bindings use this "multiple" parameter attribute to maybe find the proper syntax and to see how this parameter is handled.

@lolodomo
Copy link
Contributor

In addition to tr064 binding, it is used in the following bindings:

  • bluetooth.roaming
  • enocean
  • http
  • ipcamera
  • modbus
  • mqtt.homeassistant
  • smhi
  • telegram
  • openhabcloud

@lsiepel lsiepel self-assigned this Jan 1, 2023
@lsiepel
Copy link
Contributor

lsiepel commented Jan 4, 2023

@Spiev Only thing left here is to make a small change to the readme.md to clarify the lines vs comma

I think this:

Parameters that accept lists (e.g. `macOnline`, `wanBlockIPs`) can contain comments.

Can be altered to something like:

Some parameters (e.g. `macOnline`, `wanBlockIPs`) accept lists. 
List items are configured one per line in the UI, or are comma separated values when using textual config.
These parameters that accept list can also contain comments.
....

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.

[tr064] Binding OH3 MAC adresses for LANDevice
4 participants