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

add mdns discovery for network coordinators #704

Merged
merged 13 commits into from
May 6, 2023
Merged

add mdns discovery for network coordinators #704

merged 13 commits into from
May 6, 2023

Conversation

Tarik2142
Copy link
Contributor

I described everything in the request here #Koenkk/zigbee2mqtt#17430
I don't know much about typescript so improvements are welcome
image
image

@Koenkk
Copy link
Owner

Koenkk commented May 1, 2023

Thanks for the PR, in order to get this merged:

@Tarik2142
Copy link
Contributor Author

Thanks for the PR, in order to get this merged:

image
Ok, thanks for the reply. I think there will be no problems with the documentation, but I've never written tests, so I'll have to figure it out.
I ran a local check and somehow it passes. Here on github everything correctly shows that there are no tests for lines 85-115
I would be grateful for a hint why the local check is passing

@Koenkk
Copy link
Owner

Koenkk commented May 2, 2023

Can you try to do a npm ci and then a npm run-test-with-coverage?

removed the timer in the detection function, improved messages in the log, added missing types
@Tarik2142
Copy link
Contributor Author

Can you try to do a npm ci and then a npm run-test-with-coverage?

Can you try to do a npm ci and then a npm run-test-with-coverage?

Unfortunately that didn't help so I'll look here sometimes.
Also, I got a strange thing in the test system, when passing the LoggerStub to the Adapter, the logger for some reason still remains undefined and causes an error

-Adapter mdns without type test
-Adapter mdns detection test
-Minor improvements
@Tarik2142
Copy link
Contributor Author

Tarik2142 commented May 3, 2023

why line 107 is uncovered?
do i need to check all radio types?

@Tarik2142
Copy link
Contributor Author

After the PR to the documentation, I will update the code and add a links to the documentation in the error messages

const mdnsDevice = serialPortOptions.path.substring(7);
if(mdnsDevice.length == 0){
throw new Error(
`You must specify the adapter type after mdns://`+
Copy link
Owner

Choose a reason for hiding this comment

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

Do I understand correctly that the adapter type has to be specified in the serial.port? What about using the existing serial.adapter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

z2m zeroconf format
This is a misunderstanding, I meant MDNS service type. This error text really needs to be edited.

image
The user needs to specify only the MDNS service type of the device, all other configuration data will be received from the device via Zeroconf

Potentially, all Zeroconf devices listed here should work except tube
tube uses the esphomelib type, that is, if there are other esphome devices on the network, they will probably be found instead of the coordinator

Copy link
Owner

Choose a reason for hiding this comment

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

I see, this means that the config from the z2m configuration.yaml is ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, some parameters from the config will be ignored because they will be received from the coordinator

Copy link
Owner

Choose a reason for hiding this comment

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

I see, this should be clearly stated in the docs, could you make a pr for the docs? (https://www.zigbee2mqtt.io/guide/configuration/adapter-settings.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'm currently working on PR for docs

@Tarik2142
Copy link
Contributor Author

So the links exceed the length limit, how should I shorten them?

@Tarik2142
Copy link
Contributor Author

@Koenkk
PR to docs is ready Koenkk/zigbee2mqtt.io#2028

@Koenkk
Copy link
Owner

Koenkk commented May 6, 2023

@Tarik2142 I made some updates, can you check if everything is still ok? If yes, I will merge this.

@Tarik2142
Copy link
Contributor Author

Tarik2142 commented May 6, 2023

@Koenkk
Tested the changes on my installation.
image
Everything works.

Also tested handling of some errors, except the wrong packet, this requires changing the firmware of the coordinator
image
image

@Koenkk
Copy link
Owner

Koenkk commented May 6, 2023

Great, thanks for this cool feature!

@Koenkk Koenkk merged commit 08e0aa1 into Koenkk:master May 6, 2023
1 check passed
@stefa168
Copy link

stefa168 commented Jun 1, 2023

@Tarik2142 sorry for the ping; what functionalities this PR would bring to a user? I am just curious about it 👀

@Tarik2142
Copy link
Contributor Author

Tarik2142 commented Jun 1, 2023

@Tarik2142 sorry for the ping; what functionalities this PR would bring to a user? I am just curious about it 👀

https://www.zigbee2mqtt.io/guide/configuration/adapter-settings.html#mdns-zeroconf-discovery
If your coordinator supports this functionality, you can type mdns name in the configuration, IP, port and adapter type will be determined automatically. Even if the IP of the adapter changes, it will still be found. This simplifies the configuration

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

3 participants