Re: [BACpypes-developers] AtomicReadFileRequest returns nothing if file data is larger than max apdu size

[Devin B. <dev...@gm...>]

Hi Joel,

Have you had a chance to give this any more thought? I don't suppose you
have already put it in a branch? Or would you prefer me to enter an issue?


Thanks,
Devin

On Thu, Jun 4, 2015 at 1:05 PM, Joel Bender <jo...@ca...> wrote:

> Devin,
>
>
> Your email presents a bit of a puzzler, and it touches on some pieces that
> have been around for years, so I've spent some time cogitating.
>
> > I did a little digging and I believe the problem lies in the following
> section of code within appservice.py (line 770):
> >
> > if (self.remoteDevice.segmentationSupported != 'segmentedReceive') and
> (self.remoteDevice.segmentationSupported != 'segmentedBoth'):
> >                     abort =
> self.abort(AbortReason.SEGMENTATIONNOTSUPPORTED)
> >                     self.request(abort)
> >                     return
>
> So the problem shows itself there, but the real problem is...
>
> > For starters, remoteDevice.segmentationSupported is not initialized
> properly. When getting information about the remote device (line 839), it
> only gets the source address of the device and not segmentation or max APDU:
> >
> > self.remoteDevice = self.ssmSAP.get_device_info(apdu.pduSource)
> >
> > Then since remoteDevice.segmentationSupported defaults to
> 'no-segmentation', it tries to send an abort (line 772) but the abort never
> makes it out because it should be self.response(abort).
>
> You are correct that the abort doesn't go downstream so it never gets sent
> to the device.  More thought is needed on this piece.  I remember something
> about the intent to send a message upstream to let it know the response it
> built and wanted to send won't make it, so maybe a new response can be
> formulated that will, but that doesn't fit with other applications I've
> written.  Humph.
>
> But even more than that you are correct, the get_device_info() function
> should be smarter than simply returning a generic DeviceInfo instance.
> That should at least reflect the information contain in the APCI of the
> request, namely the apduSA (segmented response accepted), apduMaxSegs
> (maximum segments accepted) and apduMaxResp (max response accepted).  See
> [1].
>
> In some very old code, there was no layer distinction between the
> ApplicationServiceAccessPoint, StateMachineAccessPoint, and the
> Application.  That was before the application ostensibly supported
> segmentation and it was even harder than it is now following the different
> paths of requests and responses.  The DeviceInfo structure was a cached
> copy of the contents of the I-Am message (or something out of a database
> that provided the same device/address binding information without the
> Who-Is/I-Am process for short running processes).
>
> So get_device_info() only needed the peer address because everything else
> in the DeviceInfo field was already cached, and I didn't check to make sure
> that the values in the APCI matched the cached values.  In the case of a
> request, using default values was acceptable.
>
> Now that the layers have been split up, the get_device_info() function
> needs to communicate to the Application instance which is responsible for
> managing the DeviceInfo cache.  The application is responsible for doing
> something with the contents of I-Am messages it receives, like making them
> "sticky" and exposing them with the deviceAddressBinding property of the
> device object.
>
> Having a get_device_info() function in the Application class that is
> called by the StateMachineAccessPoint will make it so other applications
> can use other techniques for providing that information (like from a
> database), rather than building a derived class of the
> StateMachineAccessPoint and overriding it there.  If the cache contents are
> to be reflected in the deviceAddressBinding list, at least the Application
> instance has easy access to its localDevice instance.
>
> Stepping back one level of indirection, get_device_info() is called from
> two different contexts, the ClientSSM and the ServerSSM (SSM = segmentation
> state machine).  In the case of the ClientSSM there is no request APCI to
> get default values from and it would be a good time to trigger a Who-Is if
> necessary for a cache miss.  In the case of the ServerSSM it will be good
> enough to accept whatever is encoded with the request and it doesn't even
> need to be cached, per se.
>
> So there will be two API functions,
> get_server_device_info(apdu.pduDestination) called in the ClientSSM code,
> and get_client_device_info(apdu.pduSource, apdu.apduSA, apdu.apduMaxSegs,
> apdu.apduMaxResp) called in the ServerSSM code.
>
> I think I'll put this in a branch until it can knocked around the block a
> bit.
>
> Thank you!
>
>
> Joel
>
> [1] Those parameter names are left over from when I did a lot of C++
> programming and my convention was to have the class name all lower case in
> the front, followed by the parameter name as camel case.  I wonder if it
> wouldn't serve us better to have them called what the standard calls them,
> segmentedResponseAccepted, maxSegmentsAccepted and maxAPDULengthAccepted.
> I'm not changing that now, but it's something more to think about.
>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> BACpypes-developers mailing list
> BAC...@li...
> https://lists.sourceforge.net/lists/listinfo/bacpypes-developers
>