Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/empty seq #3025

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Bugfix/empty seq #3025

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50504c3
feat: implemented empty cmd seq event
jsilveira1409 Nov 13, 2024
1f9b7a6
Merge branch 'nasa:devel' into bugfix/empty-seq
jsilveira1409 Nov 14, 2024
8a2bca2
feat: improved empty seq warnings
jsilveira1409 Nov 16, 2024
8535003
feat: implemented ut for NoRecords Event
jsilveira1409 Nov 17, 2024
9034815
sp
LeStarch Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Svc/CmdSequencer/Events.fppi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,11 @@ event CS_JoinWaitingNotComplete() \
severity warning high \
id 24 \
format "Still waiting for sequence file to complete"

event CS_FileEmpty(
#fileName: string size 60 @< The name of the sequence file
) \
severity warning high \
id 25 \
format "Sequence file is empty. Ignoring."
#format "Sequence file {} is empty. Ignoring."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad for the comment, forgot about them!

cmdSeq.CS_VALIDATE does validate empty command sequences:

image

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

Copy link
Contributor Author

@jsilveira1409 jsilveira1409 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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;
    }

( ... )

}

Copy link
Collaborator

@zimri-leisher zimri-leisher Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

7 changes: 6 additions & 1 deletion Svc/CmdSequencer/FPrimeSequence.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,14 @@ namespace Svc {
}

void CmdSequencerComponentImpl::FPrimeSequence ::
nextRecord(Record& record)
nextRecord(Record& record)
{
Fw::SerializeStatus status = this->deserializeRecord(record);
if (status == Fw::SerializeStatus::FW_DESERIALIZE_BUFFER_EMPTY)
{
this->m_component.log_WARNING_HI_CS_FileEmpty();
return;
}
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
}

Expand Down
Loading