Re: [Bacnet-developers] TSM ideas

[Steve K. <st...@ka...>]

Hi Peter,

> I'm doing a bit of work with some client side code at the moment and had
> ideas for some tweaks to the TSM code that might make handling responses
> easier and allow for sending multiple requests out and matching up the
> responses that come back simpler..

I assume you reviewed Julien's client side code in the branch to see
if there are some useful nuggets in there.
https://bacnet.svn.sourceforge.net/svnroot/bacnet/branches/jbennet/bacnet-stack-0-5-7/

> 1. Instead of having the acknowledge handlers decode responses as they
> arrive I was thinking of adding storage for the complete response into the
> TSM struct and having the client call the decoders when they find the
> response has arrived. Alternativly the copy of the sent request could be
> over written with the response, the (comments for some of the code suggest
> that it be usefull to  examine the sent packet when the response arrives but
> how realistic is it to expect to decode it in those circumstances?).
> Instead of being set to idle when the response arrives, the status would be
> set to indicate "response received".

The demo/handler/h_arf_a.c AtomicReadFileACK uses the TSM to figure
out the file instance of a returned chunk of file.  The ARF-A reqeust
could be re-written to save the invoke ID and file instance locally
before sending.

> 2. Abort, Reject and Error responses would get stored in the TSM so that
> they can be examined without relying on global variables. Again invoke id
> status would be marked as "response received" and a flag could be examined
> to see if it was a normal response or one of the error types.

So maybe using a structure like this (with an IFDEF for segmentation or not):
typedef struct bacnet_tsm_data {
    bool Allocated;

    BACNET_TSM_STATE State;
    uint8_t ActualWindowSize;
    uint32_t SequenceNumber;
    uint32_t NumSegments;
    uint8_t ProposedWindowSize;
    uint8_t SentAllSegments;
    uint8_t RetryCount;
    uint8_t SegmentRetryCount;
    uint8_t InitialSequenceNumber;
    uint8_t LastSequenceNumber;
    uint32_t SegmentTimer;
    uint32_t ApduTimer;
    uint8_t InvokeID;

    uint32_t RequestTimer;

    BACNET_ADDRESS ClientAddress;
    BACNET_ADDRESS ServerAddress;
    /* if we are answering the question */
    bool AmServer;
    /* NPDU priority */
    uint8_t Priority;

    /* the network layer info */
    BACNET_NPDU_DATA NpduData;

    /* values retrieved from the original request */
    bool SegmentedResponseAccepted;
    uint16_t MaxSegmentsAccepted;
    uint16_t MaxApduLengthAccepted;

    /* APDU size to use */
    uint16_t SegmentLength;

    uint8_t *TransmitBuffers[MAX_WINDOW_SIZE];

    uint8_t MessageBuffer[MAX_MESSAGE_LEN];
    uint32_t MessageLength;
    uint8_t ServiceChoice;
    BOOL Confirmed;
    uint8_t SegmentAckBuffer[MAX_NPDU + 3];

    BACNET_TSM_CALLBACK CallbackFunction;
    void *CallbackHandle;

    BACNET_ERROR_CLASS ErrorClass;
    BACNET_ERROR_CODE ErrorCode;

    uint32_t DroppedSegment;
} BACNET_TSM_DATA;

> 3. We don't matchup the invoke id and source address at the moment so there
> is a chance that a delayed response from some device could trigger a "false
> positive" if an invoke ID has been re used in the mean time. It is not VERY
> likely but if it occurred it would be a difficult one to track down.
> Implementing some of this in a way that can avoid breaking existing code if
> the feature is not required may require some thinking so I thought I'd
> solicit some opinions first.

It would be good to rework our existing simplistic TSM module to fix that bug.

I have been thinking for some time to change the TSM to be more
central to the code - maybe  for the 0.6 release (I hope to make a
0.5.8 release this week - I didn't finish it over Thanksgiving).
Perhaps even have a small tsm-tiny.c and a tsm.c module.

Some of the changes I am looking to do are shown here in a
ReadProperty example (untested code):
    tsm = tsm_alloc(SERVICE_CONFIRMED_READ_PROPERTY);
    if (tsm) {
        tsm_npdu_set(src, dest, false, true, MESSAGE_PRIORITY_NORMAL);
        /* encode the APDU portion of the packet */
        data.object_type = object_type;
        data.object_instance = object_instance;
        data.object_property = object_property;
        data.array_index = array_index;
        len = rp_encode_apdu(
            tsm_message_buffer(tsm),
            tsm_message_size(tsm),
            &data);
        if (len < 0) {
            tsm_free(tsm);
            return false;
        }
        tsm_message_length_set(tsm, len);
        tsm_send_request(tsm, CallbackFunction, CallbackHandle);
    }

a. supply the buffer used in creating responses or requests (a
tsm_alloc that is called instead of using the Handler_Transmit_Buffer
global).

b. use the tsm to send to the right datalink(s) (in the case of
routing or multihome).

c. change buffer handling of high level encode/decode functions to
handle the length safely by adding an extra length parameter. This
would keep the developer from needing duplicate or over-length buffers
and actually have the code be safe from buffer overrun.

Best Regards,

Steve
-- 
http://steve.kargs.net/