onDisconnect(callback) does not specify unexpected disconnect reason

[ce07c3]

Why leave it out?

[ppetrosh]

I don't think we can provide any meaningful information in the reason argument. It's just that the watchdog just caught the fact that we are disconnected. So the only information we can provide here is whether the disconnect was expected or unexpected.

I don't see what else we can do about it. Carl, do you have any suggestions?

[ce07c3]

Net info has latesterror field?

[ppetrosh]

Correct, but my point is that as onDisconnect is called asynchronously we can't guarantee that's the actual reason for disconnection. And you can always call net.info() from the callback as needed. So I'd probably keep this out of the library scope.

[ce07c3]

Yes, but on the other hand, it's a bit superfluous to use the connection manager and the onunexpecteddisconnect handler. Is there a way to combine these, perhaps? :)

[ppetrosh]

You don't have to, do you? You use ConnectionManager.onDisconnect and in the provided callback you call net.info() as needed.

The whole idea of using ConnectionManager is that you don't have to use onunexpecteddisconnect.

[ce07c3]

Exactly my point. If the idea is not to use onunexpecteddisconnect, then I would need to acquire the reason from the ConnectionManager. If I call net.info() in my own callback, then that's putting a responsibility on the developer where they can't reasonably decide if the value in net.info() is old or not.

[ppetrosh]

Got you point. Makes sense to me in general. Let me see if I can add this with a minimal change to the library.

[ppetrosh]

I dug deeper into the issue and unfortunately don't see a reliable way to provide an error reason to the app developer due to the following reasons:

1. server.onunexpecteddisconnect is called only after a minute of reconnect attempts. While the ConnectionManager.onDisconnect callback (called by a polling loop) is executed much earlier.
   And there is a reason for that: The imp attempts reconnection, without telling Squirrel it’s doing so. This is done to avoid over-reacting to brief outages.
2. when impOS tries to reconnect on the background https://electricimp.com/docs/resources/wifistatediagram/ (RETURN_ON_ERROR flow, ACTIVE CONNECTING state), imp.net.info().lasterror returns whatever current current reason is based on the reconnection cycle phase (it can be NO_WIFI, NO_IP_ADDRESS or NO_SERVER depending on where it currently is)

Even if a developer gets the reason of the disconnect what can be done about it other than to try to reconnect? Of course it's a good feature to have for debugging the connectivity, but unfortunately at this point it's not feasible to implement.

I made my initial attempt to implement it with the existing pull request. But I had to roll it back as it didn't work reliably as expected because of the above reasons.

[ppetrosh]

Having said unfortunately I'd suggest to close the issue as won't fix for now.

[ce07c3]

I am inclined to agree. I believe the library calls server.connect, which will have a reason code on completion (whether successful or not). Perhaps that one can be included.

It's not really pressing, so let's close it.

[ppetrosh]

server.connect is called in two cases: a) either we detected the disconnect happened and b) user called CM.connect(). The tricky part is that impOS tries to reconnect when it looses connection in the ACTIVE CONNECTING state https://electricimp.com/docs/resources/wifistatediagram/ (RETURN_ON_ERROR flow). It happens on the background, the Squirrel code is not notified and there is no way to retrieve the state of this operation rather than wait for onunexpecteddisconnect call (which will be called in a minute is not that useful).

So just to sum it up:

1. There is no reliable way to retrieve a failed connection attempt reason with CM in a 100% of the cases;
2. Even if we could, there is not much the Squirrel code can do about it, rather than try to reconnect. However, agree, this might be nice to have for debugging purposes at a development phase.

Thanks!

[ce07c3]

I agree, but if you call connect, you can pass in a callback which will give you the reason for it not being able to connect. This is different from unexpecteddisconnect, I think?

[blindman2k]

The latest pull request might help a bit as you get an onTimeout event when
connecting fails and an onDisconnect event when a running connection
terminates. Is that enough differentiation for you Carl?

On Tuesday, 18 October 2016, Carl notifications@github.com wrote:

I agree, but if you call connect, you can pass in a callback which will
give you the reason for it not being able to connect. This is different
from unexpecteddisconnect, I think?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvF3dJ56hPRol18By-JnrwYkRzOrxxFks5q06gzgaJpZM4KTlkP
.

Aron Steg

_BCDE Investments: Mystic Pants, *_SMS Broker, *SMS Diagnostics, Zaq,
Explainers

Mob: +61 4 3333 4444 or +1 415 728 6707 | Fax: +61 3 8677 7613

[ppetrosh]

@ce07c3, well, semantics of the reason values is the same. But mechanisms behind calling the connect and onunexpecteddisconnect callbacks are different. Though as I mentioned this is not sufficient to provide a reliable delivery of a disconnection reason as there is an internal impOS reconnection procedure that we have no control/track of from the Squirrel code.