Skip to content

Query for every available interface #32

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

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

mcspr
Copy link
Contributor

@mcspr mcspr commented Jul 14, 2022

resolving #25 (comment) issue on Windows, list finally returns available boards instead of nothing
receiving end stays the same, sendQuery uses specific Interface to send things

not really sure about

  • what errorCB supposed to do when one interface fails, but the rest don't
  • what happens when multiple interfaces return the same thing (e.g. lan wifi & ethernet)

query every interface that is both UP and MULTICAST'able
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 15, 2022
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

Hi @mcspr, thank you for the patch!

I've added just two remarks but the patch looks good overall.

what happens when multiple interfaces return the same thing (e.g. lan wifi & ethernet)

If the "ports" are the same, for cleanness I'd like to send just one event upstream.
BTW the pluggable discovery specification explicitly allows sending more "add" events for the same port to update port metadata, so even if we do not ignore the second event it should be fine anyway.

what errorCB supposed to do when one interface fails, but the rest don't

ErrorCallback is a callback function to signal unrecoverable errors to the client while the discovery is in "event" mode after a START_SYNC. Once the discovery signals an error it means that no more port events will be delivered until the client performs a STOP+START_SYNC cycle.
In our case, IMHO the best thing to do is to just ignore the error and continue to deliver events from the other interfaces.

But, thinking a bit more, there is also another case: if an interface came back online after the discovery has been started then this new interface will be completely ignored requiring a restart of the IDE. Probably the best solution is to poll the available interfaces (every minute?) and possibly start a listener on any new interface that appears.

Do you want to make it a try at implementing this logic?
I see that the main function is getting very complicated, it would be better to move some of the anonymous goroutines go func(...) into a real function or method to make it more readable.

mcspr added 2 commits July 21, 2022 14:51
queries are re-created each loop. in case netif disappears,
we won't use it next time (and caching will handle removal)
clean-up timeouts, additional interval for queries
clean-up control flow, split up main loop func a bit
@mcspr
Copy link
Contributor Author

mcspr commented Jul 21, 2022

re. <-time.After, those were there for some quick log.Printf checks :)
new commit removes those and tries to implement your proposed polling mechanism.

@mcspr mcspr marked this pull request as ready for review July 21, 2022 12:05
@cmaglie cmaglie self-assigned this Jul 21, 2022
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

Ok, I've added some nitpicks about code readability, but besides that the patch LGTM. Thank you!

@cmaglie
Copy link
Member

cmaglie commented Jul 26, 2022

@per1234 @ubidefeo do we have some spare time to test this patch?

Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

Thanks!

Now I need to test the patch on all OS, it may take a while...

@ubidefeo
Copy link

ubidefeo commented Aug 5, 2022

@per1234 @cmaglie
anyone been testing this one?

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I tested it on Windows and Linux with MKR 1000, ESP8266, ESP32:

Board discovered by mdns-discovery before/after

OS MKR 1000 ESP8266 ESP32
Windows ❌/❌ ❌/✔️ ❌/✔️
Linux ✔️/✔️ ✔️/✔️ ✔️/✔️

The boards are all discovered by Arduino IDE 1.8.19 even on Windows.

Even if it is still not working perfectly, this is undoubtedly a huge improvement. Thanks @mcspr!

@per1234 per1234 removed their assignment Aug 8, 2022
@cmaglie cmaglie merged commit 7c37417 into arduino:main Aug 10, 2022
@mcspr mcspr deleted the mdns/query-all-netifs branch August 11, 2022 07:42
@per1234 per1234 mentioned this pull request Aug 17, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants