aseba icon indicating copy to clipboard operation
aseba copied to clipboard

Do not advertise targets being used

Open stephanemagnenat opened this issue 7 years ago • 13 comments

Currently, all targets are advertised using zeroconf. It would be better, from an end-user point of view, to only advertise the one that are not used, that is, the ones to which there is no connection or the connection did not see any data for at least 3 seconds.

Completing this issue requires:

  • [x] basic support in Zeroconf
  • [x] support in Studio
  • [x] support in Playground
  • [x] support in Dummy node
  • [ ] support in new Switch

stephanemagnenat avatar Sep 26 '17 11:09 stephanemagnenat

I am guessing that the intention here is to prevent connected nodes from appearing in the Aseba Studio switcher. The usual mechanism would be to add "status=busy" to the TXT record, and rely on the switcher to respect this status. Let me try to understand the alternative proposed here.

An Aseba node can't refuse a connection, so once connected a node would stop advertising itself so that it doesn't appear in the switcher. This does not prevent a direct connection by someone who knew the IP and port, but reduces the chance that someone would connect by mistake. If the connection closes, then the node starts advertising itself again. If the connection becomes inactive, then the node closes the TCP/Ip circuit it and starts advertising itself again.

This only applies to self-advertising nodes. A node connected to a switch is always connected, so it never advertises itself. The switch advertises itself as a switch, but does not advertise the individual nodes nodes.

Here is a concrete case, that I have been working on. I have the following processes:

asebadummynode --port 33335 0
asebaswitch --port 33333 tcp:port=33335 ser:name=Thymio

The dummy node advertises itself on the network as "Aseba Dummy Node 0", and the switch advertises itself as "Aseba Switch". So Studio can connect directly to the Dummy Node 0, or (exclusive or) connect indirectly through Aseba Switch.

If I understand this proposal, Dummy Node 0 would recognize that it is connected to the switch, and would never advertise itself? And it is forbidden to connect directly to Dummy Node 0?

davidjsherman avatar Sep 28 '17 08:09 davidjsherman

If I understand this proposal, Dummy Node 0 would recognize that it is connected to the switch, and would never advertise itself? And it is forbidden to connect directly to Dummy Node 0?

Yes, the idea is to not advertise a connection that would break another connection, with the relaxing option that if nothing happened on that connection for 3 seconds, it is considered "zombied" (no client active taking care of probing the network), and we assume that we can honestly take control of it, and hence advertise it.

stephanemagnenat avatar Sep 28 '17 08:09 stephanemagnenat

Could you record here the reason why this is better than a status flag in the TXT record? So we don't forget

davidjsherman avatar Sep 28 '17 08:09 davidjsherman

I would not say it is better, just rather simpler to implement. Given our limited resources, I would rather make it simple first, and extend the protocol when we see the need. But if you think that it is ugly, we can use the status flag, I'm also fine with it, it is not hard to parse. Maybe it is better indeed.

stephanemagnenat avatar Sep 28 '17 08:09 stephanemagnenat

Also, I should add that this removes the need to implement Zeroconf in the event loop, since a node is either exclusively advertising itself, or exclusively communicating with its client.

davidjsherman avatar Sep 28 '17 08:09 davidjsherman

Assuming we add support for busy flag, should we add it to the constructor of Zeroconf::TxtRecord, to be consistent? In that case, I suggest to add it after type. In a first test implementation, we could always set busy to true if there is a client connected, regardless of its activity.

stephanemagnenat avatar Sep 28 '17 08:09 stephanemagnenat

Yes, that would be easy.

Right now I am struggling with advertising for Aseba Switch

davidjsherman avatar Sep 28 '17 08:09 davidjsherman

Commit 0012396 adds support for the busy flag, and I have local code that adds support for DashelTarget, but it seems that the update of the TXT record is broken (see #692).

stephanemagnenat avatar Sep 28 '17 09:09 stephanemagnenat

Commit 72e62c7a implements the support of the busy flag in the DashelTarget dialogue box in Studio.

stephanemagnenat avatar Sep 28 '17 13:09 stephanemagnenat

After implementing service remove support (see commit f65ae55f05322bb8b754e49aebb33cb8afeccf66), it appears that the busy flag might be uselessly complex. Getting updates of the TXT flag on the local client requires doing an activeDNSServiceResolveReply at regular intervals, while connection and disconnection of services are properly notified. Therefore, I consider switching back to removing the service when it is busy. Commit de3b413d implements the small change for dummynode, as a way to test this (full support for busy flag is still in the code).

There is still the point that we might want to be updated of TXT changes, for example when the list of targets behind a switch are changing dynamically. But that is not very often or very important. According to the documentation of DNSServiceResolveReply, querying of updates for TXT should be done through DNSServiceQueryRecord and its callback DNSServiceQueryRecordReply should be used. Issue #713 tracks this feature.

stephanemagnenat avatar Nov 08 '17 16:11 stephanemagnenat

Because of time constraints, I have decided that for 1.6 we will deregister busy services instead of updating the TXT field. Nevertheless, commit 5326d03b46cd8e13bb9f7391e020e08d72b3559d introduces a clean and consistent API in Zeroconf for reporting de-registration and adds the list of detected targets to the Zeroconf object. This will make it easy to add support for TXT field update through DNSServiceQueryRecord in a later release.

stephanemagnenat avatar Nov 15 '17 16:11 stephanemagnenat

Commit 127133ce04e8452963fcb936a35c5acc297ec090 adds support for de-advertising used targets in Playground.

stephanemagnenat avatar Nov 16 '17 10:11 stephanemagnenat

Commit a38b483e4a46d38875493ef944025ba6cc90dcc8 adds basic support for Zeroconf to Http. However, the connection model of Http does not properly defines the notion of being used.

Hence, I am removing Http and adding Switch and moving this issue to 1.7.

stephanemagnenat avatar Nov 16 '17 14:11 stephanemagnenat