Skip to content

Fix for #2576 #2646

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

Closed
wants to merge 1 commit into from
Closed

Fix for #2576 #2646

wants to merge 1 commit into from

Conversation

ronguest
Copy link

I’ve had a couple issues with unreliable Yun discovery. This change has
fixed those issue for me and would appear to address concerns raised in
#2576.

I’ve had a couple issues with unreliable Yun discovery. This change has
fixed those issue for me and would appear to address concerns raised in
arduino#2576.
@ffissore
Copy link
Contributor

Hi @roadfun
I see you're using InetAddress.isReachable(). Unfortunately, that won't work. IsReachable does not behave the same way on all platforms and in general is not reliable.
Google for inetaddress.isreachable problem to see some interesting reports. Here is one http://bordet.blogspot.it/2006/07/icmp-and-inetaddressisreachable.html

Still, I admit using advertised port 80 is not be a smart move, because it's not the one used by the IDE. We should add port 22 to the advertised ports

@ffissore
Copy link
Contributor

Or we can just use the ping command and let the OS deal with ICMP. Ping command line is not equal across different OSs, so we'll have to define a method for each Platform object

@ronguest
Copy link
Author

Hi Federico. I did also consider those two alternatives. A couple of observations.

First, there is something wrong with the NetUtils implementation beyond the port being used which is why I didn't change it to use the port 22 for example. If I use NetUtils.isReachable() Yun discovery remains unreliable. At times the IDE will not find any Yuns at all (as reported by the OP and experienced by myself and others). With the implementation I submitted it works perfectly.

I did consider doing a cross-platform call to ping but I rejected that for two reasons. First, like your claim about InetAddress.isReachable() the implementation on each platform is different. Windows does not support a true ping but instead uses TCP on a specific port. If you set up the Yun to be more secure it will not respond to echo. And in fact when I lock down a system it won't respond to ping either (could be an extreme case). Even the article you linked to on Blogspot says ping will often fail.

I think unreliable checks for isReachable() are worse than no check at all (false positives). I'd much rather have false positives than have none of my Yuns showing up in the Ports menu or have them disappearing randomly. If checks are deemed essential then I think a reliable implementation mimicking an upload connection is the only reasonable implementation.

Since I experience both of these problems constantly using the official IDE I will spend a little more time to see if I can find an approach to checking the port availability that won't suffer the issue of the current method. Feel free to share any other thoughts or advice.

@ffissore
Copy link
Contributor

Thanks for all your feedback @roadfun. I agree with you: ping is not reliable, not even when you run the one provided by the OS, especially if you've hardened your yun. And I also agree that attempting an upload is the best way to know if the actual upload will work.

The yun uses SSH, other future boards may use something else (who knows). We should add some metadata to yun's bonjour message to tell the IDE which test to perform.

I'll do some tests with a yun tomorrow morning, but I'm eager to share some code: please take a look at https://github.com/ffissore/Arduino/commit/abc376bd941da0b6f3265024967b5763613be06d

Variables naming apart, it attempts to establish a connection on both port 80 and port 22. Timeout is raised to 300ms: that will slow down the Ports menu, but the yun may be slow at responding.

I'll also try to make that check asynchronous, so that we could raise timeout even to a second

@ronguest
Copy link
Author

Hi Federico. I left a comment on your code. With one change I think it works. I think there is an additional change which could be an improvement and that is to refactor to stop on the first isReachable success since you are checking multiple ports. In that case there is no need to keep checking is there?

@ffissore
Copy link
Contributor

Thanks @roadfun. Fixed and opened PR #2658

@ffissore
Copy link
Contributor

Code merged into #2658. Closing

@ffissore ffissore closed this Feb 19, 2015
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.

2 participants