DHB::replyToReply() ignores if a reply is missed which is not the last reply #112

Open
opened 2020-06-18 13:51:05 +02:00 by mohr · 4 comments
Owner

This might (needs to be checked) lead to a command which has multiple replies, to be reported as succeeded even if the first (or not last) reply was missed.

This might (needs to be checked) lead to a command which has multiple replies, to be reported as succeeded even if the first (or not last) reply was missed.
gaisser added this to the ASTP 1.1.0 milestone 2021-04-20 18:55:17 +02:00
gaisser added the
bug
label 2021-05-04 15:17:27 +02:00
Owner

Checked replyToReply and I can confirm it.

    if (info->expectedReplies == 0) {
        .....
        if (info->sendReplyTo != NO_COMMANDER) {
            bool success = false;
            if(status == HasReturnvaluesIF::RETURN_OK) {
                success = true;
            }
            actionHelper.finish(success, info->sendReplyTo, command, status);
        }
        info->isExecuting = false;
    }

The success or failure is only reported if expectedReplies is 0.

A solution might be:

    ....
    if (info->expectedReplies == 0 or status != HasReturnvaluesIF::RETURN_OK) {
        ....
        if (info->sendReplyTo != NO_COMMANDER) {
            bool success = false;
            if(status == HasReturnvaluesIF::RETURN_OK) {
                success = true;
            }
            actionHelper.finish(success, info->sendReplyTo, command, status);
            info->sendReplyTo = NO_COMMANDER;
        }
        if (info->expectedReplies == 0){
        	info->isExecuting = false;
        }
        
    }


But info->sendReplyTo = NO_COMMANDER; might break something

Checked replyToReply and I can confirm it. ``` c++ if (info->expectedReplies == 0) { ..... if (info->sendReplyTo != NO_COMMANDER) { bool success = false; if(status == HasReturnvaluesIF::RETURN_OK) { success = true; } actionHelper.finish(success, info->sendReplyTo, command, status); } info->isExecuting = false; } ``` The success or failure is only reported if expectedReplies is 0. A solution might be: ``` c++ .... if (info->expectedReplies == 0 or status != HasReturnvaluesIF::RETURN_OK) { .... if (info->sendReplyTo != NO_COMMANDER) { bool success = false; if(status == HasReturnvaluesIF::RETURN_OK) { success = true; } actionHelper.finish(success, info->sendReplyTo, command, status); info->sendReplyTo = NO_COMMANDER; } if (info->expectedReplies == 0){ info->isExecuting = false; } } ``` But ``info->sendReplyTo = NO_COMMANDER; `` might break something
gaisser changed title from Unconfirmed: DHB::replyToReply() ignores if a reply is missed which is not the last reply to DHB::replyToReply() ignores if a reply is missed which is not the last reply 2021-06-15 16:54:30 +02:00
Owner

Or: We could add a field to DeviceCommandInfo to keep track if we reported it before?

Or: We could add a field to DeviceCommandInfo to keep track if we reported it before?
Owner

I think we could add a function:

virtual Returnvalue_t handleFailedMultiReply(DeviceReplyMap::iterator iter, ReturnValue_t status) 

And call it if status != RETURN_OK and expectedReplies > 0.

The function could contain something like:


DeviceCommandInfo* info = &(iter->second.command->second);
if (info->sendReplyTo != NO_COMMANDER and not info->reportedBefore) {
            Returnvalue_t result = actionHelper.finish(false, info->sendReplyTo, iter->first, status);
            if(result != HasReturnvaluesIF::RETURN_OK){
            	return result;
            }
            info->reportedBefore = true;
        }

This can be overwritten by any user to do something more useful for very specific devices.

Or:

Add a "isFailedBefore" Flag, which will be reported in the end. This would be a easy solution to insert because the handling of answers does not change.

Both have a flaw if two replies fail with a different reason.

I think we could add a function: ``` c++ virtual Returnvalue_t handleFailedMultiReply(DeviceReplyMap::iterator iter, ReturnValue_t status) ``` And call it if status != RETURN_OK and expectedReplies > 0. The function could contain something like: ``` c++ DeviceCommandInfo* info = &(iter->second.command->second); if (info->sendReplyTo != NO_COMMANDER and not info->reportedBefore) { Returnvalue_t result = actionHelper.finish(false, info->sendReplyTo, iter->first, status); if(result != HasReturnvaluesIF::RETURN_OK){ return result; } info->reportedBefore = true; } ``` This can be overwritten by any user to do something more useful for very specific devices. Or: Add a "isFailedBefore" Flag, which will be reported in the end. This would be a easy solution to insert because the handling of answers does not change. Both have a flaw if two replies fail with a different reason.
Owner

One small side note:

This is only relevant for replies with different allowed maximum delayCylces. If the replies have the same delayCylces and one is missing it will always be the last to be reported because others must be received before the delay. (decrementDeviceReplyMap is part of the PERFORM_OPERATION step)

One small side note: This is only relevant for replies with different allowed maximum delayCylces. If the replies have the same delayCylces and one is missing it will always be the last to be reported because others must be received before the delay. (decrementDeviceReplyMap is part of the PERFORM_OPERATION step)
gaisser modified the milestone from ASTP 1.1.0 to v2.0.0 2021-06-29 14:05:03 +02:00
mohr removed this from the v2.0.0 milestone 2021-10-01 13:18:41 +02:00
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#112
No description provided.