JoelBender / bacpypes

BACpypes provides a BACnet application layer and network layer written in Python for daemons, scripting, and graphical interfaces.
MIT License
303 stars 129 forks source link

Communication issues caused by network path discovery when using foreign device functionality #455

Open zoopp opened 2 years ago

zoopp commented 2 years ago

I noticed a communication issue when making use of the foreign device functionality. Suppose we have the following setup:

out

The bacpypes application registers as a foreign device to the BBMD, which is also a router, in order to communicate with devices present in network number 1.

What I noticed is that if the foreign device registration completes before any requests is sent out to those devices then things are fine but if communication is attempted without having an active foreign device registration first then the bacpypes application won't be able to communicate with those devices during its lifetime.

I've tracked the issue down to the NetworkServiceAccessPoint class. The NSAP keeps lists of packets waiting for a network path and while there are no known network paths it adds new packets to the lists (here and here).

The first time a packet for a never-before seen network is sent, the NSAP broadcasts a WhoIsRouterToNetwork and when the IAmRouterToNetwork response comes back, any pending packets are sent out.

However, if the IAmRouterToNetwork never arrives (i.e. because foreign device registration is pending) then no subsequent attempts to discover a path will be made and the bacpypes application won't be able to reach those devices from this point forward (unless we're lucky and an "unsolicited" IAmRouterToNetwork comes our way but I wouldn't count on it):

[2022-05-06 19:37:50] DEBUG:1281900:bacpypes.bvllservice.BIPForeign: confirmation <bacpypes.bvll.Result object at 0x7fc1b8d20370>
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint: indication <bacpypes.apdu.ConfirmedRequestPDU(12,6) instance at 0x7fc1b91e5a20>
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint:     - local_adapter: <bacpypes.netservice.NetworkAdapter object at 0x7fc1b916b760>
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint:     - apdu: <bacpypes.apdu.APDU(ConfirmedRequestPDU,6) instance at 0x7fc1b91e59f0>
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint:     - npdu: <bacpypes.npdu.NPDU object at 0x7fc1b91e4bb0>
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint:     - dnet: 1
[2022-05-06 19:37:54] DEBUG:1281900:bacpypes.netservice.NetworkServiceAccessPoint:     - already waiting for path

My initial train of thought would be to have some form of time-to-live functionality around both path discovery as well as pending packet retention. This way, if we periodically "expire" packets pending network path discovery then we could re-attempt it in the future using the same functionality that we have now.

What are your thoughts on this?

JoelBender commented 2 years ago

You are correct, this is a hole in the library that hasn't been addressed, issue 216. There is no "upstream" notification from the link layer to the network layer (or the application layer for that matter) which should be along some kind of "control packets" rather than strictly "data packets."

I was considering adding control_request(**kwargs: Any), control_indication(**kwargs: Any), etc., type functions to the client and server classes that could pass content around. In fact they were once used for add_peer and delete_peer functions, but that was it, and it was simpler to just call those functions what they were rather than more checking to see what arguments were provided.

zoopp commented 2 years ago

Mhm, just so I understand correctly, these control_ methods would act just as a way to pass custom messages between downstream/upstream directly connected "elements"? Am I right in thinking that this only helps with cohesion/code structure? Apart from that, I don't fully understand how it would help with solving this issue.

Even with that, I would think we'd still need a loop/task that periodically reaps stale entries from the NetworkServiceAccessPoint.pending_nets dictionary.