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.

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

Data Product Catalog prototype #2667

Merged
merged 41 commits into from
Apr 25, 2024
Merged

Data Product Catalog prototype #2667

merged 41 commits into from
Apr 25, 2024

Conversation

timcanham
Copy link
Collaborator

@timcanham timcanham commented Apr 8, 2024

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

Change Description

This is a prototype demonstration of Svc/DpCatalog It was used to demonstrate an early release of data products in conjunction with FPP updates and DpManager/DpWriter

Svc/DpCatalog is not complete yet - this version is to capture the demo and allow users to play with it.

Rationale

Get a consolidated set of data product features int he baseline.

Testing/Review Recommendations

The demo can be run and experimented with.

Future Work

Svc/DpCatalog work will be continued with further releases.

@timcanham timcanham requested a review from bocchino April 8, 2024 19:56
@timcanham
Copy link
Collaborator Author

Requested @bocchino for changes made to Svc/DpWriter.

Comment on lines +352 to +353
void DpCatalog ::
fileDone_handler(

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
Comment on lines +390 to +391
void DpCatalog ::
START_XMIT_CATALOG_cmdHandler(

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_initialized = true;
}

Fw::CmdResponse DpCatalog::doCatalogBuild() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

}

bool DpCatalog::checkInit() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

}

void DpCatalog::sendNextEntry() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
Comment on lines +21 to +22
DpCatalog ::
DpCatalog(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.
@@ -98,6 +98,9 @@
//! \return The time tag
Fw::Time getTimeTag() const { return this->m_timeTag; }

//! Get the product state
Fw::DpState getState() const { return this->m_dpState; }

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
// Private data structures
// ----------------------------------

struct DpStateEntry {

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
Svc/DpCatalog/DpCatalog.cpp Fixed Show fixed Hide fixed
this->m_initialized = true;
}

Fw::CmdResponse DpCatalog::doCatalogBuild() {

Check notice

Code scanning / CodeQL

Function too long Note

doCatalogBuild has too many lines (156, while 60 are allowed).
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->m_directories[dir].toChar(),
this->m_fileList[file].toChar()
);
this->log_ACTIVITY_LO_ProcessingFile(fullFile);
Copy link
Contributor

@Joshua-Anderson Joshua-Anderson Apr 9, 2024

Choose a reason for hiding this comment

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

Might be worth lowering the severity of this EVR to diagnostic, sending a event per data product file processed could add up.

@Joshua-Anderson
Copy link
Contributor

Joshua-Anderson commented Apr 9, 2024

Is storing transmit state (UNTRANSMITTED/PARTIAL/TRANSMITTED) in the data product header the long term term plan?

It seems like this would force us to re-write data products once we start transmitting them, which could be a bit of a circular dependency. It also could get a bit confusing because the onboard DP file checksum would change after you start transmitting it.

Did we consider having DpCatalog own data product transmit state instead? (I.E. have a catalog file with a list of data products and their transmit states)

This could also provide a bit more flexibility because different projects may want to track data product transmit state differently (I.E. track not just that a data product is partial but how much of the file was sent, or which portions of the file need to be retransmitted).

@timcanham
Copy link
Collaborator Author

@Joshua-Anderson The plan is to modify the header as we process the data products, i.e. DpWriter writes it as UNTRANSMITTED, then as DpCatalog starts downlinking it will change to PARTIAL, then TRANSMITTED when done. I'm not sure I see the circular dependency part, so elaborate on that for me.

You are correct that having a separate DP state file (or other variants on the idea) is an option and @bocchino and I discussed it, but I've always preferred files to be self-contained in their state so that the DpCatalog can be simpler in that it discovers state in the files and doesn't have to keep track of things in two places. Just a design preference.

config/DpCfg.hpp Outdated Show resolved Hide resolved
Svc/DpWriter/DpWriter.hpp Outdated Show resolved Hide resolved
@bocchino
Copy link
Collaborator

bocchino commented Apr 9, 2024

The changes to DpWriter look good to me! I just had a couple of comments.

~DpCatalog()
{}

void DpCatalog::configure(

Check notice

Code scanning / CodeQL

No raw arrays in interfaces Note

Raw arrays should not be used in interfaces. A container class should be used instead.
~DpCatalog()
{}

void DpCatalog::configure(

Check notice

Code scanning / CodeQL

Long function without assertion Note

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

DpCatalog ::
~DpCatalog()
{}

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
@@ -18,10 +18,14 @@
// Construction, initialization, and destruction
// ----------------------------------------------------------------------

DpWriter::DpWriter(const char* const compName) : DpWriterComponentBase(compName) {}
DpWriter::DpWriter(const char* const compName) : DpWriterComponentBase(compName), m_dpFileNamePrefix() {}

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
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.

@bocchino would you mark a final review on this demo component? Just clarify your comments were resolved.

product get port productGetOut

@ Data product request port
product request port productRequestOut
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there bothj get and request on this component and one is left in TODO state?

Fw::Success::T status
)
{
// TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be removed or demonstrated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was going to add that update to SignalGen after the demo was merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hold off for a day or so - decided to implement the missing function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@timcanham this was merged yesterday.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah... Okay, I'll do another PR for the update.

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

My comments have been addressed.

@LeStarch LeStarch merged commit 4557066 into nasa:devel Apr 25, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants