search

DelfiSpace / DelfiPQcore

0 stars 1 forks source link

SoftwareUpdateService does not check for service request #34

Closed StefanoSperetta closed 4 years ago

StefanoSperetta commented 4 years ago

SoftwareUpdateService::process() does not check if the received command is a request, reply or error (RequestType field). The service should only operate upon requests.

I am not 100% sure what should happen in case we receive a reply or error frame. PingService responds error in case the received frame is not a request. I am not sure this is the correct behavior as it might trigger a lot of spurious responses on the bus. @CasperBroekhuizen any comment?

Anyway, in DelfiPQ-PythonTestScripts/PQ9FromBin.py (https://github.com/DelfiSpace/DelfiPQ-PythonTestScripts/blob/1adc985a8685e819c34fad5a2db47099ccbc011b/PQ9FromBin.py#L89) we are sending a response frame and it is accepted by the service.

CasperBroekhuizen commented 4 years ago

SoftwareUpdateService::process() does not check if the received command is a request, reply or error (RequestType field). The service should only operate upon requests.

Correct, we're missing this check, I'll implement it and cleanup the process() structure.

I am not 100% sure what should happen in case we receive a reply or error frame...

I thought that the decision was made to always reply to any command where the module is the destination. Ignoring a command will require us to change the commandHandler (https://github.com/DelfiSpace/DelfiPQcore/blob/master/CommandHandler.h#L44).

Anyway, in DelfiPQ-PythonTestScripts/PQ9FromBin.py...

Yes, this command is actually a funky one, I didnt realize that it is a reply, but it has to do with the command being part of the slot deletion sequence.:

-- > Delete slot 1 Request -- < 'Are you sure, if so, give me a "13"?' Reply -- > 'I am sure, reply with a 13' -- < Slot deleted Reply

I propose that command 3 (the erasure confirmation command) will just be a request instead of a reply.

StefanoSperetta commented 4 years ago

I thought that the decision was made to always reply to any command where the module is the destination. Ignoring a command will require us to change the commandHandler (https://github.com/DelfiSpace/DelfiPQcore/blob/master/CommandHandler.h#L44).

Correct, it's just a doubt that is coming to my mind. The commandHandler is meant to act as a "slave" so I am not sure it makes sense no respond to responses. I was thinking to simplify services by simply adding a check in the command handler here (https://github.com/DelfiSpace/DelfiPQcore/blob/0f7515332a688c580ec6fda4245c746869fb3c66/CommandHandler.h#L34) to start the loop only if it is a request. This was we can take off quite some code from the services (as we avoid all the error handlers on malformed commands). Basically the PingService would almost be made only by this (https://github.com/DelfiSpace/DelfiPQcore/blob/master/PingService.cpp#L36-L38) and the if clause would not be needed.

Not responding to responses has the advantage of preventing cascade messages (but the obligation to respond to all request messages remains). What do you think?

I propose that command 3 (the erasure confirmation command) will just be a request instead of a reply.

I agree. Easy enough.

CasperBroekhuizen commented 4 years ago

This bug is fixed in the addition of the transport layer https://github.com/DelfiSpace/DelfiPQcore/issues/37