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

New Issue

mohr · 2020-06-18T13:51:05+02:00

mohr commented

2020-06-18 13:51:05 +02:00

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

gaisser commented

2021-06-15 16:51:07 +02:00

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

gaisser commented

2021-06-15 17:39:04 +02:00

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

gaisser commented

2021-06-16 18:01:38 +02:00

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.

gaisser commented

2021-06-16 18:09:23 +02:00

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.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#112