Bugfix/empty seq

[jsilveira1409]

Related Issue(s)	#3005
Has Unit Tests (y/n)	n
Documentation Included (y/n)	n

---

Change Description

Implemented FileEmpty warning for the Svc::CmdDispatcher Component, which is called when deserializing the next record returns an empty buffer.

Rationale

The Flight Software should raise a warning on empty command sequence files instead of crashing.

Testing/Review Recommendations

Future Work

[LeStarch]

A few minor changes and questions.

[LeStarch]

We need to clean-out the commented out code. Preferably we'd include the sequence name.

What scenario would this hit? Doesn't the sequence validation catch this (e.g. the CRC)?

Additionally, can this code be hit while a sequence is running? (e.g. the sequence file wasn't fully uplinked).

[jsilveira1409]

My bad for the comment, forgot about them!

cmdSeq.CS_VALIDATE does validate empty command sequences:

So actually the CS_VALIDATE would also need to trigger an empty command sequence event.

[jsilveira1409]

So actually, to encompass both cases, a more elegant solution could be to simply verify the number of records when loading the sequence file at FPrimeSequence level, and return false if the number of records is zero, which would trigger the following events:

This would be the minor change:

bool CmdSequencerComponentImpl::FPrimeSequence ::
    validateRecords()
  {
    Fw::SerializeBufferBase& buffer = this->m_buffer;
    const U32 numRecords = this->m_header.m_numRecords;
    Sequence::Record record;

    if (numRecords == 0)
    {
      return false;
    }

( ... )

}

[zimri-leisher]

I think the FileEmpty solution may be technically inaccurate as the file still contains a sequence header and so is not literally empty. If we're going to use an event, maybe we'd better call it SeqEmpty or NoRecords, and make it clear in the error message that there specifically weren't any commands in the sequence. I like the second solution better, if this is the direction we choose to go.

[LeStarch]

@timcanham and @zimri-leisher I'd appreciate your thoughts on this PR.

[zimri-leisher]

I would add/modify a unit test to check this behavior.

EDIT--On second thought, instead of EXECUTION_ERRORing, should we just allow the user to run empty sequences? My team intentionally did this and thought it would work (we were using the seq as a placeholder). In general tools should allow as much valid input as possible to make the fewest decisions for users. Maybe we could still raise a warning just in case it was a mistake.

[jsilveira1409]

@zimri-leisher @LeStarch how about this event logs:

[zimri-leisher]

In my opinion this could be a low warning, but otherwise the event looks good! I assume it's the same for CS_VALIDATE.

[jsilveira1409]

@zimri-leisher yeah makes sense to make it low warning, here is the full logs with CS_VALIDATE too :

[zimri-leisher]

Looks good, just missing a unit test.

[timcanham]

LGTM!

[LeStarch]

Looks good to me! I added a commit to allow the improperly flagged misspellings. Thanks for working this through! @zimri-leisher thanks for the review!