Skip to content

FpySequencer 0.2 #3552

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.

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

Merged
merged 28 commits into from
May 14, 2025
Merged

FpySequencer 0.2 #3552

merged 28 commits into from
May 14, 2025

Conversation

zimri-leisher
Copy link
Collaborator

@zimri-leisher zimri-leisher commented May 3, 2025

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

Change Description

Local variables

  • Byte arrays which can store arbitrary, serialized data, much like prmdb.
  • Cleared after sequence stops.
  • These are the "workspace" of the sequence, which it can use for arguments, variables, or as a go between with prm/tlm/other sequences
  • Currently only accessed by the IF directive
  • By default, each sequencer has 16 local variables

New commands

Name Description
DEBUG_SET_BREAKPOINT Sets the debugging breakpoint which will pause the execution of the sequencer when reached, until unpaused by the DEBUG_CONTINUE command.
DEBUG_BREAK Pauses the execution of the sequencer, just before it is about to dispatch the next statement, until unpaused by the DEBUG_CONTINUE command.
DEBUG_CONTINUE Continues the execution of the sequence after it has been paused by a debug break.
DEBUG_CLEAR_BREAKPOINT Clears the debugging breakpoint, but does not continue executing the sequence.

New directives

Name Description
NO_OP Does nothing
SET_LVAR Sets a local variable to a byte array value
GOTO Sets the index of the next statement to be executed
IF Interprets an lvar as a boolean. If false, goto a specified statement index, otherwise proceed

Misc

  • 87% UT coverage
  • A couple bugfixes

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

this->tlmWrite_Debug(this->getDebugTelemetry());
}

FpySequencer_DebugTelemetry FpySequencer::getDebugTelemetry() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
FpySequencer_IfDirective ifDirective;
FpySequencer_NoOpDirective noOp;

DirectiveUnion() {}

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
FpySequencer_NoOpDirective noOp;

DirectiveUnion() {}
~DirectiveUnion() {}

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
return Fw::Success::FAILURE;
}
}
return Fw::Success::SUCCESS;
}

// dispatches a deserialized sequencer directive to the right handler.
// return success if successfully handled.
void FpySequencer::dispatchDirective(const DirectiveUnion& directive, const Fpy::DirectiveId& id) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
return Fw::Success::SUCCESS;
}

Fw::Success FpySequencer::readHeader() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
return Fw::Success::FAILURE;
}

Fw::Success FpySequencer::readBody() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
}

deserStatus = this->m_sequenceBuffer.deserialize(this->m_sequenceObj.getfooter());
Fw::Success FpySequencer::readFooter() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
// return true if successful
Fw::Success FpySequencer::readBytes(Os::File& file, FwSizeType readLen, bool updateCRC) {
// return success if successful
Fw::Success FpySequencer::readBytes(Os::File& file, FwSizeType expectedReadLen, bool updateCrc) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@zimri-leisher zimri-leisher marked this pull request as ready for review May 4, 2025 03:28
m_runtime(),
m_tlm()
{}
FpySequencer ::FpySequencer(const char* const compName)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
m_statementsDispatched(0),
m_runtime(),
m_debug(),
m_tlm() {}

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
//! Pauses the execution of the sequencer once, just before it is about to dispatch the next statement,
//! until unpaused by the DEBUG_CONTINUE command. This command is only valid in the RUNNING state.
//! Debug settings are cleared after a sequence ends execution.
void FpySequencer::DEBUG_BREAK_cmdHandler(FwOpcodeType opCode, //!< The opcode

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
//!
//! Continues the execution of the sequence after it has been paused by a debug break. This command
//! is only valid in the RUNNING.DEBUG_BROKEN state.
void FpySequencer::DEBUG_CONTINUE_cmdHandler(FwOpcodeType opCode, //!< The opcode

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@zimri-leisher
Copy link
Collaborator Author

@LeStarch this is ready for review

::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}
}

Check warning

Code scanning / CppCheck

Could not find a newline character at the end of the file. Warning test

Could not find a newline character at the end of the file.
ASSERT_CMD_RESPONSE(0, FpySequencer::OPCODE_RUN, 0, Fw::CmdResponse::OK);
}

}

Check warning

Code scanning / CppCheck

Could not find a newline character at the end of the file. Warning test

Could not find a newline character at the end of the file.
@zimri-leisher
Copy link
Collaborator Author

I transitioned all the UTs to using test fixtures. Not sure why we aren't doing this normally. This reduces the number of needed files and boilerplate.

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

The biggest design issue I see is type-safety of local variables. Right now that buffer could contain anything. Consider adding a type enumeration to each, using PolyType, or some other update to allow for not implicitly changing types.

I'd also like to understand more about how you are using UT fixtures.

Otherwise, the implementation and changes look really solid.

return;
}
this->sequencer_sendSignal_cmd_DEBUG_BREAK(
FpySequencer_DebugBreakpointArgs(true, breakOnce, this->m_runtime.nextStatementIndex));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow-Up: can we have a ships passing in the night scenario?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow you got me I don't know what that is but it sounds interesting

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented May 7, 2025

  • will want to check if we serialize too large of a value in the bytecode.
  • also would be nice to write all fpp constants to the dictionary

when we do lvar allocation:

  • start out by assuming infinitely many of them, then do a pass to lower them down

big question: probably thinking about changing to stack based only?
no^ will need them for global variables etc

if you have functions, you probably need a function call stack. and if you have a call stack, you might as well use it
primary advantage of registers is hardware speed anyways...

@LeStarch
Copy link
Collaborator

LeStarch commented May 8, 2025

@zimri-leisher I Agree with with your responses. Just need to resolve the last two. Was the last comment for this iteration (as there are no functions yet).

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented May 10, 2025

@LeStarch standby on responses to your last two questions. Yes, the last comment is for the next version of FpySequencer.

@LeStarch @timcanham I'm going to put one more feature in this PR as it was rather quick to develop and adds a huge amount of functionality: getting telemetry channel time tags and values. This is a top priority for my team, I hope you won't mind the scope creep. I still think we can get this merged by May 20th.

  1. Question about this: if FW_TLM_BUF_MAX_SIZE is greater than our LVAR_BUF_MAX_SIZE, should we allow it to compile or static assert? If we allow it to compile, how should we handle the case in which we get a tlm chan with a value of size greater than we can store in an lvar? Should we fail the "GET" directive or should we truncate the tlm chan? EDIT--I made it fail the "GET" directive.
  2. I'm thinking of renaming local variables to "registers" to be more consistent with general compiler terminology. Thoughts?

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented May 10, 2025

Second question: noticing some inconsistencies around PrmGet and TlmGet ports.

PrmDb impl of PrmGet does not modify ParamBuffer if it fails to find the prm:
image

But TlmChan impl of TlmGet does set TlmBuffer to 0:
image

I propose we have PrmDb set size to 0. Just be consistent!
Although I guess it's okay if we don't do this because PrmDb has the ParamValid enum as a return code

@zimri-leisher
Copy link
Collaborator Author

Another question:
what should we do when the sequencer makes a request for an "ignored" channel? At the moment, the TlmPacketizer doesn't store the bytes of ignored channels. But this means that we may not be able to access all channels at runtime. In the future, perhaps TlmPacketizer could store the value of ignored channels, just not telemeter them to the ground.

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented May 10, 2025

Another question:
What should we do when the sequencer requests a valid channel, but the channel hasn't been written into the tlm db yet? For TlmChan it returns tlmbuf of size 0, but for tlmpacketizer it returns an undefined tlmbuf/timetag.
Likely we want both to return a tlmbuf of size 0.

EDIT--I went ahead and implemented this ^

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented May 10, 2025

Also while I'm at it I added GET_PRM_VAL. I apologize for the scope creep but these were features I'd already developed on a separate branch and adding them in this PR means we can actually start using this sequencer for non-trivial things! At the moment, all you can do is IF conditions with boolean tlm/prms, but that's enough to really start to hook the sequencer into the wider fprime system.

Promise that's it for scope creep.

@zimri-leisher
Copy link
Collaborator Author

Note:
TlmChan has weird behavior when calling TlmGet port on it. Because it alternates between two buffers, the value it returns from TlmGet can alternate between two values, or one value and no value. I believe this is a mistake in the implementation of TlmChan::TlmGet so I'll go ahead and fix it.

@zimri-leisher
Copy link
Collaborator Author

  • UT for TlmPacketizer and TlmChan
  • GDS PR
  • Change signature of TlmGet to have TlmValid. Also add IGNORED status perhaps?
  • cut down on duplication by having one GET_TLM
  • ponder TlmChan edge cases

@LeStarch
Copy link
Collaborator

@zimri-leisher added some comments. Almost all were repeating your above todos.

You should also merge in the base branch, as configuration changed.

@zimri-leisher
Copy link
Collaborator Author

@LeStarch all feedback is complete I believe. Ready for merge

return Signal::stmtResponse_success;
}

Signal FpySequencer::getTlm_directiveHandler(const FpySequencer_GetTlmDirective& directive) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
return Signal::stmtResponse_success;
}

Signal FpySequencer::getPrm_directiveHandler(const FpySequencer_GetPrmDirective& directive) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@@ -56,31 +58,81 @@
this->pingOut_out(0, key);
}

void TlmChan::TlmGet_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& timeTag, Fw::TlmBuffer& val) {
Fw::TlmValid TlmChan::TlmGet_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& timeTag, Fw::TlmBuffer& val) {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@@ -56,31 +58,81 @@
this->pingOut_out(0, key);
}

void TlmChan::TlmGet_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& timeTag, Fw::TlmBuffer& val) {
Fw::TlmValid TlmChan::TlmGet_handler(FwIndexType portNum, FwChanIdType id, Fw::Time& timeTag, Fw::TlmBuffer& val) {

Check notice

Code scanning / CodeQL

Function too long Note

TlmGet_handler has too many lines (76, while 60 are allowed).
this->m_lock.unLock();
}
}
}

//! Handler for input port TlmGet
Fw::TlmValid TlmPacketizer ::TlmGet_handler(FwIndexType portNum, //!< The port number

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
this->m_lock.unLock();
}
}
}

//! Handler for input port TlmGet
Fw::TlmValid TlmPacketizer ::TlmGet_handler(FwIndexType portNum, //!< The port number

Check notice

Code scanning / CodeQL

Function too long Note

TlmGet_handler has too many lines (78, while 60 are allowed).
Comment on lines 106 to +217
FpySequencer_WaitAbsDirective directive;
status = argBuf.deserialize(directive);
new (&deserializedDirective.waitAbs) FpySequencer_WaitAbsDirective();
status = argBuf.deserialize(deserializedDirective.waitAbs);
if (status != Fw::SerializeStatus::FW_SERIALIZE_OK || argBuf.getBuffLeft() != 0) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}
break;
}
case Fpy::DirectiveId::SET_LVAR: {
// set local var has some custom deserialization behavior
// we don't write a custom class for it though because that deserialization behavior only
// applies for the initial time we deserialize it out of the statement

// the behavior in question is that it will grab the entire remaining part of the statement
// arg buf. that is, it uses the remaining length of the statement arg buf to determine the length
// of its value buf. this way we get to save on serializing the value length

// TODO do some trades on the best way to do this. not confident on this one

// first deserialize the index
U8 index;
status = argBuf.deserialize(index);
if (status != Fw::SerializeStatus::FW_SERIALIZE_OK) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}

new (&deserializedDirective.setLVar) FpySequencer_SetLocalVarDirective();
deserializedDirective.setLVar.setindex(index);

// okay, now deserialize the remaining bytes in the stmt arg buf into the value buf

// how many bytes are left?
FwSizeType valueSize = argBuf.getBuffLeft();

// check to make sure the value will fit in the FpySequencer_SetLocalVarDirective::value buf
if (valueSize > Fpy::MAX_LOCAL_VARIABLE_BUFFER_SIZE) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
Fw::SerializeStatus::FW_DESERIALIZE_FORMAT_ERROR,
argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}

// okay, it will fit. put it in
status = argBuf.deserialize(deserializedDirective.setLVar.getvalue(), valueSize, true);

if (status != Fw::SerializeStatus::FW_SERIALIZE_OK) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}

// now there should be nothing left, otherwise coding err
FW_ASSERT(argBuf.getBuffLeft() == 0, static_cast<FwAssertArgType>(argBuf.getBuffLeft()));

// and set the buf size now that we know it
deserializedDirective.setLVar.set_valueSize(valueSize);
break;
}
case Fpy::DirectiveId::GOTO: {
new (&deserializedDirective.gotoDirective) FpySequencer_GotoDirective();
status = argBuf.deserialize(deserializedDirective.gotoDirective);
if (status != Fw::SerializeStatus::FW_SERIALIZE_OK || argBuf.getBuffLeft() != 0) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}
break;
}
case Fpy::DirectiveId::IF: {
new (&deserializedDirective.ifDirective) FpySequencer_IfDirective();
status = argBuf.deserialize(deserializedDirective.ifDirective);
if (status != Fw::SerializeStatus::FW_SERIALIZE_OK || argBuf.getBuffLeft() != 0) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}
break;
}
case Fpy::DirectiveId::NO_OP: {
new (&deserializedDirective.noOp) FpySequencer_NoOpDirective();
// no op does not need deser
if (argBuf.getBuffLeft() != 0) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
Fw::SerializeStatus::FW_DESERIALIZE_SIZE_MISMATCH,
argBuf.getBuffLeft(), argBuf.getBuffLength());
return Fw::Success::FAILURE;
}
break;
}
case Fpy::DirectiveId::GET_TLM: {
new (&deserializedDirective.getTlm) FpySequencer_GetTlmDirective();
status = argBuf.deserialize(deserializedDirective.getTlm);
if (status != Fw::SerializeStatus::FW_SERIALIZE_OK || argBuf.getBuffLeft() != 0) {
this->log_WARNING_HI_DirectiveDeserializeError(stmt.getopCode(), this->m_runtime.nextStatementIndex - 1,
status, argBuf.getBuffLeft(), argBuf.getBuffLength());

Check notice

Code scanning / CodeQL

Long switch case Note

Switch has at least one case that is too long:
SET_LVAR (51 lines)
.
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I see the expected changes!

@timcanham would you please review the changes to Svc/Tlm*

Also, please agree/disagree with the algorithm of "comparable":

  1. If time comparable, return newest by time
  2. If time incomparable, return "updated"
  3. If neither updated the return "active" buffer for lack of a better choice.

Note: this algorithm only runs when both buffers are valid.

@LeStarch LeStarch requested a review from timcanham May 14, 2025 18:10
Copy link
Collaborator

@timcanham timcanham left a comment

Choose a reason for hiding this comment

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

Minor findings that could be done on the next release, but nothing broken that I can see.

@ Size set to 0 if channel not found, or if no value
@ has been received for this channel yet.
ref val: Fw.TlmBuffer
) -> Fw.TlmValid
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a note describing what conditions return INVALID

@LeStarch LeStarch merged commit 29989ca into nasa:devel May 14, 2025
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants